Issue 13807. Support for type promotion rules.

Issue 13807. Support for type promotion rules.
https://code.google.com/p/dart/issues/detail?id=13807

Basic version.
No union types (and hopefully no need for them).
So, language tests should be triaged.
No support for ?: yet.

R=brianwilkerson@google.com, jwren@google.com
BUG=

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

[jwren, 2013-10-23 07:37:12]

LGTM.  I saw two nits.  I'd let Brian look through it as he has read this part
of the spec closer than me recently.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java:199:
private TypePromotionManager promoteManager = new TypePromotionManager();
Why create a manager when you are about to reference one from the
ResolverVisitor in the constructor.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/TypePromotionManager.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/TypePromotionManager.java:24:
* Instances of the class {@code TypeOverrideManager} manage the ability to
promote types of local
nit:  TypeOverrideManager -> TypePromotionManager

[Brian Wilkerson, 2013-10-23 15:23:26]

LGTM! Thanks for doing this!

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LocalVariableElementImpl.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LocalVariableElementImpl.java:29:
* Is {@code true} is this variables is potentially mutated somewhere in its
scope.
Second "is" --> "if"?
"variables" --> "variable"

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LocalVariableElementImpl.java:82:
* Specifies that this variables is potentially mutated somewhere in its scope.
"variables" --> "variable"

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ParameterElementImpl.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ParameterElementImpl.java:30:
* Is {@code true} is this variables is potentially mutated somewhere in its
scope.
Second "is" --> "if"?
"variables" --> "variable"

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ParameterElementImpl.java:139:
* Specifies that this variables is potentially mutated somewhere in its scope.
"variables" --> "variable"

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/VariableElementImpl.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/VariableElementImpl.java:97:
* @return {@code true} if this variable is potentially mutated somewhere in its
scope
Suggest you add "Return {@code true} if this variable is potentially mutated
somewhere in its scope. This information is only available for local variables
(including parameters).".

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:826:
* Return the static element associated with the given expression whose type can
be propagated, or
"propagated" --> "promoted"

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:827:
* {@code null} if there is no element whose type can be overridden.
"overridden" --> "promoted"

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:1041:
* TODO(scheglov)
Missing lots of comments.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:1067:
private void clearWrongTypePromotions(ASTNode target) {
I think this method is only used for if statements, so perhaps a name like
"clearWrongTypePromotionsInIf" would make it more clear as to what's going on
here.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:1236:
this.inClosure = true;
nit: You don't need the "this.".

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/TypePromotionManager.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/TypePromotionManager.java:141:
* Set the overridden type of the given element to the given type
nit: missing period

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java:2992:
"bool tt() => true;",
I don't think we need this first line.

[scheglov, 2013-10-23 16:08:28]

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LocalVariableElementImpl.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LocalVariableElementImpl.java:29:
* Is {@code true} is this variables is potentially mutated somewhere in its
scope.
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> Second "is" --> "if"?
> "variables" --> "variable"

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LocalVariableElementImpl.java:82:
* Specifies that this variables is potentially mutated somewhere in its scope.
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> "variables" --> "variable"

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ParameterElementImpl.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ParameterElementImpl.java:30:
* Is {@code true} is this variables is potentially mutated somewhere in its
scope.
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> Second "is" --> "if"?
> "variables" --> "variable"

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ParameterElementImpl.java:139:
* Specifies that this variables is potentially mutated somewhere in its scope.
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> "variables" --> "variable"

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/VariableElementImpl.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/VariableElementImpl.java:97:
* @return {@code true} if this variable is potentially mutated somewhere in its
scope
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> Suggest you add "Return {@code true} if this variable is potentially mutated
> somewhere in its scope. This information is only available for local variables
> (including parameters).".

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:826:
* Return the static element associated with the given expression whose type can
be propagated, or
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> "propagated" --> "promoted"

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:827:
* {@code null} if there is no element whose type can be overridden.
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> "overridden" --> "promoted"

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:1041:
* TODO(scheglov)
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> Missing lots of comments.

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:1067:
private void clearWrongTypePromotions(ASTNode target) {
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> I think this method is only used for if statements, so perhaps a name like
> "clearWrongTypePromotionsInIf" would make it more clear as to what's going on
> here.

Removed.
It can be replaced with other two methods (module double iteration).

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:1236:
this.inClosure = true;
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> nit: You don't need the "this.".

Yes, I need.
Local variable hides field "inClosure".

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/StaticTypeAnalyzer.java:199:
private TypePromotionManager promoteManager = new TypePromotionManager();
On 2013/10/23 07:37:12, jwren wrote:
> Why create a manager when you are about to reference one from the
> ResolverVisitor in the constructor.

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/TypePromotionManager.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/TypePromotionManager.java:24:
* Instances of the class {@code TypeOverrideManager} manage the ability to
promote types of local
On 2013/10/23 07:37:12, jwren wrote:
> nit:  TypeOverrideManager -> TypePromotionManager

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/TypePromotionManager.java:141:
* Set the overridden type of the given element to the given type
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> nit: missing period

Done.

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java
(right):

https://codereview.chromium.org/34523011/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java:2992:
"bool tt() => true;",
On 2013/10/23 15:23:26, Brian Wilkerson wrote:
> I don't think we need this first line.

Done.

[scheglov, 2013-10-23 16:09:39]

Committed patchset #2 manually as r29095 (presubmit successful).