Project

General

Profile

Actions

Feature #12944

closed

Change Kernel#warn to call Warning.warn

Added by jeremyevans0 (Jeremy Evans) over 7 years ago. Updated almost 7 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:78156]

Description

Currently, Kernel#warn is basically the same as $stderr.puts. With the new Warning.warn support in ruby 2.4, it makes sense for Kernel#warn to call Warning.warn, otherwise you will not be able to use Warning.warn to filter/process warning messages generated by Kernel#warn.

The Kernel#warn API is different than the Warning.warn API, the attached patch tries to get similar behavior, but there are probably corner cases where the behavior is different.


Files

0001-Change-Kernel-warn-to-call-Warning.warn.patch (1.88 KB) 0001-Change-Kernel-warn-to-call-Warning.warn.patch jeremyevans0 (Jeremy Evans), 11/15/2016 08:13 PM
0001-Change-Kernel-warn-to-call-Warning.warn.patch (1.89 KB) 0001-Change-Kernel-warn-to-call-Warning.warn.patch jeremyevans0 (Jeremy Evans), 11/26/2016 03:55 AM
0001-Change-Kernel-warn-to-call-Warning.warn.patch (2.36 KB) 0001-Change-Kernel-warn-to-call-Warning.warn.patch jeremyevans0 (Jeremy Evans), 11/26/2016 07:46 AM
0001-Change-Kernel-warn-to-call-Warning.warn.patch (2.3 KB) 0001-Change-Kernel-warn-to-call-Warning.warn.patch Use rb_ary_join jeremyevans0 (Jeremy Evans), 12/13/2016 04:23 PM
0001-Change-Kernel-warn-to-call-Warning.warn.patch (3.23 KB) 0001-Change-Kernel-warn-to-call-Warning.warn.patch flatten array, append newlines to each line, then join jeremyevans0 (Jeremy Evans), 12/14/2016 03:40 PM
0001-Restore-behavior-of-Kernel-warn-accepting-arrays-arg.patch (1.23 KB) 0001-Restore-behavior-of-Kernel-warn-accepting-arrays-arg.patch Eregon (Benoit Daloze), 04/25/2017 12:06 PM

Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #14006: 2.5.0preview1でWarning.warnを再定義するとSystemStackErrorが発生する ClosedActions

Updated by matz (Yukihiro Matsumoto) over 7 years ago

Accepted.

Matz.

Updated by nobu (Nobuyoshi Nakada) over 7 years ago

rb_str_cat modifies the argument, it is unexpected to Kernel#warn, I think.
Also it doesn't work on a frozen string.

Updated by jeremyevans0 (Jeremy Evans) over 7 years ago

Here's an updated patch that uses rb_str_dup before calling rb_str_cat, so it doesn't modify a passed in string, and works with frozen strings.

Updated by nobu (Nobuyoshi Nakada) over 7 years ago

You can't compare and append the last byte simply in wide character encodings.
E.g., str_end_with_asciichar in io.c.

Updated by jeremyevans0 (Jeremy Evans) over 7 years ago

Here's an updated patch that uses str_end_with_asciichar instead of just manually checking the last byte for \n.

Updated by jeremyevans0 (Jeremy Evans) over 7 years ago

This didn't make 2.4.0rc1, but as matz has already accepted it, it should probably be committed before 2.4.0 final. If further changes are needed to the patch, please let me know.

Updated by naruse (Yui NARUSE) over 7 years ago

Jeremy Evans wrote:

This didn't make 2.4.0rc1, but as matz has already accepted it, it should probably be committed before 2.4.0 final. If further changes are needed to the patch, please let me know.

OK.
nobu, could you handle this?

Updated by naruse (Yui NARUSE) over 7 years ago

  • Target version set to 2.4

Updated by nobu (Nobuyoshi Nakada) over 7 years ago

One thing to be confirmed, this patch lets warn ["foo", "bar"] print one line while two lines are printed now.
Is this OK?

Updated by jeremyevans0 (Jeremy Evans) over 7 years ago

The attached patch uses rb_ary_join to create the string to pass to Warning.warn, which should do a better job of handling corner cases such as warn ["foo", "bar"].

Updated by nobu (Nobuyoshi Nakada) over 7 years ago

Seems warn ["foo\n", "bar"] would print "foo\n\nbar\n".

Updated by jeremyevans0 (Jeremy Evans) over 7 years ago

In order to handle embedded arrays (similar to how puts handles them), the attached patch flattens the array, processes each string individually making sure it ends in a newline, then joins the array with the empty string.

Updated by jeremyevans0 (Jeremy Evans) over 7 years ago

Since the ruby_2_4 branch will be created on Friday, I think this patch needs to be merged by then. If additional changes are needed, please let me know.

Updated by hsbt (Hiroshi SHIBATA) over 7 years ago

We will discuss incompatible changes and feature details at https://bugs.ruby-lang.org/projects/ruby/wiki/DevelopersMeeting20161221Japan with Matz

Updated by shyouhei (Shyouhei Urabe) over 7 years ago

So we discussed at today's developer meeting. Attendees felt in common that Kernel#warn's complicated pitfall-ish behaviour is not useful very much. Does anybody actually pass array to it? Warning.warn is much straight-forward. It makes relatively less sense to emulate Kernel#warn's old corner cases using Warning.warn.

We want to re-think Kernel#warn API. In order to do so 2.4 is too immediate. Let us leave it as is now. Don't get me wrong; I basically think this feature is a good one to have. Just a bit bigger than it was thought to be.

Updated by naruse (Yui NARUSE) over 7 years ago

  • Target version deleted (2.4)

Updated by matz (Yukihiro Matsumoto) about 7 years ago

Go ahead. I want to check compatibility issues early.

Matz.

Actions #18

Updated by nobu (Nobuyoshi Nakada) about 7 years ago

  • Status changed from Open to Closed

Applied in changeset r57370.


Change Kernel#warn to call Warning.warn

This allows Warning.warn to filter/process warning messages
generated by Kernel#warn. Currently, Warning.warn can only handle
messages generated by the rb_warn/rb_warning C functions.

The Kernel#warn API is different than the Warning.warn API, this
tries to get similar behavior, but there are probably corner cases
where the behavior is different.

This makes str_end_with_asciichar in io.c no longer static so it
can be called from error.c.

[Feature #12944]
Author: Jeremy Evans

Updated by Eregon (Benoit Daloze) almost 7 years ago

It seems it is a compatibility issue.
For instance Thread.exclusive warning is now printed incorrectly:

$ ruby -ve 'def m; Thread.exclusive {}; end; m'
ruby 2.4.0p0 (2016-12-24 revision 57164) [x86_64-linux]
Thread.exclusive is deprecated, use Thread::Mutex
-e:1:in `m'
-e:1:in `<main>'

$ ruby -ve 'def m; Thread.exclusive {}; end; m'
ruby 2.5.0dev (2017-04-24 trunk 58466) [x86_64-linux]
Thread.exclusive is deprecated, use Thread::Mutex
["-e:1:in `m'", "-e:1:in `<main>'"]

The fix is easy:

   def self.exclusive
-    warn "Thread.exclusive is deprecated, use Thread::Mutex", caller
+    warn "Thread.exclusive is deprecated, use Thread::Mutex", *caller
     MUTEX_FOR_THREAD_EXCLUSIVE.synchronize{

But this is not the only usage of warn + caller.
https://github.com/search?l=Ruby&p=2&q=warn+caller&type=Code&utf8=%E2%9C%93
I think it would be best to remain compatible and join("\n") #warn's arguments.

Updated by Eregon (Benoit Daloze) almost 7 years ago

Also, similar to #13505, I think a single warning String (containing newlines) should be sent to Warning.warn.
Otherwise, warn "my warning", caller will appear as two different calls to Warning.warn and the second call cannot be filtered reliably without the necessary "my warning" context.

The original code also did a single rb_io_puts call so I think it makes most sense to make a single call to Warning.warn with the joined arguments.

Updated by Eregon (Benoit Daloze) almost 7 years ago

I attach a patch based on Jeremy "Use rb_ary_join" patch, rebased to current trunk.

I think using args.join("\n") is a good way to restore the needed compatibility as shown above and yet simple enough.
It's also what I intuitively used when redefining Kernel#warn in Ruby.
It fixes the wrong output for the Thread.exclusive warning, allows ruby/spec to no longer have different tests for 2.5's #warn and sends a single String to Warning.warn for proper filtering.

Can I commit this patch?

Updated by nobu (Nobuyoshi Nakada) almost 7 years ago

Nitpicking an edge case, these two are different.

puts "foo", "" #=> "foo\n" and "\n"
puts ["foo", ""].join("\n") #=> "foo\n"

Updated by Eregon (Benoit Daloze) almost 7 years ago

nobu (Nobuyoshi Nakada) wrote:

Nitpicking an edge case, these two are different.

I see, do you think it is important?
I think the second output is actually more natural for a warning.

r58487 is not enough, it calls Warning#write and not Warning.warn:

ruby -ve 'def Warning.warn(msg); p msg; end; def m; Thread.exclusive {}; end; m'

should send to Warning.warn so it can be filtered.
I also think Warning#write should not be defined.

The entire warning should also be sent as a single String, so for instance I can filter based on where the warning was emitted:

def Warning.warn(message)
  case message
  when /Thread\.exclusive is deprecated.+\n.+thread\/exclusive_spec\.rb/
    # ignore
  else
    super(message)
  end
end

Updated by Eregon (Benoit Daloze) almost 7 years ago

Thank you nobu for fixing this, r58490 solves all my concerns for Kernel#warn.

Actions #25

Updated by wanabe (_ wanabe) over 6 years ago

  • Related to Bug #14006: 2.5.0preview1でWarning.warnを再定義するとSystemStackErrorが発生する added
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0