Implement cycle-checking and code generation for redirecting constructors.

Implement cycle-checking and code generation for redirecting constructors.

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

[kasperl, 2012-02-14 14:24:47]

DBC:

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

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/resolver.da...
frog/leg/resolver.dart:77: seen.add(element);
Do you ever add more than one element to the |seen| set? This looks fishy.

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:550: assert (Initializers.isSuperConstructorCall(call)
||
No space after assert.

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/warnings.da...
frog/leg/warnings.dart:71: static final REDIRECTING_CONSTRUCTOR_CYCLE = const
MessageKind(
It would be nice if we used either CTOR or CONSTRUCTOR here.

[karlklose, 2012-02-14 15:44:41]

Thank you for the comments, Kasper.

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

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/resolver.da...
frog/leg/resolver.dart:77: seen.add(element);
Done, redirection should have been added after line 82.

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:550: assert (Initializers.isSuperConstructorCall(call)
||
On 2012/02/14 14:24:47, kasperl wrote:
> No space after assert.

Done.

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/4007/frog/leg/warnings.da...
frog/leg/warnings.dart:71: static final REDIRECTING_CONSTRUCTOR_CYCLE = const
MessageKind(
On 2012/02/14 14:24:47, kasperl wrote:
> It would be nice if we used either CTOR or CONSTRUCTOR here.

Done.

[floitsch, 2012-02-14 16:38:01]

LGTM.

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

https://chromiumcodereview.appspot.com/9391004/diff/5008/frog/leg/resolver.da...
frog/leg/resolver.dart:70: if (tree.initializers != null) {
maybe verify in here that we have a constructor.

https://chromiumcodereview.appspot.com/9391004/diff/5008/frog/leg/resolver.da...
frog/leg/resolver.dart:184: compiler.reportError(node, error);
message?

https://chromiumcodereview.appspot.com/9391004/diff/5008/frog/leg/resolver.da...
frog/leg/resolver.dart:290: noConstructor);
indendation

https://chromiumcodereview.appspot.com/9391004/diff/5008/frog/leg/resolver.da...
frog/leg/resolver.dart:346: visitor.compiler.cancel('internal error: invalid
initializer',
This is not an internal error. The parser accepts expressions here.

[karlklose, 2012-02-15 13:10:35]

Thanks, Florian.

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

https://chromiumcodereview.appspot.com/9391004/diff/5008/frog/leg/resolver.da...
frog/leg/resolver.dart:70: if (tree.initializers != null) {
On 2012/02/14 16:38:02, floitsch wrote:
> maybe verify in here that we have a constructor.

Done.

https://chromiumcodereview.appspot.com/9391004/diff/5008/frog/leg/resolver.da...
frog/leg/resolver.dart:184: compiler.reportError(node, error);
On 2012/02/14 16:38:02, floitsch wrote:
> message?

Done.

https://chromiumcodereview.appspot.com/9391004/diff/5008/frog/leg/resolver.da...
frog/leg/resolver.dart:290: noConstructor);
On 2012/02/14 16:38:02, floitsch wrote:
> indendation

Done.

https://chromiumcodereview.appspot.com/9391004/diff/5008/frog/leg/resolver.da...
frog/leg/resolver.dart:346: visitor.compiler.cancel('internal error: invalid
initializer',
On 2012/02/14 16:38:02, floitsch wrote:
> This is not an internal error. The parser accepts expressions here.

Done.

[ngeoffray, 2012-02-16 11:23:50]

LGTM!

CC'ing Peter so he gets reminded that we need a different AST node between:
foo();
foo() {}

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

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/resolver.da...
frog/leg/resolver.dart:70: if (element.kind !==
ElementKind.GENERATIVE_CONSTRUCTOR_BODY &&
I don't think we ever see a GENERATIVE_CONSTRUCTOR_BODY in the resolver.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/resolver.da...
frog/leg/resolver.dart:78: // have redirections.
Instead of doing this, I suggest creating a method and doing:
if (redirection != null) resolveRedirectingConstructor();

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/resolver.da...
frog/leg/resolver.dart:89: FunctionExpression functionNode =
redirection.parseNode(compiler);
Would a compiler.getRedirectingConstructor make sense here? 'redirection' is not
an element that was given to us, so we should probably go through the compiler.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/tree/nodes....
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/tree/nodes....
frog/leg/tree/nodes.dart:534: bool hasBody() {
Please file a bug, assign it to Peter, and add a TODO to make sure a different
AST will be created for body-less methods.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/tests/leg_only/...
File frog/tests/leg_only/src/CyclicConstructorTest.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/tests/leg_only/...
frog/tests/leg_only/src/CyclicConstructorTest.dart:1: // Copyright (c) 2011, the
Dart project authors.  Please see the AUTHORS file
2011 -> 2012.

Also, this should probably be a language test.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/tests/leg_only/...
frog/tests/leg_only/src/CyclicConstructorTest.dart:16: new A.c();  /// 03:
compile-time error
As discussed yesterday about the multi-tests, will that work on dartc? class A
will be analyzed even if the constructors are not invoked.

[ahe, 2012-02-16 11:35:40]

DBC

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/tests/leg_only/...
File frog/tests/leg_only/src/CyclicConstructorTest.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/tests/leg_only/...
frog/tests/leg_only/src/CyclicConstructorTest.dart:16: new A.c();  /// 03:
compile-time error
On 2012/02/16 11:23:50, ngeoffray wrote:
> As discussed yesterday about the multi-tests, will that work on dartc? class A
> will be analyzed even if the constructors are not invoked.

Use this trick:

 A.a()
: this.b() /// 01: compile-time error
;

[karlklose, 2012-02-17 13:35:00]

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

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/resolver.da...
frog/leg/resolver.dart:70: if (element.kind !==
ElementKind.GENERATIVE_CONSTRUCTOR_BODY &&
We do, the builder resolves its elements to put into into the worklist for
codegen.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/resolver.da...
frog/leg/resolver.dart:78: // have redirections.
On 2012/02/16 11:23:50, ngeoffray wrote:
> Instead of doing this, I suggest creating a method and doing:
> if (redirection != null) resolveRedirectingConstructor();

Done.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/resolver.da...
frog/leg/resolver.dart:89: FunctionExpression functionNode =
redirection.parseNode(compiler);
I changed the code to do simple lookups using the name isntead of reusing
resolvers. I would like to keep that code in the resolver to keep it simple.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/tree/nodes....
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/leg/tree/nodes....
frog/leg/tree/nodes.dart:534: bool hasBody() {
On 2012/02/16 11:23:50, ngeoffray wrote:
> Please file a bug, assign it to Peter, and add a TODO to make sure a different
> AST will be created for body-less methods.

Done.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/tests/leg_only/...
File frog/tests/leg_only/src/CyclicConstructorTest.dart (right):

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/tests/leg_only/...
frog/tests/leg_only/src/CyclicConstructorTest.dart:1: // Copyright (c) 2011, the
Dart project authors.  Please see the AUTHORS file
On 2012/02/16 11:23:50, ngeoffray wrote:
> 2011 -> 2012.
Done.

> Also, this should probably be a language test.
Left it in leg_only for now, but we should go through all the leg_only tests to
decide which of them can be promoted to language tests.

https://chromiumcodereview.appspot.com/9391004/diff/8001/frog/tests/leg_only/...
frog/tests/leg_only/src/CyclicConstructorTest.dart:16: new A.c();  /// 03:
compile-time error
On 2012/02/16 11:23:50, ngeoffray wrote:
> As discussed yesterday about the multi-tests, will that work on dartc? class A
> will be analyzed even if the constructors are not invoked.

Done.