Better DDC sourcemap generation for lambdas

Better DDC sourcemap generation for lambdas

This doesn't fix the entire problem, there's still a Chrome issue that it's not clear we can work around, but it helps some. https://bugs.chromium.org/p/chromium/issues/detail?id=676388

There are three distinct issues here affecting Chrome sourcemap usage for DDC programs with single-line lambdas.

1 - We may introduce a synthetic "as SomeType" in a parameter. The synthetic token ends up with a large negative length, from its offset to the beginning of the file, which can confuse sourcemaps.

2 - We have no entry for the blank line following the lambda. The devtools asks for the mapping from (selectedLine, 0) to (selectedLine +1, 0) and if there's no mapping for either it refuses to set the breakpoint. So this artificially forces the mapping from the last character on the line to be to the beginning of the next line instead.

3 - With a lambda we introduce a constructed Return JS node and make a block. Those nodes weren't getting annotated, so they had no source information.

BUG=
R=jmesserly@google.com

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

[Alan Knight, 2017-04-10 20:29:47]

Description was changed from

==========
Better DDC sourcemap generation for lambdas

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

to

==========
Better DDC sourcemap generation for lambdas

There are three distinct issues here affecting Chrome sourcemap usage for DDC
programs with single-line lambdas. This is partly due to a change in Chrome's
handling.

1 - We may introduce a synthetic "as SomeType" in a parameter. The synthetic
token ends up with a large negative length, from its offset to the beginning of
the file, which can confuse sourcemaps.

2 - We have no entry for the blank line following the lambda. The devtools asks
for the mapping from (selectedLine, 0) to (selectedLine +1, 0) and if there's no
mapping for either it refuses to set the breakpoint. So this artificially forces
the mapping from the last character on the line to be to the beginning of the
next line instead.

3 - With a lambda we introduce a constructed Return JS node and make a block.
Those nodes weren't getting annotated, so they had no source information.

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

[Alan Knight, 2017-04-11 16:42:07]

Description was changed from

==========
Better DDC sourcemap generation for lambdas

There are three distinct issues here affecting Chrome sourcemap usage for DDC
programs with single-line lambdas. This is partly due to a change in Chrome's
handling.

1 - We may introduce a synthetic "as SomeType" in a parameter. The synthetic
token ends up with a large negative length, from its offset to the beginning of
the file, which can confuse sourcemaps.

2 - We have no entry for the blank line following the lambda. The devtools asks
for the mapping from (selectedLine, 0) to (selectedLine +1, 0) and if there's no
mapping for either it refuses to set the breakpoint. So this artificially forces
the mapping from the last character on the line to be to the beginning of the
next line instead.

3 - With a lambda we introduce a constructed Return JS node and make a block.
Those nodes weren't getting annotated, so they had no source information.

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

to

==========
Better DDC sourcemap generation for lambdas

This doesn't fix the entire problem, there's still a Chrome issue that it's not
clear we can work around, but it helps some.
https://bugs.chromium.org/p/chromium/issues/detail?id=676388

There are three distinct issues here affecting Chrome sourcemap usage for DDC
programs with single-line lambdas.

1 - We may introduce a synthetic "as SomeType" in a parameter. The synthetic
token ends up with a large negative length, from its offset to the beginning of
the file, which can confuse sourcemaps.

2 - We have no entry for the blank line following the lambda. The devtools asks
for the mapping from (selectedLine, 0) to (selectedLine +1, 0) and if there's no
mapping for either it refuses to set the breakpoint. So this artificially forces
the mapping from the last character on the line to be to the beginning of the
next line instead.

3 - With a lambda we introduce a constructed Return JS node and make a block.
Those nodes weren't getting annotated, so they had no source information.

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

[Alan Knight, 2017-04-11 16:43:48]

alanknight@google.com changed reviewers:
+ brianwilkerson@google.com, jmesserly@google.com, vsm@google.com

[vsm, 2017-04-11 18:39:51]

https://codereview.chromium.org/2815443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (right):

https://codereview.chromium.org/2815443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:79: var next =
unit.lineInfo.getLocation(offset + 1);
Can you add a comment here to explain?  This looks a bit magical.  :-)

[Brian Wilkerson, 2017-04-11 19:07:51]

https://codereview.chromium.org/2815443003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/ast/ast.dart (right):

https://codereview.chromium.org/2815443003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/ast/ast.dart:521: get length {
I think this is the wrong approach to fixing the problem, and that the synthetic
tokens are being created incorrectly and that that's what needs to be changed. A
synthetic token should have an offset that is equal to the end of the preceding
token and a length of zero. If you point me to the code that is creating these
tokens I'll be happy to take a look at it. (Making this change seems likely to
break server when dealing with invalid code that he parser has recovered from.)

[Brian Wilkerson, 2017-04-11 19:07:51]

https://codereview.chromium.org/2815443003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/ast/ast.dart (right):

https://codereview.chromium.org/2815443003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/ast/ast.dart:521: get length {
I think this is the wrong approach to fixing the problem, and that the synthetic
tokens are being created incorrectly and that that's what needs to be changed. A
synthetic token should have an offset that is equal to the end of the preceding
token and a length of zero. If you point me to the code that is creating these
tokens I'll be happy to take a look at it. (Making this change seems likely to
break server when dealing with invalid code that he parser has recovered from.)

[Jennifer Messerly, 2017-04-11 19:24:10]

FYI -- I think you should be able to fix the synthetic node issue in
source_map_printer.dart and/or code_generator.dart rather than needing any
Analyzer changes. Basically just ignore those nodes; they don't need to be
annotated.

We're honestly annotating expressions a lot more than I think we need to, since
the debug tool doesn't allow breakpoints inside anyway. Most important thing is
having lines marked correctly. I got perhaps a little over zealous in the
marking of spans early on, trying to capture every single expression span :)

[Alan Knight, 2017-04-11 20:31:58]

On 2017/04/11 19:24:10, Jennifer Messerly wrote:
> FYI -- I think you should be able to fix the synthetic node issue in
> source_map_printer.dart and/or code_generator.dart rather than needing any
> Analyzer changes. Basically just ignore those nodes; they don't need to be
> annotated.
> 
> We're honestly annotating expressions a lot more than I think we need to,
since
> the debug tool doesn't allow breakpoints inside anyway. Most important thing
is
> having lines marked correctly. I got perhaps a little over zealous in the
> marking of spans early on, trying to capture every single expression span :)

OK, put in an isSynthetic check to discard them.

The point that we only need to really map lines is interesting. As an
experiment, if I also skip MethodDeclaration, then the breakpoint also works
correctly for a single-line lambda. I'm not sure what other things might be
relying on source maps which this could cause a problem for, but it does seem
promising for devtools.

[Alan Knight, 2017-04-11 20:32:06]

https://codereview.chromium.org/2815443003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/ast/ast.dart (right):

https://codereview.chromium.org/2815443003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/ast/ast.dart:521: get length {
On 2017/04/11 19:07:51, Brian Wilkerson wrote:
> I think this is the wrong approach to fixing the problem, and that the
synthetic
> tokens are being created incorrectly and that that's what needs to be changed.
A
> synthetic token should have an offset that is equal to the end of the
preceding
> token and a length of zero. If you point me to the code that is creating these
> tokens I'll be happy to take a look at it. (Making this change seems likely to
> break server when dealing with invalid code that he parser has recovered
from.)

Per Jennifer, it doesn't seem like mapping these is even necessary, so I'll just
take this out of the CL.

These are being created in
pkg/dev_compiler/lib/src/compiler/reify_coercions.dart in the method
castExpression in CoercionReifier.

https://codereview.chromium.org/2815443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (right):

https://codereview.chromium.org/2815443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:79: var next =
unit.lineInfo.getLocation(offset + 1);
On 2017/04/11 18:39:51, vsm wrote:
> Can you add a comment here to explain?  This looks a bit magical.  :-)

Done.

[Jennifer Messerly, 2017-04-12 16:11:23]

LGTM!

https://codereview.chromium.org/2815443003/diff/40001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (right):

https://codereview.chromium.org/2815443003/diff/40001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:64: if (node is!
MethodDeclaration) {
Add a comment on why we're skipping MethodDeclaration in particular?

[Alan Knight, 2017-04-12 18:31:24]

Description was changed from

==========
Better DDC sourcemap generation for lambdas

This doesn't fix the entire problem, there's still a Chrome issue that it's not
clear we can work around, but it helps some.
https://bugs.chromium.org/p/chromium/issues/detail?id=676388

There are three distinct issues here affecting Chrome sourcemap usage for DDC
programs with single-line lambdas.

1 - We may introduce a synthetic "as SomeType" in a parameter. The synthetic
token ends up with a large negative length, from its offset to the beginning of
the file, which can confuse sourcemaps.

2 - We have no entry for the blank line following the lambda. The devtools asks
for the mapping from (selectedLine, 0) to (selectedLine +1, 0) and if there's no
mapping for either it refuses to set the breakpoint. So this artificially forces
the mapping from the last character on the line to be to the beginning of the
next line instead.

3 - With a lambda we introduce a constructed Return JS node and make a block.
Those nodes weren't getting annotated, so they had no source information.

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

to

==========
Better DDC sourcemap generation for lambdas

This doesn't fix the entire problem, there's still a Chrome issue that it's not
clear we can work around, but it helps some.
https://bugs.chromium.org/p/chromium/issues/detail?id=676388

There are three distinct issues here affecting Chrome sourcemap usage for DDC
programs with single-line lambdas.

1 - We may introduce a synthetic "as SomeType" in a parameter. The synthetic
token ends up with a large negative length, from its offset to the beginning of
the file, which can confuse sourcemaps.

2 - We have no entry for the blank line following the lambda. The devtools asks
for the mapping from (selectedLine, 0) to (selectedLine +1, 0) and if there's no
mapping for either it refuses to set the breakpoint. So this artificially forces
the mapping from the last character on the line to be to the beginning of the
next line instead.

3 - With a lambda we introduce a constructed Return JS node and make a block.
Those nodes weren't getting annotated, so they had no source information.

BUG=
R=jmesserly@google.com

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

[Alan Knight, 2017-04-12 18:31:25]

Committed patchset #4 (id:60001) manually as
04bb68bb09781f4c82f23f4941e4a461e66dfc28 (presubmit successful).