Issue 991: Missing compiler errors for uninitialized final fields

Issue 991: Missing compiler errors for uninitialized final fields
http://code.google.com/p/dart/issues/detail?id=991
  -Track non initialized fields at declaration for classes
  -Verify they are initialized in non-redirect constructors
  -Check for duplicate final initialization; add tests and mark VM and Frog
   as failing
BUG=
TEST=

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

[ahe, 2012-01-03 22:57:12]

Hi John,

Test and status file changes: LGTM!

Did you check this with leg:

./test.py -cleg language/ConstructorDuplicateInitializersTest

The relevant status file is: tests/language/language-leg.status.

I'd be very surprised if leg is catching this already.

Cheers,
Peter

http://codereview.chromium.org/9081001/diff/1/tests/language/src/ConstructorD...
File tests/language/src/ConstructorDuplicateInitializersTest.dart (right):

http://codereview.chromium.org/9081001/diff/1/tests/language/src/ConstructorD...
tests/language/src/ConstructorDuplicateInitializersTest.dart:10: , field_ = 2 
/// 01: compile-time error
I suggest adding a comment above explaining what the error is.

http://codereview.chromium.org/9081001/diff/1/tests/language/src/ConstructorD...
tests/language/src/ConstructorDuplicateInitializersTest.dart:13: : field_ = 2 
/// 02: compile-time error
Ditto.

[codefu, 2012-01-04 15:18:49]

Adding leg status changes as well.

http://codereview.chromium.org/9081001/diff/1/tests/language/src/ConstructorD...
File tests/language/src/ConstructorDuplicateInitializersTest.dart (right):

http://codereview.chromium.org/9081001/diff/1/tests/language/src/ConstructorD...
tests/language/src/ConstructorDuplicateInitializersTest.dart:10: , field_ = 2 
/// 01: compile-time error
On 2012/01/03 22:57:12, ahe wrote:
> I suggest adding a comment above explaining what the error is.

Done.

http://codereview.chromium.org/9081001/diff/1/tests/language/src/ConstructorD...
tests/language/src/ConstructorDuplicateInitializersTest.dart:13: : field_ = 2 
/// 02: compile-time error
On 2012/01/03 22:57:12, ahe wrote:
> Ditto.

Done.

[scheglov, 2012-01-04 15:47:26]

lgtm with nits

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:180: private
Set<String> finalsNeedingInitializing = Sets.newHashSet();
Do we have already tests for field missing initializer?

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:538:
onError(parameter, ResolverErrorCode.DUPLICATE_PARAMETER,parameter.getName());
Space missing in "PARAMETER,parameter"

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java
(right):

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:48:
FINAL_FIELD_MUST_BE_INITIALIZED("The final field %s must be initialized"),
These error constants should be sorted alphabetically.

[codefu, 2012-01-04 15:53:11]

Nits taken care of.

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:180: private
Set<String> finalsNeedingInitializing = Sets.newHashSet();
On 2012/01/04 15:47:27, scheglov wrote:
> Do we have already tests for field missing initializer?

Yes. co19:
LangGuideTest/02_Language_Constructs/02_1_Class/02_1_Class_Construction/A03/t01
Which we now pass.

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:538:
onError(parameter, ResolverErrorCode.DUPLICATE_PARAMETER,parameter.getName());
On 2012/01/04 15:47:27, scheglov wrote:
> Space missing in "PARAMETER,parameter"

Done. and "if (!"

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java
(right):

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:48:
FINAL_FIELD_MUST_BE_INITIALIZED("The final field %s must be initialized"),
On 2012/01/04 15:47:27, scheglov wrote:
> These error constants should be sorted alphabetically.

Done.

[ahe, 2012-01-04 15:57:45]

Still LGTM!

[zundel, 2012-01-04 16:30:38]

LGTM, just a couple of nits

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:537:
if(!initalizedFinals.add(parameter.getParameterName())) {
space after if

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:562: // Test for
missing final initialized variables
I would say 'initialized fields' instead of variables.

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:564: &&
initalizedFinals.size() != this.finalsNeedingInitializing.size()) {
I don't think there's a bug here, but I'd feel more comforatable with
initializedFinals.containsAll(finalsNeedingInitializing)

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:565: for(String
field : this.finalsNeedingInitializing) {
missing space after for

[codefu, 2012-01-04 17:02:44]

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:562: // Test for
missing final initialized variables
On 2012/01/04 16:30:39, zundel wrote:
> I would say 'initialized fields' instead of variables.

Done.

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:564: &&
initalizedFinals.size() != this.finalsNeedingInitializing.size()) {
On 2012/01/04 16:30:39, zundel wrote:
> I don't think there's a bug here, but I'd feel more comforatable with
> initializedFinals.containsAll(finalsNeedingInitializing)

Switched to !finalsNeedingInitializing.equals(initalizedFinals)

http://codereview.chromium.org/9081001/diff/5001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:565: for(String
field : this.finalsNeedingInitializing) {
On 2012/01/04 16:30:39, zundel wrote:
> missing space after for

Done.