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

Wrong line number reported in backtrace in hash creation #4154

Closed
dymaxionuk opened this Issue Sep 14, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@dymaxionuk

dymaxionuk commented Sep 14, 2016

Environment

ruby 2.3.0p0 (2015-12-25 revision 53290) [i386-mingw32]
jruby 9.1.2.0 (2.3.0) 2016-05-26 7357c8f Java HotSpot(TM) Client VM 25.31-b07 on 1.8.0_31-b13 +jit [mswin32-x86]
$ systeminfo | findstr /B /C:"OS Name" /C:"OS Version"
OS Name:                   Microsoft Windows 7 Enterprise
OS Version:                6.1.7601 Service Pack 1 Build 7601

Expected Behavior

Correct line number is reported in backtrace when raising error in function called within hash creation statement.

Example code:

$ cat rubyexample.rb
def mfunc(hash)
   hash.size
end

def a_func
   raise StandardError, "Blah"
end

mfunc(
  {
    :a => 1,
    :b => 2,
    :variab => a_func
  }
)

Actual Behavior

Output in MRI 2.3.0

$ ruby rubyexample.rb
rubyexample.rb:6:in `a_func': Blah (StandardError)
        from rubyexample.rb:13:in `<main>'

Output in JRuby

$ jruby rubyexample.rb
StandardError: Blah
  a_func at rubyexample.rb:6
   <top> at rubyexample.rb:9

Cross referencing another linenum issue: #4211

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 15, 2016

Member

If I put just the hash in a file, we report line 2, the first key/value pair. It would seem that MRI tracks the line number all the way through.

Here's IR for a simple hash with one simple key and one that errors:

block #2 (out: 3): LBL_1:-1
  0: %self := recv_self
  1:          line_num(lineNumber: 1)
  2:  %v_3 := call_0o(self<%self>, callType: VARIABLE, name: a_func, potentiallyRefined: false)
  3:  %v_4 := copy(hash<sym<>=>fix<1>,sym<>=>%v_3>)
  4:          return(%v_4)

So basically, we lose all line number information because the hash construction itself happens as a single unit, with each key/value pair evaluated before hand.

I suppose we could insert line number instructions around anything that might raise an error (all calls, for example). This case seems like the line number is not a big deal, but imagine having a giant literal hash with many keys that might error...finding the bad one would be troublesome.

Member

headius commented Sep 15, 2016

If I put just the hash in a file, we report line 2, the first key/value pair. It would seem that MRI tracks the line number all the way through.

Here's IR for a simple hash with one simple key and one that errors:

block #2 (out: 3): LBL_1:-1
  0: %self := recv_self
  1:          line_num(lineNumber: 1)
  2:  %v_3 := call_0o(self<%self>, callType: VARIABLE, name: a_func, potentiallyRefined: false)
  3:  %v_4 := copy(hash<sym<>=>fix<1>,sym<>=>%v_3>)
  4:          return(%v_4)

So basically, we lose all line number information because the hash construction itself happens as a single unit, with each key/value pair evaluated before hand.

I suppose we could insert line number instructions around anything that might raise an error (all calls, for example). This case seems like the line number is not a big deal, but imagine having a giant literal hash with many keys that might error...finding the bad one would be troublesome.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 15, 2016

Member

This is an IR item. Pulling in @enebo and @subbuss.

Member

headius commented Sep 15, 2016

This is an IR item. Pulling in @enebo and @subbuss.

@dymaxionuk

This comment has been minimized.

Show comment
Hide comment
@dymaxionuk

dymaxionuk Sep 15, 2016

Yes that's exactly our problem. We have a user DSL that centers around
defining a reasonably large hash, with various helper functions that assign
values to elements of that hash. However when there is a syntax error or a
parse error raised in the helper function, the user is clueless as to which
line is the root cause since the backtrace directs them to the first line
of the hash.

Thanks for looking into it.

On Thu, 15 Sep 2016, 22:17 Charles Oliver Nutter, notifications@github.com
wrote:

If I put just the hash in a file, we report line 2, the first key/value
pair. It would seem that MRI tracks the line number all the way through.

Here's IR for a simple hash with one simple key and one that errors:

block #2 (out: 3): LBL_1:-1
0: %self := recv_self
1: line_num(lineNumber: 1)
2: %v_3 := call_0o(self<%self>, callType: VARIABLE, name: a_func, potentiallyRefined: false)
3: %v_4 := copy(hash<sym<>=>fix<1>,sym<>=>%v_3>)
4: return(%v_4)

So basically, we lose all line number information because the hash
construction itself happens as a single unit, with each key/value pair
evaluated before hand.

I suppose we could insert line number instructions around anything that
might raise an error (all calls, for example). This case seems like the
line number is not a big deal, but imagine having a giant literal hash with
many keys that might error...finding the bad one would be troublesome.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4154 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABC5RUFVPkpQEA1xo77VcwJw618CSLlMks5qqVNSgaJpZM4J8XaF
.

Yes that's exactly our problem. We have a user DSL that centers around
defining a reasonably large hash, with various helper functions that assign
values to elements of that hash. However when there is a syntax error or a
parse error raised in the helper function, the user is clueless as to which
line is the root cause since the backtrace directs them to the first line
of the hash.

Thanks for looking into it.

On Thu, 15 Sep 2016, 22:17 Charles Oliver Nutter, notifications@github.com
wrote:

If I put just the hash in a file, we report line 2, the first key/value
pair. It would seem that MRI tracks the line number all the way through.

Here's IR for a simple hash with one simple key and one that errors:

block #2 (out: 3): LBL_1:-1
0: %self := recv_self
1: line_num(lineNumber: 1)
2: %v_3 := call_0o(self<%self>, callType: VARIABLE, name: a_func, potentiallyRefined: false)
3: %v_4 := copy(hash<sym<>=>fix<1>,sym<>=>%v_3>)
4: return(%v_4)

So basically, we lose all line number information because the hash
construction itself happens as a single unit, with each key/value pair
evaluated before hand.

I suppose we could insert line number instructions around anything that
might raise an error (all calls, for example). This case seems like the
line number is not a big deal, but imagine having a giant literal hash with
many keys that might error...finding the bad one would be troublesome.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4154 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABC5RUFVPkpQEA1xo77VcwJw618CSLlMks5qqVNSgaJpZM4J8XaF
.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 16, 2016

Member

I think we can just emit linenum out of order in this case before each temp var assignment for each expr that builds up the hash. I checked and the values/keys seem to have proper line number info in the ast. For pure-literal hashes we do not need to omit since they cannot raise by themselves.

Member

enebo commented Sep 16, 2016

I think we can just emit linenum out of order in this case before each temp var assignment for each expr that builds up the hash. I checked and the values/keys seem to have proper line number info in the ast. For pure-literal hashes we do not need to omit since they cannot raise by themselves.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 21, 2016

Member

@enebo I started looking at this a bit. It does not seem like we have a good method on Node to indicate that a construct can't raise an error. That's basically what we need here. We could do that with another override, like needsDefinitionCheck, or via a visitor that walks a subtree looking for nodes that could potentially raise an exception.

Member

headius commented Sep 21, 2016

@enebo I started looking at this a bit. It does not seem like we have a good method on Node to indicate that a construct can't raise an error. That's basically what we need here. We could do that with another override, like needsDefinitionCheck, or via a visitor that walks a subtree looking for nodes that could potentially raise an exception.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 21, 2016

Member

@headius I do not think we need to know. It is not about the node but more about the operands. For Hash is basically toggle between build and buildWithOrder depending whether we use anything beyond simple operands (e.g. no chance of side-effects). The problem I had when I looked at this is we do not mark newlines in node on hash elements. So we could pass in last elements line into buildWithOrder and emit linenum if it changes and it was an instr which assigns to a temp? It is more complicated than I thought but I think we might be able to use newline boolean on nodes when line changes and only emit in those cases when we pass right boolean to buildWithOrder. Should I run this paragraph on any longer? :)

Member

enebo commented Sep 21, 2016

@headius I do not think we need to know. It is not about the node but more about the operands. For Hash is basically toggle between build and buildWithOrder depending whether we use anything beyond simple operands (e.g. no chance of side-effects). The problem I had when I looked at this is we do not mark newlines in node on hash elements. So we could pass in last elements line into buildWithOrder and emit linenum if it changes and it was an instr which assigns to a temp? It is more complicated than I thought but I think we might be able to use newline boolean on nodes when line changes and only emit in those cases when we pass right boolean to buildWithOrder. Should I run this paragraph on any longer? :)

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 21, 2016

Member

Actually maybe we can just look for:

preserveOrder && !(value instanceof ImmutableLiteral)

and if so emit linenum instr. The main downside is this is a little to overly simplistic in that all assignments (which nearly universally can cause side-effects) might end up emitting a linenum for the same line multiple times (once per use). We could make a buildWithOrder(Node node, boolean preserveOrder, int lastLine) where we pass in previous processed key/value and only emit if the value is different (along with the condition above in buildWithOrder). Hmmm I will try quickly

Member

enebo commented Sep 21, 2016

Actually maybe we can just look for:

preserveOrder && !(value instanceof ImmutableLiteral)

and if so emit linenum instr. The main downside is this is a little to overly simplistic in that all assignments (which nearly universally can cause side-effects) might end up emitting a linenum for the same line multiple times (once per use). We could make a buildWithOrder(Node node, boolean preserveOrder, int lastLine) where we pass in previous processed key/value and only emit if the value is different (along with the condition above in buildWithOrder). Hmmm I will try quickly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment