Type check field initializers in the correct context.

Type check field initializers in the correct context.

Closes #27104

R=sigmund@google.com

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

[Johnni Winther, 2016-08-18 14:06:28]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2016-08-18 14:46:10]

lgtm with comments below

https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
File tests/language/regress_27104_test.dart (right):

https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
tests/language/regress_27104_test.dart:5: // Regression test for issue 27401
Let's rename the file and add some description here about what this is providing
coverage for. That way we can understand the test without having to lookup the
bug (also the bug doesn't have much details about what the problem is).

Suggestion:
 - filename: typecheck_multifield_declaration_test.dart
 - Description: Checks that we can correctly typecheck multi-variable
declarations on fields and top-levels. This is also a regression test for  Issue
27401.

https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
tests/language/regress_27104_test.dart:9: A a = new A(), b = new A();
Also - do we want to test it out on class fields too? E.g.:

class B {
  A x = new A(), y = new A();
}

main() => [new B().x, new B().y]

?

[Siggi Cherem (dart-lang), 2016-08-18 14:48:43]

On 2016/08/18 14:46:10, Siggi Cherem (dart-lang) wrote:
> lgtm with comments below
> 
>
https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
> File tests/language/regress_27104_test.dart (right):
> 
>
https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
> tests/language/regress_27104_test.dart:5: // Regression test for issue 27401
> Let's rename the file and add some description here about what this is
providing
> coverage for. That way we can understand the test without having to lookup the
> bug (also the bug doesn't have much details about what the problem is).
> 
> Suggestion:
>  - filename: typecheck_multifield_declaration_test.dart
>  - Description: Checks that we can correctly typecheck multi-variable
> declarations on fields and top-levels. This is also a regression test for 
Issue
> 27401.
> 
>
https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
> tests/language/regress_27104_test.dart:9: A a = new A(), b = new A();
> Also - do we want to test it out on class fields too? E.g.:
> 
> class B {
>   A x = new A(), y = new A();
> }
> 
> main() => [new B().x, new B().y]
> 
> ?

To make sure I understand the issue/fix: did this start failing because we used
to create an artificial resolvedAST with additional variableDeclarations objects
and that meant that this visitor didn't have to handle multi-variable
declarations before?

[Johnni Winther, 2016-08-19 07:46:07]

On 2016/08/18 14:48:43, Siggi Cherem (dart-lang) wrote:
> On 2016/08/18 14:46:10, Siggi Cherem (dart-lang) wrote:
> > lgtm with comments below
> > 
> >
>
https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
> > File tests/language/regress_27104_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
> > tests/language/regress_27104_test.dart:5: // Regression test for issue 27401
> > Let's rename the file and add some description here about what this is
> providing
> > coverage for. That way we can understand the test without having to lookup
the
> > bug (also the bug doesn't have much details about what the problem is).
> > 
> > Suggestion:
> >  - filename: typecheck_multifield_declaration_test.dart
> >  - Description: Checks that we can correctly typecheck multi-variable
> > declarations on fields and top-levels. This is also a regression test for 
> Issue
> > 27401.
> > 
> >
>
https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
> > tests/language/regress_27104_test.dart:9: A a = new A(), b = new A();
> > Also - do we want to test it out on class fields too? E.g.:
> > 
> > class B {
> >   A x = new A(), y = new A();
> > }
> > 
> > main() => [new B().x, new B().y]
> > 
> > ?
> 
> To make sure I understand the issue/fix: did this start failing because we
used
> to create an artificial resolvedAST with additional variableDeclarations
objects
> and that meant that this visitor didn't have to handle multi-variable
> declarations before?

Previously we created a synthetic VariableDefinitions node for multifields so
that there was a one-to-one relationship between the field's TreeElements and
the AST. After the change the same VariableDefinitions is shared between the
fields but they do not share their TreeElements. This let to the error that when
type checking one field you would meet the other but not have any resolution
data for it.

[Johnni Winther, 2016-08-19 07:46:16]

https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
File tests/language/regress_27104_test.dart (right):

https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
tests/language/regress_27104_test.dart:5: // Regression test for issue 27401
On 2016/08/18 14:46:09, Siggi Cherem (dart-lang) wrote:
> Let's rename the file and add some description here about what this is
providing
> coverage for. That way we can understand the test without having to lookup the
> bug (also the bug doesn't have much details about what the problem is).
> 
> Suggestion:
>  - filename: typecheck_multifield_declaration_test.dart
>  - Description: Checks that we can correctly typecheck multi-variable
> declarations on fields and top-levels. This is also a regression test for 
Issue
> 27401.

Done.

https://codereview.chromium.org/2250433007/diff/1/tests/language/regress_2710...
tests/language/regress_27104_test.dart:9: A a = new A(), b = new A();
On 2016/08/18 14:46:09, Siggi Cherem (dart-lang) wrote:
> Also - do we want to test it out on class fields too? E.g.:
> 
> class B {
>   A x = new A(), y = new A();
> }
> 
> main() => [new B().x, new B().y]
> 
> ?

Done.

[Johnni Winther, 2016-08-19 07:58:58]

Description was changed from

==========
Type check field initializers in the correct context.

Closes #27104
==========

to

==========
Type check field initializers in the correct context.

Closes #27104

R=sigmund@google.com

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

[Johnni Winther, 2016-08-19 07:58:59]

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