Skip to content

Commit

Permalink
[Truffle] Eval with a supplied binding should assign new variables wi…
Browse files Browse the repository at this point in the history
…thin that binding.
  • Loading branch information
nirvdrum committed Feb 5, 2015
1 parent a4e929d commit f49688f
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 24 deletions.
2 changes: 0 additions & 2 deletions spec/truffle/tags/core/kernel/eval_tags.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
fails:Kernel#eval evaluates within the scope of the eval
fails:Kernel#eval evaluates such that consts are scoped to the class of the eval
fails:Kernel#eval allows a binding to be captured inside an eval
fails:Kernel#eval uses the same scope for local variables when given the same binding
fails:Kernel#eval sets constants at the toplevel from inside a block
fails:Kernel#eval uses the filename of the binding if none is provided
fails:Kernel#eval uses the receiver as self inside the eval
Expand Down
34 changes: 19 additions & 15 deletions truffle/src/main/java/org/jruby/truffle/nodes/core/KernelNodes.java
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ protected RubyBinding getCallerBinding(VirtualFrame frame) {
public Object eval(VirtualFrame frame, RubyString source, UndefinedPlaceholder binding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
notDesignedForCompilation();

return eval(source, getCallerBinding(frame), filename, lineNumber);
return getContext().eval(source.getBytes(), getCallerBinding(frame), true, this);
}

@Specialization
Expand All @@ -555,36 +555,49 @@ public Object eval(VirtualFrame frame, RubyString source, RubyNilClass noBinding
public Object eval(RubyString source, RubyBinding binding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
notDesignedForCompilation();

return getContext().eval(source.getBytes(), binding, this);
return getContext().eval(source.getBytes(), binding, false, this);
}

@Specialization
public Object eval(RubyString source, RubyBinding binding, RubyString filename, UndefinedPlaceholder lineNumber) {
notDesignedForCompilation();

// TODO (nirvdrum Dec. 29, 2014) Do something with the supplied filename.
return getContext().eval(source.getBytes(), binding, this);
return getContext().eval(source.getBytes(), binding, false, this);
}

@Specialization
public Object eval(RubyString source, RubyBinding binding, RubyString filename, int lineNumber) {
notDesignedForCompilation();

// TODO (nirvdrum Dec. 29, 2014) Do something with the supplied filename and lineNumber.
return getContext().eval(source.getBytes(), binding, this);
return getContext().eval(source.getBytes(), binding, false, this);
}

@Specialization(guards = "!isRubyString(arguments[0])")
public Object eval(VirtualFrame frame, RubyBasicObject object, UndefinedPlaceholder binding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
notDesignedForCompilation();

return eval(frame, object, getCallerBinding(frame), filename, lineNumber);
return evalCoerced(frame, object, getCallerBinding(frame), true, filename, lineNumber);
}

@Specialization(guards = "!isRubyString(arguments[0])")
public Object eval(VirtualFrame frame, RubyBasicObject object, RubyBinding binding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
notDesignedForCompilation();

return evalCoerced(frame, object, binding, false, filename, lineNumber);
}

@Specialization(guards = "!isRubyBinding(arguments[1])")
public Object eval(RubyBasicObject source, RubyBasicObject badBinding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
throw new RaiseException(
getContext().getCoreLibrary().typeError(
String.format("wrong argument type %s (expected binding)",
badBinding.getLogicalClass().getName()),
this));
}

private Object evalCoerced(VirtualFrame frame, RubyBasicObject object, RubyBinding binding, boolean ownScopeForAssignments, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
Object coerced;

try {
Expand All @@ -601,7 +614,7 @@ public Object eval(VirtualFrame frame, RubyBasicObject object, RubyBinding bindi
}

if (coerced instanceof RubyString) {
return getContext().eval(((RubyString) coerced).getBytes(), binding, this);
return getContext().eval(((RubyString) coerced).getBytes(), binding, ownScopeForAssignments, this);
} else {
throw new RaiseException(
getContext().getCoreLibrary().typeError(
Expand All @@ -612,15 +625,6 @@ public Object eval(VirtualFrame frame, RubyBasicObject object, RubyBinding bindi
this));
}
}

@Specialization(guards = "!isRubyBinding(arguments[1])")
public Object eval(RubyBasicObject source, RubyBasicObject badBinding, UndefinedPlaceholder filename, UndefinedPlaceholder lineNumber) {
throw new RaiseException(
getContext().getCoreLibrary().typeError(
String.format("wrong argument type %s (expected binding)",
badBinding.getLogicalClass().getName()),
this));
}
}

@CoreMethod(names = "exec", isModuleFunction = true, required = 1, argumentsAsArray = true)
Expand Down
10 changes: 7 additions & 3 deletions truffle/src/main/java/org/jruby/truffle/runtime/RubyContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,17 @@ public Object eval(ByteList code, Object self, RubyNode currentNode) {
return execute(this, source, code.getEncoding(), TranslatorDriver.ParserContext.TOP_LEVEL, self, null, currentNode, NodeWrapper.IDENTITY);
}

public Object eval(ByteList code, RubyBinding binding, RubyNode currentNode) {
public Object eval(ByteList code, RubyBinding binding, boolean ownScopeForAssignments, RubyNode currentNode) {
final Source source = Source.fromText(code, "(eval)");
return execute(this, source, code.getEncoding(), TranslatorDriver.ParserContext.TOP_LEVEL, binding.getSelf(), binding.getFrame(), currentNode, NodeWrapper.IDENTITY);
return execute(this, source, code.getEncoding(), TranslatorDriver.ParserContext.TOP_LEVEL, binding.getSelf(), binding.getFrame(), ownScopeForAssignments, currentNode, NodeWrapper.IDENTITY);
}

public Object execute(RubyContext context, Source source, Encoding defaultEncoding, TranslatorDriver.ParserContext parserContext, Object self, MaterializedFrame parentFrame, RubyNode currentNode, NodeWrapper wrapper) {
final RubyRootNode rootNode = translator.parse(context, source, defaultEncoding, parserContext, parentFrame, currentNode, wrapper);
return execute(context, source, defaultEncoding, parserContext, self, parentFrame, true, currentNode, wrapper);
}

public Object execute(RubyContext context, Source source, Encoding defaultEncoding, TranslatorDriver.ParserContext parserContext, Object self, MaterializedFrame parentFrame, boolean ownScopeForAssignments, RubyNode currentNode, NodeWrapper wrapper) {
final RubyRootNode rootNode = translator.parse(context, source, defaultEncoding, parserContext, parentFrame, ownScopeForAssignments, currentNode, wrapper);
final CallTarget callTarget = Truffle.getRuntime().createCallTarget(rootNode);

// TODO(CS): we really need a method here - it's causing problems elsewhere
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public RubyNode parse(RubyContext context, org.jruby.ast.Node parseTree, org.jru
return translator.compileFunctionNode(null, "(unknown)", argsNode, bodyNode, sharedMethod);
}

public RubyRootNode parse(RubyContext context, Source source, Encoding defaultEncoding, ParserContext parserContext, MaterializedFrame parentFrame, RubyNode currentNode, NodeWrapper wrapper) {
public RubyRootNode parse(RubyContext context, Source source, Encoding defaultEncoding, ParserContext parserContext, MaterializedFrame parentFrame, boolean ownScopeForAssignments, RubyNode currentNode, NodeWrapper wrapper) {
// Set up the JRuby parser

final org.jruby.parser.Parser parser = new org.jruby.parser.Parser(context.getRuntime());
Expand Down Expand Up @@ -113,14 +113,14 @@ public RubyRootNode parse(RubyContext context, Source source, Encoding defaultEn
throw new RaiseException(new RubyException(context.getCoreLibrary().getSyntaxErrorClass(), context.makeString(message), RubyCallStack.getBacktrace(currentNode)));
}

return parse(currentNode, context, source, parserContext, parentFrame, node, wrapper);
return parse(currentNode, context, source, parserContext, parentFrame, ownScopeForAssignments, node, wrapper);
}

public RubyRootNode parse(RubyNode currentNode, RubyContext context, Source source, ParserContext parserContext, MaterializedFrame parentFrame, org.jruby.ast.RootNode rootNode, NodeWrapper wrapper) {
public RubyRootNode parse(RubyNode currentNode, RubyContext context, Source source, ParserContext parserContext, MaterializedFrame parentFrame, boolean ownScopeForAssignments, org.jruby.ast.RootNode rootNode, NodeWrapper wrapper) {
final SourceSection sourceSection = source.createSection("<main>", 0, source.getCode().length());
final SharedMethodInfo sharedMethodInfo = new SharedMethodInfo(sourceSection, context.getRootLexicalScope(), "<main>", false, rootNode, false);

final TranslatorEnvironment environment = new TranslatorEnvironment(context, environmentForFrame(context, parentFrame), this, allocateReturnID(), true, false, sharedMethodInfo, sharedMethodInfo.getName(), false);
final TranslatorEnvironment environment = new TranslatorEnvironment(context, environmentForFrame(context, parentFrame), this, allocateReturnID(), ownScopeForAssignments, false, sharedMethodInfo, sharedMethodInfo.getName(), false);

// Get the DATA constant

Expand Down

4 comments on commit f49688f

@nirvdrum
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrisseaton @eregon Please review to make sure this is the appropriate fix.

@chrisseaton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok. Hard to follow some of this translator stuff isn't it?

@nirvdrum
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I opted to just introduce the new argument and delegate all existing calls to it. It might make more sense to split method names by intent rather than using an extra argument.

@eregon
Copy link
Member

@eregon eregon commented on f49688f Feb 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, renaming methods by intent would be invaluable for parse/eval and such methods with 4+ arguments and 2+ versions 👍
It's hard to follow in the translator, but looks good.

Please sign in to comment.