Add named constructor name checking

Add named constructor name checking

Named constructors may not clash with any other class members.
Add a check but enable it only when --construcotr_name_check
runtime flag is passed to the VM.

Eliminate one named constructor in Process class in core library,
as well as a couple of cases in dart2js.

The dart2js team may want to rename the constructors I changed.

Issue 3990  (http://code.google.com/p/dart/issues/detail?id=3990)
 
Committed: https://code.google.com/p/dart/source/detail?r=11972

[hausner, 2012-09-05 21:50:51]

I propose to send out a BREAKING CHANGE message soon and then enable the check
by default. Meanwhile, people can fix their code if needed and enable the check
manually.

[hausner, 2012-09-05 22:09:09]

Updated changelist. Turns out Kasper already fixed the dart2js problem.

[ahe, 2012-09-06 08:54:26]

LGTM

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
File tests/language/bad_constructor_test.dart (right):

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
tests/language/bad_constructor_test.dart:4: //
VMOptions=--constructor_name_check
Please don't add VM specific options to shared language tests.

[Ivan Posva, 2012-09-06 09:12:50]

LGTM

-Ivan

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
File tests/language/bad_constructor_test.dart (right):

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
tests/language/bad_constructor_test.dart:4: //
VMOptions=--constructor_name_check
On 2012/09/06 08:54:26, ahe wrote:
> Please don't add VM specific options to shared language tests.

Would you rather have us break all of the users by not having the flag? We could
also print warnings by default instead.

[ahe, 2012-09-06 09:32:59]

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
File tests/language/bad_constructor_test.dart (right):

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
tests/language/bad_constructor_test.dart:4: //
VMOptions=--constructor_name_check
On 2012/09/06 09:12:50, Ivan Posva wrote:
> On 2012/09/06 08:54:26, ahe wrote:
> > Please don't add VM specific options to shared language tests.
> 
> Would you rather have us break all of the users by not having the flag? We
could
> also print warnings by default instead.

That's a strange trade-off. You can have a VM-specific test suite. We do that
for dart2js.

[Ivan Posva, 2012-09-06 10:39:01]

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
File tests/language/bad_constructor_test.dart (right):

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
tests/language/bad_constructor_test.dart:4: //
VMOptions=--constructor_name_check
On 2012/09/06 09:32:59, ahe wrote:
> On 2012/09/06 09:12:50, Ivan Posva wrote:
> > On 2012/09/06 08:54:26, ahe wrote:
> > > Please don't add VM specific options to shared language tests.
> > 
> > Would you rather have us break all of the users by not having the flag? We
> could
> > also print warnings by default instead.
> 
> That's a strange trade-off. You can have a VM-specific test suite. We do that
> for dart2js. 

I think it makes perfect sense to have the tests that were added below in this
shared language test. Unless you pass the flag these will fail while we accept
the old behaviour, so I expect the flag to be removed once we have flipped the
default.

[hausner, 2012-09-06 16:18:37]

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
File tests/language/bad_constructor_test.dart (right):

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
tests/language/bad_constructor_test.dart:4: //
VMOptions=--constructor_name_check
Yes, this flag goes away as soon as we enable the check by default.

There are other VM-specific flags in the language suite, this is not a first.
The test itself is not VM-specific, it applies to dart2js as well.

Another option to eliminate the flag and break some existing code.


On 2012/09/06 10:39:01, Ivan Posva wrote:
> On 2012/09/06 09:32:59, ahe wrote:
> > On 2012/09/06 09:12:50, Ivan Posva wrote:
> > > On 2012/09/06 08:54:26, ahe wrote:
> > > > Please don't add VM specific options to shared language tests.
> > > 
> > > Would you rather have us break all of the users by not having the flag? We
> > could
> > > also print warnings by default instead.
> > 
> > That's a strange trade-off. You can have a VM-specific test suite. We do
that
> > for dart2js. 
> 
> I think it makes perfect sense to have the tests that were added below in this
> shared language test. Unless you pass the flag these will fail while we accept
> the old behaviour, so I expect the flag to be removed once we have flipped the
> default.

[siva, 2012-09-06 16:52:10]

I believe the case we discussed offline needs to be fixed also.

http://codereview.chromium.org/10905109/diff/4001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/10905109/diff/4001/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:2416: }
As discussed offline how do we report errors for the case of a field or function
appearing after the named constructor?

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
File tests/language/bad_constructor_test.dart (right):

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
tests/language/bad_constructor_test.dart:27: A.foo() : m = 0;  /// 06:
compile-time error

We should add a test where the named constructor and field are flipped, e.g:

A.error() : ...
var error;  /// 07: compile-time error

[hausner, 2012-09-06 18:02:13]

Thanks for catching the error.

http://codereview.chromium.org/10905109/diff/4001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/10905109/diff/4001/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:2416: }
On 2012/09/06 16:52:10, asiva wrote:
> As discussed offline how do we report errors for the case of a field or
function
> appearing after the named constructor?
Correct. We really should have a simpler way to record the names in a scope. I
moved the check so it's now performed after the class is parsed.

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
File tests/language/bad_constructor_test.dart (right):

http://codereview.chromium.org/10905109/diff/4001/tests/language/bad_construc...
tests/language/bad_constructor_test.dart:27: A.foo() : m = 0;  /// 06:
compile-time error
On 2012/09/06 16:52:10, asiva wrote:
> 
> We should add a test where the named constructor and field are flipped, e.g:
> 
> A.error() : ...
> var error;  /// 07: compile-time error

Done.