Complain about incorrect this/super constructor initializers.

Complain about incorrect this/super constructor initializers.

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/e2eee219acd5b19459bbe4cf266c2ad50b85566e

[ahe, 2017-04-07 13:06:35]

ahe@google.com changed reviewers:
+ karlklose@google.com, paulberry@google.com

[ahe, 2017-04-07 13:06:36]

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/names.dart (right):

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/names.dart:21: final Name emptyName = new Name("");
Added this and sorted the name alphabetically.

[Paul Berry, 2017-04-07 15:56:38]

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:90: /// 4. if an
initializer will always throw.
It sounds like you're talking about a case like this:

class C {
  var x;
  C() : x = (throw new UnimplementedError());
}

But it looks from line 396 like this would be treated as an Initializer, not a
Throw.  I'm guessing what's actually happening here is that you're trying to
make sure that if a compile-time error prevented us from generating code for an
initializer, there won't be a cascading error when we try to generate the
implicit super initializer call.

Assuming I'm correct, consider changing this line to:

4. if a compile-time error prevented us from generating code for an initializer.
 This avoids cascading errors.

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:405: // TODO(ahe): This is
probably an internal error.
Why is this a TODO?

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:483: "'$superclass' has no
constructor that takes no arguments.",
Given the following code:

class C {
  C.foo();
}
class D extends C {
  D();
}

The error will fire (which is correct--there should be an error), but it will be
misleading, since it will say "'C' has no constructor that takes no arguments.",
even though C *does* have a constructor that takes no arguments.

How about if we change the error message to something like:

"'$superclass' must have an unnamed constructor that takes no arguments."

Alternatively, we could do what analyzer does, which is to report a different
error message depending on whether an unnamed constructor exists, e.g.:

initializer = buildInvalidInitializer(
    buildCompileTimeError(
        superTarget == null ?
            "'$superclass' doesn't have an unnamed constructor." :
            "The unnamed constructor for '$superclass' requires arguments.",
        builder.charOffset),
    builder.charOffset);

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart (right):

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:251: -1, "'Object'
has no constructor that takes no arguments.");
Similar issue here to the one above.

[ahe, 2017-04-07 18:19:18]

Thank you, Paul! Great suggestions.

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:90: /// 4. if an
initializer will always throw.
On 2017/04/07 15:56:38, Paul Berry wrote:
> It sounds like you're talking about a case like this:
> 
> class C {
>   var x;
>   C() : x = (throw new UnimplementedError());
> }
> 
> But it looks from line 396 like this would be treated as an Initializer, not a
> Throw.  I'm guessing what's actually happening here is that you're trying to
> make sure that if a compile-time error prevented us from generating code for
an
> initializer, there won't be a cascading error when we try to generate the
> implicit super initializer call.
> 
> Assuming I'm correct, consider changing this line to:
> 
> 4. if a compile-time error prevented us from generating code for an
initializer.
>  This avoids cascading errors.

Will do.

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:405: // TODO(ahe): This is
probably an internal error.
On 2017/04/07 15:56:38, Paul Berry wrote:
> Why is this a TODO?

Because I want to try inserting a call to internalError here later. It takes me
about 30 minutes to run all the tests, and I consider it low-priority to test.

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:483: "'$superclass' has no
constructor that takes no arguments.",
On 2017/04/07 15:56:38, Paul Berry wrote:
> Given the following code:
> 
> class C {
>   C.foo();
> }
> class D extends C {
>   D();
> }
> 
> The error will fire (which is correct--there should be an error), but it will
be
> misleading, since it will say "'C' has no constructor that takes no
arguments.",
> even though C *does* have a constructor that takes no arguments.
> 
> How about if we change the error message to something like:
> 
> "'$superclass' must have an unnamed constructor that takes no arguments."
> 
> Alternatively, we could do what analyzer does, which is to report a different
> error message depending on whether an unnamed constructor exists, e.g.:
> 
> initializer = buildInvalidInitializer(
>     buildCompileTimeError(
>         superTarget == null ?
>             "'$superclass' doesn't have an unnamed constructor." :
>             "The unnamed constructor for '$superclass' requires arguments.",
>         builder.charOffset),
>     builder.charOffset);

Will do. Those message are much better.

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart (right):

https://codereview.chromium.org/2800083002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:251: -1, "'Object'
has no constructor that takes no arguments.");
On 2017/04/07 15:56:38, Paul Berry wrote:
> Similar issue here to the one above.

Will do.

[ahe, 2017-04-18 13:04:10]

PTAL

[Paul Berry, 2017-04-19 07:56:48]

lgtm

[ahe, 2017-04-20 08:04:19]

Description was changed from

==========
Complain about incorrect this/super constructor initializers.
==========

to

==========
Complain about incorrect this/super constructor initializers.

R=paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/e2eee219acd5b19459bbe4cf266c2ad50b85566e
==========

[ahe, 2017-04-20 08:04:20]

Committed patchset #2 (id:20001) manually as
e2eee219acd5b19459bbe4cf266c2ad50b85566e (presubmit successful).