Fix peekTokenSameLine()

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
I've never quite understood peekTokenSameLine(), and I now know why -- it's
bogus.  It's supposed to return TOK_EOL if there's a newline between the end of
the current token and the start of the next token.  But consider the very start
of it:

    TokenKind peekTokenSameLine(unsigned withFlags = 0) {
        if (!onCurrentLine(currentToken().pos))
            return TOK_EOL;
        
This tests that the *end* of the *furthest-scanned* token is on the same line
as the end of the current token, instead of testing that the *start* of the
*next* token is on the same line.  I.e. it's wrong in two cases.

- If the next token is a multi-line token.

- If we've scanned ahead *two* tokens and there's a newline between the next
  token and the one after that.

These cases are pretty obscure and I've only managed to find one instance where
the first wrong case occurs in practice (which I'll describe shortly), and no
instances of the second.
(Assignee)

Comment 1

5 years ago
Created attachment 784223 [details] [diff] [review]
(part 1) - Fix peekTokenSameLine().

This patch fixes the problem, and adds copious comments.  The fix allows the
TSF_EOL flag to be removed, which is nice.
Attachment #784223 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
Here's the test case where this produces different behaviour:

  module 'ma\
  th' {
      export function sum(x, y) { return x + y; }
  }

The issue is that the module name needs to be on the same line as the |module|
keyword, and it is, but because the name is a multi-line string the old code
thinks it isn't.  Output with the old code:

  m.js:2:4 SyntaxError: missing ; before statement:
  m.js:2:4 th' {
  m.js:2:4 ....^

Output with my patch applied:

  m.js:3:4 SyntaxError: export is a reserved identifier:
  m.js:3:4     export function sum(x, y) { return x + y; }
  m.js:3:4 ....^

Now, something's a bit screwy with modules -- |module| is accepted as a keyword
but |export| isn't.  I'm not sure what's going on there, which is why I haven't
actually written a test as part of this patch, but I'm pretty sure the new
behaviour is more correct.  Eddy, can you explain this?  Are modules only 
partly implemented?
Flags: needinfo?(ejpbruel)
(Assignee)

Comment 3

5 years ago
Created attachment 784224 [details] [diff] [review]
(part 2) - Fix some comments in TokenStream.h.

I noticed these comment errors while I was in here.
Attachment #784224 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Attachment #784223 - Flags: review?(jorendorff) → review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #784224 - Flags: review?(jorendorff) → review?(jwalden+bmo)

Comment 4

5 years ago
Comment on attachment 784223 [details] [diff] [review]
(part 1) - Fix peekTokenSameLine().

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

Modules are only partially implemented, yes, so you might not be able to write an actual testcase for this.

::: js/src/frontend/TokenStream.h
@@ +321,5 @@
> +    TSF_OPERAND = 0x02,         /* looking for operand, not operator */
> +    TSF_UNEXPECTED_EOF = 0x04,  /* unexpected end of input, i.e. TOK_EOF not at top-level. */
> +    TSF_KEYWORD_IS_NAME = 0x08, /* Ignore keywords and return TOK_NAME instead to the parser. */
> +    TSF_DIRTYLINE = 0x10,       /* non-whitespace since start of line */
> +    TSF_OCTAL_CHAR = 0x20,      /* observed a octal character escape */

If you're touching all these, make this "an".  Although, I'd have just moved the trailing initializers up to the empty/removed spaces, myself...
Attachment #784223 - Flags: review?(jwalden+bmo) → review+

Updated

5 years ago
Attachment #784224 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 5

5 years ago
> Modules are only partially implemented, yes, so you might not be able to
> write an actual testcase for this.

Ok, I'll clear the needinfo.  Thanks.
Flags: needinfo?(ejpbruel)
https://hg.mozilla.org/mozilla-central/rev/31796ab06f39
https://hg.mozilla.org/mozilla-central/rev/8dcd80a370e0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.