Compute FunctionTypeImpl type parameters lazily.

Compute FunctionTypeImpl type parameters lazily.

This will allow a FunctionTypeImpl to be constructed without having to
consult the underlying element, which is necessary to allow the
element model to be reconstituted from a summary.

R=brianwilkerson@google.com

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

[Paul Berry, 2016-01-05 00:53:38]

paulberry@google.com changed reviewers:
+ brianwilkerson@google.com, jmesserly@google.com

[Brian Wilkerson, 2016-01-05 16:22:43]

LGTM

https://codereview.chromium.org/1557213002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/type.dart (right):

https://codereview.chromium.org/1557213002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:178: * True of this type is the
result of instantiating type parameters (and thus
"of" --> "if"

[Jennifer Messerly, 2016-01-05 17:08:43]

I had hoped to eventually move this type in the other direction: a Plain Old
Data type that is simply a mapping of parameter types to return type, that
matches its conceptual notion, e.g. (int, String) -> int or <T>(T) -> T.

The laziness of FunctionTypeImpl and the fact that it is strongly coupled to an
element make it hard to perform computations in the type domain (as one wants to
do, e.g. during inference). Two function types can compare equal but be backed
by different elements, which can lead to error prone code if anyone ever looks
at the element. It also makes them hard to construct.

Essentially the invariant I had been working towards (that is reflected in the
current code) is that FunctionTypeImpl is constructed last, so by the time the
`element.type = new FunctionTypeImpl(element)` happens, the FunctionTypeImpl is
free to rely on a completed element.

I had hoped at the next point to make the parameter types and returnTypes eager
as well, which would let us remove the lazy {S/T} substitutions and simplify a
lot of the code as a result.

I don't have my head around why those properties would be problems for
summaries--as long as the parameters and return types of the Element (usually an
ExecutableElement) are deserialized first, then `element.type = new
FunctionTypeImpl(element)` would work.

One possibly happy middle ground here, would be to make the type field for those
elements lazy:

    FunctionType get type {
      if (_type == null) _type = new FunctionTypeImpl(this);
      return _type;
    }

Aside from removing the `.type = new FunctionTypeImpl(element)` lines that are
everywhere, it might get around the problem you're seeing with summaries.

Anyway just some design thoughts. Happy to discuss over VC. The code changes
look fine to me, so if this is blocking you feel free to land it. In that case
Leaf or I may try and refactor in a follow up if that works for you guys.

[Jennifer Messerly, 2016-01-05 17:20:01]

On 2016/01/05 17:08:43, John Messerly wrote:
> I had hoped to eventually move this type in the other direction: a Plain Old
> Data type that is simply a mapping of parameter types to return type, that
> matches its conceptual notion, e.g. (int, String) -> int or <T>(T) -> T.
> 
> The laziness of FunctionTypeImpl and the fact that it is strongly coupled to
an
> element make it hard to perform computations in the type domain (as one wants
to
> do, e.g. during inference). Two function types can compare equal but be backed
> by different elements, which can lead to error prone code if anyone ever looks
> at the element. It also makes them hard to construct.
> 
> Essentially the invariant I had been working towards (that is reflected in the
> current code) is that FunctionTypeImpl is constructed last, so by the time the
> `element.type = new FunctionTypeImpl(element)` happens, the FunctionTypeImpl
is
> free to rely on a completed element.
> 
> I had hoped at the next point to make the parameter types and returnTypes
eager
> as well, which would let us remove the lazy {S/T} substitutions and simplify a
> lot of the code as a result.
> 
> I don't have my head around why those properties would be problems for
> summaries--as long as the parameters and return types of the Element (usually
an
> ExecutableElement) are deserialized first, then `element.type = new
> FunctionTypeImpl(element)` would work.
> 
> One possibly happy middle ground here, would be to make the type field for
those
> elements lazy:
> 
>     FunctionType get type {
>       if (_type == null) _type = new FunctionTypeImpl(this);
>       return _type;
>     }
> 
> Aside from removing the `.type = new FunctionTypeImpl(element)` lines that are
> everywhere, it might get around the problem you're seeing with summaries.
> 
> Anyway just some design thoughts. Happy to discuss over VC. The code changes
> look fine to me, so if this is blocking you feel free to land it. In that case
> Leaf or I may try and refactor in a follow up if that works for you guys.

A few more thoughts here...

Right now FunctionTypeImpl pretty strongly reflects the Element. You could
almost say it's really just the Element but a different view on it. They are
very strongly coupled. FunctionType can't answer any questions about itself
without asking its Element.

I had hoped to eventually loosen the coupling, so we can make free floating
function types.

That said -- maybe FunctionTypeImpl is not the right place. Perhaps the way to
attack function types is to leave FunctionTypeImpl joined at the hip to its
Element, and introduce a new type, that implements FunctionType but has a null
element, and implements all of its properties in simpler ways, and remove the
assumptions in the code that FunctionType can be downcast to FunctionTypeImpl
always. So maybe we'd end up with FunctionTypeImpl and
ElementBackedFunctionTypeImpl or something like that.

Aside -- given that FunctionTypes contain no information in most cases, I would
expect that they aren't really stored in summaries? Things that result from
instantiation (either a generic class or method) I would expect to not be
reflected in summaries, the "interesting" FunctionTypeImpls (ones with non
trivial typeArguments) only appear as a result of resolving Expressions. Maybe
that leaks via constants or something like that?

Sorry if this is too much text for a CL, happy to discuss this offline. :)
And definitely land if you need it, we can keep iterating on Function Types.
Summaries (\o/ !!!) are more important.

[Paul Berry, 2016-01-05 20:05:27]

Description was changed from

==========
Compute FunctionTypeImpl type parameters lazily.

This will allow a FunctionTypeImpl to be constructed without having to
consult the underlying element, which is necessary to allow the
element model to be reconstituted from a summary.
==========

to

==========
Compute FunctionTypeImpl type parameters lazily.

This will allow a FunctionTypeImpl to be constructed without having to
consult the underlying element, which is necessary to allow the
element model to be reconstituted from a summary.

R=brianwilkerson@google.com

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

[Paul Berry, 2016-01-05 20:05:28]

Committed patchset #1 (id:1) manually as
ed783f3caa5dcd6b20e59f75c77f014f73af3b10 (presubmit successful).