Rewrite how we handle synthesized constructors in the compiler. This was motivated by issue https:/…

Rewrite how we handle synthesized constructors in the compiler. This was motivated by issue https://code.google.com/p/dart/issues/detail?id=11822.

This change:
- Removes the need to create synthesized nodes for the default constructor.

- Treats the default constructor and forwarding constructors in mixins uniformaly: they both are synthesized constructors that call a super constructor.

- Makes the compiler aware that some elements may not have nodes, and acts accordingly. Before, the default constructor had a synthesized node, so the compiler was happy with it. And the forwarding constructors were handled specially to just skip to a non-synthesized constructor. The latter behavior was the cause of issue 11822, and type inference problems.

- Makes the resolver implement what is specified in the specification for forwarding constructors: we create one synthesized constructor per generative constructor of the super class.

- Implements forwarding constructor in named (aka typedef) mixin applications. See builder.dart, and mixin_typedef_constructor_test.dart

- Implements const classes in the presence of mixins: tests/language/const_constructor_mixin_test.dart.

- Fixes type inference in the presence of forwarding constructors: tests/language/inference_mixin_field_test.dart.

R=ahe@google.com, karlklose@google.com

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

[ahe, 2013-07-18 09:59:32]

I need to do another pass on this change. Below are my initial comments. 

What I have seen so far looks awesome.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:243: if (node ==
null) return new LinkedHashSet<Element>();
Not sure about this. The synthetic element may contain an implicit dependency.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3420:
FunctionSignature signature = element.computeSignature(compiler);
Why this change?

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2543: if
(function != null
I think if function is null, there is a big chance you hit the unimplemented
scenario.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:29: expression =
element.enclosingElement.parseNode(backend.compiler);
I'm not sure about this. I'll suggest an alternative on second pass.

[ahe, 2013-07-18 14:32:56]

LGTM!

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:1056: if (tree != null)
validator.validate(tree);
assert(invariant(element, !element.isSynthetic || tree == null));

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:243: if (node ==
null) return new LinkedHashSet<Element>();
On 2013/07/18 09:59:32, ahe wrote:
> Not sure about this. The synthetic element may contain an implicit dependency.

Never mind. Since we only track dependencies at the granularity of top-level
elements, the implicit dependency on the superclass' constructor is recorded on
line 150 and 169.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3289:
compiler.reportError(node, warning);
Please use:

compiler.reportErrorCode(node, MessageKind.CANNOT_FIND_CONSTRUCTOR,
{'constructorName': const SourceString('')});

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3290:
superMember = new ErroneousElementX(
I'm not sure why you include the error message in this element.  The error has
been reported, that should be sufficient.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1439: if
(constructor.isSynthesized) {
Might read nicer if you move this to a separate function.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:29: expression =
element.enclosingElement.parseNode(backend.compiler);
On 2013/07/18 09:59:32, ahe wrote:
> I'm not sure about this. I'll suggest an alternative on second pass.

Spannable spannable = element.implementation.parseNode(backend.compiler);
if (spannable == null) {
  assert(invariant(element, element.isSynthetic));
  spannable = element.enclosingElement;
}
SourceSpan span = spanFromSpannable(spannable);
// TODO(podivilov): find the right sourceFile here and remove offset checks
// below.
if (span.begin < sourceFile.text.length) {
...

[ngeoffray, 2013-07-18 15:25:14]

Thanks Peter!

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:1056: if (tree != null)
validator.validate(tree);
On 2013/07/18 14:32:56, ahe wrote:
> assert(invariant(element, !element.isSynthetic || tree == null));

Done.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3420:
FunctionSignature signature = element.computeSignature(compiler);
On 2013/07/18 09:59:32, ahe wrote:
> Why this change?

Apparently, it's not ensured that functionSignature is not null at this point.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3289:
compiler.reportError(node, warning);
On 2013/07/18 14:32:56, ahe wrote:
> Please use:
> 
> compiler.reportErrorCode(node, MessageKind.CANNOT_FIND_CONSTRUCTOR,
> {'constructorName': const SourceString('')});

Done.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3290:
superMember = new ErroneousElementX(
On 2013/07/18 14:32:56, ahe wrote:
> I'm not sure why you include the error message in this element.  The error has
> been reported, that should be sufficient.

As discussed, if the erroneous element is later used, like in codegen, having
the info is needed for providing runtime error information.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1439: if
(constructor.isSynthesized) {
On 2013/07/18 14:32:56, ahe wrote:
> Might read nicer if you move this to a separate function.

I don't want to introduce a new function at this point. When building a
constructor, there are already 4 methods you need to follow to understand what
is going on. A better refactoring of the builder where building a factory would
be in its own class would be better.

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:29: expression =
element.enclosingElement.parseNode(backend.compiler);
On 2013/07/18 14:32:56, ahe wrote:
> On 2013/07/18 09:59:32, ahe wrote:
> > I'm not sure about this. I'll suggest an alternative on second pass.
> 
> Spannable spannable = element.implementation.parseNode(backend.compiler);
> if (spannable == null) {
>   assert(invariant(element, element.isSynthetic));
>   spannable = element.enclosingElement;
> }
> SourceSpan span = spanFromSpannable(spannable);
> // TODO(podivilov): find the right sourceFile here and remove offset checks
> // below.
> if (span.begin < sourceFile.text.length) {
> ...

Done.

[karlklose, 2013-07-18 15:29:47]

LGTM.

[ngeoffray, 2013-07-18 15:51:57]

Committed patchset #5 manually as r25148 (presubmit successful).

[ahe, 2013-07-19 09:10:44]

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3420:
FunctionSignature signature = element.computeSignature(compiler);
On 2013/07/18 15:25:14, ngeoffray wrote:
> On 2013/07/18 09:59:32, ahe wrote:
> > Why this change?
> 
> Apparently, it's not ensured that functionSignature is not null at this point.

That is a bug.  It must be non-null. Calling computeSignature can start
enqueueing stuff.

I think it would be best if you can ensure the resolver has computed the
signature already.

[ngeoffray, 2013-08-15 08:23:19]

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/19754002/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3420:
FunctionSignature signature = element.computeSignature(compiler);
On 2013/07/19 09:10:44, ahe wrote:
> On 2013/07/18 15:25:14, ngeoffray wrote:
> > On 2013/07/18 09:59:32, ahe wrote:
> > > Why this change?
> > 
> > Apparently, it's not ensured that functionSignature is not null at this
point.
> 
> That is a bug.  It must be non-null. Calling computeSignature can start
> enqueueing stuff.
> 
> I think it would be best if you can ensure the resolver has computed the
> signature already.

Done in: https://codereview.chromium.org/22874003/