Fix for issue 3401: Implement casts in Analyzer

Fix for issue 3401: Implement casts in Analyzer
Committed: https://code.google.com/p/dart/source/detail?r=8397

[zundel, 2012-06-06 17:21:10]

On 2012/06/06 16:57:55, Brian Wilkerson wrote:

Did you run 

./tools/build.py ; ./tools/test.py -c dartc -r none 

after compiling this patch?  If there are some shared language tests, they
should now pass, which means the language.status file probably needs to be
updated.  If there are no language tests (under ./tests/language), we need to
add some.

[zundel, 2012-06-06 17:25:34]

http://codereview.chromium.org/10536031/diff/1/compiler/javatests/com/google/...
File compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java (right):

http://codereview.chromium.org/10536031/diff/1/compiler/javatests/com/google/...
compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java:193:
assertTrue(cast.getArg2() instanceof DartTypeExpression);
Shouldn't this affect type inference as well?  I'm not exactly sure where to
test this, but the type of 'x' should be inferred as 'int' and not the default
of Dynamic.

[Brian Wilkerson, 2012-06-06 17:25:59]

> Did you run ... after compiling this patch?

Yes.

> If there are some shared language tests...

There aren't. The spec was just changed this morning, so nobody has had a chance
to add any.

> If there are no language tests (under ./tests/language), we need to add some.

If that's true, who needs to approve the addition?

[scheglov, 2012-06-06 17:45:50]

http://codereview.chromium.org/10536031/diff/1/compiler/java/com/google/dart/...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

http://codereview.chromium.org/10536031/diff/1/compiler/java/com/google/dart/...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:696:
variableRestorer.setType(variableElement, varType);
Note, that inferVariableTypesFromIsNotConditions() is used for the case
"if (condition) return;" It looks for me that "v as T" expression does not say
us anything here. If it is successful, then "v" is "T" on "return". In not - we
have exception and leave method.

http://codereview.chromium.org/10536031/diff/1/compiler/javatests/com/google/...
File compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java (right):

http://codereview.chromium.org/10536031/diff/1/compiler/javatests/com/google/...
compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java:193:
assertTrue(cast.getArg2() instanceof DartTypeExpression);
On 2012/06/06 17:25:34, zundel wrote:
> Shouldn't this affect type inference as well?  I'm not exactly sure where to
> test this, but the type of 'x' should be inferred as 'int' and not the default
> of Dynamic.

Yes, we need to add new type inference tests somewhere close to
com.google.dart.compiler.type.TypeAnalyzerCompilerTest.test_getType_binaryExpression().

[messick, 2012-06-06 17:45:54]

lgtm

[zundel, 2012-06-06 17:58:06]

On 2012/06/06 17:25:59, Brian Wilkerson wrote:
> > Did you run ... after compiling this patch?
> 
> Yes.
> 
> > If there are some shared language tests...
> 
> There aren't. The spec was just changed this morning, so nobody has had a
chance
> to add any.
> 
> > If there are no language tests (under ./tests/language), we need to add
some.
> 
> If that's true, who needs to approve the addition?

You can add the test yourself and then ask Gilad to approve the test.

[Brian Wilkerson, 2012-06-06 21:40:28]

I have written the language tests, but have two questions. First, should I
include them in this CL or in a separate CL? Second, how do I mark them as being
expected to fail for other platforms? (Do I need to add them to the status file
for every other platform or is there some more general way to disable them?)

http://codereview.chromium.org/10536031/diff/1/compiler/java/com/google/dart/...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

http://codereview.chromium.org/10536031/diff/1/compiler/java/com/google/dart/...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:696:
variableRestorer.setType(variableElement, varType);
OK. I removed this code.

http://codereview.chromium.org/10536031/diff/1/compiler/javatests/com/google/...
File compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java (right):

http://codereview.chromium.org/10536031/diff/1/compiler/javatests/com/google/...
compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java:193:
assertTrue(cast.getArg2() instanceof DartTypeExpression);
> > Shouldn't this affect type inference as well?

Yes.

> Yes, we need to add new type inference tests somewhere close to
>
com.google.dart.compiler.type.TypeAnalyzerCompilerTest.test_getType_binaryExpression().

Done

[zundel, 2012-06-06 22:13:27]

On 2012/06/06 21:40:28, Brian Wilkerson wrote:
> I have written the language tests, but have two questions. First, should I
> include them in this CL or in a separate CL? Second, how do I mark them as
being
> expected to fail for other platforms? (Do I need to add them to the status
file
> for every other platform or is there some more general way to disable them?)

Yes, you just mark the test as failed for the other platforms in
tests/language/language.status and in language_dart2js.status 

> 
>
http://codereview.chromium.org/10536031/diff/1/compiler/java/com/google/dart/...
> File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):
> 
>
http://codereview.chromium.org/10536031/diff/1/compiler/java/com/google/dart/...
> compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:696:
> variableRestorer.setType(variableElement, varType);
> OK. I removed this code.
> 
>
http://codereview.chromium.org/10536031/diff/1/compiler/javatests/com/google/...
> File compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java
(right):
> 
>
http://codereview.chromium.org/10536031/diff/1/compiler/javatests/com/google/...
> compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java:193:
> assertTrue(cast.getArg2() instanceof DartTypeExpression);
> > > Shouldn't this affect type inference as well?
> 
> Yes.
> 
> > Yes, we need to add new type inference tests somewhere close to
> >
>
com.google.dart.compiler.type.TypeAnalyzerCompilerTest.test_getType_binaryExpression().
> 
> Done

[scheglov, 2012-06-06 23:53:42]

lgtm

[Brian Wilkerson, 2012-06-07 15:28:46]

So while I was trying to figure out how to mark the language tests that wouldn't
pass on other platforms the dart2js team added some
(http://codereview.chromium.org/10540048/) do I still need to try to add
language tests (dartc passes the tests they added, by the way).

[zundel, 2012-06-07 16:34:38]

LGTM, those tests look sufficient.