tweak FunctionTypeImpl to support generic methods

tweak FunctionTypeImpl to support generic methods

The big change here is to ensure FunctionTypeImpl.typeArguments is always initialized to match the parameters. This was previously the case, because we'd explicitly use the class's type parameters. With generic methods, we can no longer rely exclusively on that source of type parameters.

We take advantage of enclosingElement to compute typeParameters from all containing scopes that can have them. By tracking all of the typeParameters, this allows substitution to work later.

Adds a few very basic tests. Please note that generic methods are not ready to use yet.

In particular this doesn't yet do the correct substitutions in StaticTypeAnalyzer. It also doesn't include (mostly done) inference to infer type parameters from the argument types. Further, discovered a missing feature from the previous parser CL. And finally, it doesn't include the kind of ErrorVerifier checks we'll likely need.

R=brianwilkerson@google.com, leafp@google.com

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

[Jennifer Messerly, 2015-11-12 21:04:19]

Description was changed from

==========
initialize FunctionTypeImpl.typeArguments to match the parameters
This is needed for substitution to work later. Currently they are initialized to
the type arguments of the containing classs. By initializing them to the
parameters, this lets us support cases where we have additional typeParameters,
such as generic methods.
==========

to

==========
initialize FunctionTypeImpl.typeArguments to match the parameters

This is needed for substitution to work later. Currently they are initialized to
the type arguments of the containing class. By initializing them using the
already-available typeParameters, this lets us support cases where we have
additional typeParameters, such as generic methods.
==========

[Jennifer Messerly, 2015-11-13 03:13:34]

Description was changed from

==========
initialize FunctionTypeImpl.typeArguments to match the parameters

This is needed for substitution to work later. Currently they are initialized to
the type arguments of the containing class. By initializing them using the
already-available typeParameters, this lets us support cases where we have
additional typeParameters, such as generic methods.
==========

to

==========
tweak FunctionTypeImpl to support generic methods

The big change here is to ensure FunctionTypeImpl.typeArguments is always
initialized to match the parameters. This was previously the case, because we'd
explicitly use the class's type parameters. With generic methods, we can no
longer rely exclusively on that source of arguments.

But tracking all of the typeParameters, this allows substitution to work later.

We take advantage of enclosingElement to compute typeParameters from all
containing scopes that can have them.

Adds a few very basic tests. Please note that generic methods are not ready to
use yet.

In particular this doesn't yet do the correct substitutions in
StaticTypeAnalyzer. It also doesn't include (mostly done) inference to infer
type parameters from the argument types. Further, discovered a missing feature
from the previous parser CL. And finally, it doesn't include the kind of
ErrorVerifier checks we'll likely need.
==========

[Jennifer Messerly, 2015-11-13 03:13:34]

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

[Jennifer Messerly, 2015-11-13 03:14:04]

Description was changed from

==========
tweak FunctionTypeImpl to support generic methods

The big change here is to ensure FunctionTypeImpl.typeArguments is always
initialized to match the parameters. This was previously the case, because we'd
explicitly use the class's type parameters. With generic methods, we can no
longer rely exclusively on that source of arguments.

But tracking all of the typeParameters, this allows substitution to work later.

We take advantage of enclosingElement to compute typeParameters from all
containing scopes that can have them.

Adds a few very basic tests. Please note that generic methods are not ready to
use yet.

In particular this doesn't yet do the correct substitutions in
StaticTypeAnalyzer. It also doesn't include (mostly done) inference to infer
type parameters from the argument types. Further, discovered a missing feature
from the previous parser CL. And finally, it doesn't include the kind of
ErrorVerifier checks we'll likely need.
==========

to

==========
tweak FunctionTypeImpl to support generic methods

The big change here is to ensure FunctionTypeImpl.typeArguments is always
initialized to match the parameters. This was previously the case, because we'd
explicitly use the class's type parameters. With generic methods, we can no
longer rely exclusively on that source of type parameters.

We take advantage of enclosingElement to compute typeParameters from all
containing scopes that can have them. By tracking all of the typeParameters,
this allows substitution to work later.

Adds a few very basic tests. Please note that generic methods are not ready to
use yet.

In particular this doesn't yet do the correct substitutions in
StaticTypeAnalyzer. It also doesn't include (mostly done) inference to infer
type parameters from the argument types. Further, discovered a missing feature
from the previous parser CL. And finally, it doesn't include the kind of
ErrorVerifier checks we'll likely need.
==========

[Jennifer Messerly, 2015-11-13 03:14:25]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2015-11-13 03:19:52]

BTW, most of the CL is covered by just not breaking anything that already exists
:). I'd actually staged it originally purely as refactoring typeArguments
initialization. 

But it seemed worth adding a few tests to illustrate the benefits: generic
methods basically work in the element model/type system.

[Brian Wilkerson, 2015-11-13 18:56:16]

LGTM

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/element.dart (right):

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:4853: List<DartType> _typeArguments
= DartType.EMPTY_LIST;
Not sure why you removed the comment. If it was wrong, could you add one that's
right? (I like to document everything, including private members.)

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:4902: List<TypeParameterElement>
typeParameters = this.typeParameters;
See TypeParameterTypeImpl.getTypes

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:10536: * For example, a class or a
typedef. This also includes functions methods if
nit: "functions" --> "function"

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong_mode.dart:30: // TODO(jmesserly): why is this
necessary? The function type should always
Good question! I don't know. Does anything break if we change this?

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong_mode.dart:43: assert(false);
Also, why is there an assert after a throw? We can't reach this code.

[Jennifer Messerly, 2015-11-13 19:04:47]

Thanks so much! A few questions on how to best proceed...

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/element.dart (right):

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:4853: List<DartType> _typeArguments
= DartType.EMPTY_LIST;
On 2015/11/13 18:56:16, Brian Wilkerson wrote:
> Not sure why you removed the comment. If it was wrong, could you add one
that's
> right? (I like to document everything, including private members.)

The comment moved to the public getter. Normally I would keep the getter by the
field, but "sort members" moved it down. Does that work for you? Would you
prefer the comment be only on the field instead of the public getter?

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:4902: List<TypeParameterElement>
typeParameters = this.typeParameters;
On 2015/11/13 18:56:16, Brian Wilkerson wrote:
> See TypeParameterTypeImpl.getTypes

Yeah, I tried that first. Unfortunately that doesn't work, because it returns
the wrong reified List type (List<TypeParameterType> instead of List<DartType).

The good news: generic methods will help us fix this! We can have getTypes<T>,
where `T extends DartType` ... which will support both use cases. :)

I can add a TODO, if that works for you:

    // TODO(jmesserly): reuse TypeParameterTypeImpl.getTypes once it is generic,
and can return List<DartType>

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:10536: * For example, a class or a
typedef. This also includes functions methods if
On 2015/11/13 18:56:16, Brian Wilkerson wrote:
> nit: "functions" --> "function"

oops, it should be "functions and methods"

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong_mode.dart:30: // TODO(jmesserly): why is this
necessary? The function type should always
On 2015/11/13 18:56:16, Brian Wilkerson wrote:
> Good question! I don't know. Does anything break if we change this?

Yeah, I was planning to give it a shot right after this.

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong_mode.dart:43: assert(false);
On 2015/11/13 18:56:16, Brian Wilkerson wrote:
> Also, why is there an assert after a throw? We can't reach this code.

yeah--it's strange right? The asserts above seem sketchy too & inconsistent.
Should I just have it all be a throw?

(and below in setReturnType)

[Jennifer Messerly, 2015-11-13 19:17:22]

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong_mode.dart:43: assert(false);
On 2015/11/13 19:04:46, John Messerly wrote:
> On 2015/11/13 18:56:16, Brian Wilkerson wrote:
> > Also, why is there an assert after a throw? We can't reach this code.
> 
> yeah--it's strange right? The asserts above seem sketchy too & inconsistent.
> Should I just have it all be a throw?
> 
> (and below in setReturnType)

Thinking on this more, it seems like we just want a cast:

    (element as PropertyAccessorElementImpl)

same for the other "is" checks. These aren't really type tests, they're
logically asserting that certain types are expected, even though our type system
is not tracking it.

Longer term, it would be nice if we can help the type system track this better.

[Leaf, 2015-11-13 19:28:55]

lgtm

[Brian Wilkerson, 2015-11-13 20:17:14]

LGTM

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/element.dart (right):

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:4853: List<DartType> _typeArguments
= DartType.EMPTY_LIST;
> The comment moved to the public getter. Would you
> prefer the comment be only on the field instead of the public getter?

I know it's redundant, but I always comment both. Perhaps we should change
server so that if a private field doesn't have a comment we look for a public
getter with the same name (minus the `_`) and return it's comment...

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:4902: List<TypeParameterElement>
typeParameters = this.typeParameters;
Yes, a comment would be great, thanks!

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong_mode.dart (right):

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong_mode.dart:30: // TODO(jmesserly): why is this
necessary? The function type should always
Sounds good. Certainly not necessary before landing this CL.

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong_mode.dart:43: assert(false);
The rationale we've used before is that we want this to run as quickly as
possible in production, but have as much checking as possible during
development. Hence the assert statements. They disappear in production, but can
help us find problems during development. That's why we avoid casts except where
necessary to get around the limited abilities of type promotion.

It just seems odd to have code after an explicit throw because it will never be
reached.

I certainly don't expect you to make any of these changes as part of this CL.
:-)

[Jennifer Messerly, 2015-11-13 22:52:47]

Committed patchset #2 (id:40001) manually as
af7359853f8f33b2febdcfa5beeb7a7dce115dce (presubmit successful).

[Jennifer Messerly, 2015-11-13 22:53:15]

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/element.dart (right):

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:4853: List<DartType> _typeArguments
= DartType.EMPTY_LIST;
On 2015/11/13 20:17:14, Brian Wilkerson wrote:
> > The comment moved to the public getter. Would you
> > prefer the comment be only on the field instead of the public getter?
> 
> I know it's redundant, but I always comment both. Perhaps we should change
> server so that if a private field doesn't have a comment we look for a public
> getter with the same name (minus the `_`) and return it's comment...

yeah that would be good!

In the meantime, done! I wrote a line that also links to the getter (via
[typeArguments]). That way it won't be a full copy+paste. I always worry about
copy+paste as it leads to multiple places to update... which creates a bit of a
perverse incentive for writing long, high quality comments, or improving
existing ones.

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:4902: List<TypeParameterElement>
typeParameters = this.typeParameters;
On 2015/11/13 20:17:14, Brian Wilkerson wrote:
> Yes, a comment would be great, thanks!

Done.

https://codereview.chromium.org/1434983006/diff/40001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:10536: * For example, a class or a
typedef. This also includes functions methods if
On 2015/11/13 19:04:46, John Messerly wrote:
> On 2015/11/13 18:56:16, Brian Wilkerson wrote:
> > nit: "functions" --> "function"
> 
> oops, it should be "functions and methods"

Done.

[Paul Berry, 2015-11-16 17:13:12]

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

[Paul Berry, 2015-11-16 17:13:12]

It looks like this CL caused a test failure.  Repro with: python tools/test.py
-aia32 --checked -mrelease -t60 pkg/analysis_server/test/analysis/get_hover_test

Here are the failure details:
FAIL: AnalysisHoverTest | test_instanceCreation_implicit_withTypeArgument
  Expected: 'A() → A<String>'
    Actual: 'A() → A<T>'
     Which: is different.
  Expected: A() → A<String>
    Actual: A() → A<T>
                    ^
   Differ at offset 8
  
  package:unittest/src/simple_configuration.dart 128:34       
SimpleConfiguration.onExpectFailure
  package:unittest/src/simple_configuration.dart 24:13        
_ExpectFailureHandler.fail
  package:unittest/src/matcher/expect.dart 121:5              
DefaultFailureHandler.failMatch
  package:unittest/src/matcher/expect.dart 95:20               expect
  pkg/analysis_server/test/analysis/get_hover_test.dart 312:7 
AnalysisHoverTest.test_instanceCreation_implicit_withTypeArgument.<fn>
  dart:async/zone.dart 914                                     _rootRunUnary
  dart:async/zone.dart 810                                    
_CustomZone.runUnary
  dart:async/future_impl.dart 502                             
_Future._propagateToListeners.handleValueCallback
  dart:async/future_impl.dart 585                             
_Future._propagateToListeners
  dart:async/future_impl.dart 466                             
_Future._propagateToListeners
  dart:async/future_impl.dart 376                             
_Future._completeWithValue
  dart:async/future_impl.dart 430                             
_Future._asyncComplete.<fn>
  dart:async/zone.dart 907                                     _rootRun
  dart:async/zone.dart 802                                     _CustomZone.run
  dart:async/zone.dart 708                                    
_CustomZone.runGuarded
  dart:async/zone.dart 733                                    
_CustomZone.bindCallback.<fn>
  dart:async/schedule_microtask.dart 43                        _microtaskLoop
  dart:async/schedule_microtask.dart 52                       
_microtaskLoopEntry
  dart:isolate-patch/timer_impl.dart 394                       _Timer._runTimers
  dart:isolate-patch/timer_impl.dart 414                      
_Timer._handleMessage
  dart:isolate-patch/isolate_patch.dart 150                   
_RawReceivePortImpl._handleMessage

[Jennifer Messerly, 2015-11-16 18:54:47]

On 2015/11/16 17:13:12, Paul Berry wrote:
> It looks like this CL caused a test failure.  Repro with: python tools/test.py
> -aia32 --checked -mrelease -t60
pkg/analysis_server/test/analysis/get_hover_test
> 
> Here are the failure details:
> FAIL: AnalysisHoverTest | test_instanceCreation_implicit_withTypeArgument
>   Expected: 'A() → A<String>'
>     Actual: 'A() → A<T>'
>      Which: is different.
>   Expected: A() → A<String>
>     Actual: A() → A<T>
>                     ^
>    Differ at offset 8
>   
>   package:unittest/src/simple_configuration.dart 128:34       
> SimpleConfiguration.onExpectFailure
>   package:unittest/src/simple_configuration.dart 24:13        
> _ExpectFailureHandler.fail
>   package:unittest/src/matcher/expect.dart 121:5              
> DefaultFailureHandler.failMatch
>   package:unittest/src/matcher/expect.dart 95:20               expect
>   pkg/analysis_server/test/analysis/get_hover_test.dart 312:7 
> AnalysisHoverTest.test_instanceCreation_implicit_withTypeArgument.<fn>
>   dart:async/zone.dart 914                                     _rootRunUnary
>   dart:async/zone.dart 810                                    
> _CustomZone.runUnary
>   dart:async/future_impl.dart 502                             
> _Future._propagateToListeners.handleValueCallback
>   dart:async/future_impl.dart 585                             
> _Future._propagateToListeners
>   dart:async/future_impl.dart 466                             
> _Future._propagateToListeners
>   dart:async/future_impl.dart 376                             
> _Future._completeWithValue
>   dart:async/future_impl.dart 430                             
> _Future._asyncComplete.<fn>
>   dart:async/zone.dart 907                                     _rootRun
>   dart:async/zone.dart 802                                     _CustomZone.run
>   dart:async/zone.dart 708                                    
> _CustomZone.runGuarded
>   dart:async/zone.dart 733                                    
> _CustomZone.bindCallback.<fn>
>   dart:async/schedule_microtask.dart 43                        _microtaskLoop
>   dart:async/schedule_microtask.dart 52                       
> _microtaskLoopEntry
>   dart:isolate-patch/timer_impl.dart 394                      
_Timer._runTimers
>   dart:isolate-patch/timer_impl.dart 414                      
> _Timer._handleMessage
>   dart:isolate-patch/isolate_patch.dart 150                   
> _RawReceivePortImpl._handleMessage

Thanks for noticing that! I'll take a look now. I forgot that refactorings might
impact analysis_server, especially if it uses *Impl types. DOH.

[Jennifer Messerly, 2015-11-16 19:38:27]

On 2015/11/16 18:54:47, John Messerly wrote:
> On 2015/11/16 17:13:12, Paul Berry wrote:
> > It looks like this CL caused a test failure.  Repro with: python
tools/test.py
> > -aia32 --checked -mrelease -t60
> pkg/analysis_server/test/analysis/get_hover_test
> > 
> > Here are the failure details:
> > FAIL: AnalysisHoverTest | test_instanceCreation_implicit_withTypeArgument
> >   Expected: 'A() → A<String>'
> >     Actual: 'A() → A<T>'
> >      Which: is different.
> >   Expected: A() → A<String>
> >     Actual: A() → A<T>
> >                     ^
> >    Differ at offset 8
> >   
> >   package:unittest/src/simple_configuration.dart 128:34       
> > SimpleConfiguration.onExpectFailure
> >   package:unittest/src/simple_configuration.dart 24:13        
> > _ExpectFailureHandler.fail
> >   package:unittest/src/matcher/expect.dart 121:5              
> > DefaultFailureHandler.failMatch
> >   package:unittest/src/matcher/expect.dart 95:20               expect
> >   pkg/analysis_server/test/analysis/get_hover_test.dart 312:7 
> > AnalysisHoverTest.test_instanceCreation_implicit_withTypeArgument.<fn>
> >   dart:async/zone.dart 914                                     _rootRunUnary
> >   dart:async/zone.dart 810                                    
> > _CustomZone.runUnary
> >   dart:async/future_impl.dart 502                             
> > _Future._propagateToListeners.handleValueCallback
> >   dart:async/future_impl.dart 585                             
> > _Future._propagateToListeners
> >   dart:async/future_impl.dart 466                             
> > _Future._propagateToListeners
> >   dart:async/future_impl.dart 376                             
> > _Future._completeWithValue
> >   dart:async/future_impl.dart 430                             
> > _Future._asyncComplete.<fn>
> >   dart:async/zone.dart 907                                     _rootRun
> >   dart:async/zone.dart 802                                    
_CustomZone.run
> >   dart:async/zone.dart 708                                    
> > _CustomZone.runGuarded
> >   dart:async/zone.dart 733                                    
> > _CustomZone.bindCallback.<fn>
> >   dart:async/schedule_microtask.dart 43                       
_microtaskLoop
> >   dart:async/schedule_microtask.dart 52                       
> > _microtaskLoopEntry
> >   dart:isolate-patch/timer_impl.dart 394                      
> _Timer._runTimers
> >   dart:isolate-patch/timer_impl.dart 414                      
> > _Timer._handleMessage
> >   dart:isolate-patch/isolate_patch.dart 150                   
> > _RawReceivePortImpl._handleMessage
> 
> Thanks for noticing that! I'll take a look now. I forgot that refactorings
might
> impact analysis_server, especially if it uses *Impl types. DOH.

Fix on the way!