Do not use erroneouselement unless the member is static

Do not use erroneouselement unless the member is static

R=het@google.com, johnniwinther@google.com

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

[Siggi Cherem (dart-lang), 2015-04-15 01:21:36]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-04-15 01:38:04]

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

[Siggi Cherem (dart-lang), 2015-04-15 01:38:04]

https://codereview.chromium.org/1089463003/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/resolver_test.dart (right):

https://codereview.chromium.org/1089463003/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/resolver_test.dart:1189: ''',
[MessageKind.ASSIGNING_METHOD, MessageKind.SETTER_NOT_FOUND]);
I'm not happy with the second error (which is currently generated from
typechecker.dart), but I guess we can address that separately

[Johnni Winther, 2015-04-15 08:58:03]

lgtm

https://codereview.chromium.org/1089463003/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/resolver_test.dart (right):

https://codereview.chromium.org/1089463003/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/resolver_test.dart:1189: ''',
[MessageKind.ASSIGNING_METHOD, MessageKind.SETTER_NOT_FOUND]);
On 2015/04/15 01:38:04, Siggi Cherem (dart-lang) wrote:
> I'm not happy with the second error (which is currently generated from
> typechecker.dart), but I guess we can address that separately

Actually should not resolve [target] to an instance member (in ResolverVisitor)
and therefore only observe the case in typechecker.dart (where a better message
could be used).

[Harry Terkelsen, 2015-04-15 18:22:34]

het@google.com changed reviewers:
+ het@google.com

[Harry Terkelsen, 2015-04-15 18:22:34]

https://codereview.chromium.org/1089463003/diff/20001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/1089463003/diff/20001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3015: if
(Elements.isStaticOrTopLevelField(target)) {
this check causes this to print '2':

class A {
  foo() {
    final bar = 1;
    bar = 2;
    print(bar);
  }
}
main() => new A().foo();

[Siggi Cherem (dart-lang), 2015-04-15 22:39:46]

PTAL

https://codereview.chromium.org/1089463003/diff/20001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/1089463003/diff/20001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3015: if
(Elements.isStaticOrTopLevelField(target)) {
On 2015/04/15 18:22:34, Harry Terkelsen wrote:
> this check causes this to print '2':
> 
> class A {
>   foo() {
>     final bar = 1;
>     bar = 2;
>     print(bar);
>   }
> }
> main() => new A().foo();

Good catch Harry, this should also test for isLocal like I was doing below with
the local functions.

I added it here and added some tests for the warnings.

https://codereview.chromium.org/1089463003/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/resolver_test.dart (right):

https://codereview.chromium.org/1089463003/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/resolver_test.dart:1189: ''',
[MessageKind.ASSIGNING_METHOD, MessageKind.SETTER_NOT_FOUND]);
On 2015/04/15 08:58:03, Johnni Winther wrote:
> On 2015/04/15 01:38:04, Siggi Cherem (dart-lang) wrote:
> > I'm not happy with the second error (which is currently generated from
> > typechecker.dart), but I guess we can address that separately
> 
> Actually should not resolve [target] to an instance member (in
ResolverVisitor)
> and therefore only observe the case in typechecker.dart (where a better
message
> could be used).

Good point - I changed it so that the checker reports its error, but we don't
report one in the resolver in this case. Now all the tests have a single
warning.

[Harry Terkelsen, 2015-04-15 22:45:41]

lgtm

https://codereview.chromium.org/1089463003/diff/60001/tests/compiler/dart2js/...
File tests/compiler/dart2js/resolver_test.dart (right):

https://codereview.chromium.org/1089463003/diff/60001/tests/compiler/dart2js/...
tests/compiler/dart2js/resolver_test.dart:1241: // Detect assignemtns to final
fields:
s/assignemnts/assignments/

[Siggi Cherem (dart-lang), 2015-04-15 22:54:11]

https://codereview.chromium.org/1089463003/diff/60001/tests/compiler/dart2js/...
File tests/compiler/dart2js/resolver_test.dart (right):

https://codereview.chromium.org/1089463003/diff/60001/tests/compiler/dart2js/...
tests/compiler/dart2js/resolver_test.dart:1241: // Detect assignemtns to final
fields:
On 2015/04/15 22:45:41, Harry Terkelsen wrote:
> s/assignemnts/assignments/

Done. thx :)

[Siggi Cherem (dart-lang), 2015-04-15 23:10:14]

Committed patchset #4 (id:80001) manually as 45187 (presubmit successful).

[Johnni Winther, 2015-04-16 07:41:05]

LGTM with comment.

https://codereview.chromium.org/1089463003/diff/80001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/1089463003/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3025: if (node.isSuperCall)
registry.registerSuperNoSuchMethod();
This case should create an ErrenousElement since the call isn't using dynamic
dispatch. The SSA backend probably detects this case (I guess we have tests for
it) but it should have been told to produce the NSM.

[Siggi Cherem (dart-lang), 2015-04-16 15:49:45]

https://codereview.chromium.org/1089463003/diff/80001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/1089463003/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3025: if (node.isSuperCall)
registry.registerSuperNoSuchMethod();
On 2015/04/16 07:41:05, Johnni Winther wrote:
> This case should create an ErrenousElement since the call isn't using dynamic
> dispatch. The SSA backend probably detects this case (I guess we have tests
for
> it) but it should have been told to produce the NSM.

Interesting - I'll create a follow up CL with this (I submitted this one
yesterday).

Would that be true also when the "this" receiver is explicit
(node.receiver.isThis())?

[Siggi Cherem (dart-lang), 2015-04-16 19:58:15]

https://codereview.chromium.org/1089463003/diff/80001/pkg/compiler/lib/src/re...
File pkg/compiler/lib/src/resolution/members.dart (right):

https://codereview.chromium.org/1089463003/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3025: if (node.isSuperCall)
registry.registerSuperNoSuchMethod();
On 2015/04/16 15:49:44, Siggi Cherem (dart-lang) wrote:
> On 2015/04/16 07:41:05, Johnni Winther wrote:
> > This case should create an ErrenousElement since the call isn't using
dynamic
> > dispatch. The SSA backend probably detects this case (I guess we have tests
> for
> > it) but it should have been told to produce the NSM.
> 
> Interesting - I'll create a follow up CL with this (I submitted this one
> yesterday).
> 
> Would that be true also when the "this" receiver is explicit
> (node.receiver.isThis())?

I tried this out, but I think this makes the errror message for `super.x` a bit
worse. It makes us report the error here (cannot-resolve-setter or
assigning-method) rather than the typechecker error (setter-not-found-in-class)
which includes the class where we are looking up the member. (see
https://codereview.chromium.org/1090923004 for the tentative change).

Thoughts?