Activating refinements in binding.eval doesn't stick outside of the evaluated string. #4475

Closed
najamelan opened this Issue Feb 4, 2017 · 10 comments

Comments

Projects
None yet
2 participants
@najamelan

Environment

jruby -v : jruby 9.1.7.0 (2.3.1) 2017-01-11 68056ae OpenJDK 64-Bit Server VM 25.121-b13 on 1.8.0_121-8u121-b13-2-b13 +jit [linux-x86_64]
uname -a : Linux computer 4.9.0-1-amd64 #1 SMP Debian 4.9.2-2 (2017-01-12) x86_64 GNU/Linux

Expected Behavior

module Refine

	refine ::Array do

		def first= value

			self[ 0 ] = value

		end

	end

end

binding.eval 'using Refine'
binding.eval '[].first = 4'

p = [].first = 'foo'

puts p.inspect

Actual Behavior

In JRuby both uses of #first= will fail, where in MRI both work. See the outcome of this issue.

NoMethodError: undefined method `first=' for []:Array
Did you mean?  first
  <main> at test.rb:18
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 8, 2017

Member

The second behavior here is likely just an odd side effect of the eval's binding and the surrounding scope being the same. It fails for the same reason that the first behavior fails.

May I ask what purpose this behavior serves? Why would you ever want to do this?

Member

headius commented Feb 8, 2017

The second behavior here is likely just an odd side effect of the eval's binding and the surrounding scope being the same. It fails for the same reason that the first behavior fails.

May I ask what purpose this behavior serves? Why would you ever want to do this?

@najamelan

This comment has been minimized.

Show comment
Hide comment
@najamelan

najamelan Feb 8, 2017

Have a library which exposes a bunch of refinements, that users can add in one go without writing using lines for every one of them on top of every single source file that uses them with:

Susu.refine binding

# or

Susu:refine binding, [ :Array, :Hash ]

For me refinements are pretty useless if I can't do this because of the high quantity of boilerplate. I will probably even further split them up to separate refinements that change the behaviour of existing standard ruby methods and refinements that only add new methods.

REFINES =
{
	Array:   'using Susu::Refine::Array'   ,
	Date:    'using Susu::Refine::Date'    ,
	Hash:    'using Susu::Refine::Hash'    ,
	Module:  'using Susu::Refine::Module'  ,
	Numeric: 'using Susu::Refine::Numeric' ,
	String:  'using Susu::Refine::String'  ,
	Time:    'using Susu::Refine::Time'    ,

	Fs:      'using Susu::Fs::Refine'      ,
	Options: 'using Susu::Options::Refine' ,
}



def self.refine context, which = :all

	which.kind_of?( Array ) or which = [ which ]

	which.include?( :all ) and return context.eval( REFINES.values.join( "\n" ) )

	strings = REFINES.select do |key, value|

		which.include?( key )

	end.values.join( "\n" )

	context.eval strings

end

Have a library which exposes a bunch of refinements, that users can add in one go without writing using lines for every one of them on top of every single source file that uses them with:

Susu.refine binding

# or

Susu:refine binding, [ :Array, :Hash ]

For me refinements are pretty useless if I can't do this because of the high quantity of boilerplate. I will probably even further split them up to separate refinements that change the behaviour of existing standard ruby methods and refinements that only add new methods.

REFINES =
{
	Array:   'using Susu::Refine::Array'   ,
	Date:    'using Susu::Refine::Date'    ,
	Hash:    'using Susu::Refine::Hash'    ,
	Module:  'using Susu::Refine::Module'  ,
	Numeric: 'using Susu::Refine::Numeric' ,
	String:  'using Susu::Refine::String'  ,
	Time:    'using Susu::Refine::Time'    ,

	Fs:      'using Susu::Fs::Refine'      ,
	Options: 'using Susu::Options::Refine' ,
}



def self.refine context, which = :all

	which.kind_of?( Array ) or which = [ which ]

	which.include?( :all ) and return context.eval( REFINES.values.join( "\n" ) )

	strings = REFINES.select do |key, value|

		which.include?( key )

	end.values.join( "\n" )

	context.eval strings

end
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 9, 2017

Member

Pushed a fix in #4485 that carries the old scope along with the eval so it sees the activated refinements. Still need some clean tests for this behavior and some evaluation of the performance hit to eval (which may be meaningless, since it's eval).

Member

headius commented Feb 9, 2017

Pushed a fix in #4485 that carries the old scope along with the eval so it sees the activated refinements. Still need some clean tests for this behavior and some evaluation of the performance hit to eval (which may be meaningless, since it's eval).

@headius headius added this to the JRuby 9.1.8.0 milestone Feb 9, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 9, 2017

Member

Your library is an interesting one, but doesn't this mean you're always evaluating code? Evals will be slow in just about every implementation.

We also will not make all eval'ed refines apply to top-level scripts, because that's improper according to spec. The only change we will support here is that a refinement activated in a given binding will be visible to subsequent newly-parsed code in that same binding.

Of course, if that binding is the top-level binding, it may work for files loaded after the eval of using.

Can you write up some tests that illustrate all the cases you expect to work? I'm not clear how far you expect this fix to go.

Member

headius commented Feb 9, 2017

Your library is an interesting one, but doesn't this mean you're always evaluating code? Evals will be slow in just about every implementation.

We also will not make all eval'ed refines apply to top-level scripts, because that's improper according to spec. The only change we will support here is that a refinement activated in a given binding will be visible to subsequent newly-parsed code in that same binding.

Of course, if that binding is the top-level binding, it may work for files loaded after the eval of using.

Can you write up some tests that illustrate all the cases you expect to work? I'm not clear how far you expect this fix to go.

@headius headius closed this in 848bf66 Feb 9, 2017

@najamelan

This comment has been minimized.

Show comment
Hide comment
@najamelan

najamelan Feb 9, 2017

Thanks for replying.

I don't know if I understood everything your wrote. Since this is all still under development, I haven't looked into the performance of it. I wasn't aware that eval was slow. I consider that a slight performance hit in loading files is relatively unimportant, since it only happens once on application startup. I can't image this being more than a few ms per eval, but I haven't checked that. It is also just a convenience method that can be omitted by anyone who would find it to slow, it is still possible to write those 'using' lines inline.

As far as the functionality goes, as far as I understand it, running binding.eval is identical to writing the code directly in the source file (<- after testing this seems to be incorrect, at least what concerns local variables). As such, for refines, they only apply to the current source file. Consequently I have Susu.refine binding on top of every ruby file I write to get the convenience I like available without polluting the standard ruby classes, which might introduce bugs in other code.

As a side effect some unintuitive things happen. I wrote a <=> operator that allows Time and Date objects to be compared, but that won't be in effect for array.sort, since refinements are always local to the current source file or module. That's the price for keeping ones footprint clean in ruby.

I ran some quick tests, and it seems that jruby is conform mri with other side effects created by binding.eval such as introducing new constants or global variables. Just when calling using the effect differs. On how to test this, the above code is as good as any I could come up with. Since refines have specific rules about applying to a source file, I'm not sure if you can test it from within a test method. You might have to put some code in a file and call the interpreter on it.

This is the phrase I didn't understand:

Of course, if that binding is the top-level binding, it may work for files loaded after the eval of using.

Since refines only apply to the current source file and not to files required from it.

najamelan commented Feb 9, 2017

Thanks for replying.

I don't know if I understood everything your wrote. Since this is all still under development, I haven't looked into the performance of it. I wasn't aware that eval was slow. I consider that a slight performance hit in loading files is relatively unimportant, since it only happens once on application startup. I can't image this being more than a few ms per eval, but I haven't checked that. It is also just a convenience method that can be omitted by anyone who would find it to slow, it is still possible to write those 'using' lines inline.

As far as the functionality goes, as far as I understand it, running binding.eval is identical to writing the code directly in the source file (<- after testing this seems to be incorrect, at least what concerns local variables). As such, for refines, they only apply to the current source file. Consequently I have Susu.refine binding on top of every ruby file I write to get the convenience I like available without polluting the standard ruby classes, which might introduce bugs in other code.

As a side effect some unintuitive things happen. I wrote a <=> operator that allows Time and Date objects to be compared, but that won't be in effect for array.sort, since refinements are always local to the current source file or module. That's the price for keeping ones footprint clean in ruby.

I ran some quick tests, and it seems that jruby is conform mri with other side effects created by binding.eval such as introducing new constants or global variables. Just when calling using the effect differs. On how to test this, the above code is as good as any I could come up with. Since refines have specific rules about applying to a source file, I'm not sure if you can test it from within a test method. You might have to put some code in a file and call the interpreter on it.

This is the phrase I didn't understand:

Of course, if that binding is the top-level binding, it may work for files loaded after the eval of using.

Since refines only apply to the current source file and not to files required from it.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 9, 2017

Member

So a bit of explanation is in order...

Refinements apply to a lexical scope, which means normally they should only be triggered within a given body of code (e.g. a module body). They are stored in a structure that is only live as long as that body of code is alive, and they only affect code coming later in the script.

In JRuby, and to some extent in MRI, the "using" call is used to indicate that we may have refinements active for subsequent calls. This is to allow limiting refinements to only code that follows the using call, while avoiding effects before the call. It's also to avoid making every single call in the system check for refinements, which would be necessary if "using" could be called in other ways against some body of code. That's essentially what you're doing here.

Your code is triggering refinements to be used in a very roundabout way that's not officially supported by either JRuby or MRI...but it accidentally works because "eval" is treated as a completely new parse of code. By using in one eval, JRuby sets a bit indicating future calls should use refinements, and so the later evals see those refinements. It doesn't work at the top-level of a script because that code has already been parsed with no using call present, so the calls do not attempt to use refinements.

There may be ways to improve this, such as having all calls check exactly once for refinements, so you can set the refined bit programmatically. However, there's serious concurrency concerns here: if a thread doing your eval'ed "using" runs at the same time another thread attempts to execute a possibly-refined call, the results will be unpredictable.

Now, getting back to eval...

eval is most definitely not just pretending that code exists at that place in the code. As you've mentioned, variable scoping, constant assignment, and other items behave differently. In addition, eval requires the code to be parsed at runtime rather than at boot time, so doing evals in hot code is strongly discouraged on all implementations.

eval is a bit of a red herring here. What you're doing is essentially moving the "using" call to a completely different scope and then expecting it to work in the original scope. This is in addition to making the "using" call in a way that our parser can't see, so we don't treat calls as refined.

@enebo may have some thoughts here, but I think you're pushing eval and refinements a lot farther than they're designed to go. The JRuby and MRI teams agreed many years ago that refinements should be local to a given lexical scope, and you're trying to do something that disagrees with that.

Thankfully, I think there's a better way! I believe it should be possible for you to create a single refinement that aggregates other refinements. I'm not sure what that would look like, but the following code works for me:

module A
  refine String do
    def foo; puts :foo; end
  end
end

module B
  refine String do
    def bar; puts :bar; end
  end
end

module C
  include A, B
end

using C

"blah".foo
"blah".bar

I think what you want is to simply create a Susu module that includes the refinements you'd like to use, and then "using Susu" will work properly.

Member

headius commented Feb 9, 2017

So a bit of explanation is in order...

Refinements apply to a lexical scope, which means normally they should only be triggered within a given body of code (e.g. a module body). They are stored in a structure that is only live as long as that body of code is alive, and they only affect code coming later in the script.

In JRuby, and to some extent in MRI, the "using" call is used to indicate that we may have refinements active for subsequent calls. This is to allow limiting refinements to only code that follows the using call, while avoiding effects before the call. It's also to avoid making every single call in the system check for refinements, which would be necessary if "using" could be called in other ways against some body of code. That's essentially what you're doing here.

Your code is triggering refinements to be used in a very roundabout way that's not officially supported by either JRuby or MRI...but it accidentally works because "eval" is treated as a completely new parse of code. By using in one eval, JRuby sets a bit indicating future calls should use refinements, and so the later evals see those refinements. It doesn't work at the top-level of a script because that code has already been parsed with no using call present, so the calls do not attempt to use refinements.

There may be ways to improve this, such as having all calls check exactly once for refinements, so you can set the refined bit programmatically. However, there's serious concurrency concerns here: if a thread doing your eval'ed "using" runs at the same time another thread attempts to execute a possibly-refined call, the results will be unpredictable.

Now, getting back to eval...

eval is most definitely not just pretending that code exists at that place in the code. As you've mentioned, variable scoping, constant assignment, and other items behave differently. In addition, eval requires the code to be parsed at runtime rather than at boot time, so doing evals in hot code is strongly discouraged on all implementations.

eval is a bit of a red herring here. What you're doing is essentially moving the "using" call to a completely different scope and then expecting it to work in the original scope. This is in addition to making the "using" call in a way that our parser can't see, so we don't treat calls as refined.

@enebo may have some thoughts here, but I think you're pushing eval and refinements a lot farther than they're designed to go. The JRuby and MRI teams agreed many years ago that refinements should be local to a given lexical scope, and you're trying to do something that disagrees with that.

Thankfully, I think there's a better way! I believe it should be possible for you to create a single refinement that aggregates other refinements. I'm not sure what that would look like, but the following code works for me:

module A
  refine String do
    def foo; puts :foo; end
  end
end

module B
  refine String do
    def bar; puts :bar; end
  end
end

module C
  include A, B
end

using C

"blah".foo
"blah".bar

I think what you want is to simply create a Susu module that includes the refinements you'd like to use, and then "using Susu" will work properly.

@najamelan

This comment has been minimized.

Show comment
Hide comment
@najamelan

najamelan Feb 9, 2017

Thanks for looking into it. I'm sorry for the confusion. I'm clearly a ruby user and not a ruby hacker. I've never worked on any ruby implementation, so I don't grasp the consequences of this for the interpreter. I just though that I report the different behaviour between MRI and JRUBY. I hadn't found another way than eval to do this, but indeed creating a module on the fly with the right refinements in it might work. I suppose extend would be more appropriate than include.

I will try this out and report back whether that works, but I can't do that today, because it is very late where I am. I will try to find time tomorrow night.

Thanks for looking into it. I'm sorry for the confusion. I'm clearly a ruby user and not a ruby hacker. I've never worked on any ruby implementation, so I don't grasp the consequences of this for the interpreter. I just though that I report the different behaviour between MRI and JRUBY. I hadn't found another way than eval to do this, but indeed creating a module on the fly with the right refinements in it might work. I suppose extend would be more appropriate than include.

I will try this out and report back whether that works, but I can't do that today, because it is very late where I am. I will try to find time tomorrow night.

@najamelan

This comment has been minimized.

Show comment
Hide comment
@najamelan

najamelan Feb 12, 2017

I got round to trying your example and it works indeed, at least on MRI:

module A
  refine String do
    def foo; puts :foo; end
  end
end

module B
  refine String do
    def bar; puts :bar; end
  end
end

def refines( which = [ :A, :B ] )

  Array === which or which = [ which ]

  m = Object.const_set( 'Refines', Module.new )

  which.each do |mod|

    m.include( Object.const_get( mod ) )

  end

  m

end

using refines

# or using refines :A

"blah".foo
"blah".bar

On jruby it doesn't take the refine :(

$ jruby test.rb
NoMethodError: undefined method `foo' for "blah":String
  <main> at test.rb:33

I will update my code, because that is a superior solution than the eval method. Hopefully one day it will work on jruby too. Thanks for the suggestion in any case.

najamelan commented Feb 12, 2017

I got round to trying your example and it works indeed, at least on MRI:

module A
  refine String do
    def foo; puts :foo; end
  end
end

module B
  refine String do
    def bar; puts :bar; end
  end
end

def refines( which = [ :A, :B ] )

  Array === which or which = [ which ]

  m = Object.const_set( 'Refines', Module.new )

  which.each do |mod|

    m.include( Object.const_get( mod ) )

  end

  m

end

using refines

# or using refines :A

"blah".foo
"blah".bar

On jruby it doesn't take the refine :(

$ jruby test.rb
NoMethodError: undefined method `foo' for "blah":String
  <main> at test.rb:33

I will update my code, because that is a superior solution than the eval method. Hopefully one day it will work on jruby too. Thanks for the suggestion in any case.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 13, 2017

Member

Ahh, I neglected to test in JRuby. This is indeed another bug; I suspect we are not looking for refinements included into a module inside our using implementation.

Can you file a new bug for this?

Also, I find your code to be very strange. You can create an anonymous module and include those other modules like so:

m = Module.new do
  which.each { include Object.const_get(mod) }
end

I assume the setting of the "Refines" constant was just for example purposes, since it should have no effect on this code.

Member

headius commented Feb 13, 2017

Ahh, I neglected to test in JRuby. This is indeed another bug; I suspect we are not looking for refinements included into a module inside our using implementation.

Can you file a new bug for this?

Also, I find your code to be very strange. You can create an anonymous module and include those other modules like so:

m = Module.new do
  which.each { include Object.const_get(mod) }
end

I assume the setting of the "Refines" constant was just for example purposes, since it should have no effect on this code.

@najamelan

This comment has been minimized.

Show comment
Hide comment
@najamelan

najamelan Feb 13, 2017

I'll open a separate bug for this. The constant indeed does not have an impact on this snippet. I have a habit of not creating anonymous objects, since when you want to refer to the later it's rather annoying. As I liked this method over eval, I have implemented it in Susu. This is the method Susu.refines. It creates a unique name for each set of input arguments, that way all the calls to using Susu.refines get the exact same module, rather than creating a new one for every source file that is loaded. Note that this is currently on a needTesting branch which gets it's history rewritten, so the link might go dead at some point.

I'll open a separate bug for this. The constant indeed does not have an impact on this snippet. I have a habit of not creating anonymous objects, since when you want to refer to the later it's rather annoying. As I liked this method over eval, I have implemented it in Susu. This is the method Susu.refines. It creates a unique name for each set of input arguments, that way all the calls to using Susu.refines get the exact same module, rather than creating a new one for every source file that is loaded. Note that this is currently on a needTesting branch which gets it's history rewritten, so the link might go dead at some point.

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