Fix scanning of unterminated strings

Fix scanning of unterminated strings

This fixes
* lineStart tracking when synthetic strings added
* token charOffset when scanning UTF-8 stream

These changes prevent the crashes mentioned in
#29976 and #29982.

R=ahe@google.com

Committed: https://github.com/dart-lang/sdk/commit/5909efe40128b680eb65a60d072b5b26750726f7

[danrubel, 2017-07-20 20:21:28]

danrubel@google.com changed reviewers:
+ ahe@google.com

[ahe, 2017-07-21 14:09:10]

The scanner changes looks good, but the change from start to tokenStart could
use a comment.

Also, I'm not sure if you have fixed the bugs yet. Try these commands:

./pkg/front_end/tool/fasta testing compile -DupdateExpectations=true

./pkg/front_end/tool/fasta testing strong -DupdateExpectations=true

You can't mark the bugs as fixed if those are still crashing.

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/scanner/string_scanner.dart (right):

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/string_scanner.dart:63: type, source +
closingQuotes, tokenStart, source.length);
Why this change?

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/testcases...
File pkg/front_end/testcases/regress/issue_29976.dart.outline.expect (right):

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/testcases...
pkg/front_end/testcases/regress/issue_29976.dart.outline.expect:2: import self
as self;
There's no main method in this outline.

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/testcases...
File pkg/front_end/testcases/regress/issue_29982.dart.outline.expect (right):

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/testcases...
pkg/front_end/testcases/regress/issue_29982.dart.outline.expect:2: import self
as self;
No main method.

[ahe, 2017-07-21 14:10:04]

On 2017/07/21 14:09:10, ahe wrote: 
> ./pkg/front_end/tool/fasta testing compile -DupdateExpectations=true
> 
> ./pkg/front_end/tool/fasta testing strong -DupdateExpectations=true

No don't do that. Hang on.

[ahe, 2017-07-21 14:11:23]

On 2017/07/21 14:10:04, ahe wrote:
> On 2017/07/21 14:09:10, ahe wrote: 
> > ./pkg/front_end/tool/fasta testing compile -DupdateExpectations=true
> > 
> > ./pkg/front_end/tool/fasta testing strong -DupdateExpectations=true
> 
> No don't do that. Hang on.

Here's how to run the tests and update expectations:

./pkg/front_end/tool/fasta testing compile/regress/issue_29976
compile/regress/issue_29982 -DupdateExpectations=true


./pkg/front_end/tool/fasta testing strong/regress/issue_29976
strong/regress/issue_29982 -DupdateExpectations=true

[danrubel, 2017-07-21 17:24:40]

Description was changed from

==========
Fix scanning of unterminated strings

Fix #29976 and #29982
==========

to

==========
Fix scanning of unterminated strings

This fixes 
* lineStart tracking when synthetic strings added
* token charOffset when scanning UTF-8 stream

These changes prevent the crashes mentioned in
#29976 and #29982.
==========

[danrubel, 2017-07-21 17:38:19]

Comments addressed. PTAL.

On 2017/07/21 14:09:10, ahe wrote:
> You can't mark the bugs as fixed if those are still crashing.

You're right. I got bit by the tests skipped in
https://github.com/dart-lang/sdk/issues/30212
Once I patched in https://codereview.chromium.org/2985563003/
I see the failing tests and have addressed them.

I've also updated the issue description
to indicate progress on #29976 and #29982
rather than indicate that they are fixed.

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/scanner/string_scanner.dart (right):

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/string_scanner.dart:63: type, source +
closingQuotes, tokenStart, source.length);
On 2017/07/21 14:09:09, ahe wrote:
> Why this change?

It should have been tokenStart all along similar to the createSubstringToken
method above and to the parallel methods in Utf8BytesScanner

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/testcases...
File pkg/front_end/testcases/regress/issue_29976.dart.outline.expect (right):

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/testcases...
pkg/front_end/testcases/regress/issue_29976.dart.outline.expect:2: import self
as self;
On 2017/07/21 14:09:10, ahe wrote:
> There's no main method in this outline.

Correct. That's because the fasta scanner does not recover well in this
situation. Something I will address in a future CL.

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/testcases...
File pkg/front_end/testcases/regress/issue_29982.dart.outline.expect (right):

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/testcases...
pkg/front_end/testcases/regress/issue_29982.dart.outline.expect:2: import self
as self;
On 2017/07/21 14:09:10, ahe wrote:
> No main method.

Yup. I will improve fasta scanner recovery in a future CL.

[ahe, 2017-08-01 14:19:57]

lgtm

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/scanner/string_scanner.dart (right):

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/string_scanner.dart:63: type, source +
closingQuotes, tokenStart, source.length);
On 2017/07/21 17:38:18, danrubel wrote:
> On 2017/07/21 14:09:09, ahe wrote:
> > Why this change?
> 
> It should have been tokenStart all along similar to the createSubstringToken
> method above and to the parallel methods in Utf8BytesScanner

I get it now. tokenStart is recording character offsets, and start is byte
offsets. In this case, they should be the same.

We should come up with a naming convention for bytes and character offsets. What
do you think about using, for example, ByteOffset and CharOffset on all the int
variables representing offsets?

https://codereview.chromium.org/2984713002/diff/40001/pkg/front_end/testcases...
File pkg/front_end/testcases/compile.status (right):

https://codereview.chromium.org/2984713002/diff/40001/pkg/front_end/testcases...
pkg/front_end/testcases/compile.status:102: regress/issue_29976: RuntimeError #
Issue 29976.
I think we can remove "# Issue NNN" here and on the other line. The issue tracks
the crash, and you've fixed that.

https://codereview.chromium.org/2984713002/diff/40001/pkg/front_end/testcases...
File pkg/front_end/testcases/regress/issue_29976.dart.direct.expect (right):

https://codereview.chromium.org/2984713002/diff/40001/pkg/front_end/testcases...
pkg/front_end/testcases/regress/issue_29976.dart.direct.expect:5: throw
"pkg/front_end/testcases/regress/issue_29976.dart:10:1: Error: Expected a
declaration, but got '\"'.\n\n^";
I don't think this is the expectation we really want. Should we delete this (and
other expectation files that throw a string) from this CL and mark the test as
failing?

[danrubel, 2017-08-01 21:23:20]

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/scanner/string_scanner.dart (right):

https://codereview.chromium.org/2984713002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/string_scanner.dart:63: type, source +
closingQuotes, tokenStart, source.length);
On 2017/08/01 14:19:57, ahe wrote:
> On 2017/07/21 17:38:18, danrubel wrote:
> > On 2017/07/21 14:09:09, ahe wrote:
> > > Why this change?
> > 
> > It should have been tokenStart all along similar to the createSubstringToken
> > method above and to the parallel methods in Utf8BytesScanner
> 
> I get it now. tokenStart is recording character offsets, and start is byte
> offsets. In this case, they should be the same.
> 
> We should come up with a naming convention for bytes and character offsets.
What
> do you think about using, for example, ByteOffset and CharOffset on all the
int
> variables representing offsets?

That would be an improvement.

byteOffset     charOffset
byteStart      charStart
byteLength     charLength
byteEnd        charEnd

https://codereview.chromium.org/2984713002/diff/40001/pkg/front_end/testcases...
File pkg/front_end/testcases/compile.status (right):

https://codereview.chromium.org/2984713002/diff/40001/pkg/front_end/testcases...
pkg/front_end/testcases/compile.status:102: regress/issue_29976: RuntimeError #
Issue 29976.
On 2017/08/01 14:19:57, ahe wrote:
> I think we can remove "# Issue NNN" here and on the other line. The issue
tracks
> the crash, and you've fixed that.

Done.

https://codereview.chromium.org/2984713002/diff/40001/pkg/front_end/testcases...
File pkg/front_end/testcases/regress/issue_29976.dart.direct.expect (right):

https://codereview.chromium.org/2984713002/diff/40001/pkg/front_end/testcases...
pkg/front_end/testcases/regress/issue_29976.dart.direct.expect:5: throw
"pkg/front_end/testcases/regress/issue_29976.dart:10:1: Error: Expected a
declaration, but got '\"'.\n\n^";
On 2017/08/01 14:19:57, ahe wrote:
> I don't think this is the expectation we really want. Should we delete this
(and
> other expectation files that throw a string) from this CL and mark the test as
> failing?

What do we expect in this situation?
Should we update this file to match what we expect?

[danrubel, 2017-08-01 21:31:12]

Description was changed from

==========
Fix scanning of unterminated strings

This fixes 
* lineStart tracking when synthetic strings added
* token charOffset when scanning UTF-8 stream

These changes prevent the crashes mentioned in
#29976 and #29982.
==========

to

==========
Fix scanning of unterminated strings

This fixes
* lineStart tracking when synthetic strings added
* token charOffset when scanning UTF-8 stream

These changes prevent the crashes mentioned in
#29976 and #29982.

R=ahe@google.com

Committed:
https://github.com/dart-lang/sdk/commit/5909efe40128b680eb65a60d072b5b26750726f7
==========

[danrubel, 2017-08-01 21:31:14]

Committed patchset #4 (id:60001) manually as
5909efe40128b680eb65a60d072b5b26750726f7 (presubmit successful).