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

Consider dropping the "optimization" for functions without parameter default value initializers #623

Open
jorendorff opened this issue Jun 28, 2016 · 5 comments

Comments

@jorendorff
Copy link
Contributor

jorendorff commented Jun 28, 2016

Currently, FunctionDeclarationInstantiation branches at step 26:

26. If hasParameterExpressions is false, then
    a.  NOTE Only a single lexical environment is needed for the parameters
        and top-level vars.
    ...
27. Else,
    a.  NOTE A separate Environment Record is needed to ensure that closures
        created by expressions in the formal parameter list do not have visibility
        of declarations in the function body.
    ...

There's a special case (step 26) for functions without defaults; and a general case (step 27) that looks like it would work whether you have defaults or not.

Our implementation will in fact branch like this, somewhere. But why have the special case in the spec? It reads like a pure optimization, which seems undesirable for a spec. I think @allenwb wrote this - Allen, do you know what this is for?

@allenwb
Copy link
Member

allenwb commented Jun 28, 2016

I considered that possibility when I updated the algorithm to include the arguments environment. The reason I didn't go that route is that I wanted to emphasize that the separate environment was there solely to support parameter lists with defaults. Plus the overall algorithm is complicated enough that it seemed wise to normatively specify the intended semantics for the "optimized" case rather than leaving it to each implementation to figure that out.

The optimized branch could be eliminated and replaced with an informative note. However, at the time, carefully crafting the language of such a note seemed harder than just including both paths in the algorithm.

@jorendorff
Copy link
Contributor Author

Plus the overall algorithm is complicated enough that it seemed wise to normatively specify the intended semantics for the "optimized" case rather than leaving it to each implementation to figure that out.

Thanks for thinking of us implementors, and thanks for the fast response.

The NOTE in step 26.a. should say that this is meant to illustrate a pure optimization that implementations may do. As it stands, I was concerned that the branch might have been done, rather, because the two branches were not the same, that adding even a trivial default like = 0 changed behavior in some very subtle way that we were missing.

Honestly, though, I think I'd rather this be removed. Implementations will likely choose to optimize this some other way. For example, nested environments can often be fused; this particular optimization might fall out for free, as a special case of a more general analysis that looks for such opportunities.

@littledan
Copy link
Member

I like @jorendorff 's suggestion. Our implementation has a conditional too, but we only add the scope if we see sloppy direct eval being called anywhere in the function body or defaults.

@ljharb
Copy link
Member

ljharb commented Jan 3, 2020

@jorendorff any interest in making a PR?

@bakkot
Copy link
Contributor

bakkot commented Aug 26, 2021

Apart from this, there's also the argumentsObjectNeeded stuff, which is unobservable (except that you do need to avoid creating the binding for arrow functions, of course).

Similarly, the condition containing

NOTE: Non-strict functions use a separate Environment Record for top-level lexical declarations so that a direct eval can determine whether any var scoped declarations introduced by the eval code conflict with pre-existing top-level lexically scoped declarations. This is not needed for strict functions because a strict direct eval always places all declarations into a new Environment Record.

isn't observable; it is equivalent to always create the new environment.

@ljharb ljharb added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants