Use "Object" as the reified type of covariant override parameters.

Use "Object" as the reified type of covariant override parameters.

This ensures the runtime type is of the tear-off expression's static
type, even when accessed through a superclass.

I also moved the two tests into a subdirectory so they are easier to
find. (It is inscrutable to me why we would have a single directory
with 1,868 test files in it.) That involved enabling recursive
directory listing in codegen_test, which exposed a couple of things.

Fix #27484.

R=leafp@google.com

Committed: https://github.com/dart-lang/sdk/commit/590a1880663cfa92494000edba4fba8e2e6cb08e

[Bob Nystrom, 2017-01-17 21:22:37]

rnystrom@google.com changed reviewers:
+ brianwilkerson@google.com, leafp@google.com

[Bob Nystrom, 2017-01-17 21:22:37]

I'm not crazy about the name "getReifiedType()", so let me know if you have any
other ideas.

[Leaf, 2017-01-17 21:59:23]

https://codereview.chromium.org/2641543003/diff/1/pkg/analyzer/lib/dart/eleme...
File pkg/analyzer/lib/dart/element/type.dart (right):

https://codereview.chromium.org/2641543003/diff/1/pkg/analyzer/lib/dart/eleme...
pkg/analyzer/lib/dart/element/type.dart:259: FunctionType
getReifiedType(DartType objectType);
Should this really be on the type?  It feels wrong.  The way this feature is
specified, it's not really a property of types, it's a property of methods.  It
feels like it should probably be on the element model, rather than the type
model, and I think that would work fine with the way it's implemented and used.

[Brian Wilkerson, 2017-01-17 22:15:32]

https://codereview.chromium.org/2641543003/diff/1/pkg/analyzer/lib/dart/eleme...
File pkg/analyzer/lib/dart/element/type.dart (right):

https://codereview.chromium.org/2641543003/diff/1/pkg/analyzer/lib/dart/eleme...
pkg/analyzer/lib/dart/element/type.dart:259: FunctionType
getReifiedType(DartType objectType);
I agree that MethodElement would be better, but I have to ask: is this method
likely to be of use to any client other than ddc? If not, then perhaps it should
just be a utility method in ddc rather than a part of the public API for
analyzer.

[Bob Nystrom, 2017-01-18 00:46:03]

On 2017/01/17 22:15:32, Brian Wilkerson wrote:
>
https://codereview.chromium.org/2641543003/diff/1/pkg/analyzer/lib/dart/eleme...
> File pkg/analyzer/lib/dart/element/type.dart (right):
> 
>
https://codereview.chromium.org/2641543003/diff/1/pkg/analyzer/lib/dart/eleme...
> pkg/analyzer/lib/dart/element/type.dart:259: FunctionType
> getReifiedType(DartType objectType);
> I agree that MethodElement would be better, but I have to ask: is this method
> likely to be of use to any client other than ddc?

Probably not.

> If not, then perhaps it should
> just be a utility method in ddc rather than a part of the public API for
> analyzer.

The implementation needs access to ParameterElementImpl.synthetic() and
FunctionTypeImpl.synthetic() which are private to analyzer, right?

I can put it on MethodElement if you want, but I tried that and I think the code
ends up looking worse.

Inside DDC, I have to check that the element I'm looking at is a MethodElement
instead of PropertyAccessorElement, etc.

      var reifiedType = element is MethodElement
          ? element.getReifiedType(objectType)
          : element.type;

...

      // Don't add redundant signatures for inherited unchanged methods.
      var inheritedElement = lookup(name, currentLibrary);
      if (inheritedElement != null &&
          (inheritedElement is! MethodElement ||
              inheritedElement.getReifiedType(objectType) == reifiedType)) {
        continue;
      }

Meanwhile, the implementation of getReifiedType() doesn't actually access any
state on the MethodElement itself, except for its type. getReifiedType() doesn't
need anything except a list of a ParameterElements and a return type, and
FunctionType provides those. While the operation may not be meaningful for all
uses of FunctionType, it's still well-defined and only requires state available
on FunctionType itself.

I'll put it there if you want, but it doesn't seem to add any value. In general,
I try to put methods on as "low" of an object as possible for the data the
operation needs. It would be a real drag if we put this on MethodElement and
then later someone found themselves needing that method but only having a
FunctionType in hand. They would have all the data they need, but couldn't
perform the operation they wanted because the method is hung on a higher-level
class.

[Leaf, 2017-01-18 01:23:37]

> > I agree that MethodElement would be better, but I have to ask: is this
method
> > likely to be of use to any client other than ddc?
> 
> Probably not.

Maybe kernel?  Unclear at this point though whether they will re-use or
re-implement.

> Meanwhile, the implementation of getReifiedType() doesn't actually access any
> state on the MethodElement itself, except for its type. getReifiedType()
doesn't
> need anything except a list of a ParameterElements and a return type, and
> FunctionType provides those. While the operation may not be meaningful for all

No, FunctionType provides an element, which provides those.  That's why it
seemed weird to put this on FunctionType, since it's only meaningful in terms of
the underlying element.

> uses of FunctionType, it's still well-defined and only requires state
available
> on FunctionType itself.
> 
> I'll put it there if you want, but it doesn't seem to add any value. In
general,
> I try to put methods on as "low" of an object as possible for the data the
> operation needs. It would be a real drag if we put this on MethodElement and
> then later someone found themselves needing that method but only having a
> FunctionType in hand. They would have all the data they need, but couldn't
> perform the operation they wanted because the method is hung on a higher-level
> class.

If they have a FunctionType which doesn't have an element, they already can't
perform the operation they want to.  Otherwise, they can get the element and do
what they want.

I'm not dead set on it, but it definitely feels odd to me to ask a function type
for its reified type when this isn't a property of types.

[Bob Nystrom, 2017-01-18 01:30:40]

On 2017/01/18 01:23:37, Leaf wrote:
> > > I agree that MethodElement would be better, but I have to ask: is this
> method
> > > likely to be of use to any client other than ddc?
> > 
> > Probably not.
> 
> Maybe kernel?  Unclear at this point though whether they will re-use or
> re-implement.
> 
> > Meanwhile, the implementation of getReifiedType() doesn't actually access
any
> > state on the MethodElement itself, except for its type. getReifiedType()
> doesn't
> > need anything except a list of a ParameterElements and a return type, and
> > FunctionType provides those. While the operation may not be meaningful for
all
> 
> No, FunctionType provides an element, which provides those.  That's why it
> seemed weird to put this on FunctionType, since it's only meaningful in terms
of
> the underlying element.
> 
> > uses of FunctionType, it's still well-defined and only requires state
> available
> > on FunctionType itself.
> > 
> > I'll put it there if you want, but it doesn't seem to add any value. In
> general,
> > I try to put methods on as "low" of an object as possible for the data the
> > operation needs. It would be a real drag if we put this on MethodElement and
> > then later someone found themselves needing that method but only having a
> > FunctionType in hand. They would have all the data they need, but couldn't
> > perform the operation they wanted because the method is hung on a
higher-level
> > class.
> 
> If they have a FunctionType which doesn't have an element, they already can't
> perform the operation they want to.  Otherwise, they can get the element and
do
> what they want.

Ah, that's a good point.

I never quite understood why FunctionType has an Element in the first place, but
given that it does and that it delegates to that for its parameters, it makes
sense to just move this there.

Done!

[Leaf, 2017-01-18 01:51:45]

https://codereview.chromium.org/2641543003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/member.dart (right):

https://codereview.chromium.org/2641543003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/member.dart:606: FunctionType
getReifiedType(DartType objectType) => baseElement.getReifiedType(objectType);
long line?

https://codereview.chromium.org/2641543003/diff/20001/pkg/dev_compiler/lib/js...
File pkg/dev_compiler/lib/js/amd/dart_sdk.js (right):

https://codereview.chromium.org/2641543003/diff/20001/pkg/dev_compiler/lib/js...
pkg/dev_compiler/lib/js/amd/dart_sdk.js:7214: +      getters: () => ({iterator:
dart.definiteFunctionType(core.Iterator$(E), [])}),
I think this is wrong.  Actually, interestingly, it's right, but for the wrong
reason.

https://codereview.chromium.org/2641543003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2641543003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1835: (inheritedElement
is! MethodElement ||
Is this right?  You're not checking that the type is the same for getters and
setters any more, right?

[Bob Nystrom, 2017-01-18 21:42:52]

OK, should be better now. Sorry about that.

That's what I get for trying to get a fix out right before I leave the office
and in the middle of working on some other stuff.

https://codereview.chromium.org/2641543003/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/member.dart (right):

https://codereview.chromium.org/2641543003/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/member.dart:606: FunctionType
getReifiedType(DartType objectType) => baseElement.getReifiedType(objectType);
On 2017/01/18 01:51:45, Leaf wrote:
> long line?

Done.

https://codereview.chromium.org/2641543003/diff/20001/pkg/dev_compiler/lib/js...
File pkg/dev_compiler/lib/js/amd/dart_sdk.js (right):

https://codereview.chromium.org/2641543003/diff/20001/pkg/dev_compiler/lib/js...
pkg/dev_compiler/lib/js/amd/dart_sdk.js:7214: +      getters: () => ({iterator:
dart.definiteFunctionType(core.Iterator$(E), [])}),
On 2017/01/18 01:51:45, Leaf wrote:
> I think this is wrong.  Actually, interestingly, it's right, but for the wrong
> reason.  

Done.

https://codereview.chromium.org/2641543003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2641543003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1835: (inheritedElement
is! MethodElement ||
On 2017/01/18 01:51:45, Leaf wrote:
> Is this right?  You're not checking that the type is the same for getters and
> setters any more, right?

Oops, fixed. Sorry.

[Leaf, 2017-01-18 22:23:04]

lgtm

https://codereview.chromium.org/2641543003/diff/40001/tests/language_strong/c...
File tests/language_strong/covariant_override/tear_off_type_test.dart (right):

https://codereview.chromium.org/2641543003/diff/40001/tests/language_strong/c...
tests/language_strong/covariant_override/tear_off_type_test.dart:97: class Mixed
extends Superclass with Mixin1, Mixin2 implements Interface1, Interface2 {}
long line.

[Bob Nystrom, 2017-01-18 22:37:29]

https://codereview.chromium.org/2641543003/diff/40001/tests/language_strong/c...
File tests/language_strong/covariant_override/tear_off_type_test.dart (right):

https://codereview.chromium.org/2641543003/diff/40001/tests/language_strong/c...
tests/language_strong/covariant_override/tear_off_type_test.dart:97: class Mixed
extends Superclass with Mixin1, Mixin2 implements Interface1, Interface2 {}
On 2017/01/18 22:23:04, Leaf wrote:
> long line.

Done.

[Bob Nystrom, 2017-01-18 22:48:46]

Description was changed from

==========
Use "Object" as the reified type of covariant override parameters.

This ensures the runtime type is of the tear-off expression's static
type, even when accessed through a superclass.

I also moved the two tests into a subdirectory so they are easier to
find. (It is inscrutable to me why we would have a single directory
with 1,868 test files in it.) That involved enabling recursive
directory listing in codegen_test, which exposed a couple of things.

Fix #27484.
==========

to

==========
Use "Object" as the reified type of covariant override parameters.

This ensures the runtime type is of the tear-off expression's static
type, even when accessed through a superclass.

I also moved the two tests into a subdirectory so they are easier to
find. (It is inscrutable to me why we would have a single directory
with 1,868 test files in it.) That involved enabling recursive
directory listing in codegen_test, which exposed a couple of things.

Fix #27484.

R=leafp@google.com

Committed:
https://github.com/dart-lang/sdk/commit/590a1880663cfa92494000edba4fba8e2e6cb08e
==========

[Bob Nystrom, 2017-01-18 22:48:48]

Committed patchset #4 (id:60001) manually as
590a1880663cfa92494000edba4fba8e2e6cb08e (presubmit successful).