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

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

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

[Eregon (Benoit Daloze)]

As a small note on this, it's typically better to check $VERBOSE level and if the category is enabled before even generating/formatting the message String, for performance reasons.
So if that's always done the check in Warning.warn would be basically redundant.
But I agree it makes sense conceptually to check and also in some cases where e.g. the message String is static + frozen and then there is no cost for that message String.

EDIT: I should have looked again at the diff there it's clear the check is done before formatting so makes even more sense (although there can still be cost just to pass the arguments for formatting).
I was thinking this change was checking inside Warning.warn but it checks earlier, all good.

[k0kubun (Takashi Kokubun)]

• Backport changed from 3.1: WONTFIX, 3.2: REQUIRED, 3.3: REQUIRED to 3.1: WONTFIX, 3.2: REQUIRED, 3.3: DONE

ruby_3_3 a3eb5e5c70eaee12964cdd807b8f19950003141f.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 3.1: WONTFIX, 3.2: REQUIRED, 3.3: DONE to 3.1: WONTFIX, 3.2: DONE, 3.3: DONE

ruby_3_2 4f1e047f86b159528055d37ee0da2ad6e5a38c23 merged revision(s) a3eb5e5c70eaee12964cdd807b8f19950003141f.