fixes #25640, errors on function downcasts when we know it's an exact type

fixes #25640, errors on function downcasts when we know it's an exact type

R=vsm@google.com

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

[Jennifer Messerly, 2016-02-02 22:15:05]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[Jennifer Messerly, 2016-02-02 22:15:05]

Easy fix! Lots of test updates. :)

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (left):

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:683: AToB bot(BToA f) => f
as AToB;
it's hard to see in the diff, but the original code here was busted ... there
were 3 definitions of "top", "left", etc. I presume this was supposed to be
split into 3 separate test cases. As it was, only one of the definitions were
actually tested, because of the name collision.

So I split it into 3 separate test cases. Should have coverage of them all now.

[vsm, 2016-02-03 23:52:00]

lgtm

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/info.dart (right):

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/info.dart:149: if (staticElement is
FunctionElement) {
Does this guarantee a static or top-level function (as opposed to a method)?

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (right):

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:452: var bot = globalBot;
Perhaps a test to cover the static method case (error) and the instance method
case (downcast composite?).

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:522: f =
/*severe:STATIC_TYPE_ERROR*/top;
nice!

[Jennifer Messerly, 2016-02-04 00:41:30]

Thanks! Let me know thoughts RE: static methods.

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/lib/src/task/s...
File pkg/analyzer/lib/src/task/strong/info.dart (right):

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/lib/src/task/s...
pkg/analyzer/lib/src/task/strong/info.dart:149: if (staticElement is
FunctionElement) {
On 2016/02/03 23:51:59, vsm wrote:
> Does this guarantee a static or top-level function (as opposed to a method)?

It includes top-level and local functions (named, lambdas). Does not include
methods (instance or static). Also does not include things that can store
first-class functions, like variables/parameters/fields.

The common base class between MethodElement, FunctionElement, and
PropertyAccessorElement is called ExecutableElement. (There is a further up
interface, called FunctionTypedElement, that includes typedefs as well.)

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
File pkg/analyzer/test/src/task/strong/checker_test.dart (right):

https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
pkg/analyzer/test/src/task/strong/checker_test.dart:452: var bot = globalBot;
On 2016/02/03 23:52:00, vsm wrote:
> Perhaps a test to cover the static method case (error) and the instance method
> case (downcast composite?).

instance methods are covered by this test (line 1640):
https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...

Static methods are currently treated as a warning (downcast composite). They're
a bit of an odd case ... at the moment, we could safely issue an error. If we
got metaclasses: https://github.com/gbracha/metaclasses/blob/master/proposal.md
... it wouldn't be safe to issue an error anymore. I could add it with a TODO
about metaclass issue, if that sounds good? Or we could just leave it as a
warning, and say that's good enough.

[vsm, 2016-02-04 17:57:53]

On 2016/02/04 00:41:30, John Messerly wrote:
> Thanks! Let me know thoughts RE: static methods.
> 
>
https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/lib/src/task/s...
> File pkg/analyzer/lib/src/task/strong/info.dart (right):
> 
>
https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/lib/src/task/s...
> pkg/analyzer/lib/src/task/strong/info.dart:149: if (staticElement is
> FunctionElement) {
> On 2016/02/03 23:51:59, vsm wrote:
> > Does this guarantee a static or top-level function (as opposed to a method)?
> 
> It includes top-level and local functions (named, lambdas). Does not include
> methods (instance or static). Also does not include things that can store
> first-class functions, like variables/parameters/fields.
> 
> The common base class between MethodElement, FunctionElement, and
> PropertyAccessorElement is called ExecutableElement. (There is a further up
> interface, called FunctionTypedElement, that includes typedefs as well.)
> 
>
https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
> File pkg/analyzer/test/src/task/strong/checker_test.dart (right):
> 
>
https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
> pkg/analyzer/test/src/task/strong/checker_test.dart:452: var bot = globalBot;
> On 2016/02/03 23:52:00, vsm wrote:
> > Perhaps a test to cover the static method case (error) and the instance
method
> > case (downcast composite?).
> 
> instance methods are covered by this test (line 1640):
>
https://codereview.chromium.org/1663533002/diff/1/pkg/analyzer/test/src/task/...
> 
> Static methods are currently treated as a warning (downcast composite).
They're
> a bit of an odd case ... at the moment, we could safely issue an error. If we
> got metaclasses:
https://github.com/gbracha/metaclasses/blob/master/proposal.md
> ... it wouldn't be safe to issue an error anymore. I could add it with a TODO
> about metaclass issue, if that sounds good? Or we could just leave it as a
> warning, and say that's good enough.

Hmm, perhaps just file an issue pointing this out.

LGTM

[Jennifer Messerly, 2016-02-04 18:32:27]

Thanks! I went ahead and added static method support (it's an error), along with
a test for static and instance methods (the latter is a warning). Based on our
chat, I'll land it with these changes, but let me know if there's any additional
tweaks we'd like here :)

[Jennifer Messerly, 2016-02-04 18:33:23]

Description was changed from

==========
fixes #25640, errors on function downcasts when we know it's an exact type
==========

to

==========
fixes #25640, errors on function downcasts when we know it's an exact type

R=vsm@google.com

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

[Jennifer Messerly, 2016-02-04 18:33:25]

Committed patchset #2 (id:20001) manually as
86a366403eba3d7eb80014d7124a1cbcb41d1138 (presubmit successful).