Prepare for decoupling analyzer ASTs from element model.

Prepare for decoupling analyzer ASTs from element model.

This CL creates top level accessor functions which may be used to map from AST data structures to their corresponding elements and types, and begins using those accessor functions throughout the SDK.  It also adds empty interfaces ResolutionTarget and ResolutionType, which are implemented by Element and DartType respectively.

In a future CL, I will change the types stored in the AST to ResolutionTarget and ResolutionType, rather than specific element types; this will decouple the ASTs from the element model.  The presence of the accessor functions will allow clients to continue accessing elements and types in a type-safe way.

R=asgerf@google.com, brianwilkerson@google.com, scheglov@google.com, vsm@google.com

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

[Paul Berry, 2016-12-05 16:50:56]

paulberry@google.com changed reviewers:
+ asgerf@google.com, brianwilkerson@google.com, jmesserly@google.com,
scheglov@google.com, vsm@google.com

[vsm, 2016-12-05 17:01:46]

lgtm on ddc part

[vsm, 2016-12-05 17:01:48]

lgtm on ddc part

[scheglov, 2016-12-05 17:10:50]

LGTM

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/dart_test.dart (right):

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:36: import
'package:analyzer/dart/ast/resolution_accessors.dart';
Not sorted.

[Paul Berry, 2016-12-05 17:50:48]

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/dart_test.dart (right):

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/dart_test.dart:36: import
'package:analyzer/dart/ast/resolution_accessors.dart';
On 2016/12/05 17:10:50, scheglov wrote:
> Not sorted.

Fixed, thanks.

[Brian Wilkerson, 2016-12-05 18:22:11]

lgtm

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/lib/dart/ast/r...
File pkg/analyzer/lib/dart/ast/resolution_accessors.dart (right):

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/lib/dart/ast/r...
pkg/analyzer/lib/dart/ast/resolution_accessors.dart:16: ExecutableElement
bestElementForFunctionExpressionInvocation(
I am concerned about the use of top-level functions here. I think it would be
much better to provide an interface with instance methods (similar to
AstFactory). I don't know whether we're likely to want to mock that interface
for testing, but I can easily see wanting to have multiple implementations so
that we can start working on a new implementation at the same time as we
continue to transition code outside the SDK.

Also, I'd be surprised if the non-AST-based implementation didn't need some
additional data beyond the AST node, and having these as methods on an object
would allow the object to hold the other state.

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/lib/dart/ast/r...
pkg/analyzer/lib/dart/ast/resolution_accessors.dart:76: ClassElement
elementForClassDeclaration(ClassDeclaration node) => node.element;
Consider renaming functions like this to something like 'elementDeclaredBy*' to
make clear the distinction between declarations of and references to elements.

[asgerf, 2016-12-06 13:46:15]

I don't see the bigger picture here, but if you want to go through with it, the
kernel changes LGTM.

[Paul Berry, 2016-12-07 18:06:16]

Brian, PTAL.  The core changes are in the following files (all other changes are
just consequences of renames and moving the code around):

pkg/analyzer/lib/dart/ast/resolution_map.dart
pkg/analyzer/lib/dart/ast/standard_resolution_map.dart
pkg/analyzer/lib/src/dart/ast/resolution_map.dart

Other reviewers: feel free to take another look if you want, but it's not
necessary.

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/lib/dart/ast/r...
File pkg/analyzer/lib/dart/ast/resolution_accessors.dart (right):

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/lib/dart/ast/r...
pkg/analyzer/lib/dart/ast/resolution_accessors.dart:16: ExecutableElement
bestElementForFunctionExpressionInvocation(
On 2016/12/05 18:22:11, Brian Wilkerson wrote:
> I am concerned about the use of top-level functions here. I think it would be
> much better to provide an interface with instance methods (similar to
> AstFactory). I don't know whether we're likely to want to mock that interface
> for testing, but I can easily see wanting to have multiple implementations so
> that we can start working on a new implementation at the same time as we
> continue to transition code outside the SDK.
> 
> Also, I'd be surprised if the non-AST-based implementation didn't need some
> additional data beyond the AST node, and having these as methods on an object
> would allow the object to hold the other state.

Fair enough.  I reworked to use a similar model to what we're doing with
AstFactory.

https://codereview.chromium.org/2551023005/diff/1/pkg/analyzer/lib/dart/ast/r...
pkg/analyzer/lib/dart/ast/resolution_accessors.dart:76: ClassElement
elementForClassDeclaration(ClassDeclaration node) => node.element;
On 2016/12/05 18:22:11, Brian Wilkerson wrote:
> Consider renaming functions like this to something like 'elementDeclaredBy*'
to
> make clear the distinction between declarations of and references to elements.

Done.

[Brian Wilkerson, 2016-12-07 18:26:20]

lgtm

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/resolution_map.dart (right):

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/resolution_map.dart:15: class ResolutionMap {
I was expecting to see an abstract class without an implementation. Was that an
oversight, or is there a reason for leaving this a concrete class?

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/resolution_map.dart:172: CompilationUnitElement
elementForCompilationUnit(CompilationUnit node) =>
I suppose it's a little strange to say that the compilation unit "declares"
anything, and yet it doesn't exactly reference anything either, so I think I'd
lean toward the "DeclaredBy" nomenclature.

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/ast/resolution_map.dart (right):

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/ast/resolution_map.dart:21: */
nit: Given that the comments are all on the overridden methods, I'd remove them
here.

[Paul Berry, 2016-12-07 18:55:02]

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/dart/a...
File pkg/analyzer/lib/dart/ast/resolution_map.dart (right):

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/resolution_map.dart:15: class ResolutionMap {
On 2016/12/07 18:26:20, Brian Wilkerson wrote:
> I was expecting to see an abstract class without an implementation. Was that
an
> oversight, or is there a reason for leaving this a concrete class?

Whoops, this was an oversight.  I was intending to make this abstract and remove
the implementations, and I forgot.

Fixed now.

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/dart/a...
pkg/analyzer/lib/dart/ast/resolution_map.dart:172: CompilationUnitElement
elementForCompilationUnit(CompilationUnit node) =>
On 2016/12/07 18:26:20, Brian Wilkerson wrote:
> I suppose it's a little strange to say that the compilation unit "declares"
> anything, and yet it doesn't exactly reference anything either, so I think I'd
> lean toward the "DeclaredBy" nomenclature.

Done.

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/ast/resolution_map.dart (right):

https://codereview.chromium.org/2551023005/diff/40001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/ast/resolution_map.dart:21: */
On 2016/12/07 18:26:20, Brian Wilkerson wrote:
> nit: Given that the comments are all on the overridden methods, I'd remove
them
> here.

Done.

[Paul Berry, 2016-12-07 19:08:19]

Description was changed from

==========
Prepare for decoupling analyzer ASTs from element model.

This CL creates top level accessor functions which may be used to map from AST
data structures to their corresponding elements and types, and begins using
those accessor functions throughout the SDK.  It also adds empty interfaces
ResolutionTarget and ResolutionType, which are implemented by Element and
DartType respectively.

In a future CL, I will change the types stored in the AST to ResolutionTarget
and ResolutionType, rather than specific element types; this will decouple the
ASTs from the element model.  The presence of the accessor functions will allow
clients to continue accessing elements and types in a type-safe way.
==========

to

==========
Prepare for decoupling analyzer ASTs from element model.

This CL creates top level accessor functions which may be used to map from AST
data structures to their corresponding elements and types, and begins using
those accessor functions throughout the SDK.  It also adds empty interfaces
ResolutionTarget and ResolutionType, which are implemented by Element and
DartType respectively.

In a future CL, I will change the types stored in the AST to ResolutionTarget
and ResolutionType, rather than specific element types; this will decouple the
ASTs from the element model.  The presence of the accessor functions will allow
clients to continue accessing elements and types in a type-safe way.

R=asgerf@google.com, brianwilkerson@google.com, scheglov@google.com,
vsm@google.com

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

[Paul Berry, 2016-12-07 19:08:20]

Committed patchset #3 (id:40001) manually as
ae6fd961a1e3a5d0009b01b6a3f9c424a7b61ab5 (presubmit successful).