Updated parser to produce error when trying to reinitialize final variables in class constructors. …

Updated parser to produce error when trying to reinitialize final variables in class constructors. Fixes issue #29658.

BUG=
R=asiva@google.com

Example:
Foo {
  final int x = 10;
  Foo(this.x); // Error
  Foo.named() : x = 42; // Error
}
Committed: https://github.com/dart-lang/sdk/commit/b5cff3f68f728af87f88f2f76eecfb224e303278

[bkonyi, 2017-06-13 18:18:39]

bkonyi@google.com changed reviewers:
+ asiva@google.com, zra@google.com

[zra, 2017-06-13 20:19:42]

Please add some tests under //tests/language

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:3029: ReportError(init_pos, "'%s' is initialized already,
and final.",
How about "final field '%s' is already initialized" ?

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4517: for (int p = 0; p < parameters.length(); p++) {
intptr_t p

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4523: for (int i = 0; i < members->members().length(); i++)
{
intptr_t i

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4527: existing_member->field_ != NULL &&
parens around != clause

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4530: "'%s' is initialized already, and final.",
"final field %s is already initialized"

[bkonyi, 2017-06-13 22:16:47]

Added tests that are similar to the code snippets provided in the issue. PTAL.

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:3029: ReportError(init_pos, "'%s' is initialized already,
and final.",
On 2017/06/13 20:19:41, zra wrote:
> How about "final field '%s' is already initialized" ?

Done.

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4517: for (int p = 0; p < parameters.length(); p++) {
On 2017/06/13 20:19:41, zra wrote:
> intptr_t p

Done.

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4523: for (int i = 0; i < members->members().length(); i++)
{
On 2017/06/13 20:19:42, zra wrote:
> intptr_t i

Done.

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4527: existing_member->field_ != NULL &&
On 2017/06/13 20:19:42, zra wrote:
> parens around != clause

Done.

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:4530: "'%s' is initialized already, and final.",
On 2017/06/13 20:19:41, zra wrote:
> "final field %s is already initialized"

Done.

[siva, 2017-06-13 22:20:08]

Partial comments, I was reviewing while you updated the CL.

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2939553005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:3027: const String& field_name =
String::ZoneHandle(field->name());
Why do these have to be zone handles and not just regular handles?

[siva, 2017-06-13 22:37:57]

Have you checked your test with
dart -dfe option also ?

You may have to update the status files for dart2js, analyzer, ddc etc. for the
new test you added as these tools might not be generating the correct compile
time error.

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8174: }
This method is not needed, see comment later.

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:4528: existing_member->field_->IsFinalAndInitialized()) {
The correct check here is to use field_->has_initializer() as we have delayed
initialization of fields if the expression is not a simple literal.

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:4750: CheckMemberAlreadyInitializedConflicts(members,
&member);
I don't think this is quite correct, consider this example where the final field
comes after the constructor.

class Foo {
  Foo(this.x);
  final int x = 10;
}

https://codereview.chromium.org/2939553005/diff/20001/tests/language/final_at...
File tests/language/final_attempt_reinitialization_test.dart (right):

https://codereview.chromium.org/2939553005/diff/20001/tests/language/final_at...
tests/language/final_attempt_reinitialization_test.dart:8: }                  
//# 01: compile-time error
Not sure why you have so many 01: compile-time error specifications here

https://codereview.chromium.org/2939553005/diff/20001/tests/language/final_at...
tests/language/final_attempt_reinitialization_test.dart:20: }
I think this should do the trick :


class Foo {
  final int x = 42;
  Foo(this.x);  // #01: compile-time error
  Foo.name(String name) : this.name = name; // 02: compile-time error
  final String name = "Coffee Lab";
}

void main() {
  Foo f1 = new Foo(10);
  Foo f2 = new Foo.name("Tea Lab");
}

[bkonyi, 2017-06-15 22:32:49]

I've addressed asiva's last round of comments. PTAL.

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8174: }
On 2017/06/13 22:37:56, siva wrote:
> This method is not needed, see comment later.

Removed.

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:4528: existing_member->field_->IsFinalAndInitialized()) {
On 2017/06/13 22:37:57, siva wrote:
> The correct check here is to use field_->has_initializer() as we have delayed
> initialization of fields if the expression is not a simple literal.

Ah, don't know how I missed that... done.

https://codereview.chromium.org/2939553005/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:4750: CheckMemberAlreadyInitializedConflicts(members,
&member);
This seemed to work, but it probably makes more sense to do this after the
entire class is processed anyways.

https://codereview.chromium.org/2939553005/diff/20001/tests/language/final_at...
File tests/language/final_attempt_reinitialization_test.dart (right):

https://codereview.chromium.org/2939553005/diff/20001/tests/language/final_at...
tests/language/final_attempt_reinitialization_test.dart:8: }                  
//# 01: compile-time error
On 2017/06/13 22:37:57, siva wrote:
> Not sure why you have so many 01: compile-time error specifications here

Oh, I was under the assumption that you needed those specifications for those
lines to be included for an instance of a multitest. Just putting the
specification by the calls in main does the trick as well.

https://codereview.chromium.org/2939553005/diff/20001/tests/language/final_at...
tests/language/final_attempt_reinitialization_test.dart:20: }
On 2017/06/13 22:37:57, siva wrote:
> I think this should do the trick :
> 
> 
> class Foo {
>   final int x = 42;
>   Foo(this.x);  // #01: compile-time error
>   Foo.name(String name) : this.name = name; // 02: compile-time error
>   final String name = "Coffee Lab";
> }
> 
> void main() {
>   Foo f1 = new Foo(10);
>   Foo f2 = new Foo.name("Tea Lab");
> }

Yes, that works too.

[siva, 2017-06-16 00:51:10]

https://codereview.chromium.org/2939553005/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2939553005/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3030: }
Pull the handle creation outside the while loop

String& initialized_name = String::Handle(Z);
String field_name = String::Handle(Z);

while (initialize.....) {
  ...
  ...
  initialized_name ^= initialized_field->name();
  field_name ^= field->name();
  if (...) {
    ...
  }
  ....
  ....

https://codereview.chromium.org/2939553005/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:4539: }
If you invert this loop as

for (intptr_t j = 0; j < members.length(); j++) {
  ....
  if (current_member.IsConstructor()) {
    ....
    ....
    for (intptr_t p = 0; ....) {
      for (intptr k = 0; k < fields.length(); k++) {
        check for field error;
      }
    }
}

Then it might be possible to merge this into Parser::CheckConstructors which is
also looping over all the members to determine if we get cycles because of
redirection.

https://codereview.chromium.org/2939553005/diff/40001/tests/language/final_at...
File tests/language/final_attempt_reinitialization_test.dart (right):

https://codereview.chromium.org/2939553005/diff/40001/tests/language/final_at...
tests/language/final_attempt_reinitialization_test.dart:18: new
CoffeeShop.name("Covfefe Lab"); //# 02: compile-time error
Does it need # 02: in both the lines? Is it not sufficient to say #02 in line
18?

[bkonyi, 2017-06-16 19:40:45]

https://codereview.chromium.org/2939553005/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2939553005/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3030: }
On 2017/06/16 00:51:10, siva wrote:
> Pull the handle creation outside the while loop
> 
> String& initialized_name = String::Handle(Z);
> String field_name = String::Handle(Z);
> 
> while (initialize.....) {
>   ...
>   ...
>   initialized_name ^= initialized_field->name();
>   field_name ^= field->name();
>   if (...) {
>     ...
>   }
>   ....
>   ....

Done.

https://codereview.chromium.org/2939553005/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:4539: }
On 2017/06/16 00:51:10, siva wrote:
> If you invert this loop as
> 
> for (intptr_t j = 0; j < members.length(); j++) {
>   ....
>   if (current_member.IsConstructor()) {
>     ....
>     ....
>     for (intptr_t p = 0; ....) {
>       for (intptr k = 0; k < fields.length(); k++) {
>         check for field error;
>       }
>     }
> }
> 
> Then it might be possible to merge this into Parser::CheckConstructors which
is
> also looping over all the members to determine if we get cycles because of
> redirection.
>   

Done.

https://codereview.chromium.org/2939553005/diff/40001/tests/language/final_at...
File tests/language/final_attempt_reinitialization_test.dart (right):

https://codereview.chromium.org/2939553005/diff/40001/tests/language/final_at...
tests/language/final_attempt_reinitialization_test.dart:18: new
CoffeeShop.name("Covfefe Lab"); //# 02: compile-time error
On 2017/06/16 00:51:10, siva wrote:
> Does it need # 02: in both the lines? Is it not sufficient to say #02 in line
> 18?

Apparently we do, or else we get an unexpected token '}' error thrown at line
19.

[siva, 2017-06-19 18:11:14]

lgtm

https://codereview.chromium.org/2939553005/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2939553005/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5270: String& field_name = String::Handle(Z);
This handle allocation could be hoisted outside the outer for loop too.

[bkonyi, 2017-06-19 19:07:44]

Description was changed from

==========
Updated parser to produce error when trying to reinitialize final variables in
class constructors. Fixes issue #29658.

BUG=

Example:
Foo {
  final int x = 10;
  Foo(this.x); // Error
  Foo.named() : x = 42; // Error
}
==========

to

==========
Updated parser to produce error when trying to reinitialize final variables in
class constructors. Fixes issue #29658.

BUG=
R=asiva@google.com

Example:
Foo {
  final int x = 10;
  Foo(this.x); // Error
  Foo.named() : x = 42; // Error
}
Committed:
https://github.com/dart-lang/sdk/commit/b5cff3f68f728af87f88f2f76eecfb224e303278
==========

[bkonyi, 2017-06-19 19:07:45]

Committed patchset #5 (id:80001) manually as
b5cff3f68f728af87f88f2f76eecfb224e303278 (presubmit successful).