Make invalid to override method names for local nested functions

Make invalid to override method names for local nested functions

BUG= http://dartbug.com/19129
R=johnniwinther@google.com

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

[Siggi Cherem (dart-lang), 2015-04-10 00:37:29]

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

[Siggi Cherem (dart-lang), 2015-04-10 00:40:32]

Patchset #1 (id:1) has been deleted

[Johnni Winther, 2015-04-10 11:49:43]

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

https://codereview.chromium.org/1073983003/diff/20001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:2504: name, node,
ElementKind.FUNCTION, modifiers, enclosingElement);
The .isFinal property means 'declaratively final' which local functions are not.

Investigating the issue I found that actually handled assignment to functions
but failed on local functions (and super functions) for obscure reasons.

I've sent you a patch you can use.

Try adding tests for assignment to local functions as well super functions (like
`super.func = 42`) to the unittest `tests/compiler/dart2js/resolver_test.dart'
to test the patch.

[Siggi Cherem (dart-lang), 2015-04-10 23:58:47]

Patchset #2 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2015-04-11 00:00:53]

Patchset #2 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2015-04-11 00:12:21]

Thanks Johnni, PTAL

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

https://codereview.chromium.org/1073983003/diff/20001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:2504: name, node,
ElementKind.FUNCTION, modifiers, enclosingElement);
On 2015/04/10 11:49:43, Johnni Winther wrote:
> The .isFinal property means 'declaratively final' which local functions are
not.
> 
> Investigating the issue I found that actually handled assignment to functions
> but failed on local functions (and super functions) for obscure reasons.

thanks, I was not very keen of this approach, but its good to confirm - from
working with analyzer, I am used to have some element synthesize information,
for example a getter with no setter implies a synthetic final variable. 



> I've sent you a patch you can use.
> 
> Try adding tests for assignment to local functions as well super functions
(like
> `super.func = 42`) to the unittest `tests/compiler/dart2js/resolver_test.dart'
> to test the patch.

Thanks. I've updated this CL with your patch and the extra tests

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

https://codereview.chromium.org/1073983003/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3002: assert(!target.isSetter);
in your original patch, you had this as part of the condition, but it seemed to
me like isFunction implies this, or am I missing something?

https://codereview.chromium.org/1073983003/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3006: if (node.isSuperCall)
registry.registerSuperNoSuchMethod();
without this, codegen may fail later on (createInvocationMirror is not built at
the right time).

I noticed that in some places you also do
`registry.registerDynamicInvoke(selector);` as well (e.g. in resolveSend). Here
it seems to me we don't need to because we call registerSend at the end of this
method. Is my understanding correct?

[Siggi Cherem (dart-lang), 2015-04-11 01:34:59]

FYI, this might not be entirely done. I ran all the co19 tests and run into one
curious test:

  tests/co19/src/Language/12_Expressions/18_Assignment_A05_t05.dart

The test has code of the form:

class C {
  noSuchMethod(i) => ...;

  test() {
    try {
      func = () => 1;
    }
  }
}


The assignment used to be compiled as:
  this.set$func(new B.C_test_closure());

but with this change we try to short-circuit and throw nSM directly:
  H.throwNoSuchMethod("", "func=", [new B.C_test_closure()], null);

as a result, C.noSuchMethod is not invoked.

Not sure if this is a bug in this change, or if I exposed a bug in how we infer
that a nSM is a forwarding nSM. I'll keep digging in to figure out what's going
on.

[Johnni Winther, 2015-04-13 09:41:02]

On 2015/04/11 01:34:59, Siggi Cherem (dart-lang) wrote:
> FYI, this might not be entirely done. I ran all the co19 tests and run into
one
> curious test:
> 
>   tests/co19/src/Language/12_Expressions/18_Assignment_A05_t05.dart
> 
> The test has code of the form:
> 
> class C {
>   noSuchMethod(i) => ...;
> 
>   test() {
>     try {
>       func = () => 1;
>     }
>   }
> }
> 
> 
> The assignment used to be compiled as:
>   this.set$func(new B.C_test_closure());
> 
> but with this change we try to short-circuit and throw nSM directly:
>   H.throwNoSuchMethod("", "func=", [new B.C_test_closure()], null);
> 
> as a result, C.noSuchMethod is not invoked.
> 
> Not sure if this is a bug in this change, or if I exposed a bug in how we
infer
> that a nSM is a forwarding nSM. I'll keep digging in to figure out what's
going
> on.

It seems you have revealed an existing bug. Land this CL and file a bug for this
test (cc herhut@ and het@).

[Johnni Winther, 2015-04-13 09:41:09]

LGTM

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

https://codereview.chromium.org/1073983003/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3002: assert(!target.isSetter);
On 2015/04/11 00:12:21, Siggi Cherem (dart-lang) wrote:
> in your original patch, you had this as part of the condition, but it seemed
to
> me like isFunction implies this, or am I missing something?

You're right - and I did document isFunction accordingly ;-)

https://codereview.chromium.org/1073983003/diff/80001/pkg/compiler/lib/src/re...
pkg/compiler/lib/src/resolution/members.dart:3006: if (node.isSuperCall)
registry.registerSuperNoSuchMethod();
On 2015/04/11 00:12:21, Siggi Cherem (dart-lang) wrote:
> without this, codegen may fail later on (createInvocationMirror is not built
at
> the right time).
> 
> I noticed that in some places you also do
> `registry.registerDynamicInvoke(selector);` as well (e.g. in resolveSend).
Here
> it seems to me we don't need to because we call registerSend at the end of
this
> method. Is my understanding correct?

Nice catch. And you're right about the selectors.

[Siggi Cherem (dart-lang), 2015-04-13 23:49:24]

On 2015/04/13 09:41:02, Johnni Winther wrote:
> On 2015/04/11 01:34:59, Siggi Cherem (dart-lang) wrote:
> > FYI, this might not be entirely done. I ran all the co19 tests and run into
> one
> > curious test:
> > 
> >   tests/co19/src/Language/12_Expressions/18_Assignment_A05_t05.dart
> > 
> > The test has code of the form:
> > 
> > class C {
> >   noSuchMethod(i) => ...;
> > 
> >   test() {
> >     try {
> >       func = () => 1;
> >     }
> >   }
> > }
> > 
> > 
> > The assignment used to be compiled as:
> >   this.set$func(new B.C_test_closure());
> > 
> > but with this change we try to short-circuit and throw nSM directly:
> >   H.throwNoSuchMethod("", "func=", [new B.C_test_closure()], null);
> > 
> > as a result, C.noSuchMethod is not invoked.
> > 
> > Not sure if this is a bug in this change, or if I exposed a bug in how we
> infer
> > that a nSM is a forwarding nSM. I'll keep digging in to figure out what's
> going
> > on.
> 
> It seems you have revealed an existing bug. Land this CL and file a bug for
this
> test (cc herhut@ and het@).

All set - dartbug.om/23193

[Siggi Cherem (dart-lang), 2015-04-13 23:50:05]

Committed patchset #3 (id:100001) manually as 45123 (presubmit successful).