Project

General

Profile

Actions

Bug #17388

closed

Doesn't Warning.warn support `category: :experimental` in Ruby 3.0.0?

Added by jnchito (Junichi Ito) almost 4 years ago. Updated almost 4 years ago.

Status:
Closed
Assignee:
-
Target version:
-
ruby -v:
ruby 3.0.0preview2 (2020-12-08 master d7a16670c3) [x86_64-darwin20]
[ruby-core:101427]

Description

I wrote the following script:

module Warning
  def self.warn(message, category: nil)
    puts "category=#{category}"
    super
  end
end
lambda(&:foo).lambda?
Ractor.new{}

Then, run it:

$ ruby -w ~/Desktop/test.rb
category=deprecated
/Users/jnito/Desktop/test.rb:7: warning: lambda without a literal block is deprecated; use the proc without lambda instead
category=
<internal:ractor>:38: warning: Ractor is experimental, and the behavior may change in future versions of Ruby! Also there are many implementation issues.

I got category=deprecated as I expected, but got nil instead of :experimental. I feel it should be :experimental.

This issue is related to https://bugs.ruby-lang.org/issues/17122


Files

experimental.diff (5.2 KB) experimental.diff Patch for experimental warning category support jeremyevans0 (Jeremy Evans), 12/14/2020 08:34 PM

Updated by zverok (Victor Shepelev) almost 4 years ago

In addition:

warn('foo', category: :deprecated) # works
warn('foo', category: :experimental)
# ArgumentError (invalid warning category used: experimental)

It all comes down to this commit: https://github.com/ruby/ruby/commit/346301e2329c46362a6089311d0a64b8734b35ec
Stating

The only currently defined warning category is :deprecated, since that is what is already used. More categories can be added in later commits.

TBH, I am not sure why this method needs to use a separate hash, instead of supporting all the categories Warning[] supports?..

@jeremyevans0 (Jeremy Evans), can you please explain?

Updated by jeremyevans0 (Jeremy Evans) almost 4 years ago

zverok (Victor Shepelev) wrote in #note-1:

In addition:

warn('foo', category: :deprecated) # works
warn('foo', category: :experimental)
# ArgumentError (invalid warning category used: experimental)

It all comes down to this commit: https://github.com/ruby/ruby/commit/346301e2329c46362a6089311d0a64b8734b35ec
Stating

The only currently defined warning category is :deprecated, since that is what is already used. More categories can be added in later commits.

TBH, I am not sure why this method needs to use a separate hash, instead of supporting all the categories Warning[] supports?..

@jeremyevans0 (Jeremy Evans), can you please explain?

Warning.[] does not use a hash, it uses an if/else construction. So there wasn't an existing hash I could use, I had to create a new hash.

I'm not opposed to supporting an :experimental category. It's not used internally though. There are only three internal callers that respect the Warning[:experimental] = false, and none of them call the warning method in that case. When they do call the warning method, they don't use a category. If we are going to add support for an :experimental category, we should probably change the cases that respect Warning[:experimental] = false to use the :experimental category when emitting the warning. That's more complex than you probably expect, see the attached patch (no tests yet, and not sure about the ripper support).

Considering the current state of category support in Warning.warn, I would be in favor of removing category support completely in Ruby 3.0 and not adding similar support back until we have a better idea. @nobu (Nobuyoshi Nakada) mentioned that warnings should probably support multiple tags instead of a single category, and that sounds better to me. I feel that a :category keyword that only supports 1 or 2 categories is of so little practical use that it isn't worth supporting.

Updated by jnchito (Junichi Ito) almost 4 years ago

jeremyevans0 (Jeremy Evans) wrote in #note-2:

If we are going to add support for an :experimental category, we should probably change the cases that respect Warning[:experimental] = false to use the :experimental category when emitting the warning. That's more complex than you probably expect, see the attached patch (no tests yet, and not sure about the ripper support).

Thank you for your comment. IMO, it would be enough if the current implementation is documented (like "currently supported category is :deprecated only.")

Updated by nobu (Nobuyoshi Nakada) almost 4 years ago

BTW, rb_category_warn and rb_category_warning take the category as a string instead of rb_warning_category_t?

I was going to move it under "include/" when exposing functions that use it...

Actions #5

Updated by nobu (Nobuyoshi Nakada) almost 4 years ago

  • Status changed from Open to Closed
  • Backport changed from 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN to 2.5: DONTNEED, 2.6: DONTNEED, 2.7: DONTNEED
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0