Closed Bug 1472103 Opened Last year Closed Last year

[BinAST] Update in-tree IDL

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: arai, Assigned: arai)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 4 obsolete files)

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.
Attached patch (WIP) Update BinAST IDL. (obsolete) — Splinter Review
In order to run this, dependencies in js/src/frontend/binsource/Cargo.toml should be changed to point updated binjs_meta.
Priority: -- → P3
Blocks: 1455547
No longer blocks: 1472101
Depends on: 1472101
Attached patch (WIP) Update BinAST IDL. (obsolete) — Splinter Review
fixed based on bug 1473202
Attachment #8988671 - Attachment is obsolete: true
Attached patch (WIP) Part 1: Update BinAST IDL. (obsolete) — Splinter Review
Assignee: nobody → arai.unmht
Attachment #8990177 - Attachment is obsolete: true
Status: NEW → ASSIGNED
https://github.com/binast/binjs-ref/pull/144 should be fixed before this, in order to properly update in-tree rust files and dependency.
* 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)
Attachment #8995072 - Attachment is obsolete: true
Attachment #9007678 - Flags: review?(dteller)
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.
* 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)
Attachment #9007679 - Flags: review?(dteller)
* added length field to functions
  * check length in contents field, after parsing params
Attachment #9007680 - Flags: review?(dteller)
* reflect isFunctionNameCaptured directly to named lambda scope
  * added AssertedScopeKind::FunctionExprName to special case function name
    separate from existing AssertedScopeKind::Parameter
Attachment #9007681 - Flags: review?(dteller)
the result of binjs_encode for each file, with current binjs-ref master
Attachment #8995073 - Attachment is obsolete: true
Attachment #9007682 - Flags: review?(dteller)
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 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 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?
Attachment #9007678 - Flags: review?(dteller) → review+
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 on attachment 9007679 [details] [diff] [review]
Part 2: Add AssertedPositionalParameterName, AssertedRestParameterName, and AssertedParameterName.

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

(part 2)
Attachment #9007679 - Flags: review?(dteller) → review+
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.
Attachment #9007680 - Flags: review?(dteller) → review+
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?
Attachment #9007681 - Flags: review?(dteller) → review+
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?
Attachment #9007682 - Flags: review?(dteller) → review+
(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.
(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.
(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 :)
(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.
(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/
Blocks: 1490976
You need to log in before you can comment on or make changes to this bug.