Type inference in if, while, for and for-in statements

Type inference in if, while, for and for-in statements


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

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

[Brian Wilkerson, 2012-05-23 14:11:16]

LGTM

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

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:525:
Elements.setType(element, inferredType);
I don't know whether this is possible using the dartc types, but we want to set
the type to be the intersection of the inferred type and the previously known
type. If we see

  int value = 0;
  if (value is num) {

we don't want the type of "value" to be widened to "num" within the then block.

[messick, 2012-05-23 15:05:50]

LGTM

This is great stuff. I think it will make a lot of people happier with code
completion.

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/resolver/Elements.java (left):

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/resolver/Elements.java:200:
((VariableElementImplementation)element).setTypeInferred(true);
Shouldn't "true" be "inferred"?

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

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:491: public Void
visitBinaryExpression(DartBinaryExpression node) {
It seems like we ought to be able to allow any number of "&&" expressions with
the "is" expression.

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:498:
variableRestorer.setType(variableElement, arg2.getType());
To address Brian's concern below, you could use something like
TypeRefiner.mostSpecificType() here. Sorry, I guess I forgot to include a test
for that case.

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:512: * Helper to
temporary set {@link Type} of {@link VariableElement} and restore original
later.
temporarily

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1128:
visitConditionalNode(condition, node.getBody());
Why is the order reversed?

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/DartCoreDebug.java
(left):

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/DartCoreDebug.java:38:
public static final boolean ENABLE_TYPE_REFINEMENT =
isOptionTrue("debug/RefineTypes");
Also remove from DartCore/.options

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/completion/CompletionEngineTest.java
(left):

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/completion/CompletionEngineTest.java:349:
public void testCommentSnippets054() throws Exception {
Probably should add a test to prevent type widening, as Brian observed.

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/completion/CompletionEngineTest.java
(right):

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/completion/CompletionEngineTest.java:328:
test(source, "1+length", "2+length");
Add "2-value" to expected results of all four of these tests.

[scheglov, 2012-05-23 16:32:30]

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/resolver/Elements.java (left):

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/resolver/Elements.java:200:
((VariableElementImplementation)element).setTypeInferred(true);
On 2012/05/23 15:05:50, messick wrote:
> Shouldn't "true" be "inferred"?

Done.

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

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:491: public Void
visitBinaryExpression(DartBinaryExpression node) {
On 2012/05/23 15:05:50, messick wrote:
> It seems like we ought to be able to allow any number of "&&" expressions with
> the "is" expression.

Yes, we visit all expressions, so allow any number "is" expressions.
There are possible problems however.
1. May be condition was negated. I've added support for this.
2. May be there is "||" expression,. I've added tweak to visit only "&&" binary
expressions.

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:498:
variableRestorer.setType(variableElement, arg2.getType());
On 2012/05/23 15:05:50, messick wrote:
> To address Brian's concern below, you could use something like
> TypeRefiner.mostSpecificType() here. Sorry, I guess I forgot to include a test
> for that case.

Now we will change type only if "is" type is subtype of already known variable
type.
In theory there is also case when intersection of types in empty, but I will
ignore this for now.

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:512: * Helper to
temporary set {@link Type} of {@link VariableElement} and restore original
later.
On 2012/05/23 15:05:50, messick wrote:
> temporarily

Done.

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:525:
Elements.setType(element, inferredType);
On 2012/05/23 14:11:16, Brian Wilkerson wrote:
> I don't know whether this is possible using the dartc types, but we want to
set
> the type to be the intersection of the inferred type and the previously known
> type. If we see
> 
>   int value = 0;
>   if (value is num) {
> 
> we don't want the type of "value" to be widened to "num" within the then
block.

Done.

https://chromiumcodereview.appspot.com/10417044/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1128:
visitConditionalNode(condition, node.getBody());
On 2012/05/23 15:05:50, messick wrote:
> Why is the order reversed?

AFAIK order of execution is: condition, body, increment.
Nothing important for this CL, just small clean up.

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/DartCoreDebug.java
(left):

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/DartCoreDebug.java:38:
public static final boolean ENABLE_TYPE_REFINEMENT =
isOptionTrue("debug/RefineTypes");
On 2012/05/23 15:05:50, messick wrote:
> Also remove from DartCore/.options

Done.

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/completion/CompletionEngineTest.java
(left):

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/completion/CompletionEngineTest.java:349:
public void testCommentSnippets054() throws Exception {
On 2012/05/23 15:05:50, messick wrote:
> Probably should add a test to prevent type widening, as Brian observed.

Done.

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/completion/CompletionEngineTest.java
(right):

https://chromiumcodereview.appspot.com/10417044/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/completion/CompletionEngineTest.java:328:
test(source, "1+length", "2+length");
On 2012/05/23 15:05:50, messick wrote:
> Add "2-value" to expected results of all four of these tests.

Done.

[messick, 2012-05-23 16:55:53]

https://chromiumcodereview.appspot.com/10417044/diff/6001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/10417044/diff/6001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:545: if
(inferredType != null && types.isSubtype(inferredType, currentType)) {
I'm not sure you can compute this incrementally, but perhaps I'm mis-reading the
code. If I try to do code completion on this example (at the !) I should get
proposals for Dynamic, not List:

var x;
if ((x is List) && (x is String)) {
  x.!
}

[scheglov, 2012-05-23 22:50:00]

https://chromiumcodereview.appspot.com/10417044/diff/6001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/10417044/diff/6001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:545: if
(inferredType != null && types.isSubtype(inferredType, currentType)) {
On 2012/05/23 16:55:53, messick wrote:
> I'm not sure you can compute this incrementally, but perhaps I'm mis-reading
the
> code. If I try to do code completion on this example (at the !) I should get
> proposals for Dynamic, not List:
> 
> var x;
> if ((x is List) && (x is String)) {
>   x.!
> }

Actually the best Type would be some artificial Type which is at same time List
and String.
This is possible because List and String are interfaces.
But implementation of this would be not so easy.

[messick, 2012-05-23 23:11:50]

On 2012/05/23 22:50:00, scheglov wrote:
>
https://chromiumcodereview.appspot.com/10417044/diff/6001/compiler/java/com/g...
> File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):
> 
>
https://chromiumcodereview.appspot.com/10417044/diff/6001/compiler/java/com/g...
> compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:545: if
> (inferredType != null && types.isSubtype(inferredType, currentType)) {
> On 2012/05/23 16:55:53, messick wrote:
> > I'm not sure you can compute this incrementally, but perhaps I'm mis-reading
> the
> > code. If I try to do code completion on this example (at the !) I should get
> > proposals for Dynamic, not List:
> > 
> > var x;
> > if ((x is List) && (x is String)) {
> >   x.!
> > }
> 
> Actually the best Type would be some artificial Type which is at same time
List
> and String.
> This is possible because List and String are interfaces.
> But implementation of this would be not so easy.

Right; we need a type system based on set theory. You, Brian, and I should talk
about that when you're in town next time. It's easier (for me) with a
whiteboard.