Bug #20573: Warning.warn shouldn't be called for disabled warnings

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 3.1: WONTFIX, 3.2: REQUIRED, 3.3: REQUIRED

[tenderlovemaking (Aaron Patterson)]

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

[tenderlovemaking (Aaron Patterson)]

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.

[byroot (Jean Boussier)]

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.

[byroot (Jean Boussier)]

• Related to Feature #17122: Add category to Warning#warn added

[byroot (Jean Boussier)]

• Related to Feature #16345: Don't emit deprecation warnings by default. added

[byroot (Jean Boussier)]

• Related to Feature #17000: 2.7.2 turns off deprecation warnings by default added

[byroot (Jean Boussier)]

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.

[byroot (Jean Boussier)]

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.

[tenderlovemaking (Aaron Patterson)]

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.

[tenderlovemaking (Aaron Patterson)]

• 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]

[byroot (Jean Boussier)]

• 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.