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

Last expressions result gets returned from functions when there is no return #672

Closed
dvtkrlbs opened this issue Sep 1, 2020 · 6 comments · Fixed by #1528
Closed

Last expressions result gets returned from functions when there is no return #672

dvtkrlbs opened this issue Sep 1, 2020 · 6 comments · Fixed by #1528
Labels
bug Something isn't working execution Issues or PRs related to code execution
Milestone

Comments

@dvtkrlbs
Copy link

dvtkrlbs commented Sep 1, 2020

When non returning functions mutates a property of another object that modified object gets returned even though we dont use the return statement

In the repl

>> function x() {}
undefined
>> function y() { x.y = 3 }
undefined
>> x.y
undefined
>> y()
3

y() should not return anything

@dvtkrlbs dvtkrlbs added the bug Something isn't working label Sep 1, 2020
@RageKnify
Copy link
Member

This can be generalized, it seems that when the last statement in a function is an expression the expression's result is returned rather than undefined.

>> function z() { 0 }
undefined
>> z()
0

@dvtkrlbs dvtkrlbs changed the title Modified values gets returned from functions when they shouldn't Last expressions result gets returned from functions when there is no return Sep 1, 2020
@RageKnify
Copy link
Member

RageKnify commented Sep 1, 2020

Problem is in: https://github.com/boa-dev/boa/blob/be20b65a9e1a2ac6d802f5c8f3f87edd149274eb/boa/src/exec/statement_list.rs

if i + 1 == self.statements().len() {
     obj = val;
}

Regardless of the type of node, the value obtained from running it is used as the return value.

I think StatementLists are used elsewhere besides function, if so it might not be as simple as removing these lines....

PS: It's not as simple as removing them, I think some mantainer orientation is required.

@jasonwilliams
Copy link
Member

jasonwilliams commented Sep 1, 2020

Seeing that all returns in JavaScript are explicit anyway, I wonder if StatementList can Ok(undefined), it should be safe as any return will change the interpreter state so it wouldn’t get that far. You could return Ok(val) directly from the return state branch in that link @RageKnify posted.

It’s something worth playing with, but it should work.

@RageKnify
Copy link
Member

Should have been explicit on my PS, what happened when I tried removing the 3 lines was that the CLI started only returning undefined, because what the parser gives to be run with the Interpreter is a StatementList.

Changing the break to an early return wouldn't change the current flow, right?

@Razican Razican added the execution Issues or PRs related to code execution label Dec 25, 2020
@Razican Razican added this to the v0.12.0 milestone Jan 11, 2021
@Razican Razican modified the milestones: v0.12.0, v0.13.0 May 22, 2021
@JayHelton
Copy link

Hello! Is this available to work on?

@jasonwilliams
Copy link
Member

@JayHelton i believe so!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants