Ensure that ClassElement.lookupConstructor fails when looking up default constructor using Selector…

Ensure that ClassElement.lookupConstructor fails when looking up default constructor using Selector.callConstructor(), forcing client to use Selector.callDefaultConstructor() instead.

BUG=dartbug.com/5834
TEST=tests/compiler/dart2js/resolver_test.dart

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

[aam-me, 2012-10-15 01:22:20]

Kasper,

  is this what you had in mind for dartbug.com/5834?

Thanks!

[kasperl, 2012-10-15 07:26:38]

Yeah. This is definitely the direction I was heading in. Comments:

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

https://chromiumcodereview.appspot.com/11140018/diff/2001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:92: SourceString
getConstructorName(Send node, SourceString className) {
Maybe this helper could give you the selector instead of the name and you could
compute the constructorName from that where you need it?

https://chromiumcodereview.appspot.com/11140018/diff/2001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:129: SourceString constructorName =
getConstructorName(initializers.head, classElement.name);
Long line.

https://chromiumcodereview.appspot.com/11140018/diff/2001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:776: constructorName,
Isn't the constructor name always in lookupSelector.name? Maybe you don't
actually need the constructor name here anymore if you change the
getConstructorName helper to return the selector instead.

[aam-me, 2012-10-15 12:07:52]

Thanks for the review!

PTAL.

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

https://chromiumcodereview.appspot.com/11140018/diff/2001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:92: SourceString
getConstructorName(Send node, SourceString className) {
On 2012/10/15 07:26:38, kasperl wrote:
> Maybe this helper could give you the selector instead of the name and you
could
> compute the constructorName from that where you need it?

Done.

https://chromiumcodereview.appspot.com/11140018/diff/2001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:129: SourceString constructorName =
getConstructorName(initializers.head, classElement.name);
On 2012/10/15 07:26:38, kasperl wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/11140018/diff/2001/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:776: constructorName,
On 2012/10/15 07:26:38, kasperl wrote:
> Isn't the constructor name always in lookupSelector.name? Maybe you don't
> actually need the constructor name here anymore if you change the
> getConstructorName helper to return the selector instead.

Done.

[kasperl, 2012-10-15 12:14:12]

LGTM but I think you can simplify it a bit more.

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

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:92: Selector
getConstructorSelector(Send node, SourceString className, ResolverVisitor
visitor) {
Long line.

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:95: constructorName = const
SourceString("");
Why don't you just return the selector here? It feels weird to have another
switch on the constructor name later.

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:98: if (constructorName.slowToString()
== className.slowToString()) {
Add a comment that explains when this happens? This is a named constructor so
why do you need to deal with the case where the constructor name is the same as
the class name?

What happens for code like this? 

  class A {
    A.A();
  }

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:102: Selector selector;
I would get rid of this part and just put the returns in the places where you
compute the constructor name. That way you can also get rid of the
constructorName local variable.

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:115: SourceString constructorName) {
Weird indentation.

[aam-me, 2012-10-16 04:09:32]

Thank you, Kasper, for the review.
Please, see comments below.

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

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:92: Selector
getConstructorSelector(Send node, SourceString className, ResolverVisitor
visitor) {
On 2012/10/15 12:14:12, kasperl wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:95: constructorName = const
SourceString("");
On 2012/10/15 12:14:12, kasperl wrote:
> Why don't you just return the selector here? It feels weird to have another
> switch on the constructor name later.

Done.

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:98: if (constructorName.slowToString()
== className.slowToString()) {
You are right, Kasper. The code above is wrong. It doesn't properly handle
"this.C()" in redirecting constructors: when constructor name matches class
name, it gets resolved to default constructor instead of named constructor.

I added tests\compiler\dart2js_extra\this_redirecting_constructor_test.dart to
cover that.

I'm afraid I introduced this error as part of recent
https://chromiumcodereview.appspot.com/10947024 it seems.

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:102: Selector selector;
On 2012/10/15 12:14:12, kasperl wrote:
> I would get rid of this part and just put the returns in the places where you
> compute the constructor name. That way you can also get rid of the
> constructorName local variable.

Done.

https://chromiumcodereview.appspot.com/11140018/diff/6/lib/compiler/implement...
lib/compiler/implementation/resolver.dart:115: SourceString constructorName) {
On 2012/10/15 12:14:12, kasperl wrote:
> Weird indentation.

Done.

[kasperl, 2012-10-16 05:24:02]

LGTM. Looks even better to me.

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

https://chromiumcodereview.appspot.com/11140018/diff/3005/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:94: SourceString className,
Unused parameter.

https://chromiumcodereview.appspot.com/11140018/diff/3005/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:96: if (!isNamedConstructor(node)) {
Maybe invert this condition (get rid of !) so the positive case comes first?
Seems easier to understand that way.

[aam-me, 2012-10-16 12:07:49]

Thank you for the comments, Kasper.

Please take another look when you have a chance.

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

https://chromiumcodereview.appspot.com/11140018/diff/3005/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:94: SourceString className,
Quite amazing! Done.

https://chromiumcodereview.appspot.com/11140018/diff/3005/lib/compiler/implem...
lib/compiler/implementation/resolver.dart:96: if (!isNamedConstructor(node)) {
Agree. Done.

[kasperl, 2012-10-16 12:19:18]

LGTM.

[ahe, 2012-10-16 13:56:08]

LGTM

https://chromiumcodereview.appspot.com/11140018/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/11140018/diff/13001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:94: ResolverVisitor visitor) {
Passing in the ResolverVisitor to get the current element seems weird. Perhaps
this method and resolveConstructorRedirection doesn't belong here and belong in
the ResolverVisitor?

https://chromiumcodereview.appspot.com/11140018/diff/13001/tests/co19/co19-da...
File tests/co19/co19-dart2js.status (right):

https://chromiumcodereview.appspot.com/11140018/diff/13001/tests/co19/co19-da...
tests/co19/co19-dart2js.status:247:
Language/07_Classes/6_Constructors/1_Generative_Constructors_A16_t07: Pass #
co19 issue 259
Please move this to line 413.

https://chromiumcodereview.appspot.com/11140018/diff/13001/tests/compiler/dar...
File tests/compiler/dart2js_extra/this_redirecting_constructor_test.dart
(right):

https://chromiumcodereview.appspot.com/11140018/diff/13001/tests/compiler/dar...
tests/compiler/dart2js_extra/this_redirecting_constructor_test.dart:10: C.C() {
x = 2; }
I think this should be an error. I'll file a specification bug.

[aam-me, 2012-10-17 01:52:04]

Thank you, Peter, for the review.
Please have another look when you have a chance.

https://chromiumcodereview.appspot.com/11140018/diff/13001/lib/compiler/imple...
File lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/11140018/diff/13001/lib/compiler/imple...
lib/compiler/implementation/resolver.dart:94: ResolverVisitor visitor) {
On 2012/10/16 13:56:08, ahe wrote:
> Passing in the ResolverVisitor to get the current element seems weird. Perhaps
> this method and resolveConstructorRedirection doesn't belong here and belong
in
> the ResolverVisitor?

Done.

https://chromiumcodereview.appspot.com/11140018/diff/13001/tests/co19/co19-da...
File tests/co19/co19-dart2js.status (right):

https://chromiumcodereview.appspot.com/11140018/diff/13001/tests/co19/co19-da...
tests/co19/co19-dart2js.status:247:
Language/07_Classes/6_Constructors/1_Generative_Constructors_A16_t07: Pass #
co19 issue 259
On 2012/10/16 13:56:08, ahe wrote:
> Please move this to line 413.

Done.

https://chromiumcodereview.appspot.com/11140018/diff/13001/tests/compiler/dar...
File tests/compiler/dart2js_extra/this_redirecting_constructor_test.dart
(right):

https://chromiumcodereview.appspot.com/11140018/diff/13001/tests/compiler/dar...
tests/compiler/dart2js_extra/this_redirecting_constructor_test.dart:10: C.C() {
x = 2; }
Interesting. That didn't cross my mind.

[aam-me, 2012-10-25 00:52:28]

Ping.

[ahe, 2012-10-25 13:57:48]

LGTM!

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
File tests/co19/co19-dart2dart.status (left):

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
tests/co19/co19-dart2dart.status:83: LibTest/math/exp_A01_t01: Fail #
TODO(dart2dart-team): Please triage this failure.
Did you fix this, or did you test this on Windows?

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
File tests/co19/co19-dart2dart.status (right):

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
tests/co19/co19-dart2dart.status:88: LibTest/math/atan2_A01_t03: Fail #
TODO(dart2dart-team): Please triage this failure.
Ditto?

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
File tests/co19/co19-dart2js.status (right):

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
tests/co19/co19-dart2js.status:189:
Language/07_Classes/6_Constructors/1_Generative_Constructors_A16_t07: Fail #
dartbug.com/6242
Please add "http://".

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
tests/co19/co19-dart2js.status:189:
Language/07_Classes/6_Constructors/1_Generative_Constructors_A16_t07: Fail #
dartbug.com/6242
Please move this status line to the section that starts a line 667 (missing
compile-time errors).

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/compiler/dar...
File tests/compiler/dart2js_extra/this_redirecting_constructor_test.dart
(right):

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/compiler/dar...
tests/compiler/dart2js_extra/this_redirecting_constructor_test.dart:5: //
Confirm that redirecting named constructors are properly resolved.
Please add a note that this test may become invalid due to
http://dartbug.com/5940.

[aam-me, 2012-10-26 03:56:10]

Hi, Peter,

  thank you for the review.
  I removed changes to the floating-point tests statuses from co19-dart2dart,
co19-dart2js.status-files. They are unrelated to lookupConstructor changes and
seem to be caused by difference in rounding precision b/w Win, Mac and Linux. I
can create separate CL with tests status updates.

  I incorporated rest of your suggestions.

  Please take another look!

Thanks.

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
File tests/co19/co19-dart2dart.status (left):

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
tests/co19/co19-dart2dart.status:83: LibTest/math/exp_A01_t01: Fail #
TODO(dart2dart-team): Please triage this failure.
It works on my Windows7 box. I also see that there are several more test fail on
Mac(atan_A01_t01, asin_A01_t01, acos_A01_t01).
I think it would be better if I keep status file changes that are not related to
my changes out of this changelist. Would you agree?

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
File tests/co19/co19-dart2dart.status (right):

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
tests/co19/co19-dart2dart.status:88: LibTest/math/atan2_A01_t03: Fail #
TODO(dart2dart-team): Please triage this failure.
Yes, for me tan_A01_t01 passes, but this atan2_A01_t03 fails.

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
File tests/co19/co19-dart2js.status (right):

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/co19/co19-da...
tests/co19/co19-dart2js.status:189:
Language/07_Classes/6_Constructors/1_Generative_Constructors_A16_t07: Fail #
dartbug.com/6242
On 2012/10/25 13:57:48, ahe wrote:
> Please add "http://".

Done.

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/compiler/dar...
File tests/compiler/dart2js_extra/this_redirecting_constructor_test.dart
(right):

https://chromiumcodereview.appspot.com/11140018/diff/28001/tests/compiler/dar...
tests/compiler/dart2js_extra/this_redirecting_constructor_test.dart:5: //
Confirm that redirecting named constructors are properly resolved.
On 2012/10/25 13:57:48, ahe wrote:
> Please add a note that this test may become invalid due to
> http://dartbug.com/5940.

Done.

[ahe, 2012-10-26 04:39:10]

LGTM

https://chromiumcodereview.appspot.com/11140018/diff/36001/lib/compiler/imple...
File lib/compiler/implementation/elements/elements.dart (left):

https://chromiumcodereview.appspot.com/11140018/diff/36001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:1485: if
(!identical(constructorName, const SourceString('')) &&
Actually, I think this should be ==, not identical.

[aam-me, 2012-10-26 10:59:55]

Thank you, Peter! Good point regarding SourceString comparison. Fixed that.

PTAL!

https://chromiumcodereview.appspot.com/11140018/diff/36001/lib/compiler/imple...
File lib/compiler/implementation/elements/elements.dart (left):

https://chromiumcodereview.appspot.com/11140018/diff/36001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:1485: if
(!identical(constructorName, const SourceString('')) &&
On 2012/10/26 04:39:10, ahe wrote:
> Actually, I think this should be ==, not identical. 

Done.

[ahe, 2012-10-26 11:08:53]

LGTM

https://chromiumcodereview.appspot.com/11140018/diff/45001/tests/compiler/dar...
File tests/compiler/dart2js/resolver_test.dart (right):

https://chromiumcodereview.appspot.com/11140018/diff/45001/tests/compiler/dar...
tests/compiler/dart2js/resolver_test.dart:572: if (constructor !== '') {
!=