Project

General

Profile

Feature #17259

Kernel#warn should ignore <internal: entries

Added by Eregon (Benoit Daloze) about 2 months ago. Updated 10 days ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
[ruby-core:100357]

Description

Kernel#warn currently does not skip <internal: entries from core library methods defined in Ruby.
This can cause rather unhelpful locations to be used for warnings.

For instance:

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

Note that RubyGems overrides Kernel#warn since https://github.com/rubygems/rubygems/pull/2442 and https://github.com/rubygems/rubygems/blob/c1bafab1d84e0aad06e377e9db4b74cccab4b43a/lib/rubygems/core_ext/kernel_warn.rb#L42,
so --disable-gems is needed to observe this behavior.
I think it is very suboptimal that RubyGems needs to monkey-patch Kernel#warn to remove RubyGems' require from Kernel#warn location.
That is both fragile (as we've seen from various incompatible behavior and bugs in that monkey-patch) and inefficient (walking the stack multiple times).

So I would suggest to actually skip all backtraces entries starting with <internal: for Kernel#warn(message, uplevel:).
BTW this is already what TruffleRuby does.

As a bonus, by filtering out <internal:, RubyGems could define its require in an eval(code, nil, '<internal:rubygems-require>', line) and it would automatically be skipped, without needing to monkey-patch Kernel#warn at all!

#1

Updated by Eregon (Benoit Daloze) about 2 months ago

  • Description updated (diff)

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

I agree with removing the Rubygems override of Kernel#warn (the only Kernel method Rubygems should override is require). Skipping <internal: entries for uplevel makes sense to me.

Updated by nobu (Nobuyoshi Nakada) about 2 months ago

Essentially uplevel: option is useless in some cases.
For instance, it cannot work to skip frames in the same file or directory.

Updated by Eregon (Benoit Daloze) about 2 months ago

nobu (Nobuyoshi Nakada) wrote in #note-3:

Essentially uplevel: option is useless in some cases.
For instance, it cannot work to skip frames in the same file or directory.

Yes, some libraries might want to emit warnings and ignore their own my_gem/lib/.
That however would need to make warn somehow take some extra context, because only warnings of that library should ignore my_gem/lib/, other calls to warn should not.
Maybe warn(message, uplevel: n, ignore: [path1, path2]).
That would not work for RubyGems' require, where warning are not emitted by RubyGems but by anything else.

In practice, I think gems should be able to know if they call themselves directly or not, so there should be no need to skip my_gem/lib/.
I might be wrong about that.

OTOH for core library methods and RubyGems' require, it seems clear it would never be useful to show their location for warn(uplevel:).
So I think ignoring <internal: is a useful step on its own, and such entries should be ignored by default.

It also keeps compatibility with older Rubies which had less core methods defined in Ruby, which should be a transparent implementation detail, at least for warn(uplevel:).

#5

Updated by nobu (Nobuyoshi Nakada) about 2 months ago

  • Backport deleted (2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN)
  • ruby -v deleted (ruby 3.0.0preview1 (2020-09-25 master 0096d2b895) [x86_64-linux])
  • Tracker changed from Bug to Feature
#8

Updated by Eregon (Benoit Daloze) about 1 month ago

  • Status changed from Open to Closed

Applied in changeset git|cffdacb15a363321e1c1879aa7d94924acafd1cf.


Ignore <internal: entries from core library methods for Kernel#warn(message, uplevel: n)

Updated by Eregon (Benoit Daloze) 10 days ago

PR to RubyGems so RubyGems won't need to override Kernel#warn anymore: https://github.com/rubygems/rubygems/pull/4075

Also available in: Atom PDF