Pulling the element model out of global type inference.

Pulling the element model out of global type inference.

BUG=
R=johnniwinther@google.com, sigmund@google.com

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

[Emily Fortuna, 2017-03-15 18:24:42]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2017-03-15 18:26:22]

Patchset #1 (id:20001) has been deleted

[Emily Fortuna, 2017-03-15 18:35:52]

Patchset #1 (id:40001) has been deleted

[Emily Fortuna, 2017-03-15 18:37:15]

efortuna@google.com changed reviewers:
+ johnniwinther@google.com, sigmund@google.com

[Emily Fortuna, 2017-03-15 18:37:15]

This CL was much larger, but it contained a lot of half-implemented methods with
portions implemented! ^.^ I trimmed it down to the methods that are complete
thus far.

[Siggi Cherem (dart-lang), 2017-03-16 00:37:45]

lgtm, just minor comments below

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/builder_kernel.dart (right):

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:21: ///
[KernelTypeGraphBuilder] can be thought of as a type-inference graph
BTW - we should be able to update these comments a bit. They were written when
the name of this class was "SimpleTypeInferrerVisitor" so we had to explain it
was sort of like a builder :)

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:28: /// This visitor is
parameterized by an [InferenceEngine], which internally
similarly note the engine parameter is now gone (Johnni got rid of it, we seem
to still have the old comments though)

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:46: bool get inLoop =>
loopLevel > 0;
nit: I'd move htis up next to loopLevel (it mixes fields and getters, but I
think there are so coupled that it's nice to see them together.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:54: this.astAdapter = new
KernelAstAdapter(kernel, compiler.backend,
it might not be worth making this change because the adapter is temporary, but
if it were more permanent I'd try to make this an initializer so we could make
the field final. It should be possible either with a helper function or with a
factory method. For example:

  : astAdapter = _createKernelAdapter(compiler, resolvedAst);

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:57: analyzedNode =
astAdapter.getInitialKernelNode(originalElement);
will analyzerNode ever change? If not, then maybe going the factory route might
be easier to make it final as well.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:59: ast.Node node;
I'm guessing the idea is this will be replaced by the analyzedNode once we get
to change the locals handler?

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:83: initializer =
(analyzedNode as ir.Field).initializer;
If we can make analyzerNode final above, then this cast will no longer be
necessary (the is-check will promote the field).

Meanwhile, a convention we follow in dart2js is to avoid the cast and instead do
an assignment to a temporary variable with the precise type:

if (analyzedNode is ir.Field) {
  ir.Field field = analyzerdNode;
  if (field.initializer == null) ...

This makes it a cast in checked-mode, but free in production mode. In the future
with strong-mode, there wont be that difference anymore though.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:157: if (targetElement is
FunctionElement &&
I think you can drop this check too (since ConstructorBodyElement is also a
FunctionElement)

[Johnni Winther, 2017-03-16 14:16:18]

lgtm

[Emily Fortuna, 2017-03-17 01:03:02]

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/builder_kernel.dart (right):

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:21: ///
[KernelTypeGraphBuilder] can be thought of as a type-inference graph
On 2017/03/16 00:37:44, Siggi Cherem (dart-lang) wrote:
> BTW - we should be able to update these comments a bit. They were written when
> the name of this class was "SimpleTypeInferrerVisitor" so we had to explain it
> was sort of like a builder :)

Done.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:28: /// This visitor is
parameterized by an [InferenceEngine], which internally
On 2017/03/16 00:37:44, Siggi Cherem (dart-lang) wrote:
> similarly note the engine parameter is now gone (Johnni got rid of it, we seem
> to still have the old comments though)

Done.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:46: bool get inLoop =>
loopLevel > 0;
On 2017/03/16 00:37:44, Siggi Cherem (dart-lang) wrote:
> nit: I'd move htis up next to loopLevel (it mixes fields and getters, but I
> think there are so coupled that it's nice to see them together.

Done.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:54: this.astAdapter = new
KernelAstAdapter(kernel, compiler.backend,
On 2017/03/16 00:37:45, Siggi Cherem (dart-lang) wrote:
> it might not be worth making this change because the adapter is temporary, but
> if it were more permanent I'd try to make this an initializer so we could make
> the field final. It should be possible either with a helper function or with a
> factory method. For example:
> 
>   : astAdapter = _createKernelAdapter(compiler, resolvedAst);

Done.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:57: analyzedNode =
astAdapter.getInitialKernelNode(originalElement);
On 2017/03/16 00:37:44, Siggi Cherem (dart-lang) wrote:
> will analyzerNode ever change? If not, then maybe going the factory route
might
> be easier to make it final as well.

Done.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:59: ast.Node node;
On 2017/03/16 00:37:45, Siggi Cherem (dart-lang) wrote:
> I'm guessing the idea is this will be replaced by the analyzedNode once we get
> to change the locals handler?

yep. The list of things to do grows very quickly!

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:83: initializer =
(analyzedNode as ir.Field).initializer;
On 2017/03/16 00:37:45, Siggi Cherem (dart-lang) wrote:
> If we can make analyzerNode final above, then this cast will no longer be
> necessary (the is-check will promote the field).
> 
> Meanwhile, a convention we follow in dart2js is to avoid the cast and instead
do
> an assignment to a temporary variable with the precise type:
> 
> if (analyzedNode is ir.Field) {
>   ir.Field field = analyzerdNode;
>   if (field.initializer == null) ...
> 
> This makes it a cast in checked-mode, but free in production mode. In the
future
> with strong-mode, there wont be that difference anymore though.

Hmm I made this change complete with the finals, and the analyze_unused_test was
still unhappy about it.  I went with the renaming/typing the variable instead.

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:157: if (targetElement is
FunctionElement &&
On 2017/03/16 00:37:45, Siggi Cherem (dart-lang) wrote:
> I think you can drop this check too (since ConstructorBodyElement is also a
> FunctionElement)

ah, touche!

[Emily Fortuna, 2017-03-17 01:03:51]

Description was changed from

==========
Pulling the element model out of global type inference.

BUG=
==========

to

==========
Pulling the element model out of global type inference.

BUG=
R=johnniwinther@google.com, sigmund@google.com

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

[Emily Fortuna, 2017-03-17 01:03:52]

Committed patchset #2 (id:80001) manually as
0fa285f88064a30a36dc004df79a58853cca26f9 (presubmit successful).

[Siggi Cherem (dart-lang), 2017-03-17 21:10:17]

lgtm, thanks!

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
File pkg/compiler/lib/src/inferrer/builder_kernel.dart (right):

https://codereview.chromium.org/2746293006/diff/60001/pkg/compiler/lib/src/in...
pkg/compiler/lib/src/inferrer/builder_kernel.dart:83: initializer =
(analyzedNode as ir.Field).initializer;
On 2017/03/17 01:03:02, Emily Fortuna wrote:
> On 2017/03/16 00:37:45, Siggi Cherem (dart-lang) wrote:
> > If we can make analyzerNode final above, then this cast will no longer be
> > necessary (the is-check will promote the field).
> > 
> > Meanwhile, a convention we follow in dart2js is to avoid the cast and
instead
> do
> > an assignment to a temporary variable with the precise type:
> > 
> > if (analyzedNode is ir.Field) {
> >   ir.Field field = analyzerdNode;
> >   if (field.initializer == null) ...
> > 
> > This makes it a cast in checked-mode, but free in production mode. In the
> future
> > with strong-mode, there wont be that difference anymore though.
> 
> Hmm I made this change complete with the finals, and the analyze_unused_test
was
> still unhappy about it.  I went with the renaming/typing the variable instead.

interesting - I think I understand why now - this class could have a subclass
that overrides the field with a getter/setter pair and makes it non-final.... :(