Store ResolvedAst on AstElement

Store ResolvedAst on AstElement

R=sigmund@google.com

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

[Johnni Winther, 2016-05-11 08:56:02]

johnniwinther@google.com changed reviewers:
+ sra@google.com

[Johnni Winther, 2016-05-13 09:38:43]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com
- sra@google.com

[Siggi Cherem (dart-lang), 2016-05-13 19:43:51]

very nice! lgtm with some comments below.

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1924: if
(compiler.serialization.isDeserialized(element)) {
should we also revert to just do `element.hasResolvedAst here as well?

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right):

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:358: void
analyzeSuperConstructorCall(AstElement target, ArgumentsTypes arguments) {
nit: dartfmt

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart (right):

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:373:
Map<Element, ResolvedAst> resolvedAstMap*/) {
delete?

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:669: = new
ParsedResolvedAst(function, functionExpression,
nit: dartfmt

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:686: (element
as AstElementMixinZ).resolvedAst
it seems like the cast here is not necessary (element is declared as
AstElementMixinZ)

https://codereview.chromium.org/1970703002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/serialization/helper.dart (right):

https://codereview.chromium.org/1970703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization/helper.dart:234: bool
hasResolvedAst(ExecutableElement element) {
I wonder if we can get rid of `hasResovledAst` now and simply test whether
resolvedAst != null (here, and in AstElement and AstElementZ)

In particular, the `hasResolvedAst` API seems to only be used in this file and
in assertions elsewhere.

https://codereview.chromium.org/1970703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization/helper.dart:335: if
(element.hasResolvedAst) {
correct me if I'm wrong, Is this always called from
_DeserializerSystem.getResolvedAst?

If so, it seems we could check `hasResolvedAst` only in one of them.

https://codereview.chromium.org/1970703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization/helper.dart:345: return
element.resolvedAst;
could we assert here that element.resolvedAst != null?

[Johnni Winther, 2016-05-17 11:59:52]

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:1924: if
(compiler.serialization.isDeserialized(element)) {
On 2016/05/13 19:43:50, Siggi Cherem (dart-lang) wrote:
> should we also revert to just do `element.hasResolvedAst here as well?

Doesn't work for modelx (yet) since it doesn't have a single point in time where
ResolvedAst is computed.

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart (right):

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/simple_types_inferrer.dart:358: void
analyzeSuperConstructorCall(AstElement target, ArgumentsTypes arguments) {
On 2016/05/13 19:43:50, Siggi Cherem (dart-lang) wrote:
> nit: dartfmt

Done.

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/se...
File pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart (right):

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:373:
Map<Element, ResolvedAst> resolvedAstMap*/) {
On 2016/05/13 19:43:51, Siggi Cherem (dart-lang) wrote:
> delete?

Done.

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:669: = new
ParsedResolvedAst(function, functionExpression,
On 2016/05/13 19:43:51, Siggi Cherem (dart-lang) wrote:
> nit: dartfmt

Done.

https://codereview.chromium.org/1970703002/diff/20001/pkg/compiler/lib/src/se...
pkg/compiler/lib/src/serialization/resolved_ast_serialization.dart:686: (element
as AstElementMixinZ).resolvedAst
On 2016/05/13 19:43:51, Siggi Cherem (dart-lang) wrote:
> it seems like the cast here is not necessary (element is declared as
> AstElementMixinZ)

Done.

https://codereview.chromium.org/1970703002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/serialization/helper.dart (right):

https://codereview.chromium.org/1970703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization/helper.dart:234: bool
hasResolvedAst(ExecutableElement element) {
On 2016/05/13 19:43:51, Siggi Cherem (dart-lang) wrote:
> I wonder if we can get rid of `hasResovledAst` now and simply test whether
> resolvedAst != null (here, and in AstElement and AstElementZ)
> 
> In particular, the `hasResolvedAst` API seems to only be used in this file and
> in assertions elsewhere.

Soon. Adding a TODO.

https://codereview.chromium.org/1970703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization/helper.dart:335: if
(element.hasResolvedAst) {
On 2016/05/13 19:43:51, Siggi Cherem (dart-lang) wrote:
> correct me if I'm wrong, Is this always called from
> _DeserializerSystem.getResolvedAst?
> 
> If so, it seems we could check `hasResolvedAst` only in one of them.

Done.

https://codereview.chromium.org/1970703002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/serialization/helper.dart:345: return
element.resolvedAst;
On 2016/05/13 19:43:51, Siggi Cherem (dart-lang) wrote:
> could we assert here that element.resolvedAst != null?

Done.

[Johnni Winther, 2016-05-17 12:00:15]

Description was changed from

==========
Store ResolvedAst on AstElement
==========

to

==========
Store ResolvedAst on AstElement

R=sigmund@google.com

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

[Johnni Winther, 2016-05-17 12:00:16]

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