Fix "stack level too deep" error when overriding Warning.warn

[eregon]

Alternative to #3985 which does not require adding a new module next to Kernel, and also works if some class inherits from BasicObject and include Kernel.

• This fixes infinite recursion when monkey-patching Warning#warn, which is not recommended, but still should behave the same whether RubyGems is loaded or not.
• Fixes Stack level too deep when overriding Warning.warn #2588.

I will abide by the code of conduct.

cc @deivid-rodriguez

[deivid-rodriguez]

Looks good to me @eregon.

I think it's the safest approach with the least side effects.
prepend / extend look nicer, but they don't achieve the same effect and they have various corner cases where they don't work (for modules like Kernel, for classes it would work better).

Regarding this, sure, definitely better to have safer code than to have nicer looking code. Let's add an extra test that fails with the prepend / extend approach to justify it.

[eregon]

Test added.
On #3985 that test fails with:

  1) Failure:
TestGemRequire#test_no_crash_when_overriding_warn_with_warning_module [/home/eregon/code/rubygems/test/rubygems/test_require.rb:710]:
Expected /^warn comes from .+rubygems\/core_ext\/kernel_warn.rb/ to match "main.rb:2: warning: method redefined; discarding old warn\n" +
"Foo Bar\n" +
"warn comes from core\n".

That is, the original Kernel#warn is used and not RubyGems version, which is unintended.

[eregon]

The reason for this refute vs asserting it comes from core is that's it's not easy to assert it comes from core, because methods defined in Ruby will have a source_location. In Ruby 2.7:

$ ruby --disable-gems -e 'p method(:warn).source_location'
["<internal:warning>", 42]

[eregon]

Should be ready for final review & merge when CI passed now.

[deivid-rodriguez]

I'm not sure I understand this very well, but I'm not clear what the end user problem that this fixes is.

• In Bug/kernel#warn uplevel #2442, the end user problem is clear to me: a warn call during a require using the ruby 2.5+ uplevel parameter would show an unintended location for the warning, differing from the one when using standard require. The reason for customizing warn was that PR.

• In Stack level too deep when overriding Warning.warn #2588 the problem is also clear, a SystemStackError.

But this one I'm not totally sure. If not necessary for any end user behaviour, it seems nice to me that the original Kernel#warn is used? 🤔

Could you split the new checks to a separate test, since they are unrelated to test_no_crash_when_overriding_warn_with_warning_module, and try to explain this better?

[eregon]

In short, it fixes #2588, without changing any other behavior or changing the ancestors of any class.

I wish without RubyGems it would just be a SystemStackError because redefining Warning#warn is IMHO the mistake in the first place (documented in ruby/ruby#3621), but since there is a workaround in Ruby it sounds indeed nice that the RubyGems version of Kernel#warn behaves the same.
The previous version would unintentionally change the receiver for warn, which this PR addresses.

I guess your question is mostly is there a case where we'd want RubyGems' warn and the receiver is not simple the main object or Kernel.warn? Someone could do some_object.send(:warn, message) since send is skipped on CRuby at the top-level.
Someone might also pass a value >1 to uplevel: in which case it might end up on the require frame, when we always want to skip it.

[eregon]

@deivid-rodriguez I've updated the test with something more realistic, it should be clearer now.
Essentially, warn always wants to skip core library methods (such as the original require), and also RubyGems require since we believe (in the vast majority of cases) it's not itself the source of the warning (and that showing the location of RubyGems' require would be useless).

BTW it seems on Ruby 3, warn already skips <internal: core library methods defined in Ruby:

$ ruby -ve 'tap { warn "foo", uplevel: 1; p caller(0) }'
ruby 3.0.0dev (2020-09-30T06:55:07Z master ce986b41ca) [x86_64-linux]
-e:1: warning: foo
["-e:1:in `block in <main>'", "<internal:kernel>:90:in `tap'", "-e:1:in `<main>'"]

We see warn uses -e:1 and not <internal:kernel>:90 as the caller.

It's kind of a shame that we need to redefine Kernel#warn when there is existing filtering for <internal:.
Also the redefinition only works if there is one redefinition, otherwise it would only to guarantee one of them and not both.
I think it would be worth proposing a new feature on https://bugs.ruby-lang.org/ like e.g. Kernel.ignore_for_warnings %r{/rubygems/core_ext/kernel_require\.rb} (or simply pass the absolute path as a String). Thoughts?
It would only apply to new Ruby versions so we'd still need to override on older versions though.

[eregon]

I didn't notice the lint failed, fixed now.
@deivid-rodriguez OK to merge this when it's green?

[eregon]

I got a new idea, and it seems to work on Ruby 3 and on TruffleRuby, so there we wouldn't need to override Kernel#warn at all!

The idea is we could set the "file" value for RubyGems' require as starting with <internal: through eval, and that will result in it being ignored by Kernel#warn, like core library methods.

eval <<RUBY, nil, '<internal:skip-me-for-warnings>', __LINE__+1
def mycaller
  foo
end
RUBY

def foo
  warn "WARNING", uplevel: 1
end

mycaller

On Ruby 3 and on TruffleRuby it gives:

warn.rb:11: warning: WARNING

On Ruby 2 (I tried 2.7 and 2.6) it gives:

<internal:skip-me-for-warnings>:3: warning: WARNING

Anyway, we still need the fallback for Ruby 2, so I think we should first merge this PR.
But this sounds like a better way forward, isn't it?
cc @nobu

[eregon]

@deivid-rodriguez The CI passes now, could you merge this?

[eregon]

Regarding #3987 (comment) I got confused, <internal: is removed by RubyGems' Kernel#warn. With --disable=gems it shows <internal:skip-me-for-warnings>:3 on Ruby 3. I'll probably file a pull request for that.

Actually, CRuby's Kernel#warn does not seems to skip core library methods written in Ruby currently:

$ ruby -v --disable=gems -e 'def deprecated; warn "use X instead", uplevel: 2; end; tap { deprecated }' 
ruby 3.0.0preview1 (2020-09-25 master 0096d2b895) [x86_64-linux]
<internal:kernel>:90: warning: use X instead

[eregon]

Issue on the CRuby tracker to automatically skip <internal: entries, and this could be the way forward to not need to override Kernel#warn at all in RubyGems: https://bugs.ruby-lang.org/issues/17259

[deivid-rodriguez]

I rebased this PR, squashed some commits and slightly modified the test, and your commit attribution was accidentally lost in that process. I'm really sorry about that, it was completely unintentional.

[eregon]

A bit unfortunate, but not a big deal, thanks for merging.