Fasta kernel AST builder.

Fasta kernel AST builder.

R=asgerf@google.com

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

[ahe, 2017-01-13 13:09:27]

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

[ahe, 2017-01-13 13:09:27]

Last one :-)

[asgerf, 2017-01-13 18:39:07]

I have some preliminary comments

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

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:180: new List<Expression>.filled(n,
null, growable: true);
I think the recommended way of doing this is <Expression>[]..length = n

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:203: copy = new
List<Statement>.from(statements.getRange(0, i));
Maybe use ??= here

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:278: field =
classBuilder.members[node.name];
What if a setter has the same name as a final field in the class?

How does the name lookup distinguish these?

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:306: void prepareInitializers() {
It's not clear if this is part of the event listener.

I think in this class it would be helpful to add @override annotations
everywhere, and possibly make everything else private. Or use a naming
convention that makes it clear if something is an event response.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:471: return
buildMethodInvocation(toValue(receiver), new Name("call"),
The Name object should be put in a final static field.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:608: MapLiteral buildMapLiteral(Map
map) {
buildConstantStringMapLiteral?

The current name was confusing to me. It sounds like something used for handling
of map literals in general.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:996: valueType = typeArguments[0];
Should be typeArguments[1]

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:1447: var expression =
defaultArgumentFrom(function.positionalParameters[i].initializer);
Long line

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:1680: ///       element = #t;
element -> lvalue

There is no `element` mentioned in the matched pseudo-code

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:1691: forToken.next.next.charOffset);
Value return in a void method?

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:2037: Expression
buildProblemExpression(Builder builder, String name) {
Where is this method used? It's not called in this file.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:2069: class Identifier extends
InvalidExpression {
Do these need to be expressions at all? Can't they just extend Object?

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:2202: new Name("+"), value,
voidContext: voidContext);
These Name objects can be shared (they are not AST nodes).

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:2256: print("initializer.value =
${initializer?.value}");
These prints should be removed

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bu...
File pkg/fasta/lib/src/kernel/builder_accessors.dart (right):

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bu...
pkg/fasta/lib/src/kernel/builder_accessors.dart:413: /// `SomeType.toString`,
not `(SomeType).toString`. WTAF!?!
I agree, but let's not write comments like this.

[ahe, 2017-01-16 08:32:10]

Thank you, Asger!

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

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:180: new List<Expression>.filled(n,
null, growable: true);
On 2017/01/13 18:39:06, asgerf wrote:
> I think the recommended way of doing this is <Expression>[]..length = n

Who makes this recommendation?

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:203: copy = new
List<Statement>.from(statements.getRange(0, i));
On 2017/01/13 18:39:07, asgerf wrote:
> Maybe use ??= here

Done.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:278: field =
classBuilder.members[node.name];
On 2017/01/13 18:39:07, asgerf wrote:
> What if a setter has the same name as a final field in the class?
> 
> How does the name lookup distinguish these?

Added an internal error and TODO.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:306: void prepareInitializers() {
On 2017/01/13 18:39:06, asgerf wrote:
> It's not clear if this is part of the event listener.
> 
> I think in this class it would be helpful to add @override annotations
> everywhere, and possibly make everything else private. Or use a naming
> convention that makes it clear if something is an event response.

I like the idea of @override, and I could potentially put the other methods in a
mixin. I'll look into that later.

For now, there is a (non-obvious) naming convention: everything starting with
"begin", "end", or "handle" is an event. I should document that.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:471: return
buildMethodInvocation(toValue(receiver), new Name("call"),
On 2017/01/13 18:39:07, asgerf wrote:
> The Name object should be put in a final static field.

Done.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:608: MapLiteral buildMapLiteral(Map
map) {
On 2017/01/13 18:39:06, asgerf wrote:
> buildConstantStringMapLiteral?
> 
> The current name was confusing to me. It sounds like something used for
handling
> of map literals in general.

It's not used anymore.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:996: valueType = typeArguments[0];
On 2017/01/13 18:39:06, asgerf wrote:
> Should be typeArguments[1]

Done.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:1447: var expression =
defaultArgumentFrom(function.positionalParameters[i].initializer);
On 2017/01/13 18:39:06, asgerf wrote:
> Long line

Done.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:1680: ///       element = #t;
On 2017/01/13 18:39:06, asgerf wrote:
> element -> lvalue
> 
> There is no `element` mentioned in the matched pseudo-code

Done.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:1691: forToken.next.next.charOffset);
On 2017/01/13 18:39:06, asgerf wrote:
> Value return in a void method?

Changed to throw.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:2037: Expression
buildProblemExpression(Builder builder, String name) {
On 2017/01/13 18:39:06, asgerf wrote:
> Where is this method used? It's not called in this file.

It's used in builder_accessors.dart.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:2069: class Identifier extends
InvalidExpression {
On 2017/01/13 18:39:07, asgerf wrote:
> Do these need to be expressions at all? Can't they just extend Object?

That's a good point. It's a bigger change due to some variable types, so I've
added a TODO.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:2202: new Name("+"), value,
voidContext: voidContext);
On 2017/01/13 18:39:07, asgerf wrote:
> These Name objects can be shared (they are not AST nodes).

Done.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:2256: print("initializer.value =
${initializer?.value}");
On 2017/01/13 18:39:06, asgerf wrote:
> These prints should be removed

Done.

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bu...
File pkg/fasta/lib/src/kernel/builder_accessors.dart (right):

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bu...
pkg/fasta/lib/src/kernel/builder_accessors.dart:413: /// `SomeType.toString`,
not `(SomeType).toString`. WTAF!?!
On 2017/01/13 18:39:07, asgerf wrote:
> I agree, but let's not write comments like this.

Done.

[asgerf, 2017-01-16 11:52:26]

As we discussed offline, I find it very difficult to reason about correctness
here, but I understand that the plan is to improve on that, so LGTM.

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

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:180: new List<Expression>.filled(n,
null, growable: true);
On 2017/01/16 08:32:09, ahe wrote:
> On 2017/01/13 18:39:06, asgerf wrote:
> > I think the recommended way of doing this is <Expression>[]..length = n
> 
> Who makes this recommendation?

It is here:
https://api.dartlang.org/stable/1.21.1/dart-core/List/List.html

But I was nitpicking, sorry, there is really no need to change anything.

https://codereview.chromium.org/2628043005/diff/20001/pkg/fasta/lib/src/kerne...
File pkg/fasta/lib/src/kernel/body_builder.dart (right):

https://codereview.chromium.org/2628043005/diff/20001/pkg/fasta/lib/src/kerne...
pkg/fasta/lib/src/kernel/body_builder.dart:287: pop(); // Metadata.
Why metadata here, but not for top-level fields?

[ahe, 2017-01-16 12:32:38]

Thank you, Asger!

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

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:180: new List<Expression>.filled(n,
null, growable: true);
On 2017/01/16 11:52:26, asgerf wrote:
> On 2017/01/16 08:32:09, ahe wrote:
> > On 2017/01/13 18:39:06, asgerf wrote:
> > > I think the recommended way of doing this is <Expression>[]..length = n
> > 
> > Who makes this recommendation?
> 
> It is here:
> https://api.dartlang.org/stable/1.21.1/dart-core/List/List.html
> 
> But I was nitpicking, sorry, there is really no need to change anything.

I think I disagree with the advice when it comes to performance sensitive code.
I haven't measured the impact, but as I'm trying to keep garbage low, I prefer
the longer form I have now.

https://codereview.chromium.org/2628043005/diff/20001/pkg/fasta/lib/src/kerne...
File pkg/fasta/lib/src/kernel/body_builder.dart (right):

https://codereview.chromium.org/2628043005/diff/20001/pkg/fasta/lib/src/kerne...
pkg/fasta/lib/src/kernel/body_builder.dart:287: pop(); // Metadata.
On 2017/01/16 11:52:26, asgerf wrote:
> Why metadata here, but not for top-level fields?

It's due to a difference between parseMember and parseTopLevelMember.

The latter is called from parseTopLevelDeclaration which means that metadata is
parsed (and ignored) by DietParser. On the other hand, parseMember has to parse
the metadata itself. I'll add a comment about this.

[ahe, 2017-01-16 12:34:03]

Description was changed from

==========
Fasta kernel AST builder.
==========

to

==========
Fasta kernel AST builder.

R=asgerf@google.com

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

[ahe, 2017-01-16 12:34:05]

Committed patchset #4 (id:50001) manually as
d1c89a664113db175fdfb2417c8ddc7a9f826869 (presubmit successful).

[ahe, 2017-01-18 11:51:44]

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

https://codereview.chromium.org/2628043005/diff/1/pkg/fasta/lib/src/kernel/bo...
pkg/fasta/lib/src/kernel/body_builder.dart:306: void prepareInitializers() {
On 2017/01/16 08:32:09, ahe wrote:
> On 2017/01/13 18:39:06, asgerf wrote:
> > It's not clear if this is part of the event listener.
> > 
> > I think in this class it would be helpful to add @override annotations
> > everywhere, and possibly make everything else private. Or use a naming
> > convention that makes it clear if something is an event response.
> 
> I like the idea of @override, and I could potentially put the other methods in
a
> mixin. I'll look into that later.
> 
> For now, there is a (non-obvious) naming convention: everything starting with
> "begin", "end", or "handle" is an event. I should document that.

Followed up in CL 2645513002.