900346 - Fix peekTokenSameLine()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 2

[Nicholas Nethercote [inactive]]

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?

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Fix some comments in TokenStream.h.

I noticed these comment errors while I was in here.

Comment 4

[Jeff Walden [:Waldo]]

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...

Comment 5

[Nicholas Nethercote [inactive]]

> 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.

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31796ab06f39
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dcd80a370e0

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31796ab06f39
https://hg.mozilla.org/mozilla-central/rev/8dcd80a370e0