Initial task support

Initial task support

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

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

[Paul Berry, 2015-01-20 23:34:52]

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

[Paul Berry, 2015-01-20 23:34:53]

Lgtm!  I have a number of minor nits below, but I don't consider any of them
blocking issues.  Take 'em or leave 'em as you see fit.

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
File pkg/analyzer/lib/task/model.dart (right):

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:19: typedef AnalysisTask
BuildTask(AnalysisContext context, AnalysisTarget target);
Consider renaming to "TaskBuilder" so that the typename is a noun phrase.

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:27: typedef Map<String, TaskInput>
CreateTaskInputs(AnalysisTarget target);
Similarly, consider renaming to "InputBuilder".

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:84: CaughtException thrownException;
I'm not sure the word "thrown" adds any meaning here, and it clashes with the
type being "CaughtException".  Consider renaming the field to just "exception".

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:101: Source get source => target.source;
Consider removing this getter.  It doesn't add a huge amount of value since
clients can always just use "target.source" directly.  And it has two
disadvantages:

1. It's potentially confusing to clients who might wonder what the difference is
between "target.source" and "this.source".  (In fact, I was confused during my
first read of GetContentTask because I couldn't understand why the "description"
getter used "this.source" whereas "internalPerform" used "target.source".)

2. It may not be obvious to the implementor of a plug-in whether they are
intended to override AnalysisTask.source or AnalysisTarget.source.  Worse yet,
if they override both of them in inconsistent ways, they are likely to create
strange and subtle bugs (because they analysis driver will likely assume that
the two functions behave consistently).

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:140: void safelyPerform() {
Is this method ever meant to be called by clients, or only by perform()?  If
it's only meant to be called by perform(), consider making it private, or simply
inlining it inside perform().

An advantage of inlining it inside perform() is that we could really simplify
the control flow so that instead of having three nested levels of try blocks, we
could have just one.  The perform() method would look something like this:

  Stopwatch stopwatch;
  try {
    stopwatch = stopwatchMap[runtimeType];
    if (stopwatch == null) {
      ...
    }
    stopwatch.start();
    internalPerform();
  } on AnalysisException catch (exception) {
    thrownException = exception;
    rethrow;
  } catch (exception, stackTrace) {
    thrownException = new AnalysisException(...);
    throw thrownException;
  } finally {
    stopwatch.stop();
  }

(Note: the semantics are slightly different since exceptions thrown by
stopwatch.start() would likely lead to problems, and exceptions thrown by
stopwatch.stop() wouldn't be caught, but IMHO this is a negligible loss of
safety compared to the increased readability).

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:172: abstract class ContributionPoint<V>
extends ResultDescriptor<V> {
As discussed in person, consider making ContributionPoint<V> extend
ResultDescriptor<List<V>>, so there is no confusion about how the results are to
be unioned together.

[scheglov, 2015-01-21 14:50:46]

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

[scheglov, 2015-01-21 14:50:48]

LGTM

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/src/task/in...
File pkg/analyzer/lib/src/task/inputs.dart (right):

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/src/task/in...
pkg/analyzer/lib/src/task/inputs.dart:196: * The state value indicating that the
builder is positioned before the single result.
"after the single result"

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
File pkg/analyzer/test/src/task/manager_test.dart (right):

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/manager_test.dart:40: results.contains(result1);
expect(results, contains(result1));
...instead of this line

Or:
expect(results, unorderedEquals([result1]));
...instead of two lines

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/manager_test.dart:94: void
test_removeGeneralResult_absent() {
Doesn't really test "removeX".

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
File pkg/analyzer/test/src/task/model_test.dart (right):

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/model_test.dart:29: expect(contributors, isNotNull);
isEmpty already checks for null.

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/model_test.dart:41: expect(contributors.length, 2);
hasLength(2) would print elements of contributors, so simplify failure analysis.

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/model_test.dart:43: (contributors[0] == result2 &&
contributors[1] == result1)) {
Should negation be extracted to be applied to both ||-ed conditions?

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/model_test.dart:69: expect(contributors[0], result);
unorderedEquals could be used here.

[Brian Wilkerson, 2015-01-23 20:18:25]

Committed patchset #1 (id:1) manually as 43120 (presubmit successful).

[Brian Wilkerson, 2015-01-26 04:54:52]

Just realized that I didn't publish my responses before committing. Sorry about
that!

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/src/task/in...
File pkg/analyzer/lib/src/task/inputs.dart (right):

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/src/task/in...
pkg/analyzer/lib/src/task/inputs.dart:196: * The state value indicating that the
builder is positioned before the single result.
Done

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
File pkg/analyzer/lib/task/model.dart (right):

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:84: CaughtException thrownException;
Done

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:101: Source get source => target.source;
Done

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:140: void safelyPerform() {
It also changes what is being measures by the stopwatch in the case where an
exception is being thrown. I've made the method private for now and we can
consider the other change in a future CL.

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/lib/task/model....
pkg/analyzer/lib/task/model.dart:172: abstract class ContributionPoint<V>
extends ResultDescriptor<V> {
I like the idea, but it leads to a type warning that I don't want to take the
time to resolve at this point. Postponed for a future CL.

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
File pkg/analyzer/test/src/task/manager_test.dart (right):

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/manager_test.dart:40: results.contains(result1);
Done

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/manager_test.dart:94: void
test_removeGeneralResult_absent() {
Done

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
File pkg/analyzer/test/src/task/model_test.dart (right):

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/model_test.dart:29: expect(contributors, isNotNull);
Done

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/model_test.dart:41: expect(contributors.length, 2);
Done

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/model_test.dart:43: (contributors[0] == result2 &&
contributors[1] == result1)) {
> Should negation be extracted to be applied to both ||-ed conditions?

I find it easier to read this way, but don't feel that strongly.

https://codereview.chromium.org/812733004/diff/1/pkg/analyzer/test/src/task/m...
pkg/analyzer/test/src/task/model_test.dart:69: expect(contributors[0], result);
Done