Abstract over the type system.

First cut at abstracting over the type system.  This factors out uses of isAssignableTo and isSubtypeOf into calls through a type system object.  Currently there is only one kind of type system, which just falls over to the existing implementations.

BUG=
R=brianwilkerson@google.com, paulberry@google.com

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

[Leaf, 2015-09-05 00:58:35]

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

[Leaf, 2015-09-05 01:07:23]

Hi Guys - This is not ready to submit, but I wanted to run it by you to get
feedback on it.  This is one cut at factoring out the type system.  Since the
typeSystem and the typeProvider are fairly entwined, they ended up paralleling
each other fairly closely.  

There seem to be some performance sensitivities about uses of the typeProvider -
my initial try I think ended up forcing the typeProvider to be created a bit
more eagerly in places (by pulling a typeSystem out of the context, which in
turn forced the typeProvider to be generated).  This seemed to cause tremendous
slowdowns.  This version is careful to only create typeSystems in places where I
already no that a typeProvider exists.  This feels fragile to me though. 
Perhaps these should be tied together in some way?

I also don't know what to do in dart.dart.  Pulling a typeSystem out of the
context fired the assertion at the top of the typeProvider getter in
context/context.dart which seemed to be designed to prevent me from doing what I
was doing.  I'm currently just creating  a new type system any time I need one
in that file.  What's the right way to do this here?

Thoughts, comments, suggestions welcome.

I have not run the formatter on the code yet, to avoid adding noise to the CL,
will run it before final review.

[Paul Berry, 2015-09-05 03:25:12]

Overall, lgtm.  I have some concerns about how and when the TypeSystem is being
created; see below.

On 2015/09/05 01:07:23, Leaf wrote:
> Hi Guys - This is not ready to submit, but I wanted to run it by you to get
> feedback on it.  This is one cut at factoring out the type system.  Since the
> typeSystem and the typeProvider are fairly entwined, they ended up paralleling
> each other fairly closely.  
> 
> There seem to be some performance sensitivities about uses of the typeProvider
-
> my initial try I think ended up forcing the typeProvider to be created a bit
> more eagerly in places (by pulling a typeSystem out of the context, which in
> turn forced the typeProvider to be generated).  This seemed to cause
tremendous
> slowdowns.  This version is careful to only create typeSystems in places where
I
> already no that a typeProvider exists.  This feels fragile to me though. 
> Perhaps these should be tied together in some way?

Personally I would be fine with the TypeSystem and the TypeProvider being the
same class.  But I have a vague memory that I suggested it at one point, and
Brian had a reason why it was a bad idea.  Brian, do you recall?

> 
> I also don't know what to do in dart.dart.  Pulling a typeSystem out of the
> context fired the assertion at the top of the typeProvider getter in
> context/context.dart which seemed to be designed to prevent me from doing what
I
> was doing.  I'm currently just creating  a new type system any time I need one
> in that file.  What's the right way to do this here?

Yeah, that assertion is important.  The issue is that the typeProvider getter is
part of the synchronous public API of the analysis context; when you call it,
the analysis context runs whatever tasks are necessary to ensure that the type
provider has been created, and then returns it.  So if you call the typeProvider
from inside a task, you're going to wind up making a reentrant call into the
task model.  If you're lucky, the type provider will already have been created
when you ask for it, so the reentrant call will be a no-op and everything will
be fine.  But if you're unlucky and not many things have been analyzed yet,
you'll probably wind up breaking a bunch of task model invariants (we never
designed it with reentrancy in mind).

So the idea is, if task X needs access to the type provider, it shouldn't call
AnalysisContext.typeProvider; instead it should just declare an explicit
dependency on TYPE_PROVIDER in its buildInputs function.  This will cause the
task runner to ensure that BuildTypeProviderTask (and all of its dependencies)
are executed before task X, and it will pass the type provider to task X as one
of its inputs.  Alternatively, if one of task X's inputs happens to contain a
pointer to the type provider, you can use that, although IMHO it's better to
just make the dependency explicit.

Since, as you pointed out, the type system and the type provider parallel each
other, I think the right thing to do is to rename BuildTypeProviderTask to
BuildTypeSystemTask, and have it build both the type provider *and* the type
system.  Similarly, rename the ResultDescriptor TYPE_PROVIDER to TYPE_SYSTEM and
change its type to ResultDescriptor<TypeSystem>, so that everywhere we used to
be plumbing the type provider through the task model, we are now plumbing the
type system through the task model.  (There's no reason to plumb them both
through separately, since creating the TypeSystem is no extra work, and it has a
getter to retrieve the TypeProvider).

So the only question that remains is: how do we get BuildTypeSystemTask to build
the appropriate type system based on whether strong mode is enabled?  For that,
I don't think we need to do anything fancy.  Just query
AnalysisContext.analysisOptions.strongMode from inside the  and construct the
appropriate object.

> 
> Thoughts, comments, suggestions welcome.
> 
> I have not run the formatter on the code yet, to avoid adding noise to the CL,
> will run it before final review.

[Paul Berry, 2015-09-05 03:25:39]

https://codereview.chromium.org/1329743005/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right):

https://codereview.chromium.org/1329743005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:853: * The object used
to access the types from the core library.
Copy-pasted comment.

[Brian Wilkerson, 2015-09-05 16:02:41]

> > This version is careful to only create typeSystems in places where
> > I already know that a typeProvider exists.  This feels fragile to me though.

> > Perhaps these should be tied together in some way?
> 
> Personally I would be fine with the TypeSystem and the TypeProvider being the
> same class.  But I have a vague memory that I suggested it at one point, and
> Brian had a reason why it was a bad idea.  Brian, do you recall?

In my mind, at least, the classes TypeSystem and TypeProvider should not be
tightly coupled. How we find the definitions of various types is not dependent
on how we determine relationships between those classes, nor vice versa. They
are orthogonal concerns, and I'd very much like the implementation to reflect
that.

There is only one method that I know of (getLeastUpperBound) that we are likely
to want to define in the class TypeSystem that needs to have access to the
TypeProvider[1]. Rather than passing the type provider in when creating the type
system, I would rather have it passed in as an argument to getLeastUpperBound.
In addition to matching my view of what the type system should be :-) it has the
benefit that the type system can be created at any time without needing to have
partially analyzed the core libraries[2], which makes the system less fragile.

Brian

[1] Yes, we might want to have two variants of that method, one that follows the
specification and one that does what users expect, but that doesn't change my
opinion.

[2] And yes, I realize that the value of being able to create a type system
before the type hierarchy is fully manifested is marginal because the number of
type system operations that are useful in the absence of a fully realized type
hierarchy are minimal.

[Leaf, 2015-09-15 23:01:12]

OK, I've re-factored this per the previous comments, and have also added in a
strong mode type system, with associated tests.  PTAL, thanks!

https://codereview.chromium.org/1329743005/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right):

https://codereview.chromium.org/1329743005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:853: * The object used
to access the types from the core library.
On 2015/09/05 03:25:39, Paul Berry wrote:
> Copy-pasted comment.

Done.

[Brian Wilkerson, 2015-09-16 14:00:45]

LGTM. Nits can be implemented in a later CL or just ignored.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:14964: /* Create either a strong
mode or regular type system based on context.
nit: Why is this (and several other places) just a normal comment rather than a
doc comment?

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15092: * Implementation of
[TypeSystem] using the strong mode rules.
We should either document the strong mode semantics (in individual methods) or
reference external documentation.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15139: if (t.isBottom) return true;
nit: Why not just "return t.isBottom;"?

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15145: if (t.isObject) return true;
nit: Similarly, why not just "return t.isObject;"?

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15162: /* Check that f1 is a
subtype of f2.
nit: This should be a doc comment and 'f1' and 'f2 should be in square brackets,
or 'fuzzyArrows' shouldn't be in square brackets.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15267: bool _isInterfaceSubtypeOf(
nit: Add doc comment?

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/type_system_test.dart (right):

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/type_system_test.dart:374: static FunctionType
functionType(
nit: Seems like something that could go in ElementFactory, with invocations of
this method just adding the invocation of "type" from the last line. That would
make this more re-usable.

[Leaf, 2015-09-16 21:07:55]

Ok, addressed most of the nits.  If you're happy enough for now, I'll go ahead
and land this and include any further changes in a followup.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:14964: /* Create either a strong
mode or regular type system based on context.
On 2015/09/16 14:00:45, Brian Wilkerson wrote:
> nit: Why is this (and several other places) just a normal comment rather than
a
> doc comment?

Because I'm not very used to using doc comment conventions.  :)  I changed some
of the examples that I found - let me know if you see more missing cases.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15092: * Implementation of
[TypeSystem] using the strong mode rules.
On 2015/09/16 14:00:45, Brian Wilkerson wrote:
> We should either document the strong mode semantics (in individual methods) or
> reference external documentation.

Done.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15139: if (t.isBottom) return true;
On 2015/09/16 14:00:45, Brian Wilkerson wrote:
> nit: Why not just "return t.isBottom;"?

Done.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15145: if (t.isObject) return true;
On 2015/09/16 14:00:45, Brian Wilkerson wrote:
> nit: Similarly, why not just "return t.isObject;"?

Done.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15162: /* Check that f1 is a
subtype of f2.
On 2015/09/16 14:00:45, Brian Wilkerson wrote:
> nit: This should be a doc comment and 'f1' and 'f2 should be in square
brackets,
> or 'fuzzyArrows' shouldn't be in square brackets.

Done.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15267: bool _isInterfaceSubtypeOf(
On 2015/09/16 14:00:45, Brian Wilkerson wrote:
> nit: Add doc comment?

Is the recommendation to use doc comments for everything or only for public
methods?  I'd been going on the assumption that only things intended to be part
of the API should be doc commented, but I don't think that's based on anything
but my own assumption.

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/test/gene...
File pkg/analyzer/test/generated/type_system_test.dart (right):

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/test/gene...
pkg/analyzer/test/generated/type_system_test.dart:374: static FunctionType
functionType(
On 2015/09/16 14:00:45, Brian Wilkerson wrote:
> nit: Seems like something that could go in ElementFactory, with invocations of
> this method just adding the invocation of "type" from the last line. That
would
> make this more re-usable.

Done.

[Leaf, 2015-09-16 21:56:04]

Committed patchset #7 (id:120001) manually as
a19bd39da8e1b79bdb0971c5487cd5b053306965 (presubmit successful).

[Brian Wilkerson, 2015-09-16 22:54:51]

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1329743005/diff/100001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/resolver.dart:15267: bool _isInterfaceSubtypeOf(
My personal recommendation :-) is to use doc comments for every method that
someone not familiar with the code might ever need to understand (which for me
translates to everything). I'm sure a lot of people would argue that that's
overkill, or that doc comments become stale, but I still prefer documentation
over its absence. It's your call, obviously, whether you think it's worth it in
this case.