Produce error when duplicate field initializers are found

Produce error when duplicate field initializers are found

BUG=dart-1035
TEST=

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

[ahe, 2012-08-15 10:47:07]

LGTM!

I'll submit the patch when you have addressed the comments.

Thanks again for fixing my bugs :-)

Cheers,
Peter

https://chromiumcodereview.appspot.com/10834327/diff/1/lib/compiler/implement...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10834327/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:410: final Compiler compiler;
Don't need this field.

https://chromiumcodereview.appspot.com/10834327/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:557: // keep track of all "this.param"
parameters specified for constructor so
Not a proper sentence. By which I mean: please capitalize the first word, and
terminate the sentence with a period :-)

https://chromiumcodereview.appspot.com/10834327/diff/1/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:560:
constructor.computeSignature(compiler);
You can get this from visitor.compiler.

[aam-me, 2012-08-15 12:50:30]

All should be taken care of, Peter. Thank you for the code review.

[aam-me, 2012-08-15 12:55:12]

Please, hold this codereview - running tests with --host-checked and seeing
errors.

[aam-me, 2012-08-16 01:18:56]

Should be fixed now. Please, review.

[ahe, 2012-08-16 07:00:46]

Hi Alexander,

Did you consider this case:

class Fisk {
  var x;
  Fisk(this.x, this.x);
}

void main() {
  new Fisk(null, null);
}

Cheers,
Peter

[aam-me, 2012-08-16 11:32:26]

Now I have, Peter. Thank you for pointing this out.

Changes are again ready for the review.

[ahe, 2012-08-16 11:56:43]

Only minor nits now.

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:432: void
checkForDuplicateInitializers(SourceString name, Node init) {
I hoped you would do that :-)

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:562: if (functionParameters != null) {
Please use !== here.

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:562: if (functionParameters != null) {
Why do you have to test for null?

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:564: if (element.kind ==
ElementKind.FIELD_PARAMETER) {
Please use === here.

https://chromiumcodereview.appspot.com/10834327/diff/3004/tests/language/cons...
File tests/language/constructor_duplicate_initializers_test.dart (right):

https://chromiumcodereview.appspot.com/10834327/diff/3004/tests/language/cons...
tests/language/constructor_duplicate_initializers_test.dart:19:
Class.two_fields(this.field_, this.field_); /// 03: compile-time error
The test is more precise if you write it like this:

  Class.two_fields(this.field_
                  // Test against duplicate final field initialization
                  // in initializing formals.
                  , this.field_ /// 03: compile-time error
                  );

The advantage is that it reduces the chance that there is something else wrong
on the line and we end up not testing what we think we're testing.

https://chromiumcodereview.appspot.com/10834327/diff/3004/tests/language/cons...
tests/language/constructor_duplicate_initializers_test.dart:26: new
Class.two_fields(42, 42); /// 03: continued
Then this would also need rewriting.

[aam-me, 2012-08-16 12:28:16]

On 2012/08/16 11:56:43, ahe wrote:
> Only minor nits now.

Thank you for your patience and valuable input, Peter.
Addressed all your comments - change is ready for review.

[aam-me, 2012-08-16 12:28:58]

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:562: if (functionParameters != null) {
On 2012/08/16 11:56:43, ahe wrote:
> Why do you have to test for null?
"Defensive" programming, I guess :-)
But I see that computeSignature is used without such checks in other places, so
no good reason to have it here neither.

[ahe, 2012-08-16 18:23:27]

LGTM, I'll commit this tomorrow (unless I forget, so please remind me if you
don't see it showing up soon).

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10834327/diff/3004/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:562: if (functionParameters != null) {
On 2012/08/16 12:28:58, aam wrote:
> On 2012/08/16 11:56:43, ahe wrote:
> > Why do you have to test for null?
> "Defensive" programming, I guess :-)
> But I see that computeSignature is used without such checks in other places,
so
> no good reason to have it here neither.

Interesting. I think of defensive programming as writing code that I know will
fail early if I made a mistake. So I try to avoid testing for null when I'm
fairly confident that a null is an error.

On a related note: long term, I think we would like that methods that detect an
error when trying to produce a type, element, etc., return an erroneous type,
element, etc.

[aam-me, 2012-08-17 02:28:59]

Yes, agree with failing early on mistakes. 

Failing early on unexpected, unforeseen circumstances could be
different matter. Building software that gracefully fails early
while at the same time is able to recover or restart and continue
forward is not easy. In Erlang, for example, you have to come up
with explicit supervisor-worker tree, where you have to empower
supervisor to recover(or give up) if it's subordinate fails early.

Defensive to me means resilient to partially correct, broken data
scenarios. Not necessary programmers mistakes. 

In terms of checking for nulls in particular product during particular 
stage of process, though, main thing is that it has to be consistent. :-)

Yes, agree. Analysis phase of compiler wants to signal
errors by returning error/diagnostic indicator wrapped into
properly formatted structures(nodes/types/etc). Code generation
phase - that is different matter :-)

[ahe, 2012-08-20 12:41:49]

Submitted as https://chromiumcodereview.appspot.com/10860039/