-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Activating refinements in binding.eval doesn't stick outside of the evaluated string. #4475
Comments
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? |
Have a library which exposes a bunch of refinements, that users can add in one go without writing 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 |
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). |
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 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. |
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 As a side effect some unintuitive things happen. I wrote a I ran some quick tests, and it seems that jruby is conform mri with other side effects created by This is the phrase I didn't understand:
Since refines only apply to the current source file and not to files |
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 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. |
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 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. |
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 :(
I will update my code, because that is a superior solution than the |
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 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. |
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 |
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/LinuxExpected Behavior
Actual Behavior
In JRuby both uses of #first= will fail, where in MRI both work. See the outcome of this issue.
The text was updated successfully, but these errors were encountered: