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 control flow order with Constant ||= #4807

Closed
marcandre opened this Issue Oct 8, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@marcandre

marcandre commented Oct 8, 2017

I almost feel bad pointing out a bug that's probably irrelevant, but maybe it could be a real issue for some strange usage of const_missing.

Environment

$ ruby -v
jruby 9.1.13.0 (2.3.3) 2017-09-06 8e1c115 Java HotSpot(TM) 64-Bit Server VM 25.102-b14 on 1.8.0_102-b14 +jit [darwin-x86_64]

Expected Behavior

raise('a')::Foo ||= raise('b') # => RuntimeError: b (not ok, should be 'a')

In the expression scope::Constant ||= value, scope should be resolved first, then Constant should be looked up, and only then should value be resolved (assuming it is falsy or undefined). MRI does that, JRuby doesn't respect that order.

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Oct 8, 2017

There's actually something very odd going on. Here's a simple script showing how not only is JRuby not doing the right thing, but it's not even consistently doing the wrong thing...

::X = true
X ||= raise('a') # => ok
::X ||= raise('a') rescue puts "Should not raise but does!" # => not ok
::X ||= raise('a') rescue puts "Should not raise but still does!" # => not ok, but consistent
::X ||= true  # This, incredibly, changes something, because now...
::X ||= raise('a') # => D=this doesn't raise! Completely inconsistent behavior

marcandre commented Oct 8, 2017

There's actually something very odd going on. Here's a simple script showing how not only is JRuby not doing the right thing, but it's not even consistently doing the wrong thing...

::X = true
X ||= raise('a') # => ok
::X ||= raise('a') rescue puts "Should not raise but does!" # => not ok
::X ||= raise('a') rescue puts "Should not raise but still does!" # => not ok, but consistent
::X ||= true  # This, incredibly, changes something, because now...
::X ||= raise('a') # => D=this doesn't raise! Completely inconsistent behavior
@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Oct 8, 2017

FWIW, &&= doesn't exhibit the same inconsistent behavior, i.e. it seems to behave consistently badly...

::Y = false
Y &&= raise('a') # => ok
::Y &&= raise('a') rescue puts "Should not raise but does!" # => not ok

marcandre commented Oct 8, 2017

FWIW, &&= doesn't exhibit the same inconsistent behavior, i.e. it seems to behave consistently badly...

::Y = false
Y &&= raise('a') # => ok
::Y &&= raise('a') rescue puts "Should not raise but does!" # => not ok

marcandre added a commit to deep-cover/deep-cover that referenced this issue Oct 8, 2017

@MaxLap

This comment has been minimized.

Show comment
Hide comment
@MaxLap

MaxLap Oct 11, 2017

I think the root of the problem is not the control flow itself. It seems that when setting a constant in the "root" (starting with :: without nesting) using either &&= or ||=, the constant lookup gets screwed up:

  • Using ||= sets the constant somewhere else (no idea where)
  • Using &&= never finds an existing constant (and does an odd exception).

Note that all of the problems go away when not using ::

Clearly not set where it should be...

::OR_EQUAL2 ||= true
puts ::OR_EQUAL2  #=>Exception "uninitialized constant OR_EQUAL2"

Both uses of ::OR_EQUAL2 refer to the same (wrong) place, since the 1 is kept between both uses:

::OR_EQUAL2 = 0
puts (::OR_EQUAL2 ||= 1)   #=> 1
puts (::OR_EQUAL2 ||= 2)   #=> 1
puts ::OR_EQUAL2   #=> 0

The same problem happens when using &&=. But the different flow generates a weird exception early. (In MRI, there is no exception). The output of this example should be 1.

::AND_EQUAL2 = 0
puts (::AND_EQUAL2 &&= 1)   #=> Exception "uninitialized constant ::"

Note also how weird the exception message is. The refered constant is just ::.

All of the above was tested on:

  • jruby-head (jruby 9.2.0.0-SNAPSHOT (2.4.1) 2017-10-04 46782c4 OpenJDK 64-Bit Server VM 25.131-b11 on 1.8.0_131-8u131-b11-2ubuntu1.16.04.3-b11 +jit [linux-x86_64])
  • jruby-9.1.13.0

OS:
ubuntu 16.04
Linux max-u1604 4.4.0-96-generic #119-Ubuntu SMP Tue Sep 12 14:59:54 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

MaxLap commented Oct 11, 2017

I think the root of the problem is not the control flow itself. It seems that when setting a constant in the "root" (starting with :: without nesting) using either &&= or ||=, the constant lookup gets screwed up:

  • Using ||= sets the constant somewhere else (no idea where)
  • Using &&= never finds an existing constant (and does an odd exception).

Note that all of the problems go away when not using ::

Clearly not set where it should be...

::OR_EQUAL2 ||= true
puts ::OR_EQUAL2  #=>Exception "uninitialized constant OR_EQUAL2"

Both uses of ::OR_EQUAL2 refer to the same (wrong) place, since the 1 is kept between both uses:

::OR_EQUAL2 = 0
puts (::OR_EQUAL2 ||= 1)   #=> 1
puts (::OR_EQUAL2 ||= 2)   #=> 1
puts ::OR_EQUAL2   #=> 0

The same problem happens when using &&=. But the different flow generates a weird exception early. (In MRI, there is no exception). The output of this example should be 1.

::AND_EQUAL2 = 0
puts (::AND_EQUAL2 &&= 1)   #=> Exception "uninitialized constant ::"

Note also how weird the exception message is. The refered constant is just ::.

All of the above was tested on:

  • jruby-head (jruby 9.2.0.0-SNAPSHOT (2.4.1) 2017-10-04 46782c4 OpenJDK 64-Bit Server VM 25.131-b11 on 1.8.0_131-8u131-b11-2ubuntu1.16.04.3-b11 +jit [linux-x86_64])
  • jruby-9.1.13.0

OS:
ubuntu 16.04
Linux max-u1604 4.4.0-96-generic #119-Ubuntu SMP Tue Sep 12 14:59:54 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

marcandre added a commit to ruby/spec that referenced this issue Oct 11, 2017

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Oct 11, 2017

Ouch. There are probably two bugs here then. In particular, @MaxLap's issue doesn't show in the Foo::Bar case.

The fact that the constant isn't set is more important. I added a spec for it.

marcandre commented Oct 11, 2017

Ouch. There are probably two bugs here then. In particular, @MaxLap's issue doesn't show in the Foo::Bar case.

The fact that the constant isn't set is more important. I added a spec for it.

@MaxLap

This comment has been minimized.

Show comment
Hide comment
@MaxLap

MaxLap Oct 11, 2017

Ah, didn't see that other case. Here is some fun investigation into it.

The following should puts 'h1' and then fail. Instead, we get "h1" then "h3" then "h1" then fail.

(puts 'hi1'; raise 'thing'; puts 'h2';String)::Foo ||= puts 'hi3'

Looks like the "getting a constant" of ||= to check if the assignation is needed swallows the exception and assumes that it doesn't exist. Then, after evaluating the value, does the lookup again, without swallowing this time.

Note that even if the constant exists, the lookup is done twice:

(puts 'hi'; String)::Foo ||= 1 #=> Prints 'hi' twice
puts 'separator'
(puts 'hi'; String)::Foo ||= 1 #=> Prints 'hi' twice again

MaxLap commented Oct 11, 2017

Ah, didn't see that other case. Here is some fun investigation into it.

The following should puts 'h1' and then fail. Instead, we get "h1" then "h3" then "h1" then fail.

(puts 'hi1'; raise 'thing'; puts 'h2';String)::Foo ||= puts 'hi3'

Looks like the "getting a constant" of ||= to check if the assignation is needed swallows the exception and assumes that it doesn't exist. Then, after evaluating the value, does the lookup again, without swallowing this time.

Note that even if the constant exists, the lookup is done twice:

(puts 'hi'; String)::Foo ||= 1 #=> Prints 'hi' twice
puts 'separator'
(puts 'hi'; String)::Foo ||= 1 #=> Prints 'hi' twice again

@enebo enebo added this to the JRuby 9.1.14.0 milestone Oct 11, 2017

@enebo enebo added the ir label Oct 11, 2017

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 25, 2017

Member

I have been working on this one and it will be fixed but it opened up a wormhole and I discovered another class of bugs where lhs of colon2 (foo::B) will execute twice! Fixing this is more involved but pretty imperative that it is fixed first (all cases will be fixed though).

The originally reported issue was due to two issues in how we emitted instructions for this clause:

  1. we emit instrs to emulate what we can think of as defined? logic for figuring out if Foo exists. This logic has a catchall which swallows the first execution of raise 'a'.
  2. when we get to the ||= part of this I accidentally emitted the value (raise 'b') before our second calling of raise 'a'.

So just correct the second of those items would fix originally reported issue (and possibly the other ones) but it would be calling raise 'a' twice. If this was a user method which threw an exception but also made a db transaction you would be wondering why there were two updates instead of just one :|

Member

enebo commented Oct 25, 2017

I have been working on this one and it will be fixed but it opened up a wormhole and I discovered another class of bugs where lhs of colon2 (foo::B) will execute twice! Fixing this is more involved but pretty imperative that it is fixed first (all cases will be fixed though).

The originally reported issue was due to two issues in how we emitted instructions for this clause:

  1. we emit instrs to emulate what we can think of as defined? logic for figuring out if Foo exists. This logic has a catchall which swallows the first execution of raise 'a'.
  2. when we get to the ||= part of this I accidentally emitted the value (raise 'b') before our second calling of raise 'a'.

So just correct the second of those items would fix originally reported issue (and possibly the other ones) but it would be calling raise 'a' twice. If this was a user method which threw an exception but also made a db transaction you would be wondering why there were two updates instead of just one :|

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 27, 2017

Member

After having fixed design of ||= for constants all of that is working and it is not executing the same method twice.

I ran into the second bug which was that ::A ||= was recording '::' as the name of the constant and not 'A'. Amazingly it only happens for this single case of ::{constant} {op}=.

I still have to figure out &&= though. Getting much closer.

Member

enebo commented Oct 27, 2017

After having fixed design of ||= for constants all of that is working and it is not executing the same method twice.

I ran into the second bug which was that ::A ||= was recording '::' as the name of the constant and not 'A'. Amazingly it only happens for this single case of ::{constant} {op}=.

I still have to figure out &&= though. Getting much closer.

enebo added a commit that referenced this issue Oct 27, 2017

Fixes #4807. Wrong control flow order with Constant ||=.
More specifically this fixes:

 1. emitting rhs of ||= in wrong order
 1. not calling lhs and evaluating it twice
 1. making &&= semantics correct (we need specs for that somewhere)
 1. correcting return value emitted from ||= and &&= expression
 1. colon3 nodes in ||= or &&= in AST was giving itself name of '::'

So many things in a tiny esoteric corner of Ruby.

@enebo enebo closed this in 04c3cf4 Oct 27, 2017

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