Include source in kernel.

Include source in kernel.

- For now include source uncompressed.
- When running from kernel, use token position 0
  (i.e. dummy, but 'real' position) as start and end on functions
  and classes to enable Observatory to run with the dill file.
- Debugging does not work, but one can browse the source in
  Observatory.

R=kmillikin@google.com

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

[jensj, 2016-12-20 11:59:42]

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

[Kevin Millikin (Google), 2016-12-20 12:03:34]

Basically looks good with a round of comments.

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

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:336: fileUri:
"file://${element.source.fullName}");
fileUri: classNode.fileUri

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:700: sourceCode = source.contents.data;
I think context.getContents(source) will use the analysis context's source cache
instead of reading the file from disk again.  We should probably call that
instead.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:702: // if we cannot read the source code
there's nothing to do about that.
The comment should be a complete sentence (start with a capital letter).  I
prefer to make comments less conversational.

It isn't "we" that's reading the source code, it's the program.  There is
something to do about failing to read the source code, just use ''.  I also
wouldn't presume that the analyzer tried to "read" something, that seems
distracting and misleading to mention here (there are a bunch of different kinds
of Sources and the documentation just says it couldn't be accessed).

I tend to write this like:

String sourceCode;
try {
  sourceCode = context.getContents(source);
} catch (e) {
  // The source's contents could not be accessed.
  sourceCode = '';
}

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:704:
program.uriToLineStartsAndSource["file://${source.fullName}"] =
We should probably use source.uri instead of source.fullName.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:706: new List<int>.from(lineInfo.lineStarts,
growable: false),
lineInfo.lineStarts is already a List<int>, isn't it?

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

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3519: uriToLineStartsAndSource =
uriToLineStartsAndSource ?? {} {
Should be <String, LineStartsAndSource>{}.

Can't we just use

this.uriToLineStartsAndSource = const <String, LineStartsAndSource>{}

in the parameter list?

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3657: class LineStartsAndSource {
I would just call this "Source", not describe the implementation in the name.

It's a bad smell that we have to update so many identifier names just to add a
field to an object.

It doesn't scale anyway, eventually we'll have class
LineStartsAndSourceAndCompressionSchemeAndBreakpointPositionsAnd....

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:198: Map<String, LineStartsAndSource>
uriToLineStarts = {};
<String, LineStartsAndSource>{}.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_to_binary.dart (right):

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_to_binary.dart:154:
program.uriToLineStartsAndSource[uri] ??
Can this be null?  It's not clear that it can, or how.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:314: class LineStartingTableAndSource {
I'd just go with a name like SourceTable for this.

Whatever you decide, Source shouldn't be after Table because the whole thing is
a table.  That is, it's a table of line starting tables and source code.  If we
need a more complicated name, it's go to be something like
LineStartsAndSourceTable.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:328: String*& source_code_for(int i) { return
source_code_[i]; }
SourceFor(intptr_t i).

Dart VM style is to use intptr_t for array lengths.

Why return the reference?  It's cute that you can also assign it, but I think
the typical style is to have separate SourceFor and SetSourceFor functions.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:329: intptr_t* line_starts_for(int i) { return
line_starts_[i]; }
LineStartsFor(intptr_t i).

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:838: intptr_t* line_starts = new intptr_t[line_count
+ 1];
I think we should define a simple class that contains just the (pointer to the)
array of ints, but has (only) public member functions for getting the size and
zero-based get/set indexing into the payload --- it's just a representation
decision to put the size as the first element of the array and not something the
client wants to worry about.

That could be a separate change.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8720: // We cannot really answer, but at least respond with
null-entry.
Don't write that we cannot really answer, we do answer.  Write something less
conversational:

// Scripts in the AOT snapshot do not have a line starts array.
// A well-formed line number array has a leading null.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8728: info.Add(line_separator);  // new line.
Capitalize even these end-of-line comments.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9022: *last_token_index = TokenPosition(value.Value() +
42);
42 is too cute.  You could just use value.Value().  Since we have the source
string, isn't it better to use the source's length (or does the source string
include things like new line and whitespace encodings that are not counted
properly in the offsets)?

[jensj, 2016-12-20 13:30:23]

Thanks for the comments. PTAL.

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

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:336: fileUri:
"file://${element.source.fullName}");
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> fileUri: classNode.fileUri

Done.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:700: sourceCode = source.contents.data;
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> I think context.getContents(source) will use the analysis context's source
cache
> instead of reading the file from disk again.  We should probably call that
> instead.

Done.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:702: // if we cannot read the source code
there's nothing to do about that.
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> The comment should be a complete sentence (start with a capital letter).  I
> prefer to make comments less conversational.
> 
> It isn't "we" that's reading the source code, it's the program.  There is
> something to do about failing to read the source code, just use ''.  I also
> wouldn't presume that the analyzer tried to "read" something, that seems
> distracting and misleading to mention here (there are a bunch of different
kinds
> of Sources and the documentation just says it couldn't be accessed).
> 
> I tend to write this like:
> 
> String sourceCode;
> try {
>   sourceCode = context.getContents(source);
> } catch (e) {
>   // The source's contents could not be accessed.
>   sourceCode = '';
> }

Done.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:704:
program.uriToLineStartsAndSource["file://${source.fullName}"] =
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> We should probably use source.uri instead of source.fullName.

Done.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:706: new List<int>.from(lineInfo.lineStarts,
growable: false),
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> lineInfo.lineStarts is already a List<int>, isn't it?

Yes.

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

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3519: uriToLineStartsAndSource =
uriToLineStartsAndSource ?? {} {
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> Should be <String, LineStartsAndSource>{}.
> 
> Can't we just use
> 
> this.uriToLineStartsAndSource = const <String, LineStartsAndSource>{}
> 
> in the parameter list?

In loader.dart we add to it. We only have it up-front when loading from a dill
file.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3657: class LineStartsAndSource {
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> I would just call this "Source", not describe the implementation in the name.
> 
> It's a bad smell that we have to update so many identifier names just to add a
> field to an object.
> 
> It doesn't scale anyway, eventually we'll have class
> LineStartsAndSourceAndCompressionSchemeAndBreakpointPositionsAnd....

Done.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:198: Map<String, LineStartsAndSource>
uriToLineStarts = {};
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> <String, LineStartsAndSource>{}.

Done.

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_to_binary.dart (right):

https://codereview.chromium.org/2587673004/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_to_binary.dart:154:
program.uriToLineStartsAndSource[uri] ??
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> Can this be null?  It's not clear that it can, or how.

It iterates over all entries in StringIndexer, that always has a empty-string
entry. We don't have source information for that.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:314: class LineStartingTableAndSource {
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> I'd just go with a name like SourceTable for this.
> 
> Whatever you decide, Source shouldn't be after Table because the whole thing
is
> a table.  That is, it's a table of line starting tables and source code.  If
we
> need a more complicated name, it's go to be something like
> LineStartsAndSourceTable.

Done.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:328: String*& source_code_for(int i) { return
source_code_[i]; }
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> SourceFor(intptr_t i).
> 
> Dart VM style is to use intptr_t for array lengths.
> 
> Why return the reference?  It's cute that you can also assign it, but I think
> the typical style is to have separate SourceFor and SetSourceFor functions.

Done.
I think I added the '&' because I saw GrowableArray returned 'T&'... I'll remove
it.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:329: intptr_t* line_starts_for(int i) { return
line_starts_[i]; }
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> LineStartsFor(intptr_t i).

Done.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:838: intptr_t* line_starts = new intptr_t[line_count
+ 1];
On 2016/12/20 12:03:33, Kevin Millikin (Google) wrote:
> I think we should define a simple class that contains just the (pointer to
the)
> array of ints, but has (only) public member functions for getting the size and
> zero-based get/set indexing into the payload --- it's just a representation
> decision to put the size as the first element of the array and not something
the
> client wants to worry about.
> 
> That could be a separate change.

I've done another change instead. Hopefully that'll do too.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8720: // We cannot really answer, but at least respond with
null-entry.
On 2016/12/20 12:03:34, Kevin Millikin (Google) wrote:
> Don't write that we cannot really answer, we do answer.  Write something less
> conversational:
> 
> // Scripts in the AOT snapshot do not have a line starts array.
> // A well-formed line number array has a leading null.

Done.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8728: info.Add(line_separator);  // new line.
On 2016/12/20 12:03:34, Kevin Millikin (Google) wrote:
> Capitalize even these end-of-line comments.

Done.

https://codereview.chromium.org/2587673004/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9022: *last_token_index = TokenPosition(value.Value() +
42);
On 2016/12/20 12:03:34, Kevin Millikin (Google) wrote:
> 42 is too cute.  You could just use value.Value().  Since we have the source
> string, isn't it better to use the source's length (or does the source string
> include things like new line and whitespace encodings that are not counted
> properly in the offsets)?

I'll change that. I initially wrote this before including the source code,
and apparently forgot that I could fill this in correctly now.


> (or does the source string
> include things like new line and whitespace encodings that are not counted
> properly in the offsets)?

I certainly hope not. I must admit though, that I have just assumed that not to
be the case. The documentation in front_end says "Return the offset from the
beginning of the file to the first character in the syntactic entity."

[Kevin Millikin (Google), 2016-12-20 14:05:25]

kmillikin@google.com changed reviewers:
+ johnmccutchan@google.com

[Kevin Millikin (Google), 2016-12-20 14:05:25]

LGTM.

John, could you take a quick look at the VM parts of this change?

We don't have a design doc yet, but here's the idea:

We tried putting the VM's token stream in the binary file, but it doesn't have
enough information by itself to recover full source or support all observatory
features.  So we've switched back to using the source code for now.

- The binary file includes source code for each Script (in VM terms).

- Source positions on Kernel AST nodes are character offsets, because the front
end does not include the VM's tokenizer.

- We use the VM's TokenPosition class on Kernel AST nodes but we have to be
careful to interpret them differently.  TokenPosition is no longer the correct
name and we should consider changing it to SourcePosition, for example.

- There is a per-script table of line starts in the binary so we can map
character offsets to line and column numbers.

This change supports Script::Source, Script::GenerateSource, and
Script::TokenRangeAtLine correctly (interpreting token position as character
offset).  It also supports Script::GenerateLineNumberArray in a way that will
not crash observatory but isn't correct, yet.

[Cutch, 2016-12-21 00:27:12]

Using 0 as the start / end of function / class isn't right and I'm not sure why
you can't feed them the correct values now?

> The binary file includes source code for each Script (in VM terms).

Nice 

> Source positions on Kernel AST nodes are character offsets, because the front
end does not include the VM's tokenizer.

So long as we can translate between token/source position <--> line/column and
differentiate between source positions that are debuggable vs. non-debuggable it
shouldn't matter if we are using token offsets or byte offsets.

> We use the VM's TokenPosition class on Kernel AST nodes but we have to be
careful to interpret them differently.

I'm not sure I see where you're being careful interpreting them. Is that for a
later CL?

> TokenPosition is no longer the correct name and we should consider changing it
to SourcePosition, for example.

I'm fine with changing the name to SourcePosition although that conflicts with a
name in the scanner. IIRC I tried to rename it to SourcePosition but that ended
up being too big of a yak shave at the time.

John

https://codereview.chromium.org/2587673004/diff/40001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2587673004/diff/40001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:405:
function.set_end_token_pos(TokenPosition::kMinSource);
Observatory relies on knowing start / end token positions for functions. Why are
you passing in 0 here?

https://codereview.chromium.org/2587673004/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2587673004/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:3466: return TokenPosition::kMinSource;
here again, why aren't you returning the correct end token position location?

[Kevin Millikin (Google), 2016-12-21 07:20:01]

Thanks for taking a look, John.  Jens is out of the office until after the first
of January, so this CL will wait until then.

On 2016/12/21 00:27:12, Cutch wrote:
> Using 0 as the start / end of function / class isn't right and I'm not sure
why
> you can't feed them the correct values now?

The binary doesn't have start and end offsets for functions or classes.  That
just requires getting them out of the front end and making a change to the
binary format and all the reader/writer code.  I'm sure that's next, but we're
trying to break things up in small changes so we avoid long code review cycles
and so we can find out if we need to change course without wasting a lot of
development effort.

> 
> > The binary file includes source code for each Script (in VM terms).
> 
> Nice 
> 
> > Source positions on Kernel AST nodes are character offsets, because the
front
> end does not include the VM's tokenizer.
> 
> So long as we can translate between token/source position <--> line/column and
> differentiate between source positions that are debuggable vs. non-debuggable
it
> shouldn't matter if we are using token offsets or byte offsets.
> 
> > We use the VM's TokenPosition class on Kernel AST nodes but we have to be
> careful to interpret them differently.
> 
> I'm not sure I see where you're being careful interpreting them. Is that for a
> later CL?

Basically just Script::GetTokenLocation, which has already been changed before
this.  There might be other places that we become aware of as we implement
functionality.

> 
> > TokenPosition is no longer the correct name and we should consider changing
it
> to SourcePosition, for example.
> 
> I'm fine with changing the name to SourcePosition although that conflicts with
a
> name in the scanner. IIRC I tried to rename it to SourcePosition but that
ended
> up being too big of a yak shave at the time.
> 
> John
> 
>
https://codereview.chromium.org/2587673004/diff/40001/runtime/vm/kernel_reade...
> File runtime/vm/kernel_reader.cc (right):
> 
>
https://codereview.chromium.org/2587673004/diff/40001/runtime/vm/kernel_reade...
> runtime/vm/kernel_reader.cc:405:
> function.set_end_token_pos(TokenPosition::kMinSource);
> Observatory relies on knowing start / end token positions for functions. Why
are
> you passing in 0 here?
> 
> https://codereview.chromium.org/2587673004/diff/40001/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/2587673004/diff/40001/runtime/vm/object.cc#ne...
> runtime/vm/object.cc:3466: return TokenPosition::kMinSource;
> here again, why aren't you returning the correct end token position location?

[jensj, 2017-01-03 13:33:23]

Description was changed from

==========
Include source in kernel.

- For now include source uncompressed.
- When running from kernel, use token position 0
  (i.e. dummy, but 'real' position) as start and end on functions
  and classes to enable Observatory to run with the dill file.
- Debugging does not work, but one can browse the source in
  Observatory.
==========

to

==========
Include source in kernel.

- For now include source uncompressed.
- When running from kernel, use token position 0
  (i.e. dummy, but 'real' position) as start and end on functions
  and classes to enable Observatory to run with the dill file.
- Debugging does not work, but one can browse the source in
  Observatory.

R=kmillikin@google.com

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

[jensj, 2017-01-03 13:33:24]

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