Strong mode checker integration

Strong mode checker integration.  This integrates jmesserly's CL, and adds a task for computing strong mode errors and an annotated unit.

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

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

[Leaf, 2015-10-13 05:27:49]

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

[Leaf, 2015-10-13 05:35:47]

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

[Leaf, 2015-10-13 05:35:47]

I've wrapped up jmesserly's changes into this new CL, and added in a new task
which computes strong mode errors (STRONG_MODE_ERRORS) and a strong mode
annotated unit (RESOLVED_UNIT).  

I originally had in mind to make the dependencies for the units form a T, where
RESOLVED_UNIT would stay as the output of the constant evaluation, with
STRONG_MODE_RESOLVED_UNIT being an extra annotated unit that wouldn't be an
input to any of the other tasks, but could be requested by a separate
resolveCompilationUnit entry point.  I was put off from this approach though,
because there seems to be lots of code outside of dart.dart (e.g. context.dart)
that deals with RESOLVED_UNIT outputs directly, and I was concerned that this
code (which seems to be managing work) would not do the right thing for us if
STRONG_MODE_RESOLVED_UNIT was not in the dependency chain leading to
RESOLVED_UNIT.  Let me know if this concern is ill-founded, and if you think I
should structure this differently.

I'm also a little nervous about the hardcoded references to VERIFY_ERRORS in
context.dart (and I think a few other places).  Should something be happening
for STRONG_MODE_ERRORS here?  Perhaps I should feed the STRONG_MODE_ERRORS into
the VerifyUnitTask and append them to VERIFY_ERRORS?

Take a look, see what you think.  Thanks!

https://codereview.chromium.org/1396273004/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler.dart:274: unit =
entry.getValue(RESOLVED_UNIT9);
I clearly missed this in a previous renumbering CL.  These hardcoded lists feel
really brittle to me.  It seems like in so far as the internal tasks of the task
model bleed out of dart.dart at all, it would be good to do so programmatically
in some way.  For example, maybe exposing one list of results (or a few
different results of the different kinds) that code like this could just iterate
over?

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

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:1321:
newUnitEntry.setState(STRONG_MODE_ERRORS, CacheState.INVALID);
This makes me a little nervous.  Should we be doing something here?

[Brian Wilkerson, 2015-10-13 14:21:18]

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

[Brian Wilkerson, 2015-10-13 14:21:19]

LGTM, but I added Konstantin as a reviewer to answer the question about the
incremental resolver.

> Let me know if this concern is ill-founded, and if you
> think I should structure this differently.

I think the approach you took was better. We need to have a well-defined set of
results that can safely be referenced outside the task model. The RESOLVED_UNIT
result is designed to be the way to access a fully resolved AST (where "fully
resolved" is dependent on the mode), and I'd like to leave it that way.

> I'm also a little nervous about the hardcoded references
> to VERIFY_ERRORS in context.dart (and I think a few other
> places).  Should something be happening for
> STRONG_MODE_ERRORS here?

I'm not sure, but I think this method will go away once we're fully converted to
the new task model. If not, we should definitely revisit it; it's another place
where a hard coded list of results is making the code brittle.

> Perhaps I should feed the STRONG_MODE_ERRORS into the
> VerifyUnitTask and append them to VERIFY_ERRORS?

That shouldn't be necessary. You already added them when building the
LIBRARY_UNIT_ERRORS result.

https://codereview.chromium.org/1396273004/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler.dart:274: unit =
entry.getValue(RESOLVED_UNIT9);
I agree. This is very brittle and it's on the list of things that need to be
fixed.

We could probably even just iterate over the values in the entry and return the
first one that is a CompilationUnit (I haven't thought through the implications
for plugins).

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:1154: entry.setState(RESOLVED_UNIT9,
CacheState.FLUSHED);
Should this flush RESOLVED_UNIT10?

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:491: * The result is only available for
[Source]s representing a compilation unit.
"Source" --> "LibrarySpecificUnit"

[scheglov, 2015-10-13 15:21:21]

lgtm

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

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:1321:
newUnitEntry.setState(STRONG_MODE_ERRORS, CacheState.INVALID);
On 2015/10/13 05:35:47, Leaf wrote:
> This makes me a little nervous.  Should we be doing something here?

It should be fine this way.

But if STRONG_MODE_ERRORS can be computed for a single method body, and does not
require analysis of the whole unit, it would be more efficient to compute them
for the body and update the existing value, just as VERIFY_ERRORS does.

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:487: * The additional strong mode errors
produced while scanning a compilation unit.
Probably not "scanning".

[Jennifer Messerly, 2015-10-13 15:23:53]

lgtm, thanks Leaf!

https://codereview.chromium.org/1396273004/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler.dart:348:
results.add(RESOLVED_UNIT9);
I forgot to ask yesterday ... do we need the old BASIC trick of numbering things
"100" "200" so we can add numbers in between later? Or maybe floating point? ;-)

I guess this is a silly way of asking: if we could have names instead of
numbers, that would help my forgetful brain.

Anyway, not an issue with your CL in particular but I wanted to mention it.

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:2601: Iterable<AstVisitor> visitors =
linters.map((l) => l.getVisitor()).toList();
it didn't look like you actually changed this besides formatting, but:

I wonder what the purpose of the "toList()" necessary here? It seems
unnecessary.

[Brian Wilkerson, 2015-10-13 15:37:37]

https://codereview.chromium.org/1396273004/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/status/get_handler.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/status/get_handler.dart:348:
results.add(RESOLVED_UNIT9);
> I guess this is a silly way of asking: if we could have names instead of
> numbers, that would help my forgetful brain.

Yes, the names are poor. I'm currently in the process of documenting better what
each means, and that might help us find better names.

[Leaf, 2015-10-13 16:46:28]

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/context.dart:1154: entry.setState(RESOLVED_UNIT9,
CacheState.FLUSHED);
On 2015/10/13 14:21:19, Brian Wilkerson wrote:
> Should this flush RESOLVED_UNIT10?

Good catch, thanks. Done.  Also below.

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:487: * The additional strong mode errors
produced while scanning a compilation unit.
On 2015/10/13 15:21:20, scheglov wrote:
> Probably not "scanning".

Done.

https://codereview.chromium.org/1396273004/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:491: * The result is only available for
[Source]s representing a compilation unit.
On 2015/10/13 14:21:19, Brian Wilkerson wrote:
> "Source" --> "LibrarySpecificUnit"

Done.

[Leaf, 2015-10-13 17:16:39]

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