ruby-core@ruby-lang.org archive (unofficial mirror)
 help / color / mirror / Atom feed
* [ruby-core:100357] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries
@ 2020-10-10 13:29 eregontp
  2020-10-10 18:24 ` [ruby-core:100360] " merch-redmine
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: eregontp @ 2020-10-10 13:29 UTC (permalink / raw)
  To: ruby-core

Issue #17259 has been reported by Eregon (Benoit Daloze).

----------------------------------------
Bug #17259: Kernel#warn should ignore <internal: entries
https://bugs.ruby-lang.org/issues/17259

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Target version: 3.0
* ruby -v: ruby 3.0.0preview1 (2020-09-25 master 0096d2b895) [x86_64-linux]
* Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN
----------------------------------------
`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
```

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](https://github.com/oracle/truffleruby/blob/c3bcccfd8db7d460e23f0371e7ceaf5fdb71275c/src/main/ruby/truffleruby/core/kernel.rb#L656).

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!



-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [ruby-core:100360] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries
  2020-10-10 13:29 [ruby-core:100357] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries eregontp
@ 2020-10-10 18:24 ` merch-redmine
  2020-10-11  2:43 ` [ruby-core:100362] " nobu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: merch-redmine @ 2020-10-10 18:24 UTC (permalink / raw)
  To: ruby-core

Issue #17259 has been updated by jeremyevans0 (Jeremy Evans).


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.

----------------------------------------
Bug #17259: Kernel#warn should ignore <internal: entries
https://bugs.ruby-lang.org/issues/17259#change-87971

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Target version: 3.0
* ruby -v: ruby 3.0.0preview1 (2020-09-25 master 0096d2b895) [x86_64-linux]
* Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN
----------------------------------------
`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](https://github.com/oracle/truffleruby/blob/c3bcccfd8db7d460e23f0371e7ceaf5fdb71275c/src/main/ruby/truffleruby/core/kernel.rb#L656).

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!



-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [ruby-core:100362] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries
  2020-10-10 13:29 [ruby-core:100357] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries eregontp
  2020-10-10 18:24 ` [ruby-core:100360] " merch-redmine
@ 2020-10-11  2:43 ` nobu
  2020-10-11 10:23 ` [ruby-core:100370] " eregontp
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: nobu @ 2020-10-11  2:43 UTC (permalink / raw)
  To: ruby-core

Issue #17259 has been updated by nobu (Nobuyoshi Nakada).


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

----------------------------------------
Bug #17259: Kernel#warn should ignore <internal: entries
https://bugs.ruby-lang.org/issues/17259#change-87973

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Target version: 3.0
* ruby -v: ruby 3.0.0preview1 (2020-09-25 master 0096d2b895) [x86_64-linux]
* Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN
----------------------------------------
`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](https://github.com/oracle/truffleruby/blob/c3bcccfd8db7d460e23f0371e7ceaf5fdb71275c/src/main/ruby/truffleruby/core/kernel.rb#L656).

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!



-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [ruby-core:100370] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries
  2020-10-10 13:29 [ruby-core:100357] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries eregontp
  2020-10-10 18:24 ` [ruby-core:100360] " merch-redmine
  2020-10-11  2:43 ` [ruby-core:100362] " nobu
@ 2020-10-11 10:23 ` eregontp
  2020-10-13 18:21 ` [ruby-core:100397] [Ruby master Feature#17259] " eregontp
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: eregontp @ 2020-10-11 10:23 UTC (permalink / raw)
  To: ruby-core

Issue #17259 has been updated by Eregon (Benoit Daloze).


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:)`.

----------------------------------------
Bug #17259: Kernel#warn should ignore <internal: entries
https://bugs.ruby-lang.org/issues/17259#change-87981

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Target version: 3.0
* ruby -v: ruby 3.0.0preview1 (2020-09-25 master 0096d2b895) [x86_64-linux]
* Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN
----------------------------------------
`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](https://github.com/oracle/truffleruby/blob/c3bcccfd8db7d460e23f0371e7ceaf5fdb71275c/src/main/ruby/truffleruby/core/kernel.rb#L656).

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!



-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [ruby-core:100397] [Ruby master Feature#17259] Kernel#warn should ignore <internal: entries
  2020-10-10 13:29 [ruby-core:100357] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries eregontp
                   ` (2 preceding siblings ...)
  2020-10-11 10:23 ` [ruby-core:100370] " eregontp
@ 2020-10-13 18:21 ` eregontp
  2020-10-26  5:27 ` [ruby-core:100553] " matz
  2020-11-20 18:27 ` [ruby-core:100987] " eregontp
  5 siblings, 0 replies; 7+ messages in thread
From: eregontp @ 2020-10-13 18:21 UTC (permalink / raw)
  To: ruby-core

Issue #17259 has been updated by Eregon (Benoit Daloze).


PR: https://github.com/ruby/ruby/pull/3647

----------------------------------------
Feature #17259: Kernel#warn should ignore <internal: entries
https://bugs.ruby-lang.org/issues/17259#change-88010

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Target version: 3.0
----------------------------------------
`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](https://github.com/oracle/truffleruby/blob/c3bcccfd8db7d460e23f0371e7ceaf5fdb71275c/src/main/ruby/truffleruby/core/kernel.rb#L656).

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!



-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [ruby-core:100553] [Ruby master Feature#17259] Kernel#warn should ignore <internal: entries
  2020-10-10 13:29 [ruby-core:100357] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries eregontp
                   ` (3 preceding siblings ...)
  2020-10-13 18:21 ` [ruby-core:100397] [Ruby master Feature#17259] " eregontp
@ 2020-10-26  5:27 ` matz
  2020-11-20 18:27 ` [ruby-core:100987] " eregontp
  5 siblings, 0 replies; 7+ messages in thread
From: matz @ 2020-10-26  5:27 UTC (permalink / raw)
  To: ruby-core

Issue #17259 has been updated by matz (Yukihiro Matsumoto).


Accepted.

Matz.


----------------------------------------
Feature #17259: Kernel#warn should ignore <internal: entries
https://bugs.ruby-lang.org/issues/17259#change-88181

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Target version: 3.0
----------------------------------------
`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](https://github.com/oracle/truffleruby/blob/c3bcccfd8db7d460e23f0371e7ceaf5fdb71275c/src/main/ruby/truffleruby/core/kernel.rb#L656).

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!



-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [ruby-core:100987] [Ruby master Feature#17259] Kernel#warn should ignore <internal: entries
  2020-10-10 13:29 [ruby-core:100357] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries eregontp
                   ` (4 preceding siblings ...)
  2020-10-26  5:27 ` [ruby-core:100553] " matz
@ 2020-11-20 18:27 ` eregontp
  5 siblings, 0 replies; 7+ messages in thread
From: eregontp @ 2020-11-20 18:27 UTC (permalink / raw)
  To: ruby-core

Issue #17259 has been updated by Eregon (Benoit Daloze).


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

----------------------------------------
Feature #17259: Kernel#warn should ignore <internal: entries
https://bugs.ruby-lang.org/issues/17259#change-88653

* Author: Eregon (Benoit Daloze)
* Status: Closed
* Priority: Normal
* Target version: 3.0
----------------------------------------
`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](https://github.com/oracle/truffleruby/blob/c3bcccfd8db7d460e23f0371e7ceaf5fdb71275c/src/main/ruby/truffleruby/core/kernel.rb#L656).

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!



-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-11-20 18:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-10 13:29 [ruby-core:100357] [Ruby master Bug#17259] Kernel#warn should ignore <internal: entries eregontp
2020-10-10 18:24 ` [ruby-core:100360] " merch-redmine
2020-10-11  2:43 ` [ruby-core:100362] " nobu
2020-10-11 10:23 ` [ruby-core:100370] " eregontp
2020-10-13 18:21 ` [ruby-core:100397] [Ruby master Feature#17259] " eregontp
2020-10-26  5:27 ` [ruby-core:100553] " matz
2020-11-20 18:27 ` [ruby-core:100987] " eregontp

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).