Skip to content

Commit

Permalink
Fixes #4186. JRuby accepts wrong method arguments when mixing positional
Browse files Browse the repository at this point in the history
with defaults and keywords.

StaticScopes now keep track of where keyword arguments start by index.
This is missing a regression test which I will add in next commit.
enebo committed Sep 28, 2016
1 parent 69402bc commit 7b93e08
Showing 5 changed files with 44 additions and 16 deletions.
10 changes: 3 additions & 7 deletions core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
Original file line number Diff line number Diff line change
@@ -597,13 +597,9 @@ public static void checkForExtraUnwantedKeywordArgs(ThreadContext context, final
private static final RubyHash.VisitorWithState<StaticScope> CheckUnwantedKeywordsVisitor = new RubyHash.VisitorWithState<StaticScope>() {
@Override
public void visit(ThreadContext context, RubyHash self, IRubyObject key, IRubyObject value, int index, StaticScope scope) {
String keyAsString = key.asJavaString();
int slot = scope.isDefined((keyAsString));

// Found name in higher variable scope. Therefore non for this block/method def.
if ((slot >> 16) > 0) throw context.runtime.newArgumentError("unknown keyword: " + keyAsString);
// Could not find it anywhere.
if (((short) (slot & 0xffff)) < 0) throw context.runtime.newArgumentError("unknown keyword: " + keyAsString);
if (!scope.keywordExists(key.asJavaString())) {
throw context.runtime.newArgumentError("unknown keyword: " + key.asJavaString());
}
}
};

7 changes: 6 additions & 1 deletion core/src/main/java/org/jruby/parser/ParserSupport.java
Original file line number Diff line number Diff line change
@@ -167,9 +167,14 @@ public AssignableNode assignableLabelOrIdentifier(String name, Node value) {
return currentScope.assign(lexer.getPosition(), name.intern(), makeNullNil(value));
}

// We know it has to be tLABEL or tIDENTIFIER so none of the other assignable logic is needed
public AssignableNode assignableKeyword(String name, Node value) {
return currentScope.assignKeyword(lexer.getPosition(), name.intern(), makeNullNil(value));
}

// Only calls via f_kw so we know it has to be tLABEL
public AssignableNode assignableLabel(String name, Node value) {
return currentScope.assign(lexer.getPosition(), name, makeNullNil(value));
return currentScope.assignKeyword(lexer.getPosition(), name, makeNullNil(value));
}

protected void getterIdentifierError(ISourcePosition position, String identifier) {
8 changes: 4 additions & 4 deletions core/src/main/java/org/jruby/parser/RubyParser.java
Original file line number Diff line number Diff line change
@@ -5070,26 +5070,26 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
states[575] = new ParserState() {
@Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
lexer.setCurrentArg(null);
yyVal = support.keyword_arg(((Node)yyVals[0+yyTop]).getPosition(), support.assignableLabelOrIdentifier(((String)yyVals[-1+yyTop]), ((Node)yyVals[0+yyTop])));
yyVal = support.keyword_arg(((Node)yyVals[0+yyTop]).getPosition(), support.assignableKeyword(((String)yyVals[-1+yyTop]), ((Node)yyVals[0+yyTop])));
return yyVal;
}
};
states[576] = new ParserState() {
@Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
lexer.setCurrentArg(null);
yyVal = support.keyword_arg(lexer.getPosition(), support.assignableLabelOrIdentifier(((String)yyVals[0+yyTop]), new RequiredKeywordArgumentValueNode()));
yyVal = support.keyword_arg(lexer.getPosition(), support.assignableKeyword(((String)yyVals[0+yyTop]), new RequiredKeywordArgumentValueNode()));
return yyVal;
}
};
states[577] = new ParserState() {
@Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
yyVal = support.keyword_arg(support.getPosition(((Node)yyVals[0+yyTop])), support.assignableLabelOrIdentifier(((String)yyVals[-1+yyTop]), ((Node)yyVals[0+yyTop])));
yyVal = support.keyword_arg(support.getPosition(((Node)yyVals[0+yyTop])), support.assignableKeyword(((String)yyVals[-1+yyTop]), ((Node)yyVals[0+yyTop])));
return yyVal;
}
};
states[578] = new ParserState() {
@Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
yyVal = support.keyword_arg(lexer.getPosition(), support.assignableLabelOrIdentifier(((String)yyVals[0+yyTop]), new RequiredKeywordArgumentValueNode()));
yyVal = support.keyword_arg(lexer.getPosition(), support.assignableKeyword(((String)yyVals[0+yyTop]), new RequiredKeywordArgumentValueNode()));
return yyVal;
}
};
8 changes: 4 additions & 4 deletions core/src/main/java/org/jruby/parser/RubyParser.y
Original file line number Diff line number Diff line change
@@ -2370,18 +2370,18 @@ f_label : tLABEL {

f_kw : f_label arg_value {
lexer.setCurrentArg(null);
$$ = support.keyword_arg($2.getPosition(), support.assignableLabelOrIdentifier($1, $2));
$$ = support.keyword_arg($2.getPosition(), support.assignableKeyword($1, $2));
}
| f_label {
lexer.setCurrentArg(null);
$$ = support.keyword_arg(lexer.getPosition(), support.assignableLabelOrIdentifier($1, new RequiredKeywordArgumentValueNode()));
$$ = support.keyword_arg(lexer.getPosition(), support.assignableKeyword($1, new RequiredKeywordArgumentValueNode()));
}

f_block_kw : f_label primary_value {
$$ = support.keyword_arg(support.getPosition($2), support.assignableLabelOrIdentifier($1, $2));
$$ = support.keyword_arg(support.getPosition($2), support.assignableKeyword($1, $2));
}
| f_label {
$$ = support.keyword_arg(lexer.getPosition(), support.assignableLabelOrIdentifier($1, new RequiredKeywordArgumentValueNode()));
$$ = support.keyword_arg(lexer.getPosition(), support.assignableKeyword($1, new RequiredKeywordArgumentValueNode()));
}


27 changes: 27 additions & 0 deletions core/src/main/java/org/jruby/parser/StaticScope.java
Original file line number Diff line number Diff line change
@@ -36,6 +36,7 @@
import org.jruby.ast.AssignableNode;
import org.jruby.ast.DAsgnNode;
import org.jruby.ast.DVarNode;
import org.jruby.ast.IScopedNode;
import org.jruby.ast.LocalAsgnNode;
import org.jruby.ast.LocalVarNode;
import org.jruby.ast.Node;
@@ -97,6 +98,8 @@ public class StaticScope implements Serializable {

private long commandArgumentStack;

private int firstKeywordIndex = -1;

// Method/Closure that this static scope corresponds to. This is used to tell whether this
// scope refers to a method scope or to determined IRScope of the parent of a compiling eval.
private IRScope irScope;
@@ -353,6 +356,30 @@ public AssignableNode assign(ISourcePosition position, String name, Node value)
return assign(position, name, value, this, 0);
}

/**
* Register a keyword argument with this staticScope. It additionally will track
* where the first keyword argument started so we can test and tell whether we have
* a kwarg or an ordinary variable during live execution (See keywordExists).
* @param position
* @param name
* @param value
* @return
*/
public AssignableNode assignKeyword(ISourcePosition position, String name, Node value) {
AssignableNode assignment = assign(position, name, value, this, 0);

// register first keyword index encountered
if (firstKeywordIndex == -1) firstKeywordIndex = ((IScopedNode) assignment).getIndex();

return assignment;
}

public boolean keywordExists(String name) {
int slot = exists(name);

return slot >= 0 && firstKeywordIndex != -1 && slot >= firstKeywordIndex;
}

/**
* Get all visible variables that we can see from this scope that have been assigned
* (e.g. seen so far)

0 comments on commit 7b93e08

Please sign in to comment.