Non-format-changing kernel offset changes

Non-format-changing kernel offset changes

Updates to kernel that sets more offsets (and introduces end offests
and 'debuggability') - but doesn't necessarily persist them,
i.e. the format doesn't change and no C++ changes are neccessary yet.

This is step #1 in introducing these things, next step(s) will be
persisting the new stuff and using it on the C++ side.

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

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

Reopening as I reverted the change as it broke package:compiler.

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

[jensj, 2017-01-04 13:52:56]

jensj@google.com changed reviewers:
+ asgerf@google.com, kmillikin@google.com

[asgerf, 2017-01-04 14:20:02]

lgtm

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:321: ..fileOffset = element.nameOffset;
What is the class offset used for?

Is it okay that the offset on the shared mixin class is just that of one of its
uses?

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/frontend...
File pkg/kernel/lib/frontend/accessors.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/frontend...
pkg/kernel/lib/frontend/accessors.dart:126: return new PropertySet(receiver,
name, value, setter);
This node and most other accessor-created nodes are still missing offsets.

They would have to be stored in the constructor.

[Kevin Millikin (Google), 2017-01-04 14:36:58]

LGTM with some comments on the comments.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/bin/dartk.dart
File pkg/kernel/bin/dartk.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/bin/dartk.da...
pkg/kernel/bin/dartk.dart:81: 'in file.');
I think 'in file' is implicit.  I'd just leave it off.  If you think it's
important, it needs to be 'in the file'.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/ast_from_analyzer.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:567: // Debug-stop on the "}" of
a function, not after.
Debug-stop is not really a word.  Can't the end token be a semicolon for arrow
function bodies, or are they already desugared to end with a right brace?

Honestly, this comment doesn't really say much.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:1338: // Use a catch-all for
StringInterpolation and AdjacentStrings:
I wouldn't tie this to the debugger, there are a lot of other tools that might
want source positions.  Just say that this case covers StringInterpolation and
AdjacentStrings.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:2584: // Debug-stop on the "}" of
a member, not after.
Same comment as above.

[jensj, 2017-01-05 09:07:55]

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/bin/dartk.dart
File pkg/kernel/bin/dartk.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/bin/dartk.da...
pkg/kernel/bin/dartk.dart:81: 'in file.');
On 2017/01/04 14:36:58, Kevin Millikin (Google) wrote:
> I think 'in file' is implicit.  I'd just leave it off.  If you think it's
> important, it needs to be 'in the file'.

I'll delete it.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/ast_from_analyzer.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:567: // Debug-stop on the "}" of
a function, not after.
On 2017/01/04 14:36:58, Kevin Millikin (Google) wrote:
> Debug-stop is not really a word.  Can't the end token be a semicolon for arrow
> function bodies, or are they already desugared to end with a right brace?
> 
> Honestly, this comment doesn't really say much.

It was an attempt to explain the use of "endToken.offset" instead of "end".
I'll remove it.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:1338: // Use a catch-all for
StringInterpolation and AdjacentStrings:
On 2017/01/04 14:36:58, Kevin Millikin (Google) wrote:
> I wouldn't tie this to the debugger, there are a lot of other tools that might
> want source positions.  Just say that this case covers StringInterpolation and
> AdjacentStrings.

The reason I put the comment (and tied it to the debugger) was that I thought it
was weird that the position for a StringInterpolation (e.g.) was the *end* of
it. That's what the debugger does for dart files, so I guess that's how it is,
but I, at least, find it rather strange... Other offsets are the start of stuff,
not the end.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:2584: // Debug-stop on the "}" of
a member, not after.
On 2017/01/04 14:36:58, Kevin Millikin (Google) wrote:
> Same comment as above.

I'll remove that too.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:321: ..fileOffset = element.nameOffset;
On 2017/01/04 14:20:01, asgerf wrote:
> What is the class offset used for?
> 
> Is it okay that the offset on the shared mixin class is just that of one of
its
> uses?

In general class offsets is used by Observatory to show the right lines from the
source file, when viewing info about a class. At the same time though, if I
remember correctly, it has to be a real offset (i.e. >= 0) or stuff will crash
(at least in some instances).

For dart files, object.cc's TokenPosition Class::ComputeEndTokenPos() seems to
return the position of the first use/creation/whatnot of the mixin class, and
then special case it (if (is_synthesized_class() || IsMixinApplication() ||
IsTopLevel())) to just show the first line...

I don't think we can really do that, unless we extend the class object (both on
dart and c++ side) to include the end token position, in which case we can set
it "correctly" here and get the same behavior for dill files as for dart files.
For now I'll change it to only set the offset, if it's not already set, that way
at least the first line shown will be the same line for dart and dill.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/frontend...
File pkg/kernel/lib/frontend/accessors.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/frontend...
pkg/kernel/lib/frontend/accessors.dart:126: return new PropertySet(receiver,
name, value, setter);
On 2017/01/04 14:20:01, asgerf wrote:
> This node and most other accessor-created nodes are still missing offsets.

Yes, I'm not claiming that I won't have to make changes to kernel again; I've
only added the stuff I've seen that I need to make debugging of the dill files
I've tested on behave like the corresponding dart files.

> 
> They would have to be stored in the constructor.

I don't think I understand this comment.

[asgerf, 2017-01-05 11:37:41]

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:321: ..fileOffset = element.nameOffset;
On 2017/01/05 09:07:55, jensj wrote:
> On 2017/01/04 14:20:01, asgerf wrote:
> > What is the class offset used for?
> > 
> > Is it okay that the offset on the shared mixin class is just that of one of
> its
> > uses?
> 
> In general class offsets is used by Observatory to show the right lines from
the
> source file, when viewing info about a class. At the same time though, if I
> remember correctly, it has to be a real offset (i.e. >= 0) or stuff will crash
> (at least in some instances).
> 
> For dart files, object.cc's TokenPosition Class::ComputeEndTokenPos() seems to
> return the position of the first use/creation/whatnot of the mixin class, and
> then special case it (if (is_synthesized_class() || IsMixinApplication() ||
> IsTopLevel())) to just show the first line...
> 
> I don't think we can really do that, unless we extend the class object (both
on
> dart and c++ side) to include the end token position, in which case we can set
> it "correctly" here and get the same behavior for dill files as for dart
files.
> For now I'll change it to only set the offset, if it's not already set, that
way
> at least the first line shown will be the same line for dart and dill.

Thanks for the clarification. Looks good to me then.

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/frontend...
File pkg/kernel/lib/frontend/accessors.dart (right):

https://codereview.chromium.org/2610133002/diff/40001/pkg/kernel/lib/frontend...
pkg/kernel/lib/frontend/accessors.dart:126: return new PropertySet(receiver,
name, value, setter);
On 2017/01/05 09:07:55, jensj wrote:
> On 2017/01/04 14:20:01, asgerf wrote:
> > This node and most other accessor-created nodes are still missing offsets.
> 
> Yes, I'm not claiming that I won't have to make changes to kernel again; I've
> only added the stuff I've seen that I need to make debugging of the dill files
> I've tested on behave like the corresponding dart files.
> 
> > 
> > They would have to be stored in the constructor.
> 
> I don't think I understand this comment.

I just meant that in order to implement the last part, we would have to pass the
offsets to the Accessor constructors and store them in fields on the Accessor
object. I'm not saying it needs to be in this CL.

[jensj, 2017-01-05 12:25:53]

Description was changed from

==========
Non-format-changing kernel offset changes

Updates to kernel that sets more offsets (and introduces end offests
and 'debuggability') - but doesn't necessarily persist them,
i.e. the format doesn't change and no C++ changes are neccessary yet.

This is step #1 in introducing these things, next step(s) will be
persisting the new stuff and using it on the C++ side.
==========

to

==========
Non-format-changing kernel offset changes

Updates to kernel that sets more offsets (and introduces end offests
and 'debuggability') - but doesn't necessarily persist them,
i.e. the format doesn't change and no C++ changes are neccessary yet.

This is step #1 in introducing these things, next step(s) will be
persisting the new stuff and using it on the C++ side.

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

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

[jensj, 2017-01-05 12:25:55]

Committed patchset #4 (id:60001) manually as
5edca8c4d3733f319ab2d328aa28ceb286a30904 (presubmit successful).

[jensj, 2017-01-05 13:22:24]

Description was changed from

==========
Non-format-changing kernel offset changes

Updates to kernel that sets more offsets (and introduces end offests
and 'debuggability') - but doesn't necessarily persist them,
i.e. the format doesn't change and no C++ changes are neccessary yet.

This is step #1 in introducing these things, next step(s) will be
persisting the new stuff and using it on the C++ side.

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

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

to

==========
Non-format-changing kernel offset changes

Updates to kernel that sets more offsets (and introduces end offests
and 'debuggability') - but doesn't necessarily persist them,
i.e. the format doesn't change and no C++ changes are neccessary yet.

This is step #1 in introducing these things, next step(s) will be
persisting the new stuff and using it on the C++ side.

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

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

Reopening as I reverted the change as it broke package:compiler.
==========

[jensj, 2017-01-05 14:54:59]

So the old version broke package:compiler because I had changed the number of
required parameters in accessors.dart.

I've now changed the new parameters into a named ones which defaults to
noOffset.

Running "python tools/test.py -mrelease dart2js" now passes locally.

Please note that "Delta from patch set" looks weird for ast_from_analyzer.dart
because I rebased to be able to land...

ptal.

[asgerf, 2017-01-05 14:56:47]

lgtm

[jensj, 2017-01-10 08:16:26]

Description was changed from

==========
Non-format-changing kernel offset changes

Updates to kernel that sets more offsets (and introduces end offests
and 'debuggability') - but doesn't necessarily persist them,
i.e. the format doesn't change and no C++ changes are neccessary yet.

This is step #1 in introducing these things, next step(s) will be
persisting the new stuff and using it on the C++ side.

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

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

Reopening as I reverted the change as it broke package:compiler.
==========

to

==========
Non-format-changing kernel offset changes

Updates to kernel that sets more offsets (and introduces end offests
and 'debuggability') - but doesn't necessarily persist them,
i.e. the format doesn't change and no C++ changes are neccessary yet.

This is step #1 in introducing these things, next step(s) will be
persisting the new stuff and using it on the C++ side.

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

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

Reopening as I reverted the change as it broke package:compiler.

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

[jensj, 2017-01-10 08:16:28]

Committed patchset #5 (id:80001) manually as
3f55b8e2b747b59a9a8d0a687ffa73332731f9db (presubmit successful).