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.

Sign up for GitHub

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

Jump to bottom

Populate and use ObjFn->arity correctly (#731) #1124

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Populate and use ObjFn->arity correctly (#731) #1124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/vm/wren_compiler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3282,6 +3282,8 @@ static void createConstructor(Compiler* compiler, Signature* signature,
Compiler methodCompiler;
initCompiler(&methodCompiler, compiler->parser, compiler, true);

methodCompiler.fn->arity = signature->arity;

// Allocate the instance.
emitOp(&methodCompiler, compiler->enclosingClass->isForeign
? CODE_FOREIGN_CONSTRUCT : CODE_CONSTRUCT);
Expand Down Expand Up @@ -3462,6 +3464,8 @@ static bool method(Compiler* compiler, Variable classVariable)
error(compiler, "A constructor cannot be static.");
}

methodCompiler.fn->arity = signature.arity;

// Include the full signature in debug messages in stack traces.
char fullSignature[MAX_METHOD_SIGNATURE];
int length;
Expand Down
6 changes: 4 additions & 2 deletions src/vm/wren_vm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1422,6 +1422,7 @@ WrenHandle* wrenMakeCallHandle(WrenVM* vm, const char* signature)
// Create a little stub function that assumes the arguments are on the stack
// and calls the method.
ObjFn* fn = wrenNewFunction(vm, NULL, numParams + 1);
fn->arity = numParams;

// Wrap the function in a closure and then in a handle. Do this here so it
// doesn't get collected as we fill it in.
Expand Down Expand Up @@ -1460,9 +1461,10 @@ WrenInterpretResult wrenCall(WrenVM* vm, WrenHandle* method)

// Discard any extra temporary slots. We take for granted that the stub
// function has exactly one slot for each argument.
vm->fiber->stackTop = &vm->fiber->stack[closure->fn->maxSlots];
const int closureNumArgs = closure->fn->arity + 1;
vm->fiber->stackTop = &vm->fiber->stack[closureNumArgs];

wrenCallFunction(vm, vm->fiber, closure, 0);
wrenCallFunction(vm, vm->fiber, closure, closureNumArgs);
Copy link
Member

Choose a reason for hiding this comment

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

this code might not behave as expected? The closure being called here is the stub function created by the call handle, which only has bytecode to forward the call to the receiver. The closure itself has 0 args marked here specifically to avoid interacting with the stack afaict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old behavior was ONLY correct when passing the correct number of arguments using broken logic. There was a bogus relation between what the last wrenCallFunction argument was intended to be and what it was really used for. With that change, the argument is now the number of arguments that are on the stack to perform the execution call. Because of that it is not 0 anymore, since obviously a closure can always have any number arguments.
Because a closure has to accept its arity or more arguments, as per Fn.call, there MUST be a stack interaction if there is more arguments than required.

WrenInterpretResult result = runInterpreter(vm, vm->fiber);

// If the call didn't abort, then set up the API stack to point to the
Expand Down
8 changes: 7 additions & 1 deletion src/vm/wren_vm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,18 @@ static inline void wrenCallFunction(WrenVM* vm, ObjFiber* fiber,
fiber->frameCapacity = max;
}

// Functions allows to be called with more arguments than required. So the
// the [fiber] [stackTop] has to be realined to match the [closure] [arity].
const int closureNumArgs = closure->fn->arity + 1;
ASSERT(closureNumArgs <= numArgs, "Expect more arguments");
fiber->stackTop -= numArgs - closureNumArgs;

// Grow the stack if needed.
int stackSize = (int)(fiber->stackTop - fiber->stack);
int needed = stackSize + closure->fn->maxSlots;
wrenEnsureStack(vm, fiber, needed);

wrenAppendCallFrame(vm, fiber, closure, fiber->stackTop - numArgs);
wrenAppendCallFrame(vm, fiber, closure, fiber->stackTop - closureNumArgs);
}

// Marks [obj] as a GC root so that it doesn't get collected.
Expand Down
5 changes: 5 additions & 0 deletions test/regression/731.wren
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@

Fn.new {
var foo
System.print(foo) // expect: null
}.call("Bug")