Skip to content

Conditionally emit rest parameters #518

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

Closed

Conversation

duncanmak
Copy link

This is a sketch to omit code emission for the ...rest parameter when it's not used in the body.

With this patch, the example in #498 will be compiled without generating code for the ...rest parameter.

If this is the right approach, I can complete the implementation by handling the other cases in the new isParameterReferencedInBody method.

@DanielRosenwasser
Copy link
Member

(Note: I've moved this comment to the pull-request's main comments so that you can easily come back to it.)

Hi @duncanmak! After re-reading your original pull request message, I realized that you may be looking for some guidance in making this contribution. Let me just say thank you for your interest and efforts so far.

I think what you're trying to do is to appropriately traverse the body of a function in a smart way to short-circuit on a return statement if you haven't seen the parameter yet.

Firstly, you will need to recursively traverse the body - in other words, within the function, you will have to also traverse the bodies of for/while loops (as well as the expressions which they test!), if-else constructs (again, be sure to check the condition expressions they test), etc. separately.

We actually have a function that makes this pretty darn easy - forEachChild (found in parser.ts). A pretty clear (well I guess that's subjective, since I was the one who wrote it...) example where this is used is the forEachReturnStatement function which can be found here. The name of forEachChild is kind of misleading, since the function actually short-circuits if you return a "truthy" value - but it's useful in exactly this sort of situation!

Secondly, I would avoid trying to be too smart here - JavaScript has a funny tendency to surprise you when you try to do that sort of thing! For instance, if I'm right that you're trying to bail out if you hit a return statement early, that can lead to some issues:

function f(a: string, b: number, ...rest: any[]) {
    console.log(g());
    return;

    function g(): any {
        return rest[0];
    }
}

This will be a problem, since such an approach will incorrectly assume that rest is unused - so perhaps a naïve approach would be best for now.

If you have any other questions, please don't hesitate to ask!

Still need to figure out the correct way to match the parameter against
the child Node.
@duncanmak
Copy link
Author

Hello, @DanielRosenwasser!

Thanks for your notes about using forEachChild. I made another commit and I think I'm getting a little closer. I have 2 questions:

  1. Are there other SyntaxKinds that I need to handle, other than Identifier, and the 3 Literals?
  2. What's the proper way to check if the Identifier is referring to the same thing as my parameter? I thought the id would be appropriate, but it doesn't seem to work that way. Checking the name would work in simple cases, but then there are cases where there's aliasing.

For a test program like:

function foo(x, y, z, ...rest) {
    console.log(rest);
    var rest = [];
    console.log(rest);
}

I get this with my patch applied:

{ name: 'foo', id: undefined } 'vs' { name: 'rest', id: 14739 }
{ name: 'x', id: undefined } 'vs' { name: 'rest', id: 14739 }
{ name: 'y', id: undefined } 'vs' { name: 'rest', id: 14739 }
{ name: 'z', id: undefined } 'vs' { name: 'rest', id: 14739 }
{ name: 'rest', id: undefined } 'vs' { name: 'rest', id: 14739 }
{ name: 'console', id: 15515 } 'vs' { name: 'rest', id: 14739 }
{ name: 'log', id: undefined } 'vs' { name: 'rest', id: 14739 }
{ name: 'rest', id: 15517 } 'vs' { name: 'rest', id: 14739 }
{ name: 'rest', id: undefined } 'vs' { name: 'rest', id: 14739 }
{ name: 'console', id: 15520 } 'vs' { name: 'rest', id: 14739 }
{ name: 'log', id: undefined } 'vs' { name: 'rest', id: 14739 }
{ name: 'rest', id: 15522 } 'vs' { name: 'rest', id: 14739 }

{ name: 'rest', id: 15517 } refers to the same thing as { name: 'rest', id: 14739 }, but { name: 'rest', id: 15522 } is a fresh binding.

So what's the right way to write this test?

Thanks!


function isReferenced(node: Node): boolean {

if (node === undefined)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you can ever run into this situation unless node is explicitly passed undefined.

@DanielRosenwasser
Copy link
Member

When you run into an identifier, you can fetch the symbol that semantically represents it with getSymbolOfNode and use that symbol's valueDeclaration property (if it exists). valueDeclaration points to the first place a symbol was declared. At that point, it's a simple matter of checking if that declaration is the same as the parameter declaration.

Once you've gotten this down, we can dive a little into the constructs you need to explicitly check, and what you can get away with eliding.

@duncanmak
Copy link
Author

It feels a bit like fumbling in the dark, but I think I made some progress.

This is my current ad-hoc set of tests:

function shouldEmit(x, y, z, ...rest) {
    return rest;
}

function shouldEmit2(x, y, z, ...rest) {
    var a = rest.concat([x, y, z]);
}

function shouldNotEmit(x, y, z, ...rest) {
    return;
}

var shouldNotEmit2 = (a, b, ...c) => a.concat([1,2,3]);

function shouldNotEmit3(x, y, z, ...rest) {
    var rest = [];
    return rest;
}

With the latest patch, I got the expected code generated, except for shouldNotEmit3.

P.S. getSymbolOfNode didn't work for me, I always got back undefined. I found getSymbolForEntityName to work better.

@@ -7098,6 +7098,31 @@ module ts {
return false;
}

function isParameterReferencedInBody(body: FunctionDeclaration, param: ParameterDeclaration) {

function equals(a: Symbol, b: Symbol) {
Copy link
Member

Choose a reason for hiding this comment

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

You can actually simply check if the symbol's valueDeclaration is equal to the parameter itself (using nothing but ===).


function isReferenced(node: Node): boolean {
switch (node.kind) {
case SyntaxKind.Parameter:
Copy link
Member

Choose a reason for hiding this comment

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

This is a little inaccurate because you have to consider default arguments:

function f(a: number, ...rest: number[]) {
    function g(x: number = rest[0]): number {
        return a || x;
    }

    return g();
}

Additionally, case/default clauses should all be indented in our codebase.

@duncanmak
Copy link
Author

I figured out how to deal with default arguments, and I fixed up the indentation as well.

I'm still unsure about shouldNotEmit3 - I don't know the specs, so I can't tell what the correct behavior should be. Perhaps the bug is in getSymbolForEntityName, as it shouldn't return the same Declaration?

function shouldNotEmit3(x, y, z, ...rest) {
    var rest = [];
    return rest;
}

@DanielRosenwasser
Copy link
Member

No, the problem is that in JavaScript, even if you do

function f(...rest: number[]) {
    var rest = [1,2,3];

    return rest[0];
}

you are still talking about the same rest. That's why shouldNotEmit3 is a bit of a difficult case, and to be honest, being conservative but correct is okay here - it's permissible to say that rest is referenced even when it's not, since it won't break any emitted code.


In the mean time, you may be noticing that the Travis CI builds are failing on this pull request. This is because our baseline references need to be updated. Additionally, we'll need a new test for this change.

Here's how to go about this:

  1. In tests/cases/compiler, create a new file called restParameterEmit.ts or something like that.
  2. In the file that you added, add a series of cases where the rest parameters should and should not be emitted. Feel free to use some of the same examples we wrote here.
  3. When you're ready, run jake runtests. To determine if the outputs are desirable, diff the emits in tests/baselines/reference and tests/baselines/local.
  4. If you think the changes are appropriate, run jake baseline-accept, and add the new test and the updated baselines in git.

switch (node.kind) {
case SyntaxKind.Parameter:
var p = <ParameterDeclaration> node;
return (p.initializer) ? isReferenced(p.initializer) : false;
Copy link
Member

Choose a reason for hiding this comment

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

Might as well just let forEachChild take care of this.

Copy link
Author

Choose a reason for hiding this comment

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

The reason why this is necessary is because when forEachChild descends into body (which is a FunctionDeclaration), it will go through the parameters first before the body.

Without this case, isParameterReferencedInBody will always return true because it can always find param in the function's parameters.

Copy link
Author

Choose a reason for hiding this comment

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

You mentioned earlier that I might want to make this function a bit more general - what do you have in mind?

I'm also thinking that it might be good to rename the function -
isParameterUsedInFunction(func: FunctionDeclaration, param: ParameterDeclaration): boolean
might be a better signature. Do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

You can rename body to func, and call forEachChild(func.body, isReferenced).

Actually, now that I think about it, this only works if the parameter declaration is the last parameter. Perhaps you should just iterate over the parameters, calling isReferenced on the initializers, and then call forEachChild(func.body, isReferenced)

What I was thinking was that instead of a ParameterDeclaration, you just take a Declaration, but I wouldn't bother with it now. We can always do that in a different commit when we need it.

@duncanmak
Copy link
Author

I will be working on updating the baseline tests later tonight.

After getting all the reviews, I can do a squash to clean up the intermediate commits within this pull request.

@CyrusNajmabadi
Copy link
Contributor

Overall this change looks fairly reasonable. The code should definitely be simplified and clarified. But i think it's a nice to have.

Renamed isParameterReferencedInBody to isParameterUsedInFunction.
@duncanmak
Copy link
Author

@RyanCavanaugh Did you refer to this in #153? If someone take a look and let me know what remains to be done, I would love to land this.

Thanks!

@DanielRosenwasser
Copy link
Member

Apparently your build is still failing on Travis - I think you updated a test after updating the baselines for emitRestParameter.ts. Just run another jake runtests, diff the locals/baselines to make sure it's correct, and run jake baseline-accept.

@duncanmak
Copy link
Author

@DanielRosenwasser - is there anything else left for me to do with this PR? Are we waiting on @mhegazy to review the changes I made to the baselines?

@DanielRosenwasser
Copy link
Member

Terribly sorry about the delay - I'm enthused for this change, but I just wanted to wait on @mhegazy regarding what we need to do with the tests before committing to anything. Hope you'll understand, as we've been so busy with 1.1, and I can vouch for Mohamed when I say he's dealing with quite a share. =)

@duncanmak
Copy link
Author

Oh, that's okay.

Is this change to be included in 1.1, or will it be landed later?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 2, 2014

sorry about the delay. totally fell off my radar. I just need to run some perf tests, I am concerned about the approach, walking the function body to identify if we should optimize the arg away can be costly.

@DanielRosenwasser
Copy link
Member

What would be a more optimal approach? Something along the lines of getReferencesInNode?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 2, 2014

I would say something along the lines of what we do with this capture.

@DanielRosenwasser
Copy link
Member

Hey @duncanmak, as Mohamed mentioned, the smarter way to go about this is to make it work the same way that this-capturing works.

As background, there are situations in which we need to perform a capture on this from an outer context - specifically, arrow functions don't introduce a new lexical value for this, so any time this is used in an arrow function, we need to keep track of this so that the emitter can appropriately perform the capture for <ES6 emit.

However, we don't traverse every arrow function body looking for this - rather, we only do work if we encounter a this expression and check whether it occurs inside of an arrow function. If this is the case, we use flags to represent how to capture this and flip them on appropriately.

What I suggest you do is that when you encounter an identifier now, find out if it was declared as a rest parameter, and if so, turn on a flag (which you may have to add - maybe call it EmitRest or EmitRestParam) for the containing function. During emit, you won't have to call a function - just check the flags.

@mhegazy mhegazy closed this Feb 12, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants