Introduce a shadow hierarchy and factory class for use by fasta's BodyBuilder.

Introduce a shadow hierarchy and factory class for use by fasta's BodyBuilder.

In the long run, this shadow hierarchy is intended to be used both for
kernel and analyzer ASTS; this should allow a single BodyBuilder class
to build either a kernel or an analyzer AST depending on which factory
it's connected to.

For now we only have a kernel shadow hierarchy, and it's incomplete,
so the code is not yet used.  I intend to follow up with CLs that
complete the implementation and hook it up to BodyBuilder.

R=ahe@google.com, scheglov@google.com

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

[Paul Berry, 2017-03-24 02:53:07]

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

[Paul Berry, 2017-03-24 02:53:08]

I'm hoping I can get this committed in the morning (US west coast time) so that
Peter can build on it while I'm away next week.

There are a number of outstanding TODO items which I hope to start addressing
later in the day, or after I return.  (Peter, if you need to address a TODO item
while I'm away in order to unblock your progress, feel free).

To see roughly where I'm heading with this, have a look at my type inference
prototype: https://codereview.chromium.org/2768533002/.

[scheglov, 2017-03-24 03:19:17]

LGTM!

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:1: // Copyright (c) 2016,
the Dart project authors.  Please see the AUTHORS file
2017

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:28: /// that's what
analyzer ASTs need.
Probably not only offsets, but also modifiers like isConst, and a lot of other
tokens, like "<" and ">" in type arguments?

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:35: Block
block(List<Statement> statements, int charOffset);
What is charOffset?

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:43: /// [type] should be
`null`.
[type] -> [typeArgument]?

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/shadow_ast.dart:54: DartType
shadowMakeFunctionType();
Why not getter?

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/shadow_ast.dart:108: /// Shadow mixin
representing a "read" reference to a variable.
I don't understand this yet.
Is it a node or element in Analyzer terms?
Is it partially resolved, so it knows the referenced variable?
Are these variables limited to locals, or can be also top-levels and fields?
If it also includes not locals, are we going to use shadowInitializer or only
shadowType for them?

[ahe, 2017-03-24 12:00:41]

lgtm

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:28: /// that's what
analyzer ASTs need.
As I'm nervous about accidentally storing tokens in Fasta, I've tried to avoid
using Token in most API. I'm not sure if this matters for an AST factory, but
here's a suggestion for how we can keep KernelAstFactory free of tokens:

class AstFactory<L> {
  L toLocation(Token token);

  Block block(List<Statement> statements, L location);
  ...
}

class KernelAstBuilder extends AstFactory<int> {
  int toLocation(Token token) => token.charOffset;
  ...
}

class AnalyzerAstBuilder extends AstFactory<Token> {
  Token toLocation(Token token) => token;
  ...
}

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:58: /// kernel, which
desugars each variable declaration to its own statement.
FYI: The parser does generate a plural event for variable declarations so it
should be possible to support that.

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/shadow_ast.dart:1: // Copyright (c) 2016,
the Dart project authors.  Please see the AUTHORS file
This comment is so last year ;-)

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/shadow_ast.dart:12: /// "shadow" in order to
avoid naming conflicts with code in kernel or analyzer.
It would probably help debugging if we also add Shadow prefix to the classes
defined here.

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/ast_factory.dart:1: import
'package:kernel/ast.dart' show DartType;
Missing copyright.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/ast_factory.dart:3: import
'../builder/ast_factory.dart' as builder;
Could you rename this file to kernel_ast_factory.dart? It makes it easier for me
in Emacs :-)

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/ast_factory.dart:8: class AstFactory
implements builder.AstFactory {
KernelAstFactory?

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/shadow_ast.dart:1: // Copyright (c) 2017, the
Dart project authors.  Please see the AUTHORS file
\o/

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/shadow_ast.dart:25: class Block extends
kernel.Block with builder.Block implements Statement {
KernelBlock?

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/shadow_ast.dart:136: /// `null` to be stored
in [kernel.VariableDeclaration] prior to type inference.
Alternative: create a subclass of DynamicType called MissingType.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/shadow_ast.dart:136: /// `null` to be stored
in [kernel.VariableDeclaration] prior to type inference.
Long line.

[Paul Berry, 2017-03-24 14:55:24]

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:1: // Copyright (c) 2016,
the Dart project authors.  Please see the AUTHORS file
On 2017/03/24 03:19:16, scheglov wrote:
> 2017

Done.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:28: /// that's what
analyzer ASTs need.
On 2017/03/24 03:19:16, scheglov wrote:
> Probably not only offsets, but also modifiers like isConst, and a lot of other
> tokens, like "<" and ">" in type arguments?

Agreed.  Updated the TODO comment.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:28: /// that's what
analyzer ASTs need.
On 2017/03/24 12:00:40, ahe wrote:
> As I'm nervous about accidentally storing tokens in Fasta, I've tried to avoid
> using Token in most API. I'm not sure if this matters for an AST factory, but
> here's a suggestion for how we can keep KernelAstFactory free of tokens:
> 
> class AstFactory<L> {
>   L toLocation(Token token);
> 
>   Block block(List<Statement> statements, L location);
>   ...
> }
> 
> class KernelAstBuilder extends AstFactory<int> {
>   int toLocation(Token token) => token.charOffset;
>   ...
> }
> 
> class AnalyzerAstBuilder extends AstFactory<Token> {
>   Token toLocation(Token token) => token;
>   ...
> }

Good point.  I'll try this in a follow-up CL.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:35: Block
block(List<Statement> statements, int charOffset);
On 2017/03/24 03:19:16, scheglov wrote:
> What is charOffset?

The location of the opening `{` token.  I'll clean this up in a future CL--see
the discussion at line 28.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:43: /// [type] should be
`null`.
On 2017/03/24 03:19:16, scheglov wrote:
> [type] -> [typeArgument]?

Done.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:58: /// kernel, which
desugars each variable declaration to its own statement.
On 2017/03/24 12:00:40, ahe wrote:
> FYI: The parser does generate a plural event for variable declarations so it
> should be possible to support that.

Acknowledged.

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/shadow_ast.dart:1: // Copyright (c) 2016,
the Dart project authors.  Please see the AUTHORS file
On 2017/03/24 12:00:40, ahe wrote:
> This comment is so last year ;-)

Done.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/shadow_ast.dart:12: /// "shadow" in order to
avoid naming conflicts with code in kernel or analyzer.
On 2017/03/24 12:00:40, ahe wrote:
> It would probably help debugging if we also add Shadow prefix to the classes
> defined here.

Fair enough.  Done.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/shadow_ast.dart:54: DartType
shadowMakeFunctionType();
On 2017/03/24 03:19:17, scheglov wrote:
> Why not getter?

Done.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/shadow_ast.dart:108: /// Shadow mixin
representing a "read" reference to a variable.
On 2017/03/24 03:19:16, scheglov wrote:
> I don't understand this yet.
> Is it a node or element in Analyzer terms?
> Is it partially resolved, so it knows the referenced variable?
> Are these variables limited to locals, or can be also top-levels and fields?
> If it also includes not locals, are we going to use shadowInitializer or only
> shadowType for them?

I haven't prototyped this yet, but here's my current thinking:

(Note that I'm following Peter's advice and renaming the classes in this file to
start with "Shadow...", and the derived classes in fasta/kernel/shadow_ast to
"Kernel...".  I'll follow the new conventions in this explanation, and I'll use
"Analyzer..." to talk about the derived/wrapper classes I hope to introduce in
the future for analyzer.)

- This will be used for locals, top-level variables, and fields.

- For kernel there really isn't a separate notion of element and AST--everything
is a kernel tree node.  So when a KernelVariableGet is constructed, it will just
store the pointer to the KernelVariableDeclaration.

- For analyzer, the equivalent of a VariableGet is SimpleIdentifier (or
sometimes PrefixedIdentifier), and that points to an element, not an AST node. 
The way this will work is that AnalyzerVariableDeclaration will be a wrapper
that stores both a pointer to an AST node and an element.  For local variables,
both will be populated.  For non-local variables, whatever Fasta code creates
the element model will also create an AnalyzerVariableDeclaration, and that will
point just to the element.  In both cases, when an AnalyzerVariableGet is
created, it will extract the element from the AnalyzerVariableDeclaration, and
point the Identifier to it.

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/ast_factory.dart:1: import
'package:kernel/ast.dart' show DartType;
On 2017/03/24 12:00:40, ahe wrote:
> Missing copyright.

Done.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/ast_factory.dart:3: import
'../builder/ast_factory.dart' as builder;
On 2017/03/24 12:00:40, ahe wrote:
> Could you rename this file to kernel_ast_factory.dart? It makes it easier for
me
> in Emacs :-)

Done.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/ast_factory.dart:8: class AstFactory
implements builder.AstFactory {
On 2017/03/24 12:00:40, ahe wrote:
> KernelAstFactory?

Done.

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

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/shadow_ast.dart:25: class Block extends
kernel.Block with builder.Block implements Statement {
On 2017/03/24 12:00:41, ahe wrote:
> KernelBlock?

Done.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/shadow_ast.dart:136: /// `null` to be stored
in [kernel.VariableDeclaration] prior to type inference.
On 2017/03/24 12:00:40, ahe wrote:
> Alternative: create a subclass of DynamicType called MissingType.

Interesting idea.  I've noted it in the TODO.

https://codereview.chromium.org/2769723004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/shadow_ast.dart:136: /// `null` to be stored
in [kernel.VariableDeclaration] prior to type inference.
On 2017/03/24 12:00:41, ahe wrote:
> Long line.

Done.

[Paul Berry, 2017-03-24 15:26:05]

Description was changed from

==========
Introduce a shadow hierarchy and factory class for use by fasta's BodyBuilder.

In the long run, this shadow hierarchy is intended to be used both for
kernel and analyzer ASTS; this should allow a single BodyBuilder class
to build either a kernel or an analyzer AST depending on which factory
it's connected to.

For now we only have a kernel shadow hierarchy, and it's incomplete,
so the code is not yet used.  I intend to follow up with CLs that
complete the implementation and hook it up to BodyBuilder.
==========

to

==========
Introduce a shadow hierarchy and factory class for use by fasta's BodyBuilder.

In the long run, this shadow hierarchy is intended to be used both for
kernel and analyzer ASTS; this should allow a single BodyBuilder class
to build either a kernel or an analyzer AST depending on which factory
it's connected to.

For now we only have a kernel shadow hierarchy, and it's incomplete,
so the code is not yet used.  I intend to follow up with CLs that
complete the implementation and hook it up to BodyBuilder.

R=ahe@google.com, scheglov@google.com

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

[Paul Berry, 2017-03-24 15:26:06]

Committed patchset #1 (id:1) manually as
415c868589d02e98eb839f48150f4203d5cecdb0 (presubmit successful).