Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Line number in runtime warnings is one greater than the actual line number #1446

Closed
DavidEGrayson opened this issue Jan 26, 2014 · 5 comments
Milestone

Comments

@DavidEGrayson
Copy link
Contributor

I have been trying to fix the warnings that happen when I run JRuby tests and I noticed that when JRuby prints a typical runtime warning it always reports the wrong line number. The reported line number is always one greater than the actual line number.

Here is an example of the bad behavior:

$ jruby -v -e 'A=1;A=2;puts'
jruby 1.7.10 (1.9.3p392) 2014-01-09 c4ecd6b on Java HotSpot(TM) 64-Bit Server VM 1.7.0_45-b18 [Windows 8-amd64]
-e:2 warning: already initialized constant A

In the example above, everything is on line 1, but the warning indicates line 2. I would expect JRuby to print "-e:1" but instead it prints "-e:2".

In contrast, the warning from MRI reports indicates line 1:

$ ruby -v -e 'A=1;A=2;puts'
ruby 2.0.0p247 (2013-06-27 revision 41674) [x86_64-linux]
-e:1: warning: already initialized constant A
-e:1: warning: previous definition of A was here

The reason I call puts in these examples is to make it totally clear that the code that produces the warning is entirely contained within line 1.

I think this bug will make it harder to debug JRuby warnings, especially for new programmers. Time will be wasted looking at the wrong line.

@eldritchideen
Copy link
Contributor

The warning line numbers do seem to be off by one. I put your example into a simple script with each expression on its own line, to see what you happen with JRuby and MRI:

$ cat foo.rb 
A=1
A=2
puts 'abc'

JRuby

$ bin/jruby -v foo.rb 
jruby 9000.dev (2.1.0.dev) 2014-01-25 7a31a1c on Java HotSpot(TM) 64-Bit Server VM 1.7.0_45-b18 [darwin-x86_64]
foo.rb:3 warning: already initialized constant A
abc

2.1.0 MRI

$ ruby -v foo.rb 
ruby 2.1.0p0 (2013-12-25 revision 44422) [x86_64-darwin12.0]
foo.rb:2: warning: already initialized constant A
foo.rb:1: warning: previous definition of A was here
abc

It seems that JRuby's warning line number is one greater than it should be.

A very simple, but probably quite dodgy solution would be below.

diff --git a/core/src/main/java/org/jruby/common/RubyWarnings.java b/core/src/main/java/org/jruby/common/RubyWarnings.java
index bb18271..689fc07 100644
--- a/core/src/main/java/org/jruby/common/RubyWarnings.java
+++ b/core/src/main/java/org/jruby/common/RubyWarnings.java
@@ -97,7 +97,7 @@ public class RubyWarnings implements IRubyWarnings, WarnCallback {
             line = -1;
         } else {
             file = stack[0].getFileName();
-            line = stack[0].getLineNumber();
+            line = stack[0].getLineNumber() - 1;
         }

         warn(id, file, line, message);

Would need to look further into how the stack trace is built up behind the scenes here, rather than just adjust the line number back down by 1 where it is reported like the above snippet does.

DavidEGrayson added a commit to DavidEGrayson/jruby that referenced this issue Jan 27, 2014
…warnings is one greater than the actual line number.
DavidEGrayson added a commit to DavidEGrayson/jruby that referenced this issue Jan 27, 2014
@DavidEGrayson
Copy link
Contributor Author

Hello @eldritchideen. Actually the stack trace was generated properly and the line number was totally fine up until the moment it got converted into a string. I fixed the issue in pull request #1450.

@enebo
Copy link
Member

enebo commented Jan 27, 2014

Closing since I think PR #1450 fixed this. Re-open if I am mistaken.

@enebo enebo closed this as completed Jan 27, 2014
@DavidEGrayson
Copy link
Contributor Author

@enebo, you are correct. Thanks!

DavidEGrayson added a commit to DavidEGrayson/jruby that referenced this issue Jan 28, 2014
@chrisseaton
Copy link
Contributor

There was still a couple of failing tests for line numbers. I've fixed them now a7c82e9. The build is still failing but I don't think it has anything to do with line numbers any more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants