Bug #20573
closedWarning.warn shouldn't be called for disabled warnings
Description
Currently Warning.warn
will be called for all warnings, even if that particular category is disabled.
For example
module Warning
def warn(message, category:)
p message => category
end
end
def get_var
$=
end
p Warning[:deprecated]
get_var
I think that internally we should not call Warning.warn
unless the category is enabled.
I've sent a PR here: https://github.com/ruby/ruby/pull/10960
Updated by tenderlovemaking (Aaron Patterson) 17 days ago
Oops, I send this before pasting the output of the script:
$ ./miniruby test.rb
false
{"test.rb:8: warning: variable $= is no longer effective\n"=>:deprecated}
You can see that "deprecated" warnings are not enabled, but Warning.warn
is still called.
Updated by byroot (Jean Boussier) 17 days ago
Agreed, Warning.warn
patches shouldn't be called for disabled categories.
I was actually 100% convinced this was the behavior, and this fix is consistent with Warning.warn
not being invoked for verbose warnings when $VERBOSE = false
.
Updated by byroot (Jean Boussier) 17 days ago
- Related to Feature #17122: Add category to Warning#warn added
Updated by byroot (Jean Boussier) 17 days ago
- Related to Feature #16345: Don't emit deprecation warnings by default. added
Updated by byroot (Jean Boussier) 16 days ago
- Related to Feature #17000: 2.7.2 turns off deprecation warnings by default added
Updated by byroot (Jean Boussier) 16 days ago
In [Feature #16345] it was stated:
We chose Warning.disable(:deprecated) instead of
re-defining Warning.warn in order to avoid string object generation.
The intent was clearly for Warning.warn
not to be called.
Updated by byroot (Jean Boussier) 16 days ago
On Ruby 2.7.8 when Warning[:deprecated] = false
was introduced:
def Warning.warn(message, **)
p [:warn, message]
end
def foo(a, **b)
[a, b]
end
hash = {bar: 2}
foo(1, hash)
Produces no output, Warning.warn
isn't called. I think this was the intent all along.
Updated by tenderlovemaking (Aaron Patterson) 16 days ago
byroot (Jean Boussier) wrote in #note-6:
In [Feature #16345] it was stated:
We chose Warning.disable(:deprecated) instead of
re-defining Warning.warn in order to avoid string object generation.The intent was clearly for
Warning.warn
not to be called.
I'm reading the ticket the same way. It sounds like this is a bug.
Updated by tenderlovemaking (Aaron Patterson) 16 days ago
- Status changed from Open to Closed
Applied in changeset git|1271ff72d5b627854c6812baefe796a2976cd793.
Don't call Warning.warn
unless the category is enabled
The warning category should be enabled if we want to call
Warning.warn
.
This commit speeds up the following benchmark:
eval "def test; " +
1000.times.map { "' '.chomp!" }.join(";") + "; end"
def run_benchmark count
i = 0
while i < count
start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
yield
ms = Process.clock_gettime(Process::CLOCK_MONOTONIC) - start
puts "itr ##{i}: #{(ms * 1000).to_i}ms"
i += 1
end
end
run_benchmark(25) do
250.times do
test
end
end
On master
this runs at about 92ms per iteration. With this patch, it
is 7ms per iteration.
[Bug #20573]
Updated by byroot (Jean Boussier) 15 days ago
- Backport changed from 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN to 3.1: WONTFIX, 3.2: REQUIRED, 3.3: REQUIRED
Given it's a bug fix I think we should mark it for backport. But of course it's up to branch maintainers to decide whether the fix is worth backporting.