1296814 - Remove ParseHandler::getPosition(Node)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jeff Walden [:Waldo]]

It's correct for ParseHandler = FullParseHandler, because then Node = ParseNode* which has ParseNode::pos().  But it's blatant lies for ParseHandler = SyntaxParseHandler, using the current tokenStream position which may be totally unrelated to the alleged node.

Basically getPosition(Node) needs to disappear, and the callers of it will have to compute the relevant position some other way.  This is probably fairly simple in some cases (peek at the position to get a begin, use current position to get an end), harder in others where we're acting on something long since computed.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Implement TokenStream::peekOffset for cases where peekTokenPos was used only to get an offset

I'm not yet finished excising SyntaxParseHandler::getPosition, but I have a large stack of patches that remove all but (depending how you count) ten or so direct and indirect uses of the function.  Along the way, I've also significantly cleaned up the parser's error-reporting mechanism -- it's a lot more readable in the end.

It's pretty common to need to peek for an offset to report errors at specific locations, so adding a way to do that (and not require grabbing an entire Position) is a useful first step.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Give correct position information to pending errors during potential destructuring patterns

One general thrust of all this bug's work is going to be to eliminate "null()" as a thing anyone ever specifies.  This patch removes such things with respect to PossibleError.  It's pretty simple.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Give correct position information to prototype mutation when syntax-parsing

An easy case, because (now) we have an offset lying around to use.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Don't use ParseHandler::getPosition in a FullParseHandler-specialized function -- just use pn_pos directly

Comment 5

[Jeff Walden [:Waldo]]

Attachment: When |continue x| is found outside of a loop, report an error that points at |continue|, not at |x|

This obviously changes semantics somewhat.  Given the way the error message is worded, this seems clearly desirable to me.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Split Parser::report into Parser::reportWithNode that derives an offset from a Node and a separate function that always uses the current offset

Parser::report takes both nodes and null.  From this bug's point of view, only the former case is bad -- as long as callers really do intend to pick up the current position in the latter case.  So, split them in two.

The name of the non-reportWithNode function may be a little controversial.  Because I had to touch so many places, I *needed* a function with a different name to catch mistakes when I removed the Node argument from the signature.  The name was simply something that's easy to search for and was of the same length, nothing more.  Ordinarily there'd probably be a request to squash patches together to avoid these goofy names at intermediate states, but because using simpler functions requires other refactoring along the way, it won't be entirely possible to do so.  I hope it's not too much of a problem if I end up landing this patch *un*-squashed, as long as it lands as part of a revset that doesn't leave the temporary name in place.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Split out Parser::strictError for the two calls that don't pass |bool strict = false|, so that |bool strict| can be removed from the current signature

Parser::report or whatever is just not a very good name.  That we "report" errors is an implementation detail, but from the point of view of parser code, we're just handling things that are strict mode errors, or errors, or (SpiderMonkey-specific) warnings.  Let's start adding functions with far-simpler names that hide all the reporting gunk outside the name.

Unfortunately, because all the reporting functions use varargs right now, having better-named functions for these is going to result in near-duplication of Parser::report's basic code.  Can't be helped for now.  And it's really very little duplication, so it's not the end of the world for the moment.  Maybe when we're done we can kill some of it off, maybe even by getting rid of varargs entirely from error reporting code.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Remove the |bool strict| argument from the report-at-current-offset Parser function

The strictness argument to Parser::report is almost always false, and it's really only used for ParseStrictError (which is slowly being removed in favor of strictModeError, and in a later patch here strictModeErrorAt).  Remove it from the signature, bump Parser::report to yet another uniquely-searchable name.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Introduce Parser::extraWarning

This is tons better than having to specify ParseExtraWarning manually, amirite?

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Introduce Parser::warning

Same song, second verse.  There may be other ParseWarning uses to be cleaned up after this patch: this only changes the simple ones.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Introduce Parser::error(unsigned errorNumber, ...) to reduce reporting errors at the current offset to its bare essentials

And now Parser::report in its latest bizarro-name is gone, in favor of Parser::error that's as stripped-down as it can possibly be.  (A Parser::errorAt to handle custom-offset cases is a later patch.)

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Specify an explicit offset when warning about "use asm" found in the directive prologue of a script (rather than a function body)

A Parser::warningAt to do this slightly more simply is further down in the patch stack.  I'm loathe to rearrange to be able to use it, tho.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Report strict mode errors in unary deletions using an explicit offset

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Track strict mode errors in |for (var i = ... in ...)| correctly when syntax-parsing

It's arguable exactly what offset should be used for the error, for some of this stuff.  I generally tried to do *a* reasonable thing, at least reasonable enough to point at the general area.  But there might be some mild debate in some cases.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Move FunctionDeclaration-as-consequent/alternative handling out of Parser::functionStmt into Parser::consequentOrAlternative

There's a |handler.getPosition| call deep in function-parsing code that needs to go.  But first, a bunch of refactoring to move it upward to the point where the offset is actually available, one bit at a time.

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Move a little bit of Parser::functionDefinition into callers

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Move the Parser::checkFunctionDefinition call, out of Parser::functionDefinition, into its one Statement-related caller

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Inline Parser::checkFunctionDefinition into its sole caller

It's somewhat worth noting the position was always the current offset -- but that was hardly obvious with the byzantine current code.  This removes that pesky getPosition call.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Introduce Parser::errorAt to reduce reporting an error at a particular offset to its bare essentials

Similar to Parser::error, Parser::errorAt for when you don't want the current offset.

I should belatedly note that error/errorAt are both void, while the other functions that sometimes return true, sometimes false, are MOZ_MUST_USE.  A nice win from the current setup where we *can't* have Parser::report's return value be MOZ_MUST_USE because many callers just assume it returns false based on a particular constant argument.

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Remove Parser::reportBadReturn and report simpler errors that don't use the offset of a node as location

After all the various refactoring, now we have *two* transgressors to remove: getPosition calls, but also now reportWithNode calls.  Unfortunately picking these off is tedious, but at least it can be a one-at-a-time thing and is pretty bite-size.

Comment 21

[Jeff Walden [:Waldo]]

I should note on that last patch that, yes, I do futz with error messages a bit.  It doesn't seem to me the function name is useful enough to warrant a bunch of extra code to include it in the error message.  Most of our error messages are oriented toward exactly what was done wrong, in the narrow code that was touched.  This doesn't really need to be different.  And I expect it's a pretty rare error to hit (not least because it requires legacy generators), so it's not worth the trouble to prettify it up.

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Report bad-class-member errors using a specified offset instead of a node's offset

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Report for-loop-decl-with-initializer errors using a specified offset instead of a node's offset

We could probably justify a bunch of offsets for these error messages.  Pointing at the initializer seems fair enough to me.

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Introduce Parser::warningAt for warnings at specified offsets

Happily, at this point reportWithOffset is gone, so that's one reporting mechanism dead, rather than continuing to add them.

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Report the error for uninitialized const-declaration in for(;;) loop head using an explicit offset

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Report some errors about invalid left-hand-sides in for-in/of loop heads using much-simpler code with an explicitly computed offset

Actually, a whole bunch of assignment-target-validating code is a bit verbose/complicated for no good reason.  So there's a bit of refactoring here, in order to simplify it, and also to eliminate reportWithNode calls.

I wrote these mostly by stepwise-refactoring the existing code into this.  It's an open question whether that's the best way to write it and to review it, or just going to the spec semantics is easier and faster.  Up to you.  (I'm leaning on tests to pick up any trash.  :-) )

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Remove for-in/of loop parsing code that redundantly marks the loop target as assigned -- Parser::forHeadStart already does this

I could be mistaken about this assessment, but as I compare the target-validating code and the assignment-checking code, they look totally duplicative, so the latter can be removed.  I haven't actually tested this yet (except cursorily with subsets of jstests), so it's possible I'm wrong.  But I doubt it.

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Simplify checking of the left-hand side of assignment and compound assignment expressions

The for-in/of target validating code and the assignment target validating code are now pretty similar.  They *could* be unified, except for the destructuring distinction.  But I'm also a little leery of passing around an offset for error-reporting purposes unless I absolutely have to, and I think having the elaboration in two places is probably clearer to the reader anyway.

Comment 29

[Jeff Walden [:Waldo]]

Attachment: Specify an explicit offset when reporting an error for a for-of loop whose target is an expression that begins with 'let'

Comment 30

[Jeff Walden [:Waldo]]

And, that's where I sit right now, as far as getPosition/reportWithNode removals goes.  Still more work to do to complete this bug.

But I'm getting tired of having the 30-odd patches bloating up my queue, tho, even if the overall work isn't done yet, so I wanted to get it out there so maybe I can not have to deal with such a big queue again.  (Also, with the Parser::report renaming, any change to error reporting in the parser means I have to deal with bitrot in several places in my mq, so not having to do that would be great.  And I expect people will really like the new error-reporting APIs versus the current one that basically *requires* copypasta.)

Comment 31

[Tooru Fujisawa [:arai]]

Comment on attachment 8810046 [details] [diff] [review]
Split Parser::report into Parser::reportWithNode that derives an offset from a Node and a separate function that always uses the current offset

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

::: js/src/frontend/Parser.cpp
@@ +4688,5 @@
>          if (!tokenStream.getToken(&tt))
>              return false;
>  
>          if (tt != TOK_NAME || tokenStream.currentName() != context->names().as) {
> +            reportWithOffset(ParseError, false, pos().begin, JSMSG_AS_AFTER_IMPORT_STAR);

maybe zeport ?
(of course this has same effect tho)

Comment 32

[Tooru Fujisawa [:arai]]

Comment on attachment 8810061 [details] [diff] [review]
Remove Parser::reportBadReturn and report simpler errors that don't use the offset of a node as location

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

::: js/src/js.msg
@@ +196,5 @@
>  MSG_DEF(JSMSG_BAD_FOR_LEFTSIDE,        0, JSEXN_SYNTAXERR, "invalid for-in/of left-hand side")
>  MSG_DEF(JSMSG_LEXICAL_DECL_DEFINES_LET,0, JSEXN_SYNTAXERR, "a lexical declaration can't define a 'let' binding")
>  MSG_DEF(JSMSG_LET_STARTING_FOROF_LHS,  0, JSEXN_SYNTAXERR, "an expression X in 'for (X of Y)' must not start with 'let'")
> +MSG_DEF(JSMSG_BAD_FUNCTION_YIELD,      0, JSEXN_TYPEERR, "can't use 'yield' in a function that can return a value")
> +MSG_DEF(JSMSG_BAD_GENERATOR_RETURN,    0, JSEXN_TYPEERR,   "generator function can't return a value")

It's better suggesting to add "*" to "function" or method name.

these errors can happen in 2 cases.
  a) someone want to use legacy generator, and didn't know that "return v" is forbidden
  b) someone want to use ES6 generator, but forgot to add "*" to "function", or before method name

these days, (b) should happen more.
so suggesting legacy generator's expected syntax is misleading and unhelpful.

of course that is not-related to this refactoring, and I'm fine to leave it to other bug :)

Comment 33

[Tooru Fujisawa [:arai]]

Comment on attachment 8810069 [details] [diff] [review]
Specify an explicit offset when reporting an error for a for-of loop whose target is an expression that begins with 'let'

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

looks like these patches imply my "error notes" patch needs to be rewritten to follow the new name/signature ;)

Comment 34

[Tooru Fujisawa [:arai]]

maybe, Parser<ParseHandler>::reportHelper will be removed in later patch?

Comment 35

[André Bargull [:anba]]

Comment on attachment 8810042 [details] [diff] [review]
Give correct position information to pending errors during potential destructuring patterns

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

::: js/src/frontend/Parser.h
@@ +834,5 @@
>  
>          // Set a pending expression error. Only a single error may be set per
>          // instance, i.e. subsequent calls to this method are ignored and won't
>          // overwrite the existing pending error.
> +        void setPendingExpressionErrorAt(const TokenPos& pos, unsigned errorNumber);

Nit: The example code in PossibleError's doc-comment needs to be updated.

Comment 36

[André Bargull [:anba]]

Comment on attachment 8810058 [details] [diff] [review]
Move the Parser::checkFunctionDefinition call, out of Parser::functionDefinition, into its one Statement-related caller

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

::: js/src/frontend/ParseNode.h
@@ +921,5 @@
>      {
>          MOZ_ASSERT(kind == PNK_FUNCTION || kind == PNK_MODULE);
> +        MOZ_ASSERT(op == JSOP_NOP || // statement
> +                   op == JSOP_LAMBDA_ARROW || // arrow function
> +                   op == JSOP_LAMBDA); // expression, method, comprehension, accessor, module, &c.

nit: `module` uses JSOP_NOP, not JSOP_LAMBDA.

Maybe add: MOZ_ASSERT_IF(kind == PNK_MODULE, op == JSOP_NOP);

::: js/src/frontend/Parser.cpp
@@ +9358,5 @@
> +        break;
> +
> +      default:
> +        MOZ_CRASH("unexpected property type");
> +    }

Why was FunctionSyntaxKindFromPropertyType() removed? I'd probably keep it for consistency with GeneratorKindFromPropertyType and AsyncKindFromPropertyType.

::: js/src/frontend/SyntaxParseHandler.h
@@ +346,5 @@
>      MOZ_MUST_USE bool setLastFunctionFormalParameterDefault(Node funcpn, Node pn) { return true; }
> +
> +    Node newFunctionStatement() { return NodeFunctionDefinition; }
> +    Node newArrowFunction() { return NodeFunctionDefinition; }
> +    Node newFunctionExpression() { return NodeFunctionDefinition; }

Nit: Move newFunctionExpression() before newArrowFunction() to match FullParseHandler

Comment 37

[André Bargull [:anba]]

Comment on attachment 8810061 [details] [diff] [review]
Remove Parser::reportBadReturn and report simpler errors that don't use the offset of a node as location

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

::: js/src/js.msg
@@ +195,5 @@
>  MSG_DEF(JSMSG_BAD_FOR_EACH_LOOP,       0, JSEXN_SYNTAXERR, "invalid for each loop")
>  MSG_DEF(JSMSG_BAD_FOR_LEFTSIDE,        0, JSEXN_SYNTAXERR, "invalid for-in/of left-hand side")
>  MSG_DEF(JSMSG_LEXICAL_DECL_DEFINES_LET,0, JSEXN_SYNTAXERR, "a lexical declaration can't define a 'let' binding")
>  MSG_DEF(JSMSG_LET_STARTING_FOROF_LHS,  0, JSEXN_SYNTAXERR, "an expression X in 'for (X of Y)' must not start with 'let'")
> +MSG_DEF(JSMSG_BAD_FUNCTION_YIELD,      0, JSEXN_TYPEERR, "can't use 'yield' in a function that can return a value")

Nit: Align message string with previous line.

Comment 38

[André Bargull [:anba]]

Comment on attachment 8810063 [details] [diff] [review]
Report for-loop-decl-with-initializer errors using a specified offset instead of a node's offset

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

::: js/src/js.msg
@@ +264,1 @@
>  MSG_DEF(JSMSG_INVALID_FOR_IN_DECL_WITH_INIT,0,JSEXN_SYNTAXERR,"for-in loop head declarations may not have initializers")

JSMSG_OF_AFTER_FOR_LOOP_DECL, JSMSG_IN_AFTER_LEXICAL_FOR_DECL, and JSMSG_INVALID_FOR_IN_DECL_WITH_INIT should probably use similar error identifiers and messages strings.

Comment 39

[André Bargull [:anba]]

Comment on attachment 8810066 [details] [diff] [review]
Report some errors about invalid left-hand-sides in for-in/of loop heads using much-simpler code with an explicitly computed offset

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

AssignmentFlavor::ForInOrOfTarget doesn't seem to be used anymore.

Comment 40

[André Bargull [:anba]]

Comment on attachment 8810067 [details] [diff] [review]
Remove for-in/of loop parsing code that redundantly marks the loop target as assigned -- Parser::forHeadStart already does this

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

I'm just as confused as you are why this code is duplicated. But the removal looks correct.

Comment 41

[André Bargull [:anba]]

Comment on attachment 8810068 [details] [diff] [review]
Simplify checking of the left-hand side of assignment and compound assignment expressions

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

AssignmentFlavor::PlainAssignment and AssignmentFlavor::CompoundAssignment are now also unused, aren't they?

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/560587c09cb3
Implement TokenStream::peekOffset for cases where peekTokenPos was used only to get an offset.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d72c9a1dfd6
Give correct position information to pending errors during potential destructuring patterns.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3620a96d8e
Give correct position information to prototype mutation when syntax-parsing.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5874a0aa94
Don't use ParseHandler::getPosition in a FullParseHandler-specialized function -- just use pn_pos directly.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa42c1f05ae9
When |continue x| is found outside of a loop, report an error that points at |continue|, not at |x|.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/645e9d747ed9
Split Parser::report into Parser::zeport (a temporary name) that uses the current offset, and Parser::reportWithNode that derives it from a Node.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe83ace68894
Split out Parser::strictError for the two calls that don't pass |bool strict = false|, so that |bool strict| can be removed from the current signature.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8ade7340f1
Remove the |bool strict| argument from the report-at-current-offset Parser function.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/1404e070a44c
Introduce Parser::extraWarning.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6329e0a6f96
Introduce Parser::warning.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae58d7e8cbf0
Introduce Parser::error(unsigned errorNumber, ...) to reduce reporting errors at the current offset to its bare essentials.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26d1552f8a1
Specify an explicit offset when warning about "use asm" found in the directive prologue of a script (rather than a function body).  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0b80ff460d
Track strict mode errors in unary deletions correctly when syntax-parsing.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5162f3e8c6
Track strict mode errors in |for (var i = ... in ...)| correctly when syntax-parsing.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/c770f757dbc6
Move FunctionDeclaration-as-consequent/alternative handling out of Parser::functionStmt into Parser::consequentOrAlternative.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ee58a58aa7
Move a little bit of Parser::functionDefinition into callers.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/5908c363b5ee
Move the Parser::checkFunctionDefinition call, out of Parser::functionDefinition, into its few Statement-related callers.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f6b90c78c4b
Inline GeneratorKindFromPropertyType and AsyncKindFromPropertyType into their sole caller.  r=me as trivial, and/or as response to review comment on previous patch
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b4b6a642aeb
Inline Parser::checkFunctionDefinition into its sole caller.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec9d6149b5e
Introduce Parser::errorAt to reduce reporting an error at a particular offset to its bare essentials.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/a09a93a0b5a9
Remove Parser::reportBadReturn and report simpler errors that don't use the offset of a node as location.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/8035dbcdb2c3
Report bad-class-member errors using a specified offset instead of a node's offset.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4352d5b580d3
Report for-loop-decl-with-initializer errors using a specified offset instead of a node's offset.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaee337ec646
Introduce Parser::warningAt for warnings at specified offsets.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea1d15d3f86
Report the error for uninitialized const-declaration in for(;;) loop head using an explicit offset.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bce123564a3
Report some errors about invalid left-hand-sides in for-in/of loop heads using much-simpler code with an explicitly computed offset.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/06a39a03a254
Remove for-in/of loop parsing code that redundantly marks the loop target as assigned -- Parser::forHeadStart already does this.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff9398b03d6
Simplify checking of the left-hand side of assignment and compound assignment expressions.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6b4c2d10f5
Specify an explicit offset when reporting an error for a for-of loop whose target is an expression that begins with 'let'.  r=arai

Comment 43

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #31)
> maybe zeport ?

Fixed up in a later patch, somewhere.


(In reply to Tooru Fujisawa [:arai] from comment #32)
> @@ +196,5 @@
> >  MSG_DEF(JSMSG_BAD_FOR_LEFTSIDE,        0, JSEXN_SYNTAXERR, "invalid for-in/of left-hand side")
> >  MSG_DEF(JSMSG_LEXICAL_DECL_DEFINES_LET,0, JSEXN_SYNTAXERR, "a lexical declaration can't define a 'let' binding")
> >  MSG_DEF(JSMSG_LET_STARTING_FOROF_LHS,  0, JSEXN_SYNTAXERR, "an expression X in 'for (X of Y)' must not start with 'let'")
> > +MSG_DEF(JSMSG_BAD_FUNCTION_YIELD,      0, JSEXN_TYPEERR, "can't use 'yield' in a function that can return a value")
> > +MSG_DEF(JSMSG_BAD_GENERATOR_RETURN,    0, JSEXN_TYPEERR,   "generator function can't return a value")
> 
> It's better suggesting to add "*" to "function" or method name.
> 
> these errors can happen in 2 cases.
>   a) someone want to use legacy generator, and didn't know that "return v"
> is forbidden
>   b) someone want to use ES6 generator, but forgot to add "*" to "function",
> or before method name
> 
> these days, (b) should happen more.
> so suggesting legacy generator's expected syntax is misleading and unhelpful.

Fair point.  Filed bug 1317520.  (Although, if we could remove legacy generators instead, that'd be even better.)

(In reply to Tooru Fujisawa [:arai] from comment #34)
> maybe, Parser<ParseHandler>::reportHelper will be removed in later patch?

Ideally.  See also musings in comment 7.

(In reply to André Bargull from comment #36)
> Why was FunctionSyntaxKindFromPropertyType() removed? I'd probably keep it
> for consistency with GeneratorKindFromPropertyType and
> AsyncKindFromPropertyType.

I removed it because I needed it to be clear that the caller, at that spot, wasn't dealing with function statements at all.

For consistency, I inlined the other two functions (also only called in this one spot) as well.  Less code, more readable than having to scroll a bunch to figure out what the functions did.

(In reply to André Bargull from comment #38)
> JSMSG_OF_AFTER_FOR_LOOP_DECL, JSMSG_IN_AFTER_LEXICAL_FOR_DECL, and
> JSMSG_INVALID_FOR_IN_DECL_WITH_INIT should probably use similar error
> identifiers and messages strings.

Filed bug 1317523.

(In reply to André Bargull from comment #39)
> AssignmentFlavor::ForInOrOfTarget doesn't seem to be used anymore.
(In reply to André Bargull from comment #41)
> AssignmentFlavor::PlainAssignment and AssignmentFlavor::CompoundAssignment
> are now also unused, aren't they?

Hm, I thought I'd removed these.  Done.

More patching still to come, but at least now |hg qser| fits on a single terminal screen again, and rebasing doesn't take forever and a day.

Comment 44

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #43)
> (In reply to André Bargull from comment #36)
> > Why was FunctionSyntaxKindFromPropertyType() removed? I'd probably keep it
> > for consistency with GeneratorKindFromPropertyType and
> > AsyncKindFromPropertyType.
> 
> I removed it because I needed it to be clear that the caller, at that spot,
> wasn't dealing with function statements at all.
> 
> For consistency, I inlined the other two functions (also only called in this
> one spot) as well.  Less code, more readable than having to scroll a bunch
> to figure out what the functions did.

Okay, works for me. :-)


> (In reply to André Bargull from comment #38)
> > JSMSG_OF_AFTER_FOR_LOOP_DECL, JSMSG_IN_AFTER_LEXICAL_FOR_DECL, and
> > JSMSG_INVALID_FOR_IN_DECL_WITH_INIT should probably use similar error
> > identifiers and messages strings.
> 
> Filed bug 1317523.

Thanks!


> More patching still to come, but at least now |hg qser| fits on a single
> terminal screen again, and rebasing doesn't take forever and a day.

Achievement unlocked!

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/560587c09cb3
https://hg.mozilla.org/mozilla-central/rev/2d72c9a1dfd6
https://hg.mozilla.org/mozilla-central/rev/5d3620a96d8e
https://hg.mozilla.org/mozilla-central/rev/da5874a0aa94
https://hg.mozilla.org/mozilla-central/rev/fa42c1f05ae9
https://hg.mozilla.org/mozilla-central/rev/645e9d747ed9
https://hg.mozilla.org/mozilla-central/rev/fe83ace68894
https://hg.mozilla.org/mozilla-central/rev/6b8ade7340f1
https://hg.mozilla.org/mozilla-central/rev/1404e070a44c
https://hg.mozilla.org/mozilla-central/rev/a6329e0a6f96
https://hg.mozilla.org/mozilla-central/rev/ae58d7e8cbf0
https://hg.mozilla.org/mozilla-central/rev/f26d1552f8a1
https://hg.mozilla.org/mozilla-central/rev/8b0b80ff460d
https://hg.mozilla.org/mozilla-central/rev/7b5162f3e8c6
https://hg.mozilla.org/mozilla-central/rev/c770f757dbc6
https://hg.mozilla.org/mozilla-central/rev/d2ee58a58aa7
https://hg.mozilla.org/mozilla-central/rev/5908c363b5ee
https://hg.mozilla.org/mozilla-central/rev/2f6b90c78c4b
https://hg.mozilla.org/mozilla-central/rev/4b4b6a642aeb
https://hg.mozilla.org/mozilla-central/rev/9ec9d6149b5e
https://hg.mozilla.org/mozilla-central/rev/a09a93a0b5a9
https://hg.mozilla.org/mozilla-central/rev/8035dbcdb2c3
https://hg.mozilla.org/mozilla-central/rev/4352d5b580d3
https://hg.mozilla.org/mozilla-central/rev/aaee337ec646
https://hg.mozilla.org/mozilla-central/rev/4ea1d15d3f86
https://hg.mozilla.org/mozilla-central/rev/3bce123564a3
https://hg.mozilla.org/mozilla-central/rev/06a39a03a254
https://hg.mozilla.org/mozilla-central/rev/0ff9398b03d6
https://hg.mozilla.org/mozilla-central/rev/5f6b4c2d10f5

Comment 46

[Jeff Walden [:Waldo]]

Attachment: Inline Parser::checkAssignmentToCall into its one caller for clarity

Comment 47

[Jeff Walden [:Waldo]]

Attachment: Simplify increment/decrement operand checking

Comment 48

[Jeff Walden [:Waldo]]

Attachment: Simplify checking of targets within destructuring patterns

Comment 49

[Jeff Walden [:Waldo]]

Attachment: Report errors for bad increment/decrement operands using explicitly-specified offsets

Comment 50

[Jeff Walden [:Waldo]]

After those patches, all that remain are two bits of code still to fix:

1. Forbidding strict mode binding of arguments/eval-named functions.
2. Something about the position of implicit return statements in expression-closure functions (and I think including arrow functions) that's strangely not-quite trivial.

I have ideas for mostly fixing the former case -- and for a very small semantic change for the bits that may be unfixable.  (I only think I just understood the reason in the last 30s, so I might be missing something still.  More work needed.)  As for the latter, I still need to figure it out.

The end is in sight!

Comment 51

[André Bargull [:anba]]

Comment on attachment 8812068 [details] [diff] [review]
Simplify increment/decrement operand checking

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

::: js/src/js.msg
@@ +201,5 @@
>  MSG_DEF(JSMSG_LET_STARTING_FOROF_LHS,  0, JSEXN_SYNTAXERR, "an expression X in 'for (X of Y)' must not start with 'let'")
>  MSG_DEF(JSMSG_BAD_FUNCTION_YIELD,      0, JSEXN_TYPEERR,   "can't use 'yield' in a function that can return a value")
>  MSG_DEF(JSMSG_BAD_GENERATOR_RETURN,    0, JSEXN_TYPEERR,   "generator function can't return a value")
>  MSG_DEF(JSMSG_BAD_GENEXP_BODY,         1, JSEXN_SYNTAXERR, "illegal use of {0} in generator expression")
> +MSG_DEF(JSMSG_BAD_INCOP_OPERAND,       0, JSEXN_SYNTAXERR, "invalid increment/decrement operand")

|"use strict"; ++f();| will now report a syntax error instead of a reference error. But given that we also report a syntax error for |++0;| contrary to the spec, I don't think this is an issue. (I still hope the spec will be changed one day to use only early syntax errors.)

Comment 52

[André Bargull [:anba]]

Comment on attachment 8812069 [details] [diff] [review]
Simplify checking of targets within destructuring patterns

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

AssignmentFlavor can be removed now, too.

Comment 53

[André Bargull [:anba]]

Comment on attachment 8812070 [details] [diff] [review]
Report errors for bad increment/decrement operands using explicitly-specified offsets

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

checkAndMarkAsIncOperand() no longer marks the operand, so maybe it should be renamed.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca3b35d540c
Inline Parser::checkAssignmentToCall into its one caller for clarity.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/05a3ca69cea3
Simplify increment/decrement operand checking.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/38a15e0978e0
Simplify checking of targets within destructuring patterns.  r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2c8cac755b
Report errors for bad increment/decrement operands using explicitly-specified offsets.  r=anba

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ca3b35d540c
https://hg.mozilla.org/mozilla-central/rev/05a3ca69cea3
https://hg.mozilla.org/mozilla-central/rev/38a15e0978e0
https://hg.mozilla.org/mozilla-central/rev/cf2c8cac755b

Comment 56

[Jeff Walden [:Waldo]]

Attachment: Don't report an extra warning (throw, if werror is set) when delazifying a function whose code is *not* strict mode, that creates a binding that *would* be forbidden in strict mode code

This was a tricky getPosition call to get rid of.  It passes jstests/jit-tests, but it *is* a little finicky to get rid of an existing check and make it up in theory later.  Might be I'm missing edge cases in this.  Caution warranted!

Comment 57

[Jeff Walden [:Waldo]]

Attachment: v2

Comment 58

[André Bargull [:anba]]

Without this patch, `var fn = function eval() {}` emits this warning (when invoking SM with -w -s):
---
/tmp/t.js:1:18 strict warning: redefining eval is deprecated:
/tmp/t.js:1:18 strict warning: var fn = function eval() {}
/tmp/t.js:1:18 strict warning: ..................^
---

But with the proposed patch, no warning is emitted at all. So I'm still not sure I understand why we no longer want to emit a strict-warning for a function expression named "eval". Especially because we're still showing a strict-warning for other bindings named "eval", e.g. `function f(eval) {}`:
---
/tmp/t.js:1:11 strict warning: redefining eval is deprecated:
/tmp/t.js:1:11 strict warning: function f(eval){}
/tmp/t.js:1:11 strict warning: ...........^
---

Comment 59

[Jeff Walden [:Waldo]]

"no longer want to" is maybe a bit strong.  :-)  My assumption was that Parser::bindingIdentifier would already be emitting such warning for us.  Looks like it doesn't.  That should be fixable.  The only immediate concern that pops up, is that there's a potential that making it warn, might result in multiple warnings in some places.  I *think* this might not be a real issue.  But I'm not quite certain of it.

Comment 60

[Jeff Walden [:Waldo]]

Attachment: v3

Comment 61

[Jeff Walden [:Waldo]]

Attachment: Remove Parser::reportWithNode and its remaining callers (all of which have a full ParseNode* at hand, whose offset can be directly accessed)

Comment 62

[Jeff Walden [:Waldo]]

Comment on attachment 8817232 [details] [diff] [review]
v3

Bah, this isn't working on other things.  It might be the case we haven't actually tracked, at the use-strict transition, all the information needed to perform the correct check here.  Back to the drawing board either way.

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7260d996472
Remove Parser::reportWithNode and its remaining callers (all of which have a full ParseNode* at hand, whose offset can be directly accessed).  r=arai

Comment 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a7260d996472

Comment 65

[Jeff Walden [:Waldo]]

Attachment: Inline Parser::reportHelper into its callers

Comment 66

[Jeff Walden [:Waldo]]

Attachment: Remove Parser::reportHelper

Two separate patches for this because otherwise the diff turns into an intertwingled, unreadable hash.

Comment 67

[Jeff Walden [:Waldo]]

Attachment: Rename TokenStream::reportStrictWarningErrorNumberVA to TokenStream::reportExtraWarningErrorNumberVA for clarity

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ebf2b7541d8
Inline Parser::reportHelper into its callers.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/20100807a776
Remove Parser::reportHelper.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffb071233ea
Rename TokenStream::reportStrictWarningErrorNumberVA to TokenStream::reportExtraWarningErrorNumberVA for clarity.  r=arai

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ebf2b7541d8
https://hg.mozilla.org/mozilla-central/rev/20100807a776
https://hg.mozilla.org/mozilla-central/rev/0ffb071233ea

Comment 70

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:Waldo, maybe it's time to close this bug?

Comment 71

[Jeff Walden [:Waldo]]

Sure.  The function is gone now, but technically I think ParseHandler::getFunctionNameOffset is the remnant of ParseHandler::getPosition that still really ought be removed.  However, even *if* that should be removed, at this point a fresh bug for it would be in order.