Resolve typedefs.

Resolve typedefs.

[Johnni Winther, 2012-07-30 07:53:03]

This is a merge of http://codereview.chromium.org/10628007/ on top of
http://codereview.chromium.org/10826045/.

I'm landing this CL since Karl is occupied otherwise.

[ahe, 2012-07-30 10:30:05]

Comments so far. As we discussed, perhaps we should fix scope handling before we
move forward with this.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/el...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/el...
lib/compiler/implementation/elements/elements.dart:204: || e.kind ===
ElementKind.TYPEDEF) return e;
I'd prefer if you keep the condition on the same line and the return on a line
by itself (preferably wrapped in curly braces).

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/el...
lib/compiler/implementation/elements/elements.dart:434:
LinkedHashMap<SourceString, TypeVariableElement> typeParameters;
I don't think this should be a map. The overhead of using a map is huge and in
99% of the cases it will contain less than two elements.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/el...
lib/compiler/implementation/elements/elements.dart:441:
compiler.resolveTypedef(this);
This method may be called twice. You need to ensure that cachedType only is
assigned once and not by side-effect. I'm having a hard time seeing that
cachedType is only assigned once.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolver.dart:219: Scope context = new
TopScope(element.getLibrary());
It seems ad hoc how we build the scope here. I would be more comfortable
building the scope up by traversing the enclosing element chain.

Something like:

Link<Element> scopeChain = new EmptyLink<Element>();
for (Element e = element.enclosingElement; e !== null; e = e.enclosingElement) {
  scopeChain = scopeChain.prepend(e);
}
Scope scope = null;
for (Element enclosing in scopeChain) {
  // Extend scope based on enclosing.
}

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolver.dart:366: new
TypedefResolverVisitor(compiler, element.getLibrary(), element);
Construction of the visitor should also be measured.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolver.dart:1677: report = warning;
This creates a new function closure object on each instantiation. That is a lot
of overhead compared to a boolean.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolver.dart:1680: abstract Map<SourceString,
Element> get typeParameters();
What is this for?

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/wa...
File lib/compiler/implementation/warnings.dart (right):

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/wa...
lib/compiler/implementation/warnings.dart:158: "a typedef may not refer to the
type it is defining");
I think this message should include the name of the typedef that is referring to
itself. Also, how about "itself" instead of "the type is it defining"?

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

http://codereview.chromium.org/10834061/diff/1/tests/co19/co19-leg.status#new...
tests/co19/co19-leg.status:265: # may sometime be out of date.
Please follow the instructions in this comment.

[Johnni Winther, 2012-08-01 10:12:28]

PTAL

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/el...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/el...
lib/compiler/implementation/elements/elements.dart:204: || e.kind ===
ElementKind.TYPEDEF) return e;
On 2012/07/30 10:30:05, ahe wrote:
> I'd prefer if you keep the condition on the same line and the return on a line
> by itself (preferably wrapped in curly braces).

Removed.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/el...
lib/compiler/implementation/elements/elements.dart:434:
LinkedHashMap<SourceString, TypeVariableElement> typeParameters;
On 2012/07/30 10:30:05, ahe wrote:
> I don't think this should be a map. The overhead of using a map is huge and in
> 99% of the cases it will contain less than two elements.

Done.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/el...
lib/compiler/implementation/elements/elements.dart:441:
compiler.resolveTypedef(this);
On 2012/07/30 10:30:05, ahe wrote:
> This method may be called twice. You need to ensure that cachedType only is
> assigned once and not by side-effect. I'm having a hard time seeing that
> cachedType is only assigned once.

Done.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
File lib/compiler/implementation/resolver.dart (right):

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolver.dart:219: Scope context = new
TopScope(element.getLibrary());
On 2012/07/30 10:30:05, ahe wrote:
> It seems ad hoc how we build the scope here. I would be more comfortable
> building the scope up by traversing the enclosing element chain.
> 
> Something like:
> 
> Link<Element> scopeChain = new EmptyLink<Element>();
> for (Element e = element.enclosingElement; e !== null; e = e.enclosingElement)
{
>   scopeChain = scopeChain.prepend(e);
> }
> Scope scope = null;
> for (Element enclosing in scopeChain) {
>   // Extend scope based on enclosing.
> }

Done.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolver.dart:366: new
TypedefResolverVisitor(compiler, element.getLibrary(), element);
On 2012/07/30 10:30:05, ahe wrote:
> Construction of the visitor should also be measured.

Done.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolver.dart:1677: report = warning;
On 2012/07/30 10:30:05, ahe wrote:
> This creates a new function closure object on each instantiation. That is a
lot
> of overhead compared to a boolean.

TODO

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/re...
lib/compiler/implementation/resolver.dart:1680: abstract Map<SourceString,
Element> get typeParameters();
On 2012/07/30 10:30:05, ahe wrote:
> What is this for?
Removed.

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/wa...
File lib/compiler/implementation/warnings.dart (right):

http://codereview.chromium.org/10834061/diff/1/lib/compiler/implementation/wa...
lib/compiler/implementation/warnings.dart:158: "a typedef may not refer to the
type it is defining");
On 2012/07/30 10:30:05, ahe wrote:
> I think this message should include the name of the typedef that is referring
to
> itself. Also, how about "itself" instead of "the type is it defining"?

Done.

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

http://codereview.chromium.org/10834061/diff/1/tests/co19/co19-leg.status#new...
tests/co19/co19-leg.status:265: # may sometime be out of date.
On 2012/07/30 10:30:05, ahe wrote:
> Please follow the instructions in this comment.

Done.

[ahe, 2012-08-02 06:43:14]

I think we need to sit down and go through these changes and get them submitted
in smaller chunks. The stacking of this change upon the substitution changes is
making it really hard to make progress on this.

I'm pretty comfortable making such changes with git myself, so I would be more
than happy to sit down and cherrypick changes with you today.

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
File lib/compiler/implementation/elements/elements.dart (right):

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:225: * enclosing element will
be the parent scope.
Could you explain what the difference between these two scopes is?

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:457:
createTypeVariables(this, node.typeParameters);
How about adding:

assert(cachedType === null);

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:903: 
Extra line.

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:909: InterfaceType supertype;
Eventually an interface may extend a typedef. So I would prefer to use Type
here.

Generally, I would prefer everything but "type" remained loosely typed. It would
be useful to have an erroneous later, so flexibility is good.

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:953: Link<TypeVariableType>
get typeVariables() => type.typeArguments;
Same applies here, so I would prefer Link<Type>.

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/elements/elements.dart:1288: String toString() =>
"${enclosingElement.toString()}.${name.slowToString()}";
This change is fine, but am I the only one thinking this return type is a bit
redundant?

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

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:755: inScope =
inClass.buildScope();//new ClassScope(inClass, inClass.getLibrary());
Remove comment.

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:778:
compiler.resolver.toResolve.add(cls);
Please add a comment to explain why it is safe to not resolve the class
immediately.

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:828: if (typeArgument is
TypeAnnotation) {
typeArgument.asTypeAnnotation() !== null

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:833: } else if (typeArgument is
TypeVariable) {
asTypeVariable

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:834: // This happens in default
clauses!
This case is really not making any sense to me.

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:838: onFailure(node,
MessageKind.CANNOT_RESOLVE_TYPE, [typeArgument.name]);
Doesn't the spec say that the type variables must be identical? So if I have:

interface Foo default Bar<X> {} 

class Bar<Y> {}

The error is that X is not Y. So what you need to do here is run through the
type parameters of the class and the default clause and compare them. I don't
think this is the right place to do so. In other words, we shouldn't treat a
default clause as regular TypeAnnotation.

http://codereview.chromium.org/10834061/diff/6001/lib/compiler/implementation...
lib/compiler/implementation/resolver.dart:1823: // TODO(johnniwinther): Handle
the variables in default clauses separately
I don't think the code you have here is working at all, so I'd prefer if you
just replace lines 1822-1829 with a TODO.