1472103 - [BinAST] Update in-tree IDL

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

We should update the in-tree IDL and the implementation, to match it the latest spec, once the binjs-ref is also updated.

WIP patch for binjs-ref is here:
  https://github.com/arai-a/binjs-ref/commits/spec-update

I also have WIP patch for SpiderMonkey which updates at least non-lazy part, but not yet touched lazy part.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: (WIP) Update BinAST IDL.

In order to run this, dependencies in js/src/frontend/binsource/Cargo.toml should be changed to point updated binjs_meta.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: (WIP) Update BinAST IDL.

fixed based on bug 1473202

Comment 3

[Tooru Fujisawa [:arai]]

also updates in-tree test files
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb63c5a46a7b40624f5bf9521af3ff321feed010

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: (WIP) Part 1: Update BinAST IDL.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: (WIP) Part 2: Update BinAST test files.

Comment 6

[Tooru Fujisawa [:arai]]

https://github.com/binast/binjs-ref/pull/144 should be fixed before this, in order to properly update in-tree rust files and dependency.

Comment 7

[Tooru Fujisawa [:arai]]

Addressed https://github.com/binast/ecmascript-binary-ast/pull/52
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c1f518bb6ae891b4a6a876c7b821015dfbcd579

Comment 8

[Tooru Fujisawa [:arai]]

latest patches
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a562e0a64a97b7e5fc24ef618992e858837e976

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Update BinAST IDL.

* renamed Skippable => Lazy
  * added `contents` fields to functions
  * added AssertedDeclaredName/AssertedBoundName to scope
  * removed `capturedNames` field from scope, which is moved to `isCaptured` field of AssertedDeclaredName/AssertedBoundName
  * moved the "update" part of BinASTParser::{parseAndUpdateCapturedNames,parseAndUpdateScopeNames} into updateScopeName+captureName
    "parse" part is now auto-generated
  * added BinASTParser::getDeclaredScope to map AssertedDeclaredKind to internal kind/scope
  * added BinASTParser::getBoundScope to map catch/parameter to internal kind/scope
  * increment format version number from 0 to 1 (that matches binjs-ref)

Comment 10

[Tooru Fujisawa [:arai]]

forgot to mention.
I used "0.3.9" for binjs_meta in Cargo.toml.
will update Cargo.lock and in-tree cache once it's released.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add AssertedPositionalParameterName, AssertedRestParameterName, and AssertedParameterName.

* added AssertedPositionalParameterName/AssertedRestParameterName/AssertedParameterName, replacing AssertedBoundName for AssertedParameterScope
  * added unsinged long handling for parameter index
  * added BinASTParser::checkPositionalParameterIndices which corresponds to CheckPositionalParameterIndices in the spec
    (actually, the out-of-range error is thrown earlier than the spec)

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add length field to functions.

* added length field to functions
  * check length in contents field, after parsing params

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Remove function expression name from parameter scope.

* reflect isFunctionNameCaptured directly to named lambda scope
  * added AssertedScopeKind::FunctionExprName to special case function name
    separate from existing AssertedScopeKind::Parameter

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Update BinAST tests.

the result of binjs_encode for each file, with current binjs-ref master

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9007678 [details] [diff] [review]
Part 1: Update BinAST IDL.

Review of attachment 9007678 [details] [diff] [review]:
-----------------------------------------------------------------

Splinter has lost this review more than once, so I'm publishing intermediate comments instead of waiting for them to vanish again. Apologies if this makes the review a bit odd.

::: js/src/frontend/BinSource.h
@@ +155,5 @@
>  
>      // Ensure that this parser will never be used again.
>      void poison();
>  
> +    enum class AssertedScopeKind {

Could you add one word to mention that these are the kinds of the Asserted*Scope *as specified in the webidl*?

@@ +176,5 @@
>      JS::Result<FunctionBox*>
>      buildFunctionBox(GeneratorKind generatorKind, FunctionAsyncKind functionAsyncKind, FunctionSyntaxKind syntax, ParseNode* name);
>  
> +    // Add name to a given scope.
> +    MOZ_MUST_USE JS::Result<Ok> updateScopeName(AssertedScopeKind scopeKind, HandleAtom name,

It's not entirely an "update", as it can't redeclare a variable. Perhaps you could rename it "addScopeName" or something such?

@@ +180,5 @@
> +    MOZ_MUST_USE JS::Result<Ok> updateScopeName(AssertedScopeKind scopeKind, HandleAtom name,
> +                                                ParseContext::Scope* scope,
> +                                                DeclarationKind declKind,
> +                                                bool isCaptured);
> +    MOZ_MUST_USE JS::Result<Ok> getDeclaredScope(AssertedScopeKind scopeKind,

That returns the current Asserted{Block, ScriptGlobal, Var}Scope, right? If so, could you document this?

Also, can you document what it sets `declKind` to?

@@ +184,5 @@
> +    MOZ_MUST_USE JS::Result<Ok> getDeclaredScope(AssertedScopeKind scopeKind,
> +                                                 AssertedDeclaredKind kind,
> +                                                 ParseContext::Scope*& scope,
> +                                                 DeclarationKind& declKind);
> +    MOZ_MUST_USE JS::Result<Ok> getBoundScope(AssertedScopeKind scopeKind,

Pretty much as above :)

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9007678 [details] [diff] [review]
Part 1: Update BinAST IDL.

Review of attachment 9007678 [details] [diff] [review]:
-----------------------------------------------------------------

Part 2.

::: js/src/frontend/BinSource.cpp
@@ +253,4 @@
>  {
> +    auto ptr = scope->lookupDeclaredNameForAdd(name);
> +    if (ptr)
> +        return raiseError("Variable redeclaration");

I think that this style should now change to

```
if (ptr) {
    // ...
}
```

@@ +260,3 @@
>  
> +    if (isCaptured)
> +        MOZ_TRY(captureName(scopeKind, name));

Mmmh... we don't seem to be using the same `scope` in `capturedName` and here. Is that normal?

@@ +285,5 @@
> +      case AssertedDeclaredKind::ConstLexical:
> +        declKind = DeclarationKind::Const;
> +        scope = parseContext_->innermostScope();
> +        break;
> +    }

Not for this bug, but this looks like the kind of method that should eventually be shared with the regular parser.

@@ +303,5 @@
> +        scope = parseContext_->innermostScope();
> +        break;
> +      case AssertedScopeKind::Parameter:
> +        declKind = DeclarationKind::PositionalFormalParameter;
> +        scope = &parseContext_->functionScope();

What happens if we're not in a function scope? Can it happen?

@@ +306,5 @@
> +        declKind = DeclarationKind::PositionalFormalParameter;
> +        scope = &parseContext_->functionScope();
> +        break;
> +      default:
> +        MOZ_CRASH();

Nit: Looks like a MOZ_ASSERT_UNREACHABLE.

@@ +316,4 @@
>  
> +template<typename Tok> JS::Result<Ok>
> +BinASTParser<Tok>::captureName(AssertedScopeKind scopeKind, HandleAtom name)
> +{

There seems to be some duplication with `getBoundScope`/`getDeclaredScope`. Not a problem, but if an opportunity ever arises to reuse them, we may want to take it.

@@ +317,5 @@
> +template<typename Tok> JS::Result<Ok>
> +BinASTParser<Tok>::captureName(AssertedScopeKind scopeKind, HandleAtom name)
> +{
> +    if (scopeKind == AssertedScopeKind::Parameter) {
> +        MOZ_ASSERT(parseContext_->isFunctionBox());

I don't *think* we can reach that point without being in a function box, but could you explain (as comments) what makes it so?

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9007678 [details] [diff] [review]
Part 1: Update BinAST IDL.

Review of attachment 9007678 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BinSource.yaml
@@ +579,5 @@
> +    extra-args: paramsOut, bodyOut
> +    fields:
> +        isThisCaptured:
> +            after: |
> +                (void) isThisCaptured;

How comes we're not using this field or the next one?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9007679 [details] [diff] [review]
Part 2: Add AssertedPositionalParameterName, AssertedRestParameterName, and AssertedParameterName.

Review of attachment 9007679 [details] [diff] [review]:
-----------------------------------------------------------------

(part 1)

::: js/src/frontend/BinSource.cpp
@@ +359,5 @@
>  }
>  
>  template<typename Tok> JS::Result<Ok>
> +BinASTParser<Tok>::checkPositionalParameterIndices(Handle<GCVector<JSAtom*>> positionalParams,
> +                                                   ParseNode* params)

Are we checking somewhere that we have no collision between indices?

Also, that bit of the specs is not entirely clear to me: why do we need the indices?

@@ +384,5 @@
> +                return raiseError("AssertedPositionalParameterName: name mismatch");
> +        } else {
> +            // Destructuring or rest parameter.
> +            if (param->isKind(ParseNodeKind::Name))
> +                return raiseError("AssertedParameterName/AssertedRestParameterName: expected destructuring/rest parameter, got positional parameter");

Do you need to check that the rest parameter is last? If not, can you mention why?

::: js/src/frontend/BinSource.h
@@ +190,5 @@
>                                                DeclarationKind& declKind);
>      MOZ_MUST_USE JS::Result<Ok> captureName(AssertedScopeKind scopeKind, HandleAtom name);
>      MOZ_MUST_USE JS::Result<Ok> checkBinding(JSAtom* name);
>  
> +    MOZ_MUST_USE JS::Result<Ok> checkPositionalParameterIndices(Handle<GCVector<JSAtom*>> positionalParams,

Could you document what it checks exactly? The implementation in the .cpp is more complicated than I had guessed, so I'm probably not the only one who would be surprised :)

::: js/src/frontend/BinSource.yaml
@@ +266,5 @@
> +    extra-params: |
> +        AssertedScopeKind scopeKind,
> +        MutableHandle<GCVector<JSAtom*>> positionalParams
> +    extra-args: |
> +        scopeKind, positionalParams

Side-note: the flow of extra-args is becoming a bit complicated to follow without looking at generated source :)

If you have ideas of how to document them, that would be nice.

@@ +273,5 @@
> +    inherits: AssertedMaybePositionalParameterName
> +    fields:
> +        name:
> +            after: |
> +                // FIXME: This should be checked inside checkPositionalParameterIndices.

Followup bug?

Also, what is "this"? The lines just below?

@@ +275,5 @@
> +        name:
> +            after: |
> +                // FIXME: This should be checked inside checkPositionalParameterIndices.
> +                if (index >= positionalParams.get().length())
> +                    return raiseError("AssertedPositionalParameterName.length out of range");

Just to be sure I understand, that's a file corruption, right?

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9007679 [details] [diff] [review]
Part 2: Add AssertedPositionalParameterName, AssertedRestParameterName, and AssertedParameterName.

Review of attachment 9007679 [details] [diff] [review]:
-----------------------------------------------------------------

(part 2)

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9007680 [details] [diff] [review]
Part 3: Add length field to functions.

Review of attachment 9007680 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BinSource.h
@@ +192,5 @@
>      MOZ_MUST_USE JS::Result<Ok> checkBinding(JSAtom* name);
>  
>      MOZ_MUST_USE JS::Result<Ok> checkPositionalParameterIndices(Handle<GCVector<JSAtom*>> positionalParams,
>                                                                  ParseNode* params);
> +    MOZ_MUST_USE JS::Result<Ok> checkFunctionLength(uint32_t expectedLength);

Here, too, what does it check exactly?

I don't expect people who will maintain this file to look for 3.1.13 – we should tell them that it's an implementation of `CheckFunctionLength` from the spec, but it's even better if we also explain what it's for.

::: js/src/frontend/BinSource.webidl_
@@ +466,5 @@
>  interface EagerMethod : Node {
>    attribute boolean isAsync;
>    attribute boolean isGenerator;
>    attribute PropertyName name;
> +  attribute unsigned long length;

Could you see with @syg about documenting fields in the webidl? Here, a name such as `length` is not self-documenting.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9007681 [details] [diff] [review]
Part 4: Remove function expression name from parameter scope.

Review of attachment 9007681 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BinSource.h
@@ +159,5 @@
>      enum class AssertedScopeKind {
>          Block,
>          Catch,
>          Global,
> +        FunctionExprName,

Oh, wait, there's no AssertedFunctionExprNameScope, so I guess I misunderstood what AssertedScopeKind is. This confirms that documentation would be useful :)

(or maybe it's that FunctionExprName has a pseudo-AssertedScope of its own?)

::: js/src/frontend/BinSource.yaml
@@ +613,5 @@
>              after: |
>                  (void) isThisCaptured;
>          isFunctionNameCaptured:
>              after: |
> +                // isFunctionNameCaptured can be true for anonymous function.

That comment looks incomplete. What happens for other functions?

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 9007682 [details] [diff] [review]
Part 5: Update BinAST tests.

Review of attachment 9007682 [details] [diff] [review]:
-----------------------------------------------------------------

That's only the binaries, right?

Comment 23

[Tooru Fujisawa [:arai]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #17)
> Comment on attachment 9007678 [details] [diff] [review]
> Part 1: Update BinAST IDL.
> 
> Review of attachment 9007678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BinSource.yaml
> @@ +579,5 @@
> > +    extra-args: paramsOut, bodyOut
> > +    fields:
> > +        isThisCaptured:
> > +            after: |
> > +                (void) isThisCaptured;
> 
> How comes we're not using this field or the next one?

isThisCaptured is for arrow function and it's on TODO in BinASTParser<Tok>::buildFunction.

isFunctionNameCaptured is used in Part 4.

Comment 24

[Tooru Fujisawa [:arai]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #18)
> ::: js/src/frontend/BinSource.cpp
> @@ +359,5 @@
> >  }
> >  
> >  template<typename Tok> JS::Result<Ok>
> > +BinASTParser<Tok>::checkPositionalParameterIndices(Handle<GCVector<JSAtom*>> positionalParams,
> > +                                                   ParseNode* params)
> 
> Are we checking somewhere that we have no collision between indices?

Good point.
Added it to AssertedPositionalParameterName.


> Also, that bit of the specs is not entirely clear to me: why do we need the
> indices?

it's from https://github.com/binast/ecmascript-binary-ast/issues/50
it's to figure out which binding may share a value with `arguments[i]`.


> @@ +384,5 @@
> > +                return raiseError("AssertedPositionalParameterName: name mismatch");
> > +        } else {
> > +            // Destructuring or rest parameter.
> > +            if (param->isKind(ParseNodeKind::Name))
> > +                return raiseError("AssertedParameterName/AssertedRestParameterName: expected destructuring/rest parameter, got positional parameter");
> 
> Do you need to check that the rest parameter is last? If not, can you
> mention why?

This is handled as a set, so the order doesn't matter.

https://binast.github.io/ecmascript-binary-ast/#sec-checkparameternames
https://binast.github.io/ecmascript-binary-ast/#sec-checkpositionalparameterindex

we could add extra restriction, but I guess it's not so important,
given rest parameter can also be destructuring.


> ::: js/src/frontend/BinSource.yaml
> @@ +266,5 @@
> > +    extra-params: |
> > +        AssertedScopeKind scopeKind,
> > +        MutableHandle<GCVector<JSAtom*>> positionalParams
> > +    extra-args: |
> > +        scopeKind, positionalParams
> 
> Side-note: the flow of extra-args is becoming a bit complicated to follow
> without looking at generated source :)
> 
> If you have ideas of how to document them, that would be nice.

Where should it be?


> @@ +273,5 @@
> > +    inherits: AssertedMaybePositionalParameterName
> > +    fields:
> > +        name:
> > +            after: |
> > +                // FIXME: This should be checked inside checkPositionalParameterIndices.
> 
> Followup bug?
> 
> Also, what is "this"? The lines just below?

Yes.


> @@ +275,5 @@
> > +        name:
> > +            after: |
> > +                // FIXME: This should be checked inside checkPositionalParameterIndices.
> > +                if (index >= positionalParams.get().length())
> > +                    return raiseError("AssertedPositionalParameterName.length out of range");
> 
> Just to be sure I understand, that's a file corruption, right?

yes, invalid file.

Comment 25

[Tooru Fujisawa [:arai]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #16)
> @@ +260,3 @@
> >  
> > +    if (isCaptured)
> > +        MOZ_TRY(captureName(scopeKind, name));
> 
> Mmmh... we don't seem to be using the same `scope` in `capturedName` and
> here. Is that normal?

Oh, indeed.
I'll replace it with directly setting setClosedOver on ptr.


> @@ +303,5 @@
> > +        scope = parseContext_->innermostScope();
> > +        break;
> > +      case AssertedScopeKind::Parameter:
> > +        declKind = DeclarationKind::PositionalFormalParameter;
> > +        scope = &parseContext_->functionScope();
> 
> What happens if we're not in a function scope? Can it happen?

It shouldn't happen, given AssertedScopeKind::Parameter is used only from function.
added extra explicit assertion.


> @@ +316,4 @@
> >  
> > +template<typename Tok> JS::Result<Ok>
> > +BinASTParser<Tok>::captureName(AssertedScopeKind scopeKind, HandleAtom name)
> > +{
> 
> There seems to be some duplication with `getBoundScope`/`getDeclaredScope`.
> Not a problem, but if an opportunity ever arises to reuse them, we may want
> to take it.

Just removed it, as mentioned above :)

Comment 26

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tooru Fujisawa [:arai] from comment #10)
> forgot to mention.
> I used "0.3.9" for binjs_meta in Cargo.toml.
> will update Cargo.lock and in-tree cache once it's released.

Just released, by the way.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tooru Fujisawa [:arai] from comment #23)

> > How comes we're not using this field or the next one?
> 
> isThisCaptured is for arrow function and it's on TODO in
> BinASTParser<Tok>::buildFunction.
> 
> isFunctionNameCaptured is used in Part 4.

Makes sense.

> we could add extra restriction, but I guess it's not so important,
> given rest parameter can also be destructuring.

Agreed. Could you mention somewhere that we are treating it as a set, though?

Also, I haven't checked, but is there a check that there is only one rest param?

> > If you have ideas of how to document them, that would be nice.
>
> Where should it be?

I can't find any good place to put that doc, so I guess where you define the value that you pass as parameter?

> > There seems to be some duplication with `getBoundScope`/`getDeclaredScope`.
> > Not a problem, but if an opportunity ever arises to reuse them, we may want
> > to take it.
>
> Just removed it, as mentioned above :)

\o/

Comment 28

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dabde322d1eed4a1896a039a8af5024ae98706d8
Bug 1472103 - Part 1: Update BinAST IDL. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/505de5ae50f6a16e4a17df81a893d48dc14b4d04
Bug 1472103 - Part 1.1: Result of mach vendor rust. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/27d9c733afe70703a808b0982127d2c14bc16dc0
Bug 1472103 - Part 2: Add AssertedPositionalParameterName, AssertedRestParameterName, and AssertedParameterName. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bc01bea9a69ffa465d9a064331b36cd70abde4
Bug 1472103 - Part 3: Add length field to functions. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f744a334b722ae643e693e93e40b685ebff0f65
Bug 1472103 - Part 4: Remove function expression name from parameter scope. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8cab9c3ee74274c463865dcec5cf853af287f47
Bug 1472103 - Part 5: Update BinAST tests. r=Yoric

Comment 29

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c81a0463e13da56ffd2ab8a6397e8f6f728336af
Bug 1472103 - Followup: add reason for MOZ_ASSERT_UNREACHABLE. r=me

Comment 30

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a521c24243626f692293e02c0460d78b442f136
Bug 1472103 - Followup 2: Update binjs file in wpt test. r=me

Comment 31

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/dabde322d1ee
https://hg.mozilla.org/mozilla-central/rev/505de5ae50f6
https://hg.mozilla.org/mozilla-central/rev/27d9c733afe7
https://hg.mozilla.org/mozilla-central/rev/c9bc01bea9a6
https://hg.mozilla.org/mozilla-central/rev/1f744a334b72
https://hg.mozilla.org/mozilla-central/rev/c8cab9c3ee74
https://hg.mozilla.org/mozilla-central/rev/c81a0463e13d
https://hg.mozilla.org/mozilla-central/rev/2a521c242436