Implement parsing and resolving of type-variables.

Implement parsing and resolving of type-variables.

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

[ahe, 2012-03-15 11:17:33]

Initial comments.

https://chromiumcodereview.appspot.com/9616058/diff/9001/frog/leg/elements/el...
File frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9616058/diff/9001/frog/leg/elements/el...
frog/leg/elements/elements.dart:687: Element lookupTypeParameter(SourceString
parameterName) {
As far as I can tell, this creates a separate scope for type variables.

https://chromiumcodereview.appspot.com/9616058/diff/9001/frog/leg/elements/el...
frog/leg/elements/elements.dart:898: class TypeVariableElement extends Element {
Please override toString to include the name of the class (enclosingElement, I
presume).

https://chromiumcodereview.appspot.com/9616058/diff/9001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9616058/diff/9001/frog/leg/resolver.da...
frog/leg/resolver.dart:1694: if (!inStaticContext && name == element.name) {
I don't believe this is correct. The scope should not change based on whether or
not something is static.

For example, this should be a compile-time error because the type variable hides
the top-level field T:

var T;

class Foo<T> {
  static method() => T;
}

Also, why do you need a TypeVariableScope? Are you trying to reject this:

class Foo<T extends List<S>, S> {
}

I don't think this is supposed to be rejected.

[ngeoffray, 2012-03-15 11:47:40]

http://codereview.chromium.org/9616058/diff/9001/frog/leg/elements/elements.dart
File frog/leg/elements/elements.dart (right):

http://codereview.chromium.org/9616058/diff/9001/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:902: TypeVariableElement(name, Element
enclosing, this.node, this.type,
I prefer having enclosing at the end.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:903: [this.bound])
should that default to Object to avoid null checks?

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:413: LabelElement lookup(String label, [bool
inStaticContext]);
Why would labels care about a static context?

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:1276: if (interfaceType !== null &&
interfaceType.element.impliesType()) {
IMO interfaceType.element.impliesType should always return true. Which situation
is that?

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:1292: Type visitTypeVariable(TypeVariable node) {
Is that really necessary? And it's not obvious that it should return the bound
of the type variable. Why is it not the type of the variable?

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:1317: // TODO(ngeoffray): Use type variables.
I believe you can remove this TODO now.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/scanner/listener.dart
File frog/leg/scanner/listener.dart (right):

http://codereview.chromium.org/9616058/diff/9001/frog/leg/scanner/listener.da...
frog/leg/scanner/listener.dart:897: pushNode(new ClassNode(name, typeParameters,
null, supertypes, defaultClause,
line too long

http://codereview.chromium.org/9616058/diff/9001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9616058/diff/9001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:2504:
compiler.unimplemented('SsaBuilder.visitTypeVariable', node: node);
Please use internalError or unreachable instead.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/ssa/codegen.dart#n...
frog/leg/ssa/codegen.dart:973: ClassElement element = node.typeExpression;
This is not right, because it could be a typedef.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/typechecker.dart
File frog/leg/typechecker.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/typechecker.dart#n...
frog/leg/typechecker.dart:35: TypeVariableType(this.name, [this.element]);
How about moving these classes in another place? The typechecker is something
optional, but not these classes.

[ahe, 2012-03-16 13:37:11]

Everything but the resolver changes, LGTM!

Please address Nicolas comments before submitting, otherwise, the checked build
may break.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:963: if (resolved !== null && resolved.kind ===
ElementKind.TYPE_VARIABLE) {
I think I'd rather do this in ConstructorResolver.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:1213: classElement = context.lookup(node.name.source);
This seems brittle to me. I would rather pass in the class element to the
constructor.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/scanner/listener.dart
File frog/leg/scanner/listener.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/scanner/listener.d...
frog/leg/scanner/listener.dart:897: pushNode(new ClassNode(name, typeParameters,
null, supertypes, defaultClause,
Long line.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/scanner/listener.d...
frog/leg/scanner/listener.dart:1363: NodeList typeParameters = popNode();
TODO: don't throw away.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/typechecker.dart
File frog/leg/typechecker.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/typechecker.dart#n...
frog/leg/typechecker.dart:32: class TypeVariableType {
implements Type

http://codereview.chromium.org/9616058/diff/10017/tests/co19/co19-leg.status
File tests/co19/co19-leg.status (right):

http://codereview.chromium.org/9616058/diff/10017/tests/co19/co19-leg.status#...
tests/co19/co19-leg.status:37: Language/10_Expressions/01_Constants_A17_t03:
Skip # Cycle reference. Times out.
Please add two newlines. Are you planning on fixing this?

http://codereview.chromium.org/9616058/diff/10017/tests/co19/co19-leg.status#...
tests/co19/co19-leg.status:243: Language/13_Types/4_Interface_Types_A10_t04:
Fail # visitIdentifier for typedefs not implemented
This should be grouped with ones above.

http://codereview.chromium.org/9616058/diff/10017/tests/co19/co19-leg.status#...
tests/co19/co19-leg.status:244: Language/09_Generics/09_Generics_A01_t10: Fail
Add '# TODO(ahe): Please categorize' and two newlines.

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

http://codereview.chromium.org/9616058/diff/10017/tests/language/language-leg...
tests/language/language-leg.status:77:
TypeVariableIdentifierExpressionNegativeTest: Fail
Why? Or add 'TODO(ahe): Please categorize'.

[karlklose, 2012-03-16 14:58:47]

Thanks for the comments, PTAL.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/elements/elements.dart
File frog/leg/elements/elements.dart (right):

http://codereview.chromium.org/9616058/diff/9001/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:687: Element lookupTypeParameter(SourceString
parameterName) {
Yes, but the resolver will try to look names up using this method, too.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:898: class TypeVariableElement extends Element {
On 2012/03/15 11:17:33, ahe wrote:
> Please override toString to include the name of the class (enclosingElement, I
> presume).

Done.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/elements/elements.d...
frog/leg/elements/elements.dart:903: [this.bound])
The bound is set when resolving the class containing this element. After
resolver, having null is an error.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:413: LabelElement lookup(String label, [bool
inStaticContext]);
Done, removed all the static context booleans.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:1276: if (interfaceType !== null &&
interfaceType.element.impliesType()) {
It could be a type variable, which is illegal.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:1292: Type visitTypeVariable(TypeVariable node) {
Done, removed.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:1317: // TODO(ngeoffray): Use type variables.
On 2012/03/15 11:47:40, ngeoffray wrote:
> I believe you can remove this TODO now.

Done.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:1694: if (!inStaticContext && name == element.name) {
Done, removed.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/scanner/listener.dart
File frog/leg/scanner/listener.dart (right):

http://codereview.chromium.org/9616058/diff/9001/frog/leg/scanner/listener.da...
frog/leg/scanner/listener.dart:897: pushNode(new ClassNode(name, typeParameters,
null, supertypes, defaultClause,
On 2012/03/15 11:47:40, ngeoffray wrote:
> line too long

Done.

http://codereview.chromium.org/9616058/diff/9001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9616058/diff/9001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:2504:
compiler.unimplemented('SsaBuilder.visitTypeVariable', node: node);
On 2012/03/15 11:47:40, ngeoffray wrote:
> Please use internalError or unreachable instead.

Done.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:963: if (resolved !== null && resolved.kind ===
ElementKind.TYPE_VARIABLE) {
I am not sure, why should the cosntructor resolver care about type-variables?

http://codereview.chromium.org/9616058/diff/10017/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:1213: classElement = context.lookup(node.name.source);
On 2012/03/16 13:37:11, ahe wrote:
> This seems brittle to me. I would rather pass in the class element to the
> constructor.

Done.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/scanner/listener.dart
File frog/leg/scanner/listener.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/scanner/listener.d...
frog/leg/scanner/listener.dart:897: pushNode(new ClassNode(name, typeParameters,
null, supertypes, defaultClause,
On 2012/03/16 13:37:11, ahe wrote:
> Long line.

Done.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/scanner/listener.d...
frog/leg/scanner/listener.dart:1363: NodeList typeParameters = popNode();
On 2012/03/16 13:37:11, ahe wrote:
> TODO: don't throw away.

Done.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/ssa/codegen.dart#n...
frog/leg/ssa/codegen.dart:973: ClassElement element = node.typeExpression;
On 2012/03/15 11:47:40, ngeoffray wrote:
> This is not right, because it could be a typedef.

Done.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/typechecker.dart
File frog/leg/typechecker.dart (right):

http://codereview.chromium.org/9616058/diff/10017/frog/leg/typechecker.dart#n...
frog/leg/typechecker.dart:32: class TypeVariableType {
Done.

http://codereview.chromium.org/9616058/diff/10017/frog/leg/typechecker.dart#n...
frog/leg/typechecker.dart:35: TypeVariableType(this.name, [this.element]);
I'll do that in another CL.

http://codereview.chromium.org/9616058/diff/10017/tests/co19/co19-leg.status
File tests/co19/co19-leg.status (right):

http://codereview.chromium.org/9616058/diff/10017/tests/co19/co19-leg.status#...
tests/co19/co19-leg.status:37: Language/10_Expressions/01_Constants_A17_t03:
Skip # Cycle reference. Times out.
Not now.

http://codereview.chromium.org/9616058/diff/10017/tests/co19/co19-leg.status#...
tests/co19/co19-leg.status:243: Language/13_Types/4_Interface_Types_A10_t04:
Fail # visitIdentifier for typedefs not implemented
On 2012/03/16 13:37:11, ahe wrote:
> This should be grouped with ones above.

Done.

http://codereview.chromium.org/9616058/diff/10017/tests/co19/co19-leg.status#...
tests/co19/co19-leg.status:244: Language/09_Generics/09_Generics_A01_t10: Fail
On 2012/03/16 13:37:11, ahe wrote:
> Add '# TODO(ahe): Please categorize' and two newlines.

Done.

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

http://codereview.chromium.org/9616058/diff/10017/tests/language/language-leg...
tests/language/language-leg.status:77:
TypeVariableIdentifierExpressionNegativeTest: Fail
That was a mistake.

[ahe, 2012-03-16 15:29:21]

LGTM!

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/elements/e...
File frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/elements/e...
frog/leg/elements/elements.dart:907: toString() =>
"${enclosingElement.toString()}.${name.slowToString()}";
$kind(...)

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.d...
frog/leg/resolver.dart:1231: context = new TypeVariablesScope(context,
classElement);
Move out of loop.

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.d...
frog/leg/resolver.dart:1267: classElement.supertype = new
SimpleType(Types.OBJECT, objectElement);
I would like if this was objectElement.type.

[ngeoffray, 2012-03-18 13:40:56]

LGTM!

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/elements/e...
File frog/leg/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/elements/e...
frog/leg/elements/elements.dart:689: return result;
No need for a temporary variable.

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.d...
frog/leg/resolver.dart:461: return labels.lookup(label);
Why this change?

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.d...
frog/leg/resolver.dart:1219: // Create types and elements for type variable.
Please explain why you are doing two loops instead of one.

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.d...
frog/leg/resolver.dart:1276: if (interfaceType !== null &&
interfaceType.element.impliesType()) {
A type parameter should imply a type. I think Peter fixed this bug in another
CL, so you should be able to remove the call to impliesType.

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.d...
frog/leg/resolver.dart:1681: class TypeVariablesScope extends Scope {
I'm not sure why you need to create a special scope for type variables. In which
context do you require one? Please add a comment.

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.d...
frog/leg/resolver.dart:1682: TypeVariablesScope(parent, ClassElement element) :
super(parent, element);
Scope parent

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/resolver.d...
frog/leg/resolver.dart:1742: return library.find(name);
Why this change?

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/ssa/builde...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2504:
compiler.internalError('SsaBuilder.visitTypeVariable');
For something like if (a is T) { } the builder would see a type variable right?
If yes, then scratch my first comment on this CL, and you can change this to
unimplemented.

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/ssa/codege...
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9616058/diff/11034/frog/leg/ssa/codege...
frog/leg/ssa/codegen.dart:975: compiler.unimplemented("visitIs for type
variables");
Do we actually end up here if the builder says unimplemented? Maybe add a
comment.