Fix crash in dartc when given cyclic type variable bounds.

Fix crash in dartc when given cyclic type variable bounds.

http://code.google.com/p/dart/issues/detail?id=211

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

[zundel, 2012-01-11 22:00:19]

@Gilad, could you look @ the file CyclicTypeVariableTest.dart?  My goal is to
make this as exhaustive as possible.

[gbracha, 2012-01-11 23:04:34]

See comments: much of the test is legal code.

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
File tests/language/src/CyclicTypeVariableTest.dart (right):

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:9: >(T arg);
We don't have generic functions (or even factories) so this example is
syntactically illegal.

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:19: extends List<U> /// 03:
static type error
This should be perfectly legal.

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:31: extends List<W> /// 05:
static type error
Again, this is perfectly valid.

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:38: extends funcType<X> /// 06:
static type error
Since functType isn't valid, the point is moot, but this would otherwise be
legal.

[zundel, 2012-01-12 12:53:54]

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
File tests/language/src/CyclicTypeVariableTest.dart (right):

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:19: extends List<U> /// 03:
static type error
On 2012/01/11 23:04:34, gbracha wrote:
> This should be perfectly legal.

How could you make a legal substitution into this? 

new B<int>  // doesn't extent List<int>
new B<List<int>>// doesn't extent List<List<int>>

[mmendez, 2012-01-12 15:29:50]

Thanks for digging into this one Eric.  Lets get the tests nailed down then
we'll dig into the checker changes.

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
File tests/language/src/CyclicTypeVariableTest.dart (right):

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:9: >(T arg);
Unless there is some other countering clause, I think that this is legal per the
grammar in section 13.1 of v0.06 of the spec.

functionTypeAlias:
typedef functionPrex typeParameters? formalParameterList ';'
;



On 2012/01/11 23:04:34, gbracha wrote:
> We don't have generic functions (or even factories) so this example is
> syntactically illegal.

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:12: interface A<S
This should be a warning per the spec.

[zundel, 2012-01-12 15:33:59]

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
File tests/language/src/CyclicTypeVariableTest.dart (right):

http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:12: interface A<S
On 2012/01/12 15:29:50, mmendez wrote:
> This should be a warning per the spec.

Yes.  the 'static type error'  is at the warning level and does not stop the
compile.

[gbracha, 2012-01-12 17:02:59]

On 2012/01/12 12:53:54, zundel wrote:
>
http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
> File tests/language/src/CyclicTypeVariableTest.dart (right):
> 
>
http://codereview.chromium.org/9186017/diff/4001/tests/language/src/CyclicTyp...
> tests/language/src/CyclicTypeVariableTest.dart:19: extends List<U> /// 03:
> static type error
> On 2012/01/11 23:04:34, gbracha wrote:
> > This should be perfectly legal.
> 
> How could you make a legal substitution into this? 


interface Ordered<T> {bool gt(T x);}

class Foo extends Ordered<Foo> {boo gt(Foo f) => ...;}

class Bar extends Ordered<Bar> {boo gt(Bar b) => ...;}

class Baz<S extends Ordered<S>> {
   compare(S s1, S s2)=> s1.gt(s2);
}
> 
> new B<int>  // doesn't extent List<int>
> new B<List<int>>// doesn't extent List<List<int>>

[zundel, 2012-01-13 17:25:22]

Please take another look at the test. I added a second test file because one of
my original tests was illegal for another reason and I wanted to make sure there
was a test for it.

http://codereview.chromium.org/9186017/diff/7001/tests/language/language-leg....
File tests/language/language-leg.status (right):

http://codereview.chromium.org/9186017/diff/7001/tests/language/language-leg....
tests/language/language-leg.status:293: PseudoKWIllegalTest/01: Fail
(this is from a sync up to tip of trunk, not my change)

[gbracha, 2012-01-13 22:53:47]

See remaining comment: function type aliases cannot be generic.

http://codereview.chromium.org/9186017/diff/7001/tests/language/src/CyclicTyp...
File tests/language/src/CyclicTypeVariableTest.dart (right):

http://codereview.chromium.org/9186017/diff/7001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:12: extends T /// 01: static type
error
Again, this is syntactically invalid.

[zundel, 2012-01-17 13:17:03]

http://codereview.chromium.org/9186017/diff/7001/tests/language/src/CyclicTyp...
File tests/language/src/CyclicTypeVariableTest.dart (right):

http://codereview.chromium.org/9186017/diff/7001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:12: extends T /// 01: static type
error
On 2012/01/13 22:53:47, gbracha wrote:
> Again, this is syntactically invalid.


Not a syntax error acc'd to the spec from what we can tell, did you see Miguel's
previous comment?

"Unless there is some other countering clause, I think that this is legal per
the
grammar in section 13.3.1 of v0.06 of the spec.

functionTypeAlias:
typedef functionPrefix typeParameters? formalParameterList ';'
;
"

Did it once exist and now has been removed?  There would be lots of

[zundel, 2012-01-17 13:19:17]

http://codereview.chromium.org/9186017/diff/7001/tests/language/src/CyclicTyp...
File tests/language/src/CyclicTypeVariableTest.dart (right):

http://codereview.chromium.org/9186017/diff/7001/tests/language/src/CyclicTyp...
tests/language/src/CyclicTypeVariableTest.dart:12: extends T /// 01: static type
error
(I can easily change the parser, but I think there would be a good deal of dead
code remove if this is no longer a feature.)

[zundel, 2012-01-18 20:31:11]

Please take another look.

In addition to get ridding of the function type alias example in the test, I've
updated dartc's JUnit framework to more precisely track errors in
TypeAnalyzerTest.

[zundel, 2012-01-23 18:46:55]

This review is currently blocked waiting for word on whether or not function
type aliases can include type parameters.

[gbracha, 2012-01-24 18:13:06]

On 2012/01/23 18:46:55, zundel wrote:
> This review is currently blocked waiting for word on whether or not function
> type aliases can include type parameters.

They are allowed. 0.07 spec was released Friday night

[zundel, 2012-01-24 19:51:16]

I backed down 2 patch set 2 and merged up to tip of trunk.  Ready for review.

[gbracha, 2012-01-24 19:58:33]

On 2012/01/24 19:51:16, zundel wrote:
> I backed down 2 patch set 2 and merged up to tip of trunk.  Ready for review.

The test itself lgtm. No comment on the rest

[zundel, 2012-01-31 15:25:05]

@mmendez: ping 

Feel free to re-assign the review to someone else.

[mmendez, 2012-01-31 19:30:26]

On 2012/01/31 15:25:05, zundel wrote:
> @mmendez: ping 
> 
> Feel free to re-assign the review to someone else.

lgtm with nits.  Please review and decide if you want a second pass.

[mmendez, 2012-01-31 19:30:51]

lgtm

http://codereview.chromium.org/9186017/diff/25001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/type/Types.java (right):

http://codereview.chromium.org/9186017/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/type/Types.java:307: return
typeProvider.getObjectType();
Nit: should this be the bound of the type variable?  Or are you trying to enable
code generation in the presence of a type warning?

Either way, it would be good to add a comment here and explain what is expected.