Skip to content

Commit

Permalink
Use a defensive temp var when pulling possibly-undefined opt args.
Browse files Browse the repository at this point in the history 
Fixes #3094.
@headius
headius committed Mar 4, 2016
1 parent c6e43c0 commit c9e46b7
Showing 2 changed files with 32 additions and 3 deletions.
19 changes: 16 additions & 3 deletions core/src/main/java/org/jruby/ir/IRBuilder.java
View file
Original file line number Diff line number Diff line change
@@ -1733,7 +1733,12 @@ public Operand buildDAsgn(final DAsgnNode dasgnNode) {
int depth = dasgnNode.getDepth();
Variable arg = getLocalVariable(dasgnNode.getName(), depth);
Operand value = build(dasgnNode.getValueNode());

// no use copying a variable to itself
if (arg == value) return value;

addInstr(new CopyInstr(arg, value));

return value;

// IMPORTANT: The return value of this method is value, not arg!
@@ -1909,6 +1914,7 @@ protected void receiveNonBlockArgs(final ArgsNode argsNode) {
// Now for opt args
if (opt > 0) {
int optIndex = argsNode.getOptArgIndex();
Variable temp = createTemporaryVariable();
for (int j = 0; j < opt; j++, argIndex++) {
// Jump to 'l' if this arg is not null. If null, fall through and build the default value!
Label l = getNewLabel();
@@ -1917,10 +1923,12 @@ protected void receiveNonBlockArgs(final ArgsNode argsNode) {
Variable av = getNewLocalVariable(argName, 0);
if (scope instanceof IRMethod) addArgumentDescription(ArgumentType.opt, argName);
// You need at least required+j+1 incoming args for this opt arg to get an arg at all
addInstr(new ReceiveOptArgInstr(av, signature.required(), signature.pre(), j));
addInstr(BNEInstr.create(l, av, UndefinedValue.UNDEFINED)); // if 'av' is not undefined, go to default
build(n.getValue());
addInstr(new ReceiveOptArgInstr(temp, signature.required(), signature.pre(), j));
addInstr(BNEInstr.create(l, temp, UndefinedValue.UNDEFINED)); // if 'av' is not undefined, go to default
Operand defaultResult = build(n.getValue());
addInstr(new CopyInstr(temp, defaultResult));

This comment has been minimized.

Sign in to view

Copy link
@subbuss

subbuss Mar 4, 2016

Contributor

Is this extra tmp juggling necessary? Won't the checks in buildLocalAsgn and buildDAsgn fix this?

This comment has been minimized.

Sign in to view

Copy link
@headius

headius Mar 4, 2016

Author Member

The problem was primarily that ReceiveOptArgInstr returns undefined for unprovided opt args, so assigning that to the target variable made it possible for undefined to leak out. My change does all opt arg logic with a temp var and assigns to the real var only after that has finished.

This comment has been minimized.

Sign in to view

Copy link
@subbuss

subbuss Mar 4, 2016

Contributor

I see what is going on .. you are relying on https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java#L597 for defaultResult's retrieved value in the CopyInstr(temp, defaultResult) to be nil.

This comment has been minimized.

Sign in to view

Copy link
@headius

headius Mar 4, 2016

Author Member

Yeah, leaning on the fact that we initialize variables to nil, so the first assignment of a variable to itself isn't a problem. Perhaps there's a better way to fix this? The alternatives I came up with were more complex.

This comment has been minimized.

Sign in to view

Copy link
@enebo

enebo Mar 7, 2016

Member

Ultimately, it would be nice to eliminate ever having a null check or initial assignment unless it is needed but this particular problem is a somewhat deep hole for us to fix having check/init from IRBuilder alone. Consider:

def foo(a=defined?(a)); end
def foo(a=foo(a)); end

So I think relying on this assumption for now is the only way to go without possibly fixing this during parse and before build? The only way I see this at parse time would be to see if lvar is accessed before assigned and we just replace it with a nil?

This comment has been minimized.

Sign in to view

Copy link
@subbuss

subbuss Mar 7, 2016

Contributor

I haven't had the time to look at this yet, but I am puzzled why https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java#L597 didn't apply before this patch landed. The same thing should have worked earlier as well.
addInstr(new LabelInstr(l));
addInstr(new CopyInstr(av, temp));
}
}

@@ -2752,7 +2760,12 @@ public Operand buildLiteral(LiteralNode literalNode) {
public Operand buildLocalAsgn(LocalAsgnNode localAsgnNode) {
Variable var = getLocalVariable(localAsgnNode.getName(), localAsgnNode.getDepth());
Operand value = build(localAsgnNode.getValueNode());

// no use copying a variable to itself
if (var == value) return value;

addInstr(new CopyInstr(var, value));

return value;

// IMPORTANT: The return value of this method is value, not var!
16 changes: 16 additions & 0 deletions spec/compiler/general_spec.rb
View file
Original file line number Diff line number Diff line change
@@ -1068,5 +1068,21 @@ def a; __callee__; end
expect(y).to eq(1)
end
end

it "handles circular optional argument assignment" do
begin
verbose = $VERBOSE
$VERBOSE = nil

run('def foo(a = a, b = b, c = c); [a.inspect, b.inspect, c.inspect]; end; foo') do |x|
expect(x).to eq(["nil", "nil", "nil"])
end
run('def foo; yield; end; foo {|a = a, b = b, c = c|; [a.inspect, b.inspect, c.inspect]}') do |x|
expect(x).to eq(["nil", "nil", "nil"])
end
ensure
$VERBOSE = verbose
end
end
end
end

0 comments on commit c9e46b7

Please sign in to comment.