Implement interface types.

Implement interface types.

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

[ngeoffray, 2012-02-24 13:36:15]

LGTM!

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

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:975: // Register class of this type to be resolved and
associate it with node.
the class ... with the node.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:980: Link link = node.typeArguments != null ?
node.typeArguments.nodes
Link<TypeAnnotation>

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:985: }
Do you think you could write a full Dart test or a test in TypeCheckerTest to
make sure that the type arguments are now correctly resolved?

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
File frog/leg/typechecker.dart (right):

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:55: }
You can remove the code above.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:63: : this.arguments = arguments;
Add new line.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:63: : this.arguments = arguments;
Do you think it'd be more robust to write:
this.arguments = arguments == null ? const EmptyLink<Type>() : arguments;

Instead of giving a default value to arguments?

[ahe, 2012-02-29 12:49:15]

I have some questions, but I think it works. Let talk f2f.

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

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:947: if (node == null) {
Add comment saying this is handling "x is !Foo".

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:948: node = argument.asSend().receiver;
node = argument.asSend().receiver.asTypeAnnotation();
if (node === null) {
  cancel...
}

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:955: if (node.typeName.asSend() !== null) {
Please create a visitor.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:957: // denote named constructors or library prefixes.
Add:

// TODO(ahe): We assume it is a named constructor, fix that.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:964: className == const SourceString('void')){
Move this if-test to right after line 961. In the other case, it is a
compilation error.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
File frog/leg/typechecker.dart (right):

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:51: class TypeVariable {
What is the purpose of this class?

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:53: final Type value;
What is this?

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:62: [Link<Type> arguments = const EmptyLink<Type>()])
Why not this.arguments?

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:68: for (Link link = arguments; !link.isEmpty(); link
= link.tail) {
Could you use:
if (!arguments.isEmpty()) {
  sb.add('<');
  arguments.printOn(sb, ', ')
  sb.add('>');
}

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:78: // TODO(karlklose): merge into InterfaceType as a
named constructor.
+1

[karlklose, 2012-03-30 11:56:40]

Thanks for the comments, Peter and Nicolas. PTAL.

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

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:947: if (node == null) {
On 2012/02/29 12:49:15, ahe wrote:
> Add comment saying this is handling "x is !Foo".

Done.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:948: node = argument.asSend().receiver;
On 2012/02/29 12:49:15, ahe wrote:
> node = argument.asSend().receiver.asTypeAnnotation();
> if (node === null) {
>   cancel...
> }

Done.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:955: if (node.typeName.asSend() !== null) {
I moved this code to a function.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:957: // denote named constructors or library prefixes.
Obsolete.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:964: className == const SourceString('void')){
On 2012/02/29 12:49:15, ahe wrote:
> Move this if-test to right after line 961. In the other case, it is a
> compilation error.

Done.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:975: // Register class of this type to be resolved and
associate it with node.
Comment removed.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:980: Link link = node.typeArguments != null ?
node.typeArguments.nodes
On 2012/02/24 13:36:15, ngeoffray wrote:
> Link<TypeAnnotation>

Done.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/resolver.da...
frog/leg/resolver.dart:985: }
I will add a test to the ResolverTest.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
File frog/leg/typechecker.dart (right):

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:51: class TypeVariable {
Removed.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:53: final Type value;
Removed the class.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:55: }
On 2012/02/24 13:36:15, ngeoffray wrote:
> You can remove the code above.

Done.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:62: [Link<Type> arguments = const EmptyLink<Type>()])
On 2012/02/29 12:49:15, ahe wrote:
> Why not this.arguments?

Done.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:63: : this.arguments = arguments;
On 2012/02/24 13:36:15, ngeoffray wrote:
> Add new line.

Done.

https://chromiumcodereview.appspot.com/9431029/diff/2001/frog/leg/typechecker...
frog/leg/typechecker.dart:68: for (Link link = arguments; !link.isEmpty(); link
= link.tail) {
On 2012/02/29 12:49:15, ahe wrote:
> Could you use:
> if (!arguments.isEmpty()) {
>   sb.add('<');
>   arguments.printOn(sb, ', ')
>   sb.add('>');
> }

Done.

[karlklose, 2012-04-11 10:28:25]

Adding Paul.

[ahe, 2012-04-12 15:05:23]

LGTM!

I suggest you submit as is and address comments in a follow-up CL.

Cheers,
Peter

http://codereview.chromium.org/9431029/diff/16001/frog/tests/leg/src/Resolver...
File frog/tests/leg/src/ResolverTest.dart (right):

http://codereview.chromium.org/9431029/diff/16001/frog/tests/leg/src/Resolver...
frog/tests/leg/src/ResolverTest.dart:61: // testVarSuperclass(); // The parser
crashes with 'class Foo extends var'.
Really?

http://codereview.chromium.org/9431029/diff/16001/frog/tests/leg/src/Resolver...
frog/tests/leg/src/ResolverTest.dart:62: // testOneInterface(); // The parser
does not handle interfaces.
Really?

http://codereview.chromium.org/9431029/diff/16001/frog/tests/leg/src/Resolver...
frog/tests/leg/src/ResolverTest.dart:63: // testTwoInterfaces(); // The parser
does not handle interfaces.
Really?

http://codereview.chromium.org/9431029/diff/16001/frog/tests/leg/src/Resolver...
frog/tests/leg/src/ResolverTest.dart:75: testTypeResolving(visitor, text, name,
expectedElements) {
I think it is confusing when a helper function starts with "test".

http://codereview.chromium.org/9431029/diff/16001/frog/tests/leg/src/Resolver...
frog/tests/leg/src/ResolverTest.dart:83: while (!arguments.isEmpty()) {
Expect.isTrue(index < expectedElements.length);

http://codereview.chromium.org/9431029/diff/16001/frog/tests/leg/src/Resolver...
frog/tests/leg/src/ResolverTest.dart:93: visitor.visit(parseStatement('Foo
foo;'));
What is the purpose of this?

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
File lib/compiler/implementation/compiler.dart (right):

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/compiler.dart:225: dynamicClass =
types.dynamicType.element;
I'd prefer getting the original to work.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:630: useElement(annotation,
type.element);
Long term, could we avoid setting the element?

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:808: if (node == null) {
===

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1023: typeName.source.stringValue ==
"var") {
Is it necessary to test for "var"? Also, it shouldn't be necessary to test for
Dynamic if you can use the definition in js_helper.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1036: error(send.receiver,
MessageKind.CANNOT_RESOLVE);
Shouldn't this be a warning if typeRequired is false?

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1059: if (element ==
compiler.types.voidType.element) {
===

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1060: type = compiler.types.voidType;
Why not just:

type = element.type;

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1061: } else if (element ==
compiler.types.dynamicType.element) {
===

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1064: // TODO(ngeoffray): Should we
also resolve typedef?
I think this TODO is obsolete.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1074: }
I don't think you're validating that the number of type arguments match the
number of parameters.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1075: type = new
InterfaceType(element.name, element, arguments.toLink());
This means you create a new object every time you see a type annotation for
plain types like "int" and "string".

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1079: type = new
SimpleType(element.name, element);
Why not:

type = element.type;

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1081: type =
element.computeType(compiler);
I'm not sure about this.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1710: // Find the correct member
context to perform the lookup in.
This method was a temporary wrapper around calling the ResolverVisitor. Now that
you have added this code, this is no longer the case.

Also, why is this not needed in resolveExpression and get:currentClass?

Why is this needed?

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/codegen.dart (right):

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1449: Type type = node.typeName;
I don't see how typeName is an improvement over typeExpression. I'd expect the
type of something called fooName to be a String, Identifier, or SourceString.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1452:
compiler.unimplemented("visitIs for type variables");
Please add this argument: "instruction: node"

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/ssa/codegen.dart:1454:
compiler.unimplemented("visitIs for typedefs");
Ditto.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
File lib/compiler/implementation/ssa/nodes.dart (right):

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/ssa/nodes.dart:2084: final Type typeName;
Thank you! The name is not optimal, if you have an alternative, it would be
nice.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
File lib/compiler/implementation/typechecker.dart (right):

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/typechecker.dart:64: final SourceString name;
This field seems redundant.

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/typechecker.dart:75: arguments.printOn(sb);
Add argument: ', '

http://codereview.chromium.org/9431029/diff/16001/lib/compiler/implementation...
lib/compiler/implementation/typechecker.dart:126: dynamicType = new
SimpleType(VOID, new ClassElement(DYNAMIC, library));
VOID?