Resolve initializers in constructors.

Resolve initializers in constructors.

R=floitsch@google.com

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

[floitsch, 2011-12-19 17:25:28]

I will let nicolas do a full review. Just a question/observation...

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

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:31: SendSet init = link.head;
How can you be sure that this is a SendSet?
What about super calls?
what if the user entered something completely wrong?

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:172: if (init == null
super call is missing.
Add TODO?

[ahe, 2011-12-19 18:07:26]

LGTM for now, but perhaps add a few TODOs :-)

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

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:172: if (init == null
On 2011/12/19 17:25:29, floitsch wrote:
> super call is missing.
> Add TODO?

In addition, there might be a this(...) send.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:175: node: node.initializers);
"node.initializers" should be "link.head".

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:182: SourceString name =
init.selector.asIdentifier().source;
The above code looks a bit brittle and seems to rely on various assumptions that
may not hold. How about creating a few helper methods that return the expected
thing or report an error if it isn't.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:643: Element lookup(SourceString name) {
This concerns me as it seems to create an extra name space for statics. Also,
instead of reporting "hey, foo isn't static", you report "hey, foo cannot be
resolved".

From an IDE perspective, I'd still want to be able to jump to the declaration of
whatever isn't static so I can make it static.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:46: 'only \'this\' can be used as a receiver in an
initializer');
Perhaps this error message could be improved and shorten:

"'this' expected"

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:48: 'field #{1} is initialized more than once');
I'd like to see the previous place it is initialized.

[floitsch, 2011-12-20 14:18:27]

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

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:192: useElement(init.selector, target);
This should be useElement(init, target);

[ngeoffray, 2011-12-20 15:09:51]

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

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:166: if (node.initializers !== null) {
As discussed, I don't think this is the right place to (partially) analyze the
initializers. This code should be close to the code line 26.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:595: abstract Element lookup(SourceString name);
Thanks!

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:643: Element lookup(SourceString name) {
On 2011/12/19 18:07:26, ahe wrote:
> This concerns me as it seems to create an extra name space for statics. Also,
> instead of reporting "hey, foo isn't static", you report "hey, foo cannot be
> resolved".
> 
> From an IDE perspective, I'd still want to be able to jump to the declaration
of
> whatever isn't static so I can make it static.

Two suggestions to address this problem.
1) Create a 'mode' in FullResolverVisitor:
- when doing normal resolution
- when doing resolution of the RHS of an initializer.

2) Have this StaticScope warn about finding an instance member. It should also
deal with 'this' and 'super'. Not sure how that would fit in. Solution 1 does
easily solve the 'this' and 'super'.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:648: Element add(Element element) => parent.add(element);
Do we expect to add to this scope?

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:47: static final DUPLICATION_INITIALIZATION = const
MessageKind(
DUPLICATE_INITIALIZER?

[karlklose, 2011-12-21 10:19:43]

PTAL.

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

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:31: SendSet init = link.head;
Done, added a test.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:166: if (node.initializers !== null) {
On 2011/12/20 15:09:51, ngeoffray wrote:
> As discussed, I don't think this is the right place to (partially) analyze the
> initializers. This code should be close to the code line 26.

Done.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:172: if (init == null
On 2011/12/19 18:07:26, ahe wrote:
> On 2011/12/19 17:25:29, floitsch wrote:
> > super call is missing.
> > Add TODO?
> 
> In addition, there might be a this(...) send.

Done, added a todo for super(...) and this(...).

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:175: node: node.initializers);
On 2011/12/19 18:07:26, ahe wrote:
> "node.initializers" should be "link.head".

Done.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:182: SourceString name =
init.selector.asIdentifier().source;
On 2011/12/19 18:07:26, ahe wrote:
> The above code looks a bit brittle and seems to rely on various assumptions
that
> may not hold. How about creating a few helper methods that return the expected
> thing or report an error if it isn't.

Done.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:192: useElement(init.selector, target);
On 2011/12/20 14:18:27, floitsch wrote:
> This should be useElement(init, target);

Done.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:46: 'only \'this\' can be used as a receiver in an
initializer');
On 2011/12/19 18:07:26, ahe wrote:
> Perhaps this error message could be improved and shorten:
> 
> "'this' expected"

Done.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:47: static final DUPLICATION_INITIALIZATION = const
MessageKind(
On 2011/12/20 15:09:51, ngeoffray wrote:
> DUPLICATE_INITIALIZER?

Done.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:48: 'field #{1} is initialized more than once');
Done, but I only print the node information, not the position, because I don't
have that information available.

[ngeoffray, 2011-12-21 11:52:11]

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

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:40: void resolveInitializers(element, node, visitor) {
Types on parameters?

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:62: visitor.warning(init,
MessageKind.DUPLICATE_INITIALIZER,
The spec says it's a compile time error, so should that be a visitor.error?

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:70: compiler.cancel('internal error: invalid
initializer',
invalid -> unimplemented

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:99: bool isStaticContext;
isStaticContext always looks weird to me (is it a static method, meaning does it
have the static modifier?). I'd rather have a 'inInstanceContext'. This field
should be set during construction of the visitor:

inInstanceContext = element.isInstanceMember().

We could then use it for closures.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:354: target = lookup(node, name);
I think this should be changed to target = visit(node.selector);

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:398: getter = lookup(node,
node.selector.asIdentifier().source);
ditto.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:255: bool isInitializer() {
Not sure I understand: how can you be sure a SendSet is in an initializer list?
Or are you using 'initializer' for any kind of variable initialization?

In any case, I think these helper methods belong to the resolver. The helpers
that I would put on a SendSet are get lhs() and get rhs().

http://codereview.chromium.org/8974014/diff/15001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/8974014/diff/15001/frog/leg/warnings.dart#newc...
frog/leg/warnings.dart:52: 'field #{1} is initialized more than once (before at
#{2})');
before at -> already initialized at ?

[ahe, 2011-12-21 12:01:57]

LGTM, but I think you need to look at isInitializer before you commit.

I'd like better diagnostics, but not necessarily in this CL.

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/8974014/diff/5001/frog/leg/warnings.dart#newco...
frog/leg/warnings.dart:48: 'field #{1} is initialized more than once');
On 2011/12/21 10:19:44, karlklose wrote:
> Done, but I only print the node information, not the position, because I don't
> have that information available.

Why don't you have that information?

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

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:40: void resolveInitializers(element, node, visitor) {
I'd like a type on node.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:70: compiler.cancel('internal error: invalid
initializer',
This is not an internal error, and the initializer is not invalid. I would just
say "unimplemented".

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:259: return
receiver.asIdentifier().source.stringValue() == 'this';
== -> ===

Also, stringValue is a getter, not a function.

Also, this is dead code. You have two return statements. Did you mean &&?

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:262: SourceString
getInitializerFieldName(errorHandler()) {
How about "onError" instead of "errorHandler"?

http://codereview.chromium.org/8974014/diff/15001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/8974014/diff/15001/frog/leg/warnings.dart#newc...
frog/leg/warnings.dart:52: 'field #{1} is initialized more than once (before at
#{2})');
I'm not sure I understand this. Won't it just say:

"field foo is initialized more than once (before at foo)"

I would think there should be a diagnostic for each "duplicated" initializer.

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

http://codereview.chromium.org/8974014/diff/15001/frog/tests/leg/src/Resolver...
frog/tests/leg/src/ResolverTest.dart:436: [MessageKind.DUPLICATE_INITIALIZER]);
I expected two diagnostics here:

foo.dart:line:column: error: 'foo' is already initialized
A() : this.foo = 1, this.foo = 2;
                    ^^^^^^^^^^^^

foo.dart:line:column: note: previous initializer of 'foo'
A() : this.foo = 1, this.foo = 2;
      ^^^^^^^^^^^^

[karlklose, 2011-12-21 16:33:46]

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

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:40: void resolveInitializers(element, node, visitor) {
On 2011/12/21 11:52:11, ngeoffray wrote:
> Types on parameters?

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:40: void resolveInitializers(element, node, visitor) {
On 2011/12/21 12:01:58, ahe wrote:
> I'd like a type on node.

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:62: visitor.warning(init,
MessageKind.DUPLICATE_INITIALIZER,
On 2011/12/21 11:52:11, ngeoffray wrote:
> The spec says it's a compile time error, so should that be a visitor.error?

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:70: compiler.cancel('internal error: invalid
initializer',
On 2011/12/21 11:52:11, ngeoffray wrote:
> invalid -> unimplemented

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:70: compiler.cancel('internal error: invalid
initializer',
On 2011/12/21 12:01:58, ahe wrote:
> This is not an internal error, and the initializer is not invalid. I would
just
> say "unimplemented".

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:99: bool isStaticContext;
On 2011/12/21 11:52:11, ngeoffray wrote:
> isStaticContext always looks weird to me (is it a static method, meaning does
it
> have the static modifier?). I'd rather have a 'inInstanceContext'. This field
> should be set during construction of the visitor:
> 
> inInstanceContext = element.isInstanceMember().
> 
> We could then use it for closures.

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:354: target = lookup(node, name);
On 2011/12/21 11:52:11, ngeoffray wrote:
> I think this should be changed to target = visit(node.selector);

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:398: getter = lookup(node,
node.selector.asIdentifier().source);
On 2011/12/21 11:52:11, ngeoffray wrote:
> ditto.

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:255: bool isInitializer() {
Done, moved the helpers.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:259: return
receiver.asIdentifier().source.stringValue() == 'this';
On 2011/12/21 12:01:58, ahe wrote:
> == -> ===
> 
> Also, stringValue is a getter, not a function.
> 
> Also, this is dead code. You have two return statements. Did you mean &&?

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:262: SourceString
getInitializerFieldName(errorHandler()) {
On 2011/12/21 12:01:58, ahe wrote:
> How about "onError" instead of "errorHandler"?

Done.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/warnings.dart
File frog/leg/warnings.dart (right):

http://codereview.chromium.org/8974014/diff/15001/frog/leg/warnings.dart#newc...
frog/leg/warnings.dart:52: 'field #{1} is initialized more than once (before at
#{2})');
Removed 'before at' in favor of a second warning.

http://codereview.chromium.org/8974014/diff/15001/frog/leg/warnings.dart#newc...
frog/leg/warnings.dart:52: 'field #{1} is initialized more than once (before at
#{2})');
No, it would print the initializer node. I removed it and print another warning
now.

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

http://codereview.chromium.org/8974014/diff/15001/frog/tests/leg/src/Resolver...
frog/tests/leg/src/ResolverTest.dart:436: [MessageKind.DUPLICATE_INITIALIZER]);
On 2011/12/21 12:01:58, ahe wrote:
> I expected two diagnostics here:
> 
> foo.dart:line:column: error: 'foo' is already initialized
> A() : this.foo = 1, this.foo = 2;
>                     ^^^^^^^^^^^^
> 
> foo.dart:line:column: note: previous initializer of 'foo'
> A() : this.foo = 1, this.foo = 2;
>       ^^^^^^^^^^^^

Done.

[ngeoffray, 2011-12-21 16:44:08]

LGTM if you fix the getInitializerFieldName bug.

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

http://codereview.chromium.org/8974014/diff/16002/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:63: SourceString name = getInitializerFieldName(init,
onError);
If it's not an identifier, what do you get as a name? I think you can get rid of
getInitializerFieldName, and handle the problem here.

http://codereview.chromium.org/8974014/diff/16002/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:65: Element target =
classElement.lookupLocalElement(name);
lookupLocalElement -> lookupLocalMember (after merging with my changes)

http://codereview.chromium.org/8974014/diff/16002/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:78: [name]);
@ahe: not sure, but splitting the warning in two, is that ok for the IDE?

http://codereview.chromium.org/8974014/diff/16002/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:85: compiler.cancel('uniplemented', node:link.head);
uniMplemented