Prevent double reporting on hex, strings, and multiline comments.

Prevent double reporting on hex, strings, and multiline strings and comments.

BUG= https://code.google.com/p/dart/issues/detail?id=18574
R=ahe@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39451

[srawlins, 2014-06-20 18:54:25]

Hi Johnni, Let me know if you are not the one who should review scanner CLs.

[Johnni Winther, 2014-06-23 07:25:32]

@ahe: Could you take a look at this?

[ahe, 2014-06-23 13:50:07]

I'll review this.

[ahe, 2014-06-23 14:28:49]

Very nice.

I'm withholding l*g*t*m because I don't understand why you added a next
parameter to some of the methods, and because I think we may need more tests.

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:634: Token
expectedDeclaration(Token token) {
This makes more sense.

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/scanner/scanner.dart (left):

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/scanner/scanner.dart:952: next =
advance(); // Advance past the quote.
This line caused dart2js to incorrectly compile this program:

main() {
  print(r'
');
}

I suggest that you create a test case for this. Exceptionally, this will have to
be a "naive" negative test (which we otherwise try to avoid). For example:

tests/language/bad_raw_string_negative_test.dart:
main() {
  print(r'
');
}

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/scanner/scanner.dart (right):

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/scanner/scanner.dart:1008: int
tokenizeMultiLineString(int next, int quoteChar, int start, bool raw) {
I don't understand this change.

https://codereview.chromium.org/340703003/diff/1/tests/compiler/dart2js/messa...
File tests/compiler/dart2js/message_kind_helper.dart (right):

https://codereview.chromium.org/340703003/diff/1/tests/compiler/dart2js/messa...
tests/compiler/dart2js/message_kind_helper.dart:47:
MessageKind.UNTERMINATED_STRING, // hard to avoid for multiline strings
Could you restore the bug number and remove the comment?

My suggested fix for an unterminated multi-line string is to synthesize a token
stream that gracefully ends the current element being parsed. The beginFoo calls
in the listener can be used for this.

Alternatively, the listener can stay silent after seeing an unterminated
multi-line string (or comment).

This makes me think, do we need a test for something like:

main() => /*

[srawlins, 2014-07-15 20:07:08]

Thanks so much ahe@ for your comments! Sorry, I've been out of town, but I just
addressed your comments. Let me know of any more questions.

Thanks again!

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:634: Token
expectedDeclaration(Token token) {
On 2014/06/23 14:28:49, ahe wrote:
> This makes more sense.

Ack.

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/scanner/scanner.dart (left):

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/scanner/scanner.dart:952: next =
advance(); // Advance past the quote.
On 2014/06/23 14:28:49, ahe wrote:
> This line caused dart2js to incorrectly compile this program:
> 
> main() {
>   print(r'
> ');
> }
> 
> I suggest that you create a test case for this. Exceptionally, this will have
to
> be a "naive" negative test (which we otherwise try to avoid). For example:
> 
> tests/language/bad_raw_string_negative_test.dart:
> main() {
>   print(r'
> ');
> }

I disagree. In this example, I get the correct error:

$ cat issue-18574-222.dart
main() {
  print(r'
');
}
$ xcodebuild/ReleaseIA32/dart-sdk/bin/dart2js issue-18574-222.dart
issue-18574-222.dart:2:9:
Error: String must end with r'.
  print(r'
        ^^
Error: Compilation failed.

I see that when tokenizeSingleLineRawString() is entered in this case, next is
$LF and peek() returns the single-quote character. Since next is correctly $LF,
this will be caught on line 960.

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/scanner/scanner.dart (right):

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/scanner/scanner.dart:1008: int
tokenizeMultiLineString(int next, int quoteChar, int start, bool raw) {
On 2014/06/23 14:28:49, ahe wrote:
> I don't understand this change.

Done. Sorry, I think from a previous fix.

https://codereview.chromium.org/340703003/diff/1/tests/compiler/dart2js/messa...
File tests/compiler/dart2js/message_kind_helper.dart (right):

https://codereview.chromium.org/340703003/diff/1/tests/compiler/dart2js/messa...
tests/compiler/dart2js/message_kind_helper.dart:47:
MessageKind.UNTERMINATED_STRING, // hard to avoid for multiline strings
On 2014/06/23 14:28:49, ahe wrote:
> Could you restore the bug number and remove the comment?

Done.
> 
> My suggested fix for an unterminated multi-line string is to synthesize a
token
> stream that gracefully ends the current element being parsed. The beginFoo
calls
> in the listener can be used for this.
> 
This is the fix I implemented: gracefully end the current element. But then we
are at EOF, and so a ";" or a ")" or something is always expected. A multiline
string cannot be the last token in a file, unlike a multiline comment.

> Alternatively, the listener can stay silent after seeing an unterminated
> multi-line string (or comment).
> 
I could not see a way to do this...

> This makes me think, do we need a test for something like:
> 
> main() => /*

That should be covered in the UNTERMINATED_COMMENT tests.

[ahe, 2014-07-16 07:54:18]

If you add a regression test for r' followed by newline, I'll say LGTM :-)

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/scanner/scanner.dart (left):

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/scanner/scanner.dart:952: next =
advance(); // Advance past the quote.
What I'm trying to say is:

You removed this line. That is good. It is good because, before your change,
dart2js would accept a broken program:

$ cat foo.dart
main() {
  print(r'
');
}

$ ./sdk/bin/dart2js foo.dart
Dart file (foo.dart) compiled to JavaScript: out.js
$

So your change is good. You fixed a bug. But you should add a regression test to
ensure this bug isn't reintroduced.

On 2014/07/15 20:07:07, srawlins wrote:
> On 2014/06/23 14:28:49, ahe wrote:
> > This line caused dart2js to incorrectly compile this program:
> > 
> > main() {
> >   print(r'
> > ');
> > }
> > 
> > I suggest that you create a test case for this. Exceptionally, this will
have
> to
> > be a "naive" negative test (which we otherwise try to avoid). For example:
> > 
> > tests/language/bad_raw_string_negative_test.dart:
> > main() {
> >   print(r'
> > ');
> > }
> 
> I disagree. In this example, I get the correct error:
> 
> $ cat issue-18574-222.dart
> main() {
>   print(r'
> ');
> }
> $ xcodebuild/ReleaseIA32/dart-sdk/bin/dart2js issue-18574-222.dart
> issue-18574-222.dart:2:9:
> Error: String must end with r'.
>   print(r'
>         ^^
> Error: Compilation failed.
> 
> I see that when tokenizeSingleLineRawString() is entered in this case, next is
> $LF and peek() returns the single-quote character. Since next is correctly
$LF,
> this will be caught on line 960.

https://codereview.chromium.org/340703003/diff/1/tests/compiler/dart2js/messa...
File tests/compiler/dart2js/message_kind_helper.dart (right):

https://codereview.chromium.org/340703003/diff/1/tests/compiler/dart2js/messa...
tests/compiler/dart2js/message_kind_helper.dart:47:
MessageKind.UNTERMINATED_STRING, // hard to avoid for multiline strings
On 2014/07/15 20:07:08, srawlins wrote:
> On 2014/06/23 14:28:49, ahe wrote:
> > Could you restore the bug number and remove the comment?
> 
> Done.
> > 
> > My suggested fix for an unterminated multi-line string is to synthesize a
> token
> > stream that gracefully ends the current element being parsed. The beginFoo
> calls
> > in the listener can be used for this.
> > 
> This is the fix I implemented: gracefully end the current element. But then we
> are at EOF, and so a ";" or a ")" or something is always expected. A multiline
> string cannot be the last token in a file, unlike a multiline comment.

What you have implemented is the same pattern as is used elsewhere, so nothing
wrong with that.

However, what I mean by gracefully ending the current element is different from
skipping to EOF. I'd say skipping to EOF is rather crude, but gets the job done.

Consider this example:

main() {"""

What I'm suggesting in this case is that the listener returns this token stream
in response to the problem:

  ; } EOF

The listener could insert both the semicolon and the curly brace, and the parser
would recover (gracefully).

But I'm not suggesting that you do this, at least not in this CL. But it's what
I'd really like to get done before the above bugs are closed. If you find that
sort of thing interesting, I'd be happy to talk more about this.

> > Alternatively, the listener can stay silent after seeing an unterminated
> > multi-line string (or comment).
> > 
> I could not see a way to do this...

I've recently implemented something like that. See
https://codereview.chromium.org/324293002/.

> > This makes me think, do we need a test for something like:
> > 
> > main() => /*
> 
> That should be covered in the UNTERMINATED_COMMENT tests.

That test is slightly different because the comment is at top-level.

[srawlins, 2014-08-12 04:32:59]

Hi Peter, all should be well now! Thanks much!

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/scanner/scanner.dart (left):

https://codereview.chromium.org/340703003/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/scanner/scanner.dart:952: next =
advance(); // Advance past the quote.
On 2014/07/16 07:54:18, ahe wrote:
> What I'm trying to say is:
> 
> You removed this line. That is good. It is good because, before your change,
> dart2js would accept a broken program:
> 
> $ cat foo.dart
> main() {
>   print(r'
> ');
> }
> 
> $ ./sdk/bin/dart2js foo.dart
> Dart file (foo.dart) compiled to JavaScript: out.js
> $
> 
> So your change is good. You fixed a bug. But you should add a regression test
to
> ensure this bug isn't reintroduced.

Oh, I see! OK thank you. I've added your test as
tests/language/bad_raw_string_negative_test.dart
> 
> On 2014/07/15 20:07:07, srawlins wrote:
> > On 2014/06/23 14:28:49, ahe wrote:
> > > This line caused dart2js to incorrectly compile this program:
> > > 
> > > main() {
> > >   print(r'
> > > ');
> > > }
> > > 
> > > I suggest that you create a test case for this. Exceptionally, this will
> have
> > to
> > > be a "naive" negative test (which we otherwise try to avoid). For example:
> > > 
> > > tests/language/bad_raw_string_negative_test.dart:
> > > main() {
> > >   print(r'
> > > ');
> > > }
> > 
> > I disagree. In this example, I get the correct error:
> > 
> > $ cat issue-18574-222.dart
> > main() {
> >   print(r'
> > ');
> > }
> > $ xcodebuild/ReleaseIA32/dart-sdk/bin/dart2js issue-18574-222.dart
> > issue-18574-222.dart:2:9:
> > Error: String must end with r'.
> >   print(r'
> >         ^^
> > Error: Compilation failed.
> > 
> > I see that when tokenizeSingleLineRawString() is entered in this case, next
is
> > $LF and peek() returns the single-quote character. Since next is correctly
> $LF,
> > this will be caught on line 960.
>

https://codereview.chromium.org/340703003/diff/1/tests/compiler/dart2js/messa...
File tests/compiler/dart2js/message_kind_helper.dart (right):

https://codereview.chromium.org/340703003/diff/1/tests/compiler/dart2js/messa...
tests/compiler/dart2js/message_kind_helper.dart:47:
MessageKind.UNTERMINATED_STRING, // hard to avoid for multiline strings
On 2014/07/16 07:54:18, ahe wrote:
> On 2014/07/15 20:07:08, srawlins wrote:
> > On 2014/06/23 14:28:49, ahe wrote:
> > > Could you restore the bug number and remove the comment?
> > 
> > Done.
> > > 
> > > My suggested fix for an unterminated multi-line string is to synthesize a
> > token
> > > stream that gracefully ends the current element being parsed. The beginFoo
> > calls
> > > in the listener can be used for this.
> > > 
> > This is the fix I implemented: gracefully end the current element. But then
we
> > are at EOF, and so a ";" or a ")" or something is always expected. A
multiline
> > string cannot be the last token in a file, unlike a multiline comment.
> 
> What you have implemented is the same pattern as is used elsewhere, so nothing
> wrong with that.
> 
> However, what I mean by gracefully ending the current element is different
from
> skipping to EOF. I'd say skipping to EOF is rather crude, but gets the job
done.
> 
> Consider this example:
> 
> main() {"""
> 
> What I'm suggesting in this case is that the listener returns this token
stream
> in response to the problem:
> 
>   ; } EOF
> 
> The listener could insert both the semicolon and the curly brace, and the
parser
> would recover (gracefully).
> 
> But I'm not suggesting that you do this, at least not in this CL. But it's
what
> I'd really like to get done before the above bugs are closed. If you find that
> sort of thing interesting, I'd be happy to talk more about this.

Somehow this sorted itself out in the last few weeks. I merged master today and
UNTERMINATED_STRING does not double warn any more!
> 
> > > Alternatively, the listener can stay silent after seeing an unterminated
> > > multi-line string (or comment).
> > > 
> > I could not see a way to do this...
> 
> I've recently implemented something like that. See
> https://codereview.chromium.org/324293002/.
> 
> > > This makes me think, do we need a test for something like:
> > > 
> > > main() => /*
> > 
> > That should be covered in the UNTERMINATED_COMMENT tests.
> 
> That test is slightly different because the comment is at top-level.

[ahe, 2014-08-19 09:18:33]

LGTM/awesome!

https://codereview.chromium.org/340703003/diff/40001/tests/language/bad_raw_s...
File tests/language/bad_raw_string_negative_test.dart (right):

https://codereview.chromium.org/340703003/diff/40001/tests/language/bad_raw_s...
tests/language/bad_raw_string_negative_test.dart:1: main() {
Please add a copyright notice:

// Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Also, a small comment describing the intent of the test.

[srawlins, 2014-08-19 21:08:40]

Thanks ahe@

https://codereview.chromium.org/340703003/diff/40001/tests/language/bad_raw_s...
File tests/language/bad_raw_string_negative_test.dart (right):

https://codereview.chromium.org/340703003/diff/40001/tests/language/bad_raw_s...
tests/language/bad_raw_string_negative_test.dart:1: main() {
On 2014/08/19 09:18:33, ahe wrote:
> Please add a copyright notice:
> 
> // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
> // for details. All rights reserved. Use of this source code is governed by a
> // BSD-style license that can be found in the LICENSE file.
> 
> Also, a small comment describing the intent of the test.

Done.

[ahe, 2014-08-20 07:58:51]

Still LGTM!

[ahe, 2014-08-20 08:02:22]

I've made you a committer on the project.

[srawlins, 2014-08-21 06:47:31]

The CQ bit was checked by srawlins@google.com

[commit-bot: I haz the power, 2014-08-21 06:51:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 06:51:48]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[srawlins, 2014-08-21 14:51:52]

Committed patchset #4 manually as 39451 (presubmit successful).