Change accessors and AstFactory to use tokens rather than file offsets.

Change accessors and AstFactory to use tokens rather than file offsets.

This is a necessary prerequisite for re-using BodyBuilder to make
analyzer ASTs, since analyzer ASTs use tokens rather than offsets to
keep track of file positions.

R=ahe@google.com

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

[Paul Berry, 2017-04-25 12:37:51]

paulberry@google.com changed reviewers:
+ ahe@google.com

[Paul Berry, 2017-04-25 12:37:52]

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/builder/ast_factory.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:41: Field field(Name name,
int charOffset, {String fileUri});
Note: I realized as I was going through this exercise that this method doesn't
belong in AstFactory, because it's not used by BodyBuilder; it's used by
KernelFieldBuilder, which always builds kernel objects.  I'll submit a separate
CL that removes this method and changes KernelFieldBuilder to construct the
object directly.

[ahe, 2017-04-25 17:39:59]

lgtm

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:630: // using token.  Is
that a problem?
I think so. Try running the service tests:

nice ./tools/test.py -cdartk -rvm -mrelease --time -pcolor --report
--failure-summary service

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:841: /// problems.
[isQualified] should be true if [name] is a qualified access
Perhaps this documentation of charOffset/token isn't completely correct.

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1345: value =
symbolPartToString(pop());
Why this change?

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2646: class Identifier {
Could remove the superclass in a separate CL?

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/fasta_accessors.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/fasta_accessors.dart:486:
offsetForToken(token) + (target.name?.name?.length ?? 0),
You can also test this with the service tests.

I wonder if we can use the token's end offset here instead of adding the length
of the name?

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/token.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/token.dart:10: import
'package:kernel/ast.dart' show TreeNode;
It feels a bit much to import package:kernel here to get a symbolic name for -1.
Could we cheat and use -1?

Another alternative is to move offsetForToken to ../scanner.dart. That file also
exports Token, so perhaps you can remove the change to subpackage rules?

[Paul Berry, 2017-04-25 22:00:56]

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:630: // using token.  Is
that a problem?
On 2017/04/25 17:39:59, ahe wrote:
> I think so. Try running the service tests:
> 
> nice ./tools/test.py -cdartk -rvm -mrelease --time -pcolor --report
> --failure-summary service

Thanks!  I've verified that these tests pass and I've removed the TODO comments.

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:841: /// problems.
[isQualified] should be true if [name] is a qualified access
On 2017/04/25 17:39:59, ahe wrote:
> Perhaps this documentation of charOffset/token isn't completely correct.

Good point.  Fixed.

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1345: value =
symbolPartToString(pop());
On 2017/04/25 17:39:59, ahe wrote:
> Why this change?

This is necessary in order to decouple Identifier from Expression.  If we don't
change this, but we decouple Identifier from Expression, then popForValue() will
try to pop an Identifier and won't know what to do with it.

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2646: class Identifier {
On 2017/04/25 17:39:59, ahe wrote:
> Could remove the superclass in a separate CL?

Ok, I will do so before landing this CL.

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/fasta_accessors.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/fasta_accessors.dart:486:
offsetForToken(token) + (target.name?.name?.length ?? 0),
On 2017/04/25 17:39:59, ahe wrote:
> You can also test this with the service tests.
> 
> I wonder if we can use the token's end offset here instead of adding the
length
> of the name?

Is the idea that this is supposed to be the offset of the `.`?  If so maybe we
should just track the `.` token--analyzer is going to need that anyhow.

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/token.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/token.dart:10: import
'package:kernel/ast.dart' show TreeNode;
On 2017/04/25 17:39:59, ahe wrote:
> It feels a bit much to import package:kernel here to get a symbolic name for
-1.
> Could we cheat and use -1?
> 
> Another alternative is to move offsetForToken to ../scanner.dart. That file
also
> exports Token, so perhaps you can remove the change to subpackage rules?

What's bothering you, the import of kernel or the change to the subpackage
rules?  Cheating and using -1 would fix the former (at the expense of some
future risk if TreeNode.noOffset is ever changed) but not the latter.  Moving
offsetForToken would avoid the need to change the subpackage rules, but wouldn't
address the import of kernel (it would just be a different file that has to
import kernel).

I like using TreeNode.noOffset--it feels nice to be explicit about the fact that
we're using -1 in order to agree with kernel conventions.  But I agree that it
seems weird and unnecessary for the scanner to have to depend on kernel.  Maybe
a better approach would be to move offsetForToken somewhere inside
lib/src/fasta/kernel (e.g. lib/src/fasta/kernel/utils.dart).  That would be less
weird, since nobody would be surprised to see a file in lib/src/fasta/kernel
importing kernel.  And it would avoid the need to change the subpackage rules
too, since all of the callers of offsetForToken are in lib/src/fasta/kernel.

I'll try that.

[ahe, 2017-04-26 08:34:36]

Sorry that I'm repeating myself, but I figured there's a big chance I wasn't
clear earlier :-)

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/builder/ast_factory.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:5: import
'package:front_end/src/fasta/scanner/token.dart' show Token;
If you import package:front_end/src/fasta/scanner.dart instead, you don't need
to change the subpackage relationship.

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/token.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/token.dart:10: import
'package:kernel/ast.dart' show TreeNode;
On 2017/04/25 22:00:55, Paul Berry wrote:
> On 2017/04/25 17:39:59, ahe wrote:
> > It feels a bit much to import package:kernel here to get a symbolic name for
> -1.
> > Could we cheat and use -1?
> > 
> > Another alternative is to move offsetForToken to ../scanner.dart. That file
> also
> > exports Token, so perhaps you can remove the change to subpackage rules?
> 
> What's bothering you, the import of kernel or the change to the subpackage
> rules?

Both, I think.

>  Cheating and using -1 would fix the former (at the expense of some
> future risk if TreeNode.noOffset is ever changed) but not the latter.  Moving
> offsetForToken would avoid the need to change the subpackage rules, but
wouldn't
> address the import of kernel (it would just be a different file that has to
> import kernel).
> 
> I like using TreeNode.noOffset--it feels nice to be explicit about the fact
that
> we're using -1 in order to agree with kernel conventions.  But I agree that it
> seems weird and unnecessary for the scanner to have to depend on kernel. 
Maybe
> a better approach would be to move offsetForToken somewhere inside
> lib/src/fasta/kernel (e.g. lib/src/fasta/kernel/utils.dart).  That would be
less
> weird, since nobody would be surprised to see a file in lib/src/fasta/kernel
> importing kernel.  And it would avoid the need to change the subpackage rules
> too, since all of the callers of offsetForToken are in lib/src/fasta/kernel.
> 
> I'll try that.

OK

[Paul Berry, 2017-04-26 20:20:52]

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/builder/ast_factory.dart (right):

https://codereview.chromium.org/2841863002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:5: import
'package:front_end/src/fasta/scanner/token.dart' show Token;
On 2017/04/26 08:34:36, ahe wrote:
> If you import package:front_end/src/fasta/scanner.dart instead, you don't need
> to change the subpackage relationship.

Done.

[Paul Berry, 2017-04-26 20:29:46]

Description was changed from

==========
Change accessors and AstFactory to use tokens rather than file offsets.

This is a necessary prerequisite for re-using BodyBuilder to make
analyzer ASTs, since analyzer ASTs use tokens rather than offsets to
keep track of file positions.
==========

to

==========
Change accessors and AstFactory to use tokens rather than file offsets.

This is a necessary prerequisite for re-using BodyBuilder to make
analyzer ASTs, since analyzer ASTs use tokens rather than offsets to
keep track of file positions.

R=ahe@google.com

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

[Paul Berry, 2017-04-26 20:29:47]

Committed patchset #2 (id:20001) manually as
d7031ce9bf6bbac7be533a5c3f1b7f2728afd437 (presubmit successful).