fixes #24507, allow known functions to be treated as strict arrows

fixes #24507, allow known functions to be treated as strict arrows

In a known function, * -> * will be top -> top.
In all other function types, it will still be bottom -> top.

R=leafp@google.com

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

[Jennifer Messerly, 2016-02-13 00:34:08]

Description was changed from

==========
fixes #24507, allow known functions to be treated as strict arrows
In a known function, * -> * will be top -> top. In all other function types, it
will still be bottom -> top.
==========

to

==========
fixes #24507, allow known functions to be treated as strict arrows

In a known function, * -> * will be top -> top.
In all other function types, it will still be bottom -> top.
==========

[Jennifer Messerly, 2016-02-13 00:51:11]

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

[Jennifer Messerly, 2016-02-13 00:51:12]

Hey guys, any thoughts on this approach?

The idea here is: whenever CodeChecker gets a static type from an expression, it
will get the non-fuzzy arrow if possible.

BTW, I copied one test into two, so we don't lose coverage. There's now two
variants of that test, one for known functions and one for unknown functions.

In the previous test, the errors became not-errors.

In the new test, the errors are back to being warnings, as they were before our
recent change (which made it an error when known functions were downcast) .

[Jennifer Messerly, 2016-02-13 00:55:11]

Oh, BTW. If you like this approach, we can remove some vestigial checks in type
system.

The idea behind adjusting the type is it doesn't matter which direction we do
the subtype check in. Either way we get the right effect (a non-fuzzy arrow in
that type).

[Leaf, 2016-02-16 20:30:43]

I'm surprised that you didn't have to do anything in the analyzer proper.  Do
you have any idea why this doesn't come up there?   I would expect that if I
have code that looks like f((x : dynamic) => 3) where f expects an int -> int,
that the error verifier would do an assignability check (which for us is a
subtype check), and if fuzzy arrows are on then the check would still fail?

Otherwise lgtm, modulo the naming in the test.

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

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:599: class A {}
A little pedantic, but I think it's worth getting the terminology right here
just to make it clear what's going on.  In this test, you have a lattice induced
by the function type operator over Bottom <: A <: Top, so you get a nine point
lattice rather than a four point lattice.  So some of the top/bottom names below
aren't right, and the left right doesn't make sense.  We can only express 6 of
the 9 points, because we can't write down Bot -> Bot, A -> Bot, or Top -> Bot. 
The remaining six point lattice looks like:

           Bot -> Top
          /        \
      A -> Top    Bot -> A
       /     \      /
 Top -> Top   A -> A
         \      /
         Top -> A


You can name these points as paths through the lattice (Top, Left, LeftLeft,
Right, RightLeft, Bot), or just using their constituent types (BotTop, ATop,
TopTop, TopA, BotA, AA).  

You could fold the previous test into this, since the lattice tested above is a
sub-lattice of this lattice (it's the upper right diamond).

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:604: typedef A Bottom(A x); 
            // Bottom of the lattice
This is not the bottom type in this lattice.

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:607: A bot(A x) => x;
This is not the bottom type in this lattice.

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:608: dynamic top(dynamic x)
=> x;
This is not the top type in this lattice.

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:609: A right(dynamic x) =>
/*info:DYNAMIC_CAST*/x;
This is the bottom type in this lattice.

[Jennifer Messerly, 2016-02-16 21:04:59]

Thanks Leaf!

Hmm, you're right, this may need a change in Analyzer proper. I though I had
already fixed our tests to include Analyzer error codes, but perhaps that is
just a local branch. I'll double check what is going on there.

On test naming: yeah, what happened there is it's not a new test, it's just a
copy of the old one. There are many variants of that test which all define those
function types with those names, so I left the names as they were.

So that's how it ended up that way. Anyway, I'm happy to rename the variables to
reflect what the types actually *mean* in the type lattice. :)

[Leaf, 2016-02-16 22:25:07]

I'm fine with just using arbitrary names as well - I know these tests get copied
and tweaked lot, so in some ways it might be better.  I'm not sure if the
lattice naming actually contributes anything.  I just think it's the worst of
both worlds to use the lattice terminology inconsistently with the lattice.

On 2016/02/16 21:04:59, John Messerly wrote:
> Thanks Leaf!
> 
> Hmm, you're right, this may need a change in Analyzer proper. I though I had
> already fixed our tests to include Analyzer error codes, but perhaps that is
> just a local branch. I'll double check what is going on there.
> 
> On test naming: yeah, what happened there is it's not a new test, it's just a
> copy of the old one. There are many variants of that test which all define
those
> function types with those names, so I left the names as they were.
> 
> So that's how it ended up that way. Anyway, I'm happy to rename the variables
to
> reflect what the types actually *mean* in the type lattice. :)

[Jennifer Messerly, 2016-02-17 17:47:03]

On 2016/02/16 22:25:07, Leaf wrote:
> I'm fine with just using arbitrary names as well - I know these tests get
copied
> and tweaked lot, so in some ways it might be better.  I'm not sure if the
> lattice naming actually contributes anything.  I just think it's the worst of
> both worlds to use the lattice terminology inconsistently with the lattice.
> 
> On 2016/02/16 21:04:59, John Messerly wrote:
> > Thanks Leaf!
> > 
> > Hmm, you're right, this may need a change in Analyzer proper. I though I had
> > already fixed our tests to include Analyzer error codes, but perhaps that is
> > just a local branch. I'll double check what is going on there.
> > 
> > On test naming: yeah, what happened there is it's not a new test, it's just
a
> > copy of the old one. There are many variants of that test which all define
> those
> > function types with those names, so I left the names as they were.
> > 
> > So that's how it ended up that way. Anyway, I'm happy to rename the
variables
> to
> > reflect what the types actually *mean* in the type lattice. :)

So, it appears this does work, at least for the specific test cases. But yeah,
it doesn't give me a great feeling... since we need to merge CodeChecker in
anyway.

Here's another thought: what if we handle this in Resolver. When we push a
context type down, we take into account strict arrows. If that sounds
reasonable, I will give it a shot and see how it turns out.

[Jennifer Messerly, 2016-02-17 20:13:24]

Thanks! I fixed the test naming, if you want to take another look at that. Also
added a TODO about possible better approaches we can take here (per our earlier
discussion).

I may land this based on previous LGTM & think I got your comments addressed
(good catch on that test naming, wow, that was confusing), but definitely feel
free to send more my way if you take another look, happy to address.

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

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:599: class A {}
On 2016/02/16 20:30:43, Leaf wrote:
> A little pedantic, but I think it's worth getting the terminology right here
> just to make it clear what's going on.  In this test, you have a lattice
induced
> by the function type operator over Bottom <: A <: Top, so you get a nine point
> lattice rather than a four point lattice.  So some of the top/bottom names
below
> aren't right, and the left right doesn't make sense.  We can only express 6 of
> the 9 points, because we can't write down Bot -> Bot, A -> Bot, or Top -> Bot.

> The remaining six point lattice looks like:
> 
>            Bot -> Top
>           /        \
>       A -> Top    Bot -> A
>        /     \      /
>  Top -> Top   A -> A
>          \      /
>          Top -> A
> 
> 
> You can name these points as paths through the lattice (Top, Left, LeftLeft,
> Right, RightLeft, Bot), or just using their constituent types (BotTop, ATop,
> TopTop, TopA, BotA, AA).  
> 
> You could fold the previous test into this, since the lattice tested above is
a
> sub-lattice of this lattice (it's the upper right diamond).

Done!

* used that picture in a comment. Thanks :)
* went with the "constituent types" naming, as it makes it easier to look at one
line in isolation and know what the answer should be.
* added all 6 types we can be assigning to.
* kept "known functions" as a separate test. Tried merging them, but it was a
little confusing because some of the messages are warnings and some are errors,
which has nothing to do with the type lattice itself, only the source of the
type.

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:604: typedef A Bottom(A x); 
            // Bottom of the lattice
On 2016/02/16 20:30:43, Leaf wrote:
> This is not the bottom type in this lattice.

Fixed. See above.

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:607: A bot(A x) => x;
On 2016/02/16 20:30:43, Leaf wrote:
> This is not the bottom type in this lattice.

Fixed. See above.

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:608: dynamic top(dynamic x)
=> x;
On 2016/02/16 20:30:43, Leaf wrote:
> This is not the top type in this lattice.

Fixed. See above.

https://codereview.chromium.org/1700523002/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/checker_test.dart:609: A right(dynamic x) =>
/*info:DYNAMIC_CAST*/x;
On 2016/02/16 20:30:43, Leaf wrote:
> This is the bottom type in this lattice.

Fixed. See above.

[Jennifer Messerly, 2016-02-17 20:14:27]

Description was changed from

==========
fixes #24507, allow known functions to be treated as strict arrows

In a known function, * -> * will be top -> top.
In all other function types, it will still be bottom -> top.
==========

to

==========
fixes #24507, allow known functions to be treated as strict arrows

In a known function, * -> * will be top -> top.
In all other function types, it will still be bottom -> top.

R=leafp@google.com

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

[Jennifer Messerly, 2016-02-17 20:14:28]

Committed patchset #6 (id:100001) manually as
acc95d319573ec19c6cb60ce514182fd746b1972 (presubmit successful).