Handle type-inference on fields, consts, and inferable overrides

Handle type-inference on fields, consts, and inferable overrides
deterministically in library cycles.

R=vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/92e1373a9625309a1116a34e7a6f521979ea6b3e

[Siggi Cherem (dart-lang), 2015-03-17 00:58:06]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-03-17 00:58:11]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-17 00:59:13]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-17 01:06:35]

sigmund@google.com changed reviewers:
+ vsm@google.com

[vsm, 2015-03-17 15:55:28]

nice!

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:116: /// overriden members in super classes.
nit: overriden -> overridden

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:127: if (resolvedLibraries.any((l) =>
l.librarySource.isInSystemLibrary)) {
Is resolvedLibraries the cycle here?  I.e., if one is a system lib, all should
be?

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:193: // inferece, so their types can be used to
initialize other members below.
nit: inferece -> inference

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:197: // Track types in this strongest connected
component, ensure we visit
strongly?

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:285: if (type == null || type.isDynamic) continue;
If the initializer type is Null or Bottom, we might want to skip.  Unless it's a
final or const ... then I guess it doesn't matter and/or we're better off with
the more precise type.  Thinking of this sort of case:

var x = null;
main() {
  x = 42;
}

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:287: var element = variable.element;
Shouldn't we only update the type if it was declared implicitly dynamic (i.e.,
just var)?

[Siggi Cherem (dart-lang), 2015-03-17 16:28:37]

Patchset #2 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-17 16:29:22]

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:116: /// overriden members in super classes.
On 2015/03/17 15:55:27, vsm wrote:
> nit: overriden -> overridden

Done.

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:127: if (resolvedLibraries.any((l) =>
l.librarySource.isInSystemLibrary)) {
On 2015/03/17 15:55:27, vsm wrote:
> Is resolvedLibraries the cycle here?  I.e., if one is a system lib, all should
> be?

Yeah, exactly, I added a comment to be clear.

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:193: // inferece, so their types can be used to
initialize other members below.
On 2015/03/17 15:55:27, vsm wrote:
> nit: inferece -> inference

Done.

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:197: // Track types in this strongest connected
component, ensure we visit
On 2015/03/17 15:55:27, vsm wrote:
> strongly?

Done.

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:285: if (type == null || type.isDynamic) continue;
On 2015/03/17 15:55:27, vsm wrote:
> If the initializer type is Null or Bottom, we might want to skip.  Unless it's
a
> final or const ... then I guess it doesn't matter and/or we're better off with
> the more precise type.  Thinking of this sort of case:
> 
> var x = null;
> main() {
>   x = 42;
> }

Ah, interesting. Good point. I added a check for it and a new test to cover
this.

I thought about the option of being more precise with final/const, but it
doesn't seem to be worth it.

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:287: var element = variable.element;
On 2015/03/17 15:55:27, vsm wrote:
> Shouldn't we only update the type if it was declared implicitly dynamic (i.e.,
> just var)?

Correct, the check in line 275 above:
  if (variableList.type != null) continue;

skips any variable that has a declared type (even dynamic), so we only get here
if the type was implicitly declared dynamic.

I went ahead and added a comment to clarify.

[vsm, 2015-03-17 17:18:48]

lgtm

How do want to handle the following:

class A {
  var x;
}

class B extends A {
  var x = "hello";
}

I think currently (including your CL), we'd infer B.x as String, but it then
becomes an invalid override on A.x.

Limiting to final or const sidesteps this.  We could also consider allowing
String x to override dynamic x (and ensure we generate the right runtime check).

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1011933002/diff/60001/lib/src/checker/resolve...
lib/src/checker/resolver.dart:287: var element = variable.element;
On 2015/03/17 16:29:22, Siggi Cherem (dart-lang) wrote:
> On 2015/03/17 15:55:27, vsm wrote:
> > Shouldn't we only update the type if it was declared implicitly dynamic
(i.e.,
> > just var)?
> 
> Correct, the check in line 275 above:
>   if (variableList.type != null) continue;
> 
> skips any variable that has a declared type (even dynamic), so we only get
here
> if the type was implicitly declared dynamic.
> 
> I went ahead and added a comment to clarify.

Ah, thanks - missed the check above.

[Siggi Cherem (dart-lang), 2015-03-17 19:32:29]

Patchset #3 (id:120001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-17 19:37:41]

Patchset #3 (id:140001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-17 19:40:07]

> How do want to handle the following:
> 
> class A {
>   var x;
> }
> 
> class B extends A {
>   var x = "hello";
> }
> 
> I think currently (including your CL), we'd infer B.x as String, but it then
> becomes an invalid override on A.x.
> 
> Limiting to final or const sidesteps this.  We could also consider allowing
> String x to override dynamic x (and ensure we generate the right runtime
check).

Good catch! I went with a different approach, only infer from the RHS if there
are no overrides. So in the case above we'd treat B.x as dynamic.

In the process of fixing this I discovered a bug: multiple declarations together
would interfere with each other. For example:

class A {
  var x, y = 1;
}

class B {
  var x = "hi", y;
}

I made some small changes to infer each variable individually, not the whole
variableList.

PTAL

[vsm, 2015-03-17 19:51:04]

lgtm

https://codereview.chromium.org/1011933002/diff/160001/lib/src/checker/resolv...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1011933002/diff/160001/lib/src/checker/resolv...
lib/src/checker/resolver.dart:253: // Infer from the RHS only if there are no
overrides.
Perhaps we could allow if this is final regardless.  E.g., fine to infer B.x as
String below because it's covariant.  

class A {
  get x;
}
class B extends A {
  final x = "hello";
}

https://codereview.chromium.org/1011933002/diff/160001/lib/src/checker/resolv...
lib/src/checker/resolver.dart:281: // Only infer on variabless that don't have
any declared type.
nit: variabless to variables

[Siggi Cherem (dart-lang), 2015-03-17 20:11:10]

thanks!

https://codereview.chromium.org/1011933002/diff/160001/lib/src/checker/resolv...
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1011933002/diff/160001/lib/src/checker/resolv...
lib/src/checker/resolver.dart:253: // Infer from the RHS only if there are no
overrides.
On 2015/03/17 19:51:03, vsm wrote:
> Perhaps we could allow if this is final regardless.  E.g., fine to infer B.x
as
> String below because it's covariant.  
> 
> class A {
>   get x;
> }
> class B extends A {
>   final x = "hello";
> }

Good point. Done.

https://codereview.chromium.org/1011933002/diff/160001/lib/src/checker/resolv...
lib/src/checker/resolver.dart:281: // Only infer on variabless that don't have
any declared type.
On 2015/03/17 19:51:04, vsm wrote:
> nit: variabless to variables

Done.

[Siggi Cherem (dart-lang), 2015-03-17 20:12:55]

Committed patchset #4 (id:180001) manually as
92e1373a9625309a1116a34e7a6f521979ea6b3e (presubmit successful).