Skip to content
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

Kwargs should not need dynamic scope, since we have static handy. #3906

Merged
merged 2 commits into from
May 20, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions core/src/main/java/org/jruby/ir/IRScope.java
Original file line number Diff line number Diff line change
Expand Up @@ -662,13 +662,15 @@ private void calculateClosureScopeFlags() {
}
}

private static final EnumSet<IRFlags> NEEDS_DYNAMIC_SCOPE_FLAGS =
EnumSet.of(
CAN_RECEIVE_BREAKS,
HAS_NONLOCAL_RETURNS,CAN_RECEIVE_NONLOCAL_RETURNS,
BINDING_HAS_ESCAPED,
USES_ZSUPER);

private void computeNeedsDynamicScopeFlag() {
// SSS FIXME: checkArity for keyword args looks up a keyword arg in the static scope which
// currently requires a dynamic scope to be recovered. If there is another way to do this,
// we can get rid of this.
if (flags.contains(CAN_RECEIVE_BREAKS) || flags.contains(HAS_NONLOCAL_RETURNS) ||
flags.contains(CAN_RECEIVE_NONLOCAL_RETURNS) || flags.contains(BINDING_HAS_ESCAPED) ||
flags.contains(USES_ZSUPER) || flags.contains(RECEIVES_KEYWORD_ARGS)) {
if (flags.containsAll(NEEDS_DYNAMIC_SCOPE_FLAGS)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct ... shouldn't be containsAll ... containsAny if such a method exists.

flags.add(REQUIRES_DYNSCOPE);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public static CheckArityInstr decode(IRReaderDecoder d) {
}

public void checkArity(ThreadContext context, Object[] args, Block.Type blockType) {
IRRuntimeHelpers.checkArity(context, args, required, opt, rest, receivesKeywords, restKey, blockType);
IRRuntimeHelpers.checkArity(context.runtime, context.getCurrentStaticScope(), args, required, opt, rest, receivesKeywords, restKey, blockType);
}

@Override
Expand Down
14 changes: 6 additions & 8 deletions core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
Original file line number Diff line number Diff line change
Expand Up @@ -535,12 +535,12 @@ public static Block getBlockFromObject(ThreadContext context, Object value) {
return block;
}

public static void checkArity(ThreadContext context, Object[] args, int required, int opt, boolean rest,
public static void checkArity(Ruby runtime, StaticScope scope, Object[] args, int required, int opt, boolean rest,
boolean receivesKwargs, int restKey, Block.Type blockType) {
int argsLength = args.length;
RubyHash keywordArgs = extractKwargsHash(args, required, receivesKwargs);

if (restKey == -1 && keywordArgs != null) checkForExtraUnwantedKeywordArgs(context, keywordArgs);
if (restKey == -1 && keywordArgs != null) checkForExtraUnwantedKeywordArgs(runtime, scope, keywordArgs);

// keyword arguments value is not used for arity checking.
if (keywordArgs != null) argsLength -= 1;
Expand All @@ -549,7 +549,7 @@ public static void checkArity(ThreadContext context, Object[] args, int required
// System.out.println("NUMARGS: " + argsLength + ", REQUIRED: " + required + ", OPT: " + opt + ", AL: " + args.length + ",RKW: " + receivesKwargs );
// System.out.println("ARGS[0]: " + args[0]);

Arity.raiseArgumentError(context.runtime, argsLength, required, required + opt);
Arity.raiseArgumentError(runtime, argsLength, required, required + opt);
}
}

Expand Down Expand Up @@ -588,19 +588,17 @@ public static RubyHash extractKwargsHash(Object[] args, int requiredArgsCount, b
return null;
}

public static void checkForExtraUnwantedKeywordArgs(final ThreadContext context, RubyHash keywordArgs) {
final StaticScope scope = context.getCurrentStaticScope();

public static void checkForExtraUnwantedKeywordArgs(final Ruby runtime, final StaticScope scope, RubyHash keywordArgs) {
keywordArgs.visitAll(new RubyHash.Visitor() {
@Override
public void visit(IRubyObject key, IRubyObject value) {
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);
if ((slot >> 16) > 0) throw runtime.newArgumentError("unknown keyword: " + keyAsString);
// Could not find it anywhere.
if (((short) (slot & 0xffff)) < 0) throw context.runtime.newArgumentError("unknown keyword: " + keyAsString);
if (((short) (slot & 0xffff)) < 0) throw runtime.newArgumentError("unknown keyword: " + keyAsString);
}
});
}
Expand Down
5 changes: 3 additions & 2 deletions core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
Original file line number Diff line number Diff line change
Expand Up @@ -968,15 +968,16 @@ public void CheckArityInstr(CheckArityInstr checkarityinstr) {
if (jvm.methodData().specificArity >= 0) {
// no arity check in specific arity path
} else {
jvmMethod().loadContext();
jvmMethod().loadRuntime();
jvmMethod().loadStaticScope();
jvmMethod().loadArgs();
jvmAdapter().ldc(checkarityinstr.required);
jvmAdapter().ldc(checkarityinstr.opt);
jvmAdapter().ldc(checkarityinstr.rest);
jvmAdapter().ldc(checkarityinstr.receivesKeywords);
jvmAdapter().ldc(checkarityinstr.restKey);
jvmMethod().loadBlockType();
jvmAdapter().invokestatic(p(IRRuntimeHelpers.class), "checkArity", sig(void.class, ThreadContext.class, Object[].class, int.class, int.class, boolean.class, boolean.class, int.class, Block.Type.class));
jvmAdapter().invokestatic(p(IRRuntimeHelpers.class), "checkArity", sig(void.class, Ruby.class, StaticScope.class, Object[].class, int.class, int.class, boolean.class, boolean.class, int.class, Block.Type.class));
}
}

Expand Down