In the new task model, use explicit dependencies on the type provider.

In the new task model, use explicit dependencies on the type provider.

Previously, some of the tasks got the type provider from the context,
which meant we were at risk of invoking the task manager reentrantly
(which might have broken some task manager assumptions and potentially
degraded responsiveness).  With this CL, all tasks that need the type
provider get it using an explicit input.

Assertions are added so that if we accidentally introduce future code
paths that invoke the task manager in a reentrant fashion, we will see
the problem during unit tests.

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

Committed: https://code.google.com/p/dart/source/detail?r=45794

[Paul Berry, 2015-05-13 21:32:00]

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

[Paul Berry, 2015-05-13 21:32:00]

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/driver.dart (right):

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:210: try {
Note: it's not obvious from the diff, but the only non-whitespace changes to the
file are the addition of the try/finally blocks to this method and to the
gatherInputs() method.

[Brian Wilkerson, 2015-05-13 21:49:08]

LGTM

[scheglov, 2015-05-13 21:49:10]

lgtm

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/manager.dart (right):

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/manager.dart:40: bool isTaskRunning = false;
I'm not sure that this is the best place for this flag.
TaskManager works like a contains for tasks, not a runner of them.

[Paul Berry, 2015-05-14 17:11:01]

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/manager.dart (right):

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/manager.dart:40: bool isTaskRunning = false;
On 2015/05/13 21:49:09, scheglov wrote:
> I'm not sure that this is the best place for this flag.
> TaskManager works like a contains for tasks, not a runner of them.

Where do you think would be better?  I'm open to suggestions, but I didn't
really have any better ideas.

I'll land the CL as is for now; I'm happy to do a follow-up if/when we find a
better place.

[scheglov, 2015-05-14 17:13:04]

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/manager.dart (right):

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/manager.dart:40: bool isTaskRunning = false;
On 2015/05/14 17:11:01, Paul Berry wrote:
> On 2015/05/13 21:49:09, scheglov wrote:
> > I'm not sure that this is the best place for this flag.
> > TaskManager works like a contains for tasks, not a runner of them.
> 
> Where do you think would be better?  I'm open to suggestions, but I didn't
> really have any better ideas.

Maybe WorkOrder or AnalysisDriver.

[Paul Berry, 2015-05-14 17:27:33]

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/manager.dart (right):

https://codereview.chromium.org/1124003007/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/manager.dart:40: bool isTaskRunning = false;
On 2015/05/14 17:13:04, scheglov wrote:
> On 2015/05/14 17:11:01, Paul Berry wrote:
> > On 2015/05/13 21:49:09, scheglov wrote:
> > > I'm not sure that this is the best place for this flag.
> > > TaskManager works like a contains for tasks, not a runner of them.
> > 
> > Where do you think would be better?  I'm open to suggestions, but I didn't
> > really have any better ideas.
> 
> Maybe WorkOrder or AnalysisDriver.

WorkOrder doesn't work because we create fresh work orders all the time--we need
a singleton object in order to verify that we're not reentrant.

AnalysisDriver is a possibility, but it's a bit ugly because it's hard to get to
the AnalysisDriver from the WorkItem.

I've uploaded an alternate version of the patch which uses AnalysisDriver. 
(Unfortunately the delta from patch set 2 is a bit confusing because I also did
a rebase).  PTAL and let me know what you think.

[Brian Wilkerson, 2015-05-14 17:36:38]

LGTM

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

https://codereview.chromium.org/1124003007/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:220: assert(!isTaskRunning);
This should be before the 'try' so that we don't change the state when the
assertion is invalid.

https://codereview.chromium.org/1124003007/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:381: AnalysisDriver driver = (context as
AnalysisContextImpl).driver;
We should eventually add the accessor to InternalAnalysisContext rather than
casting it (but probably can't until after we've moved over, so a TODO would be
good).

https://codereview.chromium.org/1124003007/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:383: assert(!driver.isTaskRunning);
Ditto.

[scheglov, 2015-05-14 17:44:05]

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

https://codereview.chromium.org/1124003007/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:383: assert(!driver.isTaskRunning);
Do we need to check it here?

AFAIK AnalysisContextImpl performs all its computations using
AnalysisDriver.computeResult() or performAnalysisTask(). So, it seems that we
could check the flag just in the AnalysisDriver itself.

[Paul Berry, 2015-05-14 19:40:43]

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

https://codereview.chromium.org/1124003007/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:220: assert(!isTaskRunning);
On 2015/05/14 17:36:37, Brian Wilkerson wrote:
> This should be before the 'try' so that we don't change the state when the
> assertion is invalid.

Done.

https://codereview.chromium.org/1124003007/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:381: AnalysisDriver driver = (context as
AnalysisContextImpl).driver;
On 2015/05/14 17:36:37, Brian Wilkerson wrote:
> We should eventually add the accessor to InternalAnalysisContext rather than
> casting it (but probably can't until after we've moved over, so a TODO would
be
> good).

After taking Konstantin's suggestion below the cast proved to be unnecessary.

https://codereview.chromium.org/1124003007/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:383: assert(!driver.isTaskRunning);
On 2015/05/14 17:36:38, Brian Wilkerson wrote:
> Ditto.

Done.

https://codereview.chromium.org/1124003007/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/driver.dart:383: assert(!driver.isTaskRunning);
On 2015/05/14 17:44:05, scheglov wrote:
> Do we need to check it here?
> 
> AFAIK AnalysisContextImpl performs all its computations using
> AnalysisDriver.computeResult() or performAnalysisTask(). So, it seems that we
> could check the flag just in the AnalysisDriver itself.

That's a good point.  Moving the checks into the AnalysisDriver itself made
things much simpler (and fixd some tests which I'd inadvertently broken with
this CL).

[Paul Berry, 2015-05-14 19:49:43]

Committed patchset #3 (id:40001) manually as 45794 (presubmit successful).