Add Source.getTextLine and use it to display source snippets in error messages.

Add Source.getTextLine and use it to display source snippets in error messages.

R=asgerf@google.com, johnniwinther@google.com

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

[ahe, 2017-04-03 08:25:26]

Patchset #1 (id:1) has been deleted

[ahe, 2017-04-03 08:25:31]

Patchset #1 (id:20001) has been deleted

[ahe, 2017-04-03 08:28:18]

Patchset #1 (id:40001) has been deleted

[ahe, 2017-04-03 08:28:34]

ahe@google.com changed reviewers:
+ asgerf@google.com, karlklose@google.com

[asgerf, 2017-04-03 08:34:55]

LGTM with comments addressed

https://codereview.chromium.org/2788373002/diff/60001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/source_file.dart (right):

https://codereview.chromium.org/2788373002/diff/60001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/source_file.dart:197: //
[package:kernel/ast.dart](../../../../kernel/lib/ast.dart).
Any reason not to resolve this now?

https://codereview.chromium.org/2788373002/diff/60001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2788373002/diff/60001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4255: return cachedText.substring(lineStarts[index]);
cachedText could be null here. The initialization below should be moved up.

[ahe, 2017-04-03 09:26:39]

PTAL, significant changes.

https://codereview.chromium.org/2788373002/diff/60001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/source_file.dart (right):

https://codereview.chromium.org/2788373002/diff/60001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/source_file.dart:197: //
[package:kernel/ast.dart](../../../../kernel/lib/ast.dart).
On 2017/04/03 08:34:55, asgerf wrote:
> Any reason not to resolve this now?

I hadn't realized how easy that was before you asked :-)

https://codereview.chromium.org/2788373002/diff/60001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2788373002/diff/60001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4255: return cachedText.substring(lineStarts[index]);
On 2017/04/03 08:34:55, asgerf wrote:
> cachedText could be null here. The initialization below should be moved up.

Done.

[ahe, 2017-04-03 09:28:02]

Johnni: I've made significant changes to source_file from dart2js.

[asgerf, 2017-04-03 09:55:25]

https://codereview.chromium.org/2788373002/diff/80001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2788373002/diff/80001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4251: result = result.substring(0, result.length - 1);
The 'nextLine' variable does not actually refer to the start of the next line.
'endOfLine' would be a better name.

I also think it's bad practice to create a substring of a substring and then
throw out the first one.

Could we compute the offset directly instead? E.g.

int startOfNextLine = lineStarts[index + 1];
int endOfLine = startOfNextLine - 1; // Remove linebreak.
if (endOfLine > index && 
    cachedText[endOfLine - 1] == '\r') { 
  --endOfLine;  // Handle two-byte linebreak.
}

https://codereview.chromium.org/2788373002/diff/80001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4266: "The value of 'offset' ($offset) must be between 1
and "
between 1 and -> between 0 and

[Johnni Winther, 2017-04-03 10:21:48]

johnniwinther@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2017-04-03 10:21:48]

dart2js changes LGTM

[ahe, 2017-04-03 14:57:08]

I went a bit further getting rid of dart2js SourceFile API in favor of
package:kernel. PTAL.

The main benefit of taking it so far is that the Kernel API is now very well
tested.

https://codereview.chromium.org/2788373002/diff/80001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2788373002/diff/80001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4251: result = result.substring(0, result.length - 1);
On 2017/04/03 09:55:25, asgerf wrote:
> The 'nextLine' variable does not actually refer to the start of the next line.
> 'endOfLine' would be a better name.
> 
> I also think it's bad practice to create a substring of a substring and then
> throw out the first one.
> 
> Could we compute the offset directly instead? E.g.
> 
> int startOfNextLine = lineStarts[index + 1];
> int endOfLine = startOfNextLine - 1; // Remove linebreak.
> if (endOfLine > index && 
>     cachedText[endOfLine - 1] == '\r') { 
>   --endOfLine;  // Handle two-byte linebreak.
> }

Done.

https://codereview.chromium.org/2788373002/diff/80001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4266: "The value of 'offset' ($offset) must be between 1
and "
On 2017/04/03 09:55:25, asgerf wrote:
> between 1 and -> between 0 and

Done.

[asgerf, 2017-04-03 15:19:14]

https://codereview.chromium.org/2788373002/diff/140001/tests/compiler/dart2js...
File tests/compiler/dart2js/sourcemaps/sourcemap_html_helper.dart (right):

https://codereview.chromium.org/2788373002/diff/140001/tests/compiler/dart2js...
tests/compiler/dart2js/sourcemaps/sourcemap_html_helper.dart:518:
dartCodeBuffer.write(sourceFile.kernelSource.getTextLine(line + 1));
Wouldn't it be better to keep the name getLineText and consistently accept
0-based line indices instead of 1-based line numbers?

[ahe, 2017-04-03 15:34:55]

https://codereview.chromium.org/2788373002/diff/140001/tests/compiler/dart2js...
File tests/compiler/dart2js/sourcemaps/sourcemap_html_helper.dart (right):

https://codereview.chromium.org/2788373002/diff/140001/tests/compiler/dart2js...
tests/compiler/dart2js/sourcemaps/sourcemap_html_helper.dart:518:
dartCodeBuffer.write(sourceFile.kernelSource.getTextLine(line + 1));
On 2017/04/03 15:19:14, asgerf wrote:
> Wouldn't it be better to keep the name getLineText and consistently accept
> 0-based line indices instead of 1-based line numbers?

I'm not sure how much of this I can change, and how much is given by the source
map format.

[asgerf, 2017-04-04 08:33:59]

https://codereview.chromium.org/2788373002/diff/140001/tests/compiler/dart2js...
File tests/compiler/dart2js/sourcemaps/sourcemap_html_helper.dart (right):

https://codereview.chromium.org/2788373002/diff/140001/tests/compiler/dart2js...
tests/compiler/dart2js/sourcemaps/sourcemap_html_helper.dart:518:
dartCodeBuffer.write(sourceFile.kernelSource.getTextLine(line + 1));
On 2017/04/03 15:34:55, ahe wrote:
> On 2017/04/03 15:19:14, asgerf wrote:
> > Wouldn't it be better to keep the name getLineText and consistently accept
> > 0-based line indices instead of 1-based line numbers?
> 
> I'm not sure how much of this I can change, and how much is given by the
source
> map format.

It has nothing to do with the source map format.

I meant the method for doing this was called getLineText before this change. 
Now it is called getTextLine.

Where the method previously took N it now takes N+1.

[asgerf, 2017-04-04 08:53:05]

lgtm

https://codereview.chromium.org/2788373002/diff/140001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2788373002/diff/140001/pkg/kernel/lib/ast.dar...
pkg/kernel/lib/ast.dart:4265: "${lineStarts.length}.");
length -> last

[ahe, 2017-04-04 09:03:07]

Thank you!

https://codereview.chromium.org/2788373002/diff/140001/tests/compiler/dart2js...
File tests/compiler/dart2js/sourcemaps/sourcemap_html_helper.dart (right):

https://codereview.chromium.org/2788373002/diff/140001/tests/compiler/dart2js...
tests/compiler/dart2js/sourcemaps/sourcemap_html_helper.dart:518:
dartCodeBuffer.write(sourceFile.kernelSource.getTextLine(line + 1));
On 2017/04/04 08:33:59, asgerf wrote:
> On 2017/04/03 15:34:55, ahe wrote:
> > On 2017/04/03 15:19:14, asgerf wrote:
> > > Wouldn't it be better to keep the name getLineText and consistently accept
> > > 0-based line indices instead of 1-based line numbers?
> > 
> > I'm not sure how much of this I can change, and how much is given by the
> source
> > map format.
> 
> It has nothing to do with the source map format.
> 
> I meant the method for doing this was called getLineText before this change. 
> Now it is called getTextLine.
> 
> Where the method previously took N it now takes N+1.

Sorry, I misunderstood your question. We discussed this offline, and I'll try to
summarize it below:

* Most tools display line and column numbers starting at 1. So when I saw that
Location was 1-based, I assumed that represented conscious choice to represent
them 1-based. That made sense to me, as almost all tools I've ever worked with
have 1-based line and column numbers. Apparently, I read too much in to it, but
now that we talked about it, we all seem to like that line and column numbers
are represented as you see them.

* I've tried to remove all location-related API from SourceFile, all that's left
is getLocation and kernelSource. This is why I removed getLine, getColumn, and
getLineText from SourceFile. As I reimplemented getLineText in package:kernel, I
felt the name getTextLine was more natural, hence the rename.

* Johnni told me that source maps are also 1-based, so it would actually make
sense to change the class SourceLocation in dart2js, but I feel I'm getting
side-tracked now, so I'm stopping short of fixing that problem.

[ahe, 2017-04-05 11:34:23]

Thank you!

https://codereview.chromium.org/2788373002/diff/140001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2788373002/diff/140001/pkg/kernel/lib/ast.dar...
pkg/kernel/lib/ast.dart:4265: "${lineStarts.length}.");
On 2017/04/04 08:53:05, asgerf wrote:
> length -> last

Done.

[ahe, 2017-04-05 13:02:56]

Description was changed from

==========
Add Source.getTextLine and use it to display source snippets in error messages.
==========

to

==========
Add Source.getTextLine and use it to display source snippets in error messages.

R=asgerf@google.com, johnniwinther@google.com

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

[ahe, 2017-04-05 13:02:57]

Committed patchset #6 (id:160001) manually as
ddda1829576480fdd9d9cfe6ce28e1c00a077945 (presubmit successful).

[sra1, 2017-05-04 05:44:39]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2017-05-04 05:44:40]

https://codereview.chromium.org/2788373002/diff/160001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2788373002/diff/160001/pkg/kernel/lib/ast.dar...
pkg/kernel/lib/ast.dart:4274: "The value of '$name' ($value) must be between
$min and $max.");
Please remove this interpolation.

It is expensive to interpolate this string for every range check.
I measure ~1% of dart2js time in allocating the strings for _Smi.toString
The information in this interpolated string is pretty redundant with what the
RangeError will print without a 'message' parameter.

If you remove this parameter you will get an error like:

RangeError (offset): Invalid value: Not in range 0..20, inclusive: 100

So you could delete this helper and call RangeError.checkValueInRange from the
two call sites.

[sra1, 2017-05-04 06:03:29]

On 2017/05/04 05:44:40, sra1 wrote:
> https://codereview.chromium.org/2788373002/diff/160001/pkg/kernel/lib/ast.dart
> File pkg/kernel/lib/ast.dart (right):
> 
>
https://codereview.chromium.org/2788373002/diff/160001/pkg/kernel/lib/ast.dar...
> pkg/kernel/lib/ast.dart:4274: "The value of '$name' ($value) must be between
> $min and $max.");
> Please remove this interpolation.
> 
> It is expensive to interpolate this string for every range check.
> I measure ~1% of dart2js time in allocating the strings for _Smi.toString
> The information in this interpolated string is pretty redundant with what the
> RangeError will print without a 'message' parameter.
> 
> If you remove this parameter you will get an error like:
> 
> RangeError (offset): Invalid value: Not in range 0..20, inclusive: 100
> 
> So you could delete this helper and call RangeError.checkValueInRange from the
> two call sites.

I can make the changes. I just want to understand why we are doing interpolation
here.

[ahe, 2017-05-04 07:33:32]

On 2017/05/04 06:03:29, sra1 wrote:
> On 2017/05/04 05:44:40, sra1 wrote:
> >
https://codereview.chromium.org/2788373002/diff/160001/pkg/kernel/lib/ast.dart
> > File pkg/kernel/lib/ast.dart (right):
> > 
> >
>
https://codereview.chromium.org/2788373002/diff/160001/pkg/kernel/lib/ast.dar...
> > pkg/kernel/lib/ast.dart:4274: "The value of '$name' ($value) must be between
> > $min and $max.");
> > Please remove this interpolation.
> > 
> > It is expensive to interpolate this string for every range check.
> > I measure ~1% of dart2js time in allocating the strings for _Smi.toString
> > The information in this interpolated string is pretty redundant with what
the
> > RangeError will print without a 'message' parameter.
> > 
> > If you remove this parameter you will get an error like:
> > 
> > RangeError (offset): Invalid value: Not in range 0..20, inclusive: 100
> > 
> > So you could delete this helper and call RangeError.checkValueInRange from
the
> > two call sites.
> 
> I can make the changes. I just want to understand why we are doing
interpolation
> here.

I should have thought about the cost of interpolation, my bad. The message from
RangeError is horrible, and I don't understand it.

The ideal solution would be to fix RangeError, and if that fails, we can do the
range check ourselves.