Proper span/asset location for DDC errors in the transformer

Proper span/asset location for DDC errors in the transformer

BUG=
R=rnystrom@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/7d7f3a2a2a6fe23cb6f6ddfcb862218c0bb52539

[ochafik, 2016-02-04 19:59:21]

ochafik@google.com changed reviewers:
+ rnystrom@google.com, vsm@google.com

[ochafik, 2016-02-04 19:59:22]

Hey guys, here's a quick follow-up that attributes errors to the right location.

[Bob Nystrom, 2016-02-05 18:51:51]

https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
File lib/src/transformer/error_listener.dart (right):

https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
lib/src/transformer/error_listener.dart:56: dynamic/*=T*/
_getLineAndColumn/*<T>*/(
Doc comment?

https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
lib/src/transformer/error_listener.dart:60: for (int i = 0; i < offset; i++) {
This can be really slow. I've seen error location lookup be a significant
bottleneck in real code when there are a large number of cascaded errors. (I
think the formatter is where I ran into it).

Analyzer does track line start information for source files it's parsed. Is it
possible to plumb that through and make it available here? Why isn't this
already available in the AnalysisError?

https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
lib/src/transformer/error_listener.dart:73: return callback(line, column);
Tuples sure would be nice. :-/

[ochafik, 2016-02-08 13:31:53]

Thanks Bob!

https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
File lib/src/transformer/error_listener.dart (right):

https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
lib/src/transformer/error_listener.dart:56: dynamic/*=T*/
_getLineAndColumn/*<T>*/(
On 2016/02/05 18:51:51, Bob Nystrom wrote:
> Doc comment?

Done.

https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
lib/src/transformer/error_listener.dart:60: for (int i = 0; i < offset; i++) {
On 2016/02/05 18:51:51, Bob Nystrom wrote:
> This can be really slow. I've seen error location lookup be a significant
> bottleneck in real code when there are a large number of cascaded errors. (I
> think the formatter is where I ran into it).
> 
> Analyzer does track line start information for source files it's parsed. Is it
> possible to plumb that through and make it available here? Why isn't this
> already available in the AnalysisError?

Great point, thanks! Added a (optimistically-cached) LocationHelper to find
line&column from offset in log time, and filed
https://github.com/dart-lang/sdk/issues/25717 to get them from the
AnalysisError.

https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
lib/src/transformer/error_listener.dart:73: return callback(line, column);
On 2016/02/05 18:51:51, Bob Nystrom wrote:
> Tuples sure would be nice. :-/

Good point... Introduced an ad-hoc Location{line,column} to simplify this mess.

[ochafik, 2016-02-08 14:39:14]

On 2016/02/08 13:31:53, ochafik wrote:
> Thanks Bob!
> 
>
https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
> File lib/src/transformer/error_listener.dart (right):
> 
>
https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
> lib/src/transformer/error_listener.dart:56: dynamic/*=T*/
> _getLineAndColumn/*<T>*/(
> On 2016/02/05 18:51:51, Bob Nystrom wrote:
> > Doc comment?
> 
> Done.
> 
>
https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
> lib/src/transformer/error_listener.dart:60: for (int i = 0; i < offset; i++) {
> On 2016/02/05 18:51:51, Bob Nystrom wrote:
> > This can be really slow. I've seen error location lookup be a significant
> > bottleneck in real code when there are a large number of cascaded errors. (I
> > think the formatter is where I ran into it).
> > 
> > Analyzer does track line start information for source files it's parsed. Is
it
> > possible to plumb that through and make it available here? Why isn't this
> > already available in the AnalysisError?
> 
> Great point, thanks! Added a (optimistically-cached) LocationHelper to find
> line&column from offset in log time, and filed
> https://github.com/dart-lang/sdk/issues/25717 to get them from the
> AnalysisError.
> 
>
https://codereview.chromium.org/1667173002/diff/20001/lib/src/transformer/err...
> lib/src/transformer/error_listener.dart:73: return callback(line, column);
> On 2016/02/05 18:51:51, Bob Nystrom wrote:
> > Tuples sure would be nice. :-/
> 
> Good point... Introduced an ad-hoc Location{line,column} to simplify this
mess.

Update: switched to using AnalysisContext.getLineInfo as advised in
https://github.com/dart-lang/sdk/issues/25717

[Bob Nystrom, 2016-02-08 19:06:16]

lgtm

https://codereview.chromium.org/1667173002/diff/60001/lib/src/transformer/err...
File lib/src/transformer/error_listener.dart (right):

https://codereview.chromium.org/1667173002/diff/60001/lib/src/transformer/err...
lib/src/transformer/error_listener.dart:30: sourceUrl: sourceUrl, line:
location.lineNumber, column: location.columnNumber);
Long line.

[ochafik, 2016-02-09 00:36:35]

Thanks Bob!

https://codereview.chromium.org/1667173002/diff/60001/lib/src/transformer/err...
File lib/src/transformer/error_listener.dart (right):

https://codereview.chromium.org/1667173002/diff/60001/lib/src/transformer/err...
lib/src/transformer/error_listener.dart:30: sourceUrl: sourceUrl, line:
location.lineNumber, column: location.columnNumber);
On 2016/02/08 19:06:16, Bob Nystrom wrote:
> Long line.

Done.

[ochafik, 2016-02-09 17:06:13]

Description was changed from

==========
Proper span/asset location for DDC errors in the transformer

BUG=
==========

to

==========
Proper span/asset location for DDC errors in the transformer

BUG=
R=rnystrom@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/7d7f3a2a2a6fe23cb6f6ddfcb862...
==========

[ochafik, 2016-02-09 17:06:14]

Committed patchset #6 (id:100001) manually as
7d7f3a2a2a6fe23cb6f6ddfcb862218c0bb52539 (presubmit successful).