fixes #6, refactor to use AnalysisError/Listener throughout dev_compiler

fixes #6, refactor to use AnalysisError/Listener throughout dev_compiler

Main goal here was to get rid of the enter/exit compilation unit tracking, to allow further Compiler/Server refactoring. We can get the Uri/LineInfo/contents off Source, which AnalysisError already has
SummaryReporter is now fairly generic.

TODOs for the future:
* figure out what to do about ErrorCodes, in particular dev_compiler prefix feels off.
* skip the intermediate StaticInfo types? They're really just builders now for AnalysisError.
* don't classify all existing messages as "AnalyzerError" ... just print them ...
* skip logger. For our command line program, stdout is part of its "API", IMO it shouldn't use logging for those messages. Consider Pub, which uses stdout/err for its messages, and uses logging for more detailed troubleshooting.

R=pquitslund@google.com, vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/032419ed3c296f32ab980aec57db947afb06c7b6

[Jennifer Messerly, 2015-07-09 20:41:24]

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

[vsm, 2015-07-09 23:54:08]

lgtm

https://codereview.chromium.org/1230903002/diff/1/lib/src/dependency_graph.dart
File lib/src/dependency_graph.dart (right):

https://codereview.chromium.org/1230903002/diff/1/lib/src/dependency_graph.da...
lib/src/dependency_graph.dart:241: // TODO(jmesserly): should these be errors or
warnings?
Mostly look like real errors (e.g., inline scripts not supported, file not
found).

[pquitslund, 2015-07-10 05:17:24]

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

[pquitslund, 2015-07-10 05:17:25]

https://codereview.chromium.org/1230903002/diff/1/lib/devc.dart
File lib/devc.dart (right):

https://codereview.chromium.org/1230903002/diff/1/lib/devc.dart#newcode13
lib/devc.dart:13: show AnalysisError, AnalysisErrorListener, ErrorSeverity,
ErrorType;
Nit: I would sort this import after engine.dart.

https://codereview.chromium.org/1230903002/diff/1/lib/src/info.dart
File lib/src/info.dart (right):

https://codereview.chromium.org/1230903002/diff/1/lib/src/info.dart#newcode240
lib/src/info.dart:240: toErrorCode() => new HintCode(name, message);
Style question (for my edification): what's the rationale for eliding the return
type?

[pquitslund, 2015-07-10 05:18:19]

Oh, and needless to say (and FWIW) LGTM!

[Jennifer Messerly, 2015-07-10 16:12:05]

https://codereview.chromium.org/1230903002/diff/1/lib/devc.dart
File lib/devc.dart (right):

https://codereview.chromium.org/1230903002/diff/1/lib/devc.dart#newcode13
lib/devc.dart:13: show AnalysisError, AnalysisErrorListener, ErrorSeverity,
ErrorType;
On 2015/07/10 05:17:25, pquitslund wrote:
> Nit: I would sort this import after engine.dart.

Done.

https://codereview.chromium.org/1230903002/diff/1/lib/src/dependency_graph.dart
File lib/src/dependency_graph.dart (right):

https://codereview.chromium.org/1230903002/diff/1/lib/src/dependency_graph.da...
lib/src/dependency_graph.dart:241: // TODO(jmesserly): should these be errors or
warnings?
On 2015/07/09 23:54:07, vsm wrote:
> Mostly look like real errors (e.g., inline scripts not supported, file not
> found).

yeah, maybe ... on the other hand, HTML doesn't really have a notion of
"errors", at least not in the sense that blocks rendering. HTML parse errors
aren't even usually displayed normally, except by linters. (which is kind of a
shame, because there's a lot of useful parse errors in the spec!)

similarly, html errors don't really affect soundness (at least I hope so ;-) )
... so if we treat them as warnings, worst case, your app won't load, best cast,
maybe you can still develop some other parts (like the CSS). if we don't
generate the HTML file ... your app totally won't load. e.g. for inline scripts,
maybe you have one inline script and one src= script, you could still debug the
src= app even if the inline one doesn't work.

it also depends on --force-compile ... if we have that on always for --server,
then we have less incentive to lower the error level.

anyway, not saying I'm in favor of warnings, just don't know that it's clear
cut. Seemed worth adding a TODO at least :)

https://codereview.chromium.org/1230903002/diff/1/lib/src/info.dart
File lib/src/info.dart (right):

https://codereview.chromium.org/1230903002/diff/1/lib/src/info.dart#newcode240
lib/src/info.dart:240: toErrorCode() => new HintCode(name, message);
On 2015/07/10 05:17:25, pquitslund wrote:
> Style question (for my edification): what's the rationale for eliding the
return
> type?

Good question!

Here, it's included both the method name (ErrorCode) and expression on the right
side, as well as being easily reachable by clicking to jump to the overridden
method. In other words, both variants would repeat the type name twice on the
same line:

    ErrorCode toErrorCode() => new HintCode(name, message);
    HintCode toErrorCode() => new HintCode(name, message);

the second one has the additional downside that you now need to change two
places if you want to change which error code you're using.

for what it's worth, DDC will do this inference. Since the base type declares
ErrorCode as return type, this override would be unsound as it's declared
(dynamic </: ErrorCode), so we chose to make it work, by taking the return type
from the overridden method.

(That said, I'm hoping we can refactor this code up further in DDC, it still
doesn't quite copy the style of how ErrorCodes are set up in analyzer. So this
method probably goes away at some point.)

[Jennifer Messerly, 2015-07-10 17:15:12]

Committed patchset #2 (id:20001) manually as
032419ed3c296f32ab980aec57db947afb06c7b6 (presubmit successful).