Infer also field types in classes and top-level

Infer also field types in classes and top-level


R=brianwilkerson@google.com,pquitslund@google.com
BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=8206

[Brian Wilkerson, 2012-06-01 14:20:25]

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
File
compiler/java/com/google/dart/compiler/resolver/VariableElementImplementation.java
(right):

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/resolver/VariableElementImplementation.java:1:
// Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
nit: copyright year

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:479: if
(isInferredType(variableType) && !types.isAssignable(variableType, rhs)) {
It might be too late to change it now, but I really don't like where this is
going. I strongly believe that the code would be much cleaner if we separated
the type inferrencing from resolution. Resolution should deal with declared
types and not need to worry about inferred types. Mixing the two is error prone
and will likely lead to subtle and hard to fix bugs.

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:502: private
static boolean isInferredType(Type type) {
Why not define a method like Type.isInferred()? And similarly above
Element.hasInferredType()? IMO utility methods are fine when you don't own the
class or when the functionality is unlikely to be useful elsewhere, but this
seems like very general behavior.

[scheglov, 2012-06-01 15:11:34]

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
File
compiler/java/com/google/dart/compiler/resolver/VariableElementImplementation.java
(right):

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/resolver/VariableElementImplementation.java:1:
// Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
On 2012/06/01 14:20:25, Brian Wilkerson wrote:
> nit: copyright year

Done.

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:479: if
(isInferredType(variableType) && !types.isAssignable(variableType, rhs)) {
On 2012/06/01 14:20:25, Brian Wilkerson wrote:
> It might be too late to change it now, but I really don't like where this is
> going. I strongly believe that the code would be much cleaner if we separated
> the type inferrencing from resolution. Resolution should deal with declared
> types and not need to worry about inferred types. Mixing the two is error
prone
> and will likely lead to subtle and hard to fix bugs.

For practical usefulness we need to resolve against inferred types.
Think about following scenario:
1. User used inferred type for code completion and called some method.
2. Now he wants to rename this method using just completed invocation. Without
resolving against inferred types we could not get MethodElement and activate
rename.
3. Profit?

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:502: private
static boolean isInferredType(Type type) {
On 2012/06/01 14:20:25, Brian Wilkerson wrote:
> Why not define a method like Type.isInferred()? And similarly above
> Element.hasInferredType()? IMO utility methods are fine when you don't own the
> class or when the functionality is unlikely to be useful elsewhere, but this
> seems like very general behavior.

We have single Type instance for "int" for example.
We can not mark this instance as inferred.
We need separate instance of Type, which has same characteristics as source
Type, but also "inferred" flag on.
Solutions?
1. Declare isInferred() method in Type, add implementations that return "false"
into each Type implementation, and wrap using Proxy to return "true".
2. Just mix in InferredType marker interface into Proxy.

Well, actually default method implementation should be added only to
AbstractType, so not so bad.
Done.

[Brian Wilkerson, 2012-06-01 16:15:36]

LGTM

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/10459070/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:479: if
(isInferredType(variableType) && !types.isAssignable(variableType, rhs)) {
> For practical usefulness we need to resolve against inferred types.

Agreed. But for error checking we need to use the declared types. And as type
inference is improved over time we'll end up with more and more of this kind of
code where we're checking to see whether the type information is inferred. I
firmly believe that we need to keep these two kinds of resolution separate or
we'll introduce a lot of errors.

The most serious problem I foresee is the loss of information. It might be at
the moment that we're only associating an inferred type in cases where there was
no type information (explicitly declaring "Dynamic" is not the absence of type
information), but eventually we will want to infer more specific type
information even where type information was originally provided. We *cannot*
loose the information about what type information was explicitly provided, and
therefore we must eventually keep these two levels of information separate.

That said, this CL doesn't introduce the problem, it just makes it worse, so
I'll let it go for now. (Not to mention the fact that I wouldn't want this CL to
get larger by including a fix for this issue.)

https://chromiumcodereview.appspot.com/10459070/diff/1011/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/type/AbstractType.java (right):

https://chromiumcodereview.appspot.com/10459070/diff/1011/compiler/java/com/g...
compiler/java/com/google/dart/compiler/type/AbstractType.java:1: // Copyright
(c) 2011, the Dart project authors.  Please see the AUTHORS file
nit: copyright year

[pquitslund, 2012-06-01 17:31:38]

LGTM.

Thanks for pushing on this.  It will make our semantic highlighting of refinable
types much better!

[scheglov, 2012-06-01 18:26:33]

> Agreed. But for error checking we need to use the declared types. And as type
> inference is improved over time we'll end up with more and more of this kind
of
> code where we're checking to see whether the type information is inferred. I
> firmly believe that we need to keep these two kinds of resolution separate or
> we'll introduce a lot of errors.

  Well, will see.
  Single type works for now, while we infer type only instead of Dynamic, where
there are no errors for calling anything.
  This is not so big investment, so I don't worry even if we will need to make
changes.


> The most serious problem I foresee is the loss of information. It might be at
> the moment that we're only associating an inferred type in cases where there
was
> no type information (explicitly declaring "Dynamic" is not the absence of type

  "11.3 Variable Declaration"
"A variable declaration
statement var id; or var id = e; introduces a new variable named id with static
type Dynamic into the innermost enclosing scope."

  So, "var" and "Dynamic" are the same.


> information), but eventually we will want to infer more specific type
> information even where type information was originally provided. We *cannot*
> loose the information about what type information was explicitly provided, and
> therefore we must eventually keep these two levels of information separate.

  Well, we always can have back reference on "declaration" type.
  Just have not just isInferred() but also getDeclarationType() in Type.

  And checks for type member presence in "declaration" type.
  AFAIK this is the only place where we could violate spec.
  But this would be really strange for Dart as it is positioned as dynamic and
IDE friendly.
  AFAIK by spirit of language this should NOT produce any warning.
Object v;
if (o is String) {
  return v.length;
}

  OTOH this code does not cause warning by spec, because (yes, it sounds
strange, because in Java we expect only one directly in hierarchy) String is
Object.
foo(String s) {}
Object v;
foo(v);
  Basically check is:
  public boolean isAssignable(Type t, Type s) {
    t.getClass(); // Quick null check.
    s.getClass(); // Quick null check.
    return isSubtype(t, s) || isSubtype(s, t);
  }

[Brian Wilkerson, 2012-06-01 18:43:49]

>   So, "var" and "Dynamic" are the same.

Yes, they are semantically the same. Yes, dartc has one representation that it
uses in both cases.

But I wonder whether they are the same in the mind of our users. Personally I
think it might not be. I can easily imagine using "var" when I want to (or am
willing to) get inferred type suggestions and using "Dynamic" when I don't. I
don't know how our users feel about this though.

>   Well, we always can have back reference on "declaration" type.
>   Just have not just isInferred() but also getDeclarationType() in Type.

Interesting suggestion. I think I like it, but would like to think through the
implications a bit.