Closed
Bug 1451343
Opened 6 years ago
Closed 6 years ago
[BinAST] Verify name capture
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Yoric, Assigned: efaust)
References
Details
Attachments
(2 files)
5.24 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
5.82 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
Bug 1437004 and bug 1439855 implement most of the BinAST parser (for a subset of JavaScript), but are still missing verification of captured names. We need to implement this before releasing.
Reporter | ||
Updated•6 years ago
|
Blocks: binjs-implement
Updated•6 years ago
|
Priority: -- → P3
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → arai.unmht
Assignee | ||
Comment 1•6 years ago
|
||
I have patches for this! I didn't know there was talk of someone else implementing it, so I did >.>. Will put it up for review this afternoon, so Yoric and arai can take a look at it.
Reporter | ||
Comment 2•6 years ago
|
||
In this case, reassigning to efaust.
Assignee: arai.unmht → efaustbmo
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8970816 -
Flags: review?(dteller)
Assignee | ||
Comment 4•6 years ago
|
||
Attachment #8970817 -
Flags: review?(dteller)
Reporter | ||
Comment 5•6 years ago
|
||
Comment on attachment 8970816 [details] [diff] [review] Part 1: Properly use provided capture information Review of attachment 8970816 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BinSource.cpp @@ +198,4 @@ > return result; > } > > +static bool TryMarkCaptureInScope(ParseContext::Scope& scope, HandleAtom atom) What does the return value mean? Is it an error? If so, shouldn't you propagate it in all cases? @@ +199,5 @@ > } > > +static bool TryMarkCaptureInScope(ParseContext::Scope& scope, HandleAtom atom) > +{ > + auto name = scope.lookupDeclaredName(atom); We can have `atom == nullptr`, right? Shouldn't we bailout in that case? @@ +224,5 @@ > + MOZ_ASSERT(parseContext_->isFunctionBox()); > + > + if (parseContext_->functionBox()->function()->isNamedLambda()) { > + if (TryMarkCaptureInScope(parseContext_->namedLambdaScope(), name)) > + continue; Why don't we propagate the error (if it's an error)? @@ +234,5 @@ > + } > + > + if (kind == BinKind::AssertedVarScope) { > + if (TryMarkCaptureInScope(parseContext_->varScope(), name)) > + continue; Here, too?
Attachment #8970816 -
Flags: review?(dteller) → review+
Reporter | ||
Comment 6•6 years ago
|
||
Comment on attachment 8970817 [details] [diff] [review] Part 2: Verify capture information when parsing scopes. Review of attachment 8970817 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BinSource.cpp @@ +154,1 @@ > BinASTParser<Tok>::buildFunction(const size_t start, const BinKind kind, ParseNode* name, Nit: Could you take the opportunity to document somewhere that we call this method after creating the scope? @@ +154,5 @@ > BinASTParser<Tok>::buildFunction(const size_t start, const BinKind kind, ParseNode* name, > ParseNode* params, ParseNode* body, FunctionBox* funbox) > { > + // Check all our bindings before doing anything else. > + MOZ_TRY(checkFunctionClosedVars()); This doesn't check arguments, right? Do we need to check them, too? If not, could you document why? ::: js/src/frontend/BinSource.h @@ +195,4 @@ > DeclarationKind kind); > MOZ_MUST_USE JS::Result<Ok> parseAndUpdateCapturedNames(const BinKind kind); > MOZ_MUST_USE JS::Result<Ok> checkBinding(JSAtom* name); > + MOZ_MUST_USE JS::Result<Ok> checkClosedVars(ParseContext::Scope& scope); Nit: A little documentation would be nice, now that there is more than one person working on that code :) ::: js/src/frontend/BinSource.yaml @@ +688,4 @@ > build: | > if (!IsIdentifier(name)) > return raiseError("Invalid identifier"); > + BINJS_TRY(usedNames_.noteUse(cx_, name, parseContext_->scriptId(), parseContext_->innermostScope()->id())); Out of curiosity, what's that for?
Attachment #8970817 -
Flags: review?(dteller) → review+
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Pushed by efaustbmo@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/03130e40886b Part 1: Use provided capture information. (r=Yoric) https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0675c2f362 Part 2: Check annotated closed vars against known uses. (r=Yoric)
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/03130e40886b https://hg.mozilla.org/mozilla-central/rev/cf0675c2f362
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•