Remove some order dependency in instance member inference

Remove some order dependency in instance member inference

R=leafp@google.com, paulberry@google.com

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

[Brian Wilkerson, 2015-08-27 18:40:55]

brianwilkerson@google.com changed reviewers:
+ leafp@google.com, paulberry@google.com

[Brian Wilkerson, 2015-08-27 18:40:56]

It occurred to me that part, if not all, of the potential order dependency in
the way we're inferring instance members is due to the fact that we were
checking to see what the value of an instance member's type was before inferring
that type. I don't think that's necessary. The changes in this CL remove those
tests and instead use the presence or absence of a type annotation as the only
factor that determines whether inference will be done. In addition, we always
set the type, even if the inferred type is still 'dynamic', just to ensure that
the element model is in a well defined state after running this task.

[Leaf, 2015-08-27 22:20:15]

On 2015/08/27 18:40:56, Brian Wilkerson wrote:
> It occurred to me that part, if not all, of the potential order dependency in
> the way we're inferring instance members is due to the fact that we were
> checking to see what the value of an instance member's type was before
inferring
> that type. I don't think that's necessary. The changes in this CL remove those
> tests and instead use the presence or absence of a type annotation as the only
> factor that determines whether inference will be done. In addition, we always
> set the type, even if the inferred type is still 'dynamic', just to ensure
that
> the element model is in a well defined state after running this task.

This lgtm, but I'm not sure I really understand what the concrete reason for the
changes are.  Presumably this allows us to run this code before we've propagated
the types to the field/method variables, but we still need the types for the
fields and methods themselves, yes?  So I'm not sure I see what this buys us -
can you elaborate?

[Brian Wilkerson, 2015-08-27 23:46:03]

> This lgtm, but I'm not sure I really understand what the concrete reason for
the
> changes are.

I'm trying to address at least some of the concerns given in the comment in the
class InstanceMemberInferrer.

The basic concern, as I understand it, is illustrated by the following scenario.
We run this code over the classes in one compilation unit C1. Then some
incremental change is made to another compilation unit C2, and we have to run
this code over C1 again, but we do so without resetting the state of the AST.
The question is, will we get the same results we would have if the AST for C1
were recreated. Because we were only inferring members if they had not already
been inferred, there's a danger that we could fail to infer something that
needed to be re-inferred.

Removing the 'isDynamic' tests should negate at least that source of potential
problem.

[Leaf, 2015-08-28 00:38:19]

On 2015/08/27 23:46:03, Brian Wilkerson wrote:
> > This lgtm, but I'm not sure I really understand what the concrete reason for
> the
> > changes are.
> 
> I'm trying to address at least some of the concerns given in the comment in
the
> class InstanceMemberInferrer.
> 
> The basic concern, as I understand it, is illustrated by the following
scenario.
> We run this code over the classes in one compilation unit C1. Then some
> incremental change is made to another compilation unit C2, and we have to run
> this code over C1 again, but we do so without resetting the state of the AST.
> The question is, will we get the same results we would have if the AST for C1
> were recreated. Because we were only inferring members if they had not already
> been inferred, there's a danger that we could fail to infer something that
> needed to be re-inferred.
> 
> Removing the 'isDynamic' tests should negate at least that source of potential
> problem.

Ok, makes sense, thanks.  Still lgtm.

[Paul Berry, 2015-08-28 16:55:44]

lgtm, thanks!

[Brian Wilkerson, 2015-08-28 18:31:52]

Committed patchset #1 (id:1) manually as
68058da1a104c195df3b5c5232966b9fbf5e1b52 (presubmit successful).