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

Can we have AssertedPositionalParameterName interface which contains index? #50

Closed
arai-a opened this issue Jul 12, 2018 · 5 comments
Closed

Comments

@arai-a
Copy link

arai-a commented Jul 12, 2018

At first I thought this is the same issue as https://github.com/binast/ecmascript-binary-ast/issues/30, but I guess it's a bit different.

While creating binding data for parameters, we want a list of positional formal parameters+indices which directly maps to arguments element, so that the binding name maps to arguments slot at the point of reading scope data.
With current spec, it's unknown before parsing FormalParameters.

What I propose is the following:

interface AssertedPositionalParameterName {
  attribute unsigned short index;
  attribute IdentifierName name;
  attribute boolean isCaptured;
};

interface AssertedParameterName {
  attribute IdentifierName name;
  attribute boolean isCaptured;
};

typedef (AssertedPositionalParameterName or
         AssertedParameterName)
        AssertedMaybePositionalParameterName;

interface AssertedParameterScope {
  attribute FrozenArray<AssertedMaybePositionalParameterName> paramNames;
  attribute boolean hasDirectEval;
};

AssertedPositionalParameterName contains the index, which is the index in parameter list, and also the index in arguments. (to be clear, it's not the index in paramNames array).
AssertedParameterName is basically the same thing as current AssertedBoundName.

for example, function f(a, b=10, {c}, [d, e] = [], f, ...g) {} has the following scope data:

AssertedParameterScope {
  paramNames: [
    AssertedPositionalParameterName {
      index: 0, name: "a", isCaptured: false
    },
    AssertedParameterName {
      name: "b", isCaptured: false
    },
    AssertedParameterName {
      name: "c", isCaptured: false
    },
    AssertedParameterName {
      name: "d", isCaptured: false
    },
    AssertedParameterName {
      name: "e", isCaptured: false
    },
    AssertedPositionalParameterName {
      index: 4, name: "f", isCaptured: false
    },
    AssertedParameterName {
      name: "g", isCaptured: false
    },
  ],
  hasDirectEval: false
}

there a and f are positional parameters and their indices are 0 and 4.
isSimpleParameterList field is removed, because it's obvious from paramNames (by, whether there's at least AssertedParameterName).

@arai-a
Copy link
Author

arai-a commented Jul 13, 2018

the bug in SpiderMonkey side: https://bugzilla.mozilla.org/show_bug.cgi?id=1475458

@arai-a
Copy link
Author

arai-a commented Jul 26, 2018

@syg
Copy link
Collaborator

syg commented Jul 26, 2018

@arai-a Sorry for missing this.

Yeah, the current parameter scope is definitely deficient for capturing positional parameters. I had hoped that isSimpleParameterList alone was sufficient -- but as your example points out, having some destructuring without parameter expressions means some arguments need to be accessed by position and some not.

The additional interface for assigning positions to asserted parameter names looks good to me.

arai-a added a commit to arai-a/ecmascript-binary-ast that referenced this issue Jul 27, 2018
@arai-a
Copy link
Author

arai-a commented Jul 27, 2018

here's WIP patch that adds above interfaces.
https://github.com/arai-a/ecmascript-binary-ast/tree/spec-draft-positional-formal
I'll open PR once rest parameter issue turns out to be solvable on SpiderMonkey side

@arai-a
Copy link
Author

arai-a commented Jul 31, 2018

I think we can workaround the rest parameter issue for now by a cheat (https://bugzilla.mozilla.org/show_bug.cgi?id=1475458#c9) as long as we know which is simple rest parameter.
the change in https://github.com/binast/ecmascript-binary-ast/pull/52 should help anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants