Initial cut for a development server

Initial cut for a development server

R=jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2015-03-03 01:19:57]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-03-03 01:24:19]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2015-03-03 01:34:13]

sigmund@google.com changed reviewers:
+ jmesserly@google.com, leafp@google.com, vsm@google.com

[Siggi Cherem (dart-lang), 2015-03-03 01:34:14]

Just a couple general things to point out:

 - `dependency_graph.dart` and `dependency_graph_test.dart` do not depend on the
rest of the changes, so they can be reviewed separately if you like (let me know
if you prefer that I pull them on a separate CL, I'd be happy to do so)

 - instead of keeping the output in memory, I still emit the output to an output
folder and use a simple server to serve up those files.

 - changes in codegen are mainly to have the output in a predictable location
and self contained, so that the server can find them easily.

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:975: new JS.PropertyName(name),
js.call('function(_) {}'),
@jmesserly - I thought this change to remove the argument here was on purpose,
but for some reason it's failing for me. Maybe I'm doing something wrong?

I get this error in Chrome v42.0.2311.11:

  Uncaught SyntaxError: Unexpected token )

[Jennifer Messerly, 2015-03-03 02:24:22]

this is looking really nice

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart
File lib/devc.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode6
lib/devc.dart:6: library ddc.devc;
I wonder if this should be in "src"? just noticed we've had a decent amount of
churn in this lib

unrelated note: we forgot to rename "ddc" here

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode51
lib/devc.dart:51: List<LibraryInfo> libraries = <LibraryInfo>[];
since this is in a public library, do we want to protect any of this state?
I'm thinking specifically "libraries" ... not sure if it makes sense to mutate
it except from inside the class

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode101
lib/devc.dart:101: }
what other kinds of nodes are possible?

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode123
lib/devc.dart:123: // TODO(sigmund): split this out as a separate node in our
dependency graph?
+1 .. also that way it could have mtime tracking

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode180
lib/devc.dart:180: CheckerResults runOnce() {
made this comment elsewhere, but I think "run" would be a good name for this

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode208
lib/devc.dart:208: if (reporter is SummaryReporter) (reporter as
SummaryReporter).clear();
WAT :)

can we file this somewhere?

another idea to maybe help, is have a getter for `summaryReporter` that is typed
... or have these methods on CheckerReporter but have them do nothing ...

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/html_cod...
File lib/src/codegen/html_codegen.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/html_cod...
lib/src/codegen/html_codegen.dart:88: final _log = new
Logger('ddc.src.codegen.html_codegen');
dev_compiler?

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (left):

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1801: SourceLocation _location(int offset) {
seems worth keeping this, but simplify impl?

_location(offset) => locationFor(unit, uri, offset);

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:975: new JS.PropertyName(name),
js.call('function(_) {}'),
On 2015/03/03 01:34:14, Siggi Cherem (dart-lang) wrote:
> @jmesserly - I thought this change to remove the argument here was on purpose,
> but for some reason it's failing for me. Maybe I'm doing something wrong?
> 
> I get this error in Chrome v42.0.2311.11:
> 
>   Uncaught SyntaxError: Unexpected token )

yeah, seems that 1 arg is required for the setter per spec.

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

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:1: /// Tracks the shape of the import/export graph
and dependencies between files.
copyright?

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:34: AnalysisContext _context;
final?

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:111: Set<LibrarySourceNode> scripts = new
Set<LibrarySourceNode>();
final?

also, we are officially Java :)

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:284: var apiChangeDetected = new
Set<SourceNode>();
by default Set is LinkedHashSet, which preserves iteration order as insertion
order. Are you exploiting that in here? If not, it might be worth switching to
`new HashSet` throughout.

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:311: if (seen == null) seen = new
Set<SourceNode>();
same comment on HashSet vs LinkedHashSet

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:312: if (seen.contains(node)) return;
you can combine the `contains` and `add` in one hash operation:

if (seen.add(node)) {
  ... we found a new item ...
}

(add returns `true` if it was added, false otherwise)

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart#newco...
lib/src/utils.dart:239: SourceLocation locationFor(CompilationUnit unit, Uri
uri, int offset) {
not sure the "For" adds much. "getSourceLocation"?

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart#newco...
lib/src/utils.dart:253: var text = new StringBuffer();
I presume there is a method to this madness :-)

guessing here ... can we get back to the Source to actually just copy the input
text on demand?
however we get at the text, it seems really important to do it lazily ... spans
should be O(1) to create not O(NumTokensInSpan)

https://codereview.chromium.org/973433003/diff/40001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/973433003/diff/40001/pubspec.yaml#newcode20
pubspec.yaml:20: shelf: ^0.5.7
trivial ... are we sorting this? because these would go a bit higher (and
source_span is out of place)

https://codereview.chromium.org/973433003/diff/40001/test/checker/self_host_t...
File test/checker/self_host_test.dart (right):

https://codereview.chromium.org/973433003/diff/40001/test/checker/self_host_t...
test/checker/self_host_test.dart:24: new Compiler(options).runOnce();
I feel like "run()" would read better at most call sites

(usually when calling a method "once" is implied :-) )

https://codereview.chromium.org/973433003/diff/40001/test/dependency_graph_te...
File test/dependency_graph_test.dart (right):

https://codereview.chromium.org/973433003/diff/40001/test/dependency_graph_te...
test/dependency_graph_test.dart:1: library ddc.test.dependency_graph_test;
copyright header?

[Siggi Cherem (dart-lang), 2015-03-04 04:44:24]

thanks for all the comments!

PTAL

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart
File lib/devc.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode6
lib/devc.dart:6: library ddc.devc;
On 2015/03/03 02:24:21, John Messerly wrote:
> I wonder if this should be in "src"? just noticed we've had a decent amount of
> churn in this lib

good point - now that I made most things private, it seems ok to keep it here,
but I'd be OK moving things further down in /src/

Overall, I'm not sure there is much we want to have in lib, almost everything we
want to expose in this package will come from bin/ 

> 
> unrelated note: we forgot to rename "ddc" here

filed: https://github.com/dart-lang/dev_compiler/issues/84

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode51
lib/devc.dart:51: List<LibraryInfo> libraries = <LibraryInfo>[];
On 2015/03/03 02:24:21, John Messerly wrote:
> since this is in a public library, do we want to protect any of this state?
> I'm thinking specifically "libraries" ... not sure if it makes sense to mutate
> it except from inside the class

Good point - actually all of the properties here should be private. Done.

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode101
lib/devc.dart:101: }
On 2015/03/03 02:24:21, John Messerly wrote:
> what other kinds of nodes are possible?

True, at this point no other kind of node should reach here (PartSourceNode will
not reach here). I added an assertion to make this more clear.

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode123
lib/devc.dart:123: // TODO(sigmund): split this out as a separate node in our
dependency graph?
On 2015/03/03 02:24:21, John Messerly wrote:
> +1 .. also that way it could have mtime tracking

Good point - I've opened https://github.com/dart-lang/dev_compiler/issues/85 to
track.

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode180
lib/devc.dart:180: CheckerResults runOnce() {
On 2015/03/03 02:24:21, John Messerly wrote:
> made this comment elsewhere, but I think "run" would be a good name for this

Done.

https://codereview.chromium.org/973433003/diff/40001/lib/devc.dart#newcode208
lib/devc.dart:208: if (reporter is SummaryReporter) (reporter as
SummaryReporter).clear();
On 2015/03/03 02:24:21, John Messerly wrote:
> WAT :)
> 
> can we file this somewhere?
> 
> another idea to maybe help, is have a getter for `summaryReporter` that is
typed
> ... or have these methods on CheckerReporter but have them do nothing ...

It's sad, the promotion rules only apply to local variables. Even though this
field is final. What's crazy is that I can write this:

  CheckerReporter reporter2 = reporter;
  if (reporter2 is SummaryReporter) reporter2.clear();

and get no warnings.

Maybe we should create a DEP to improve type-promotion?

Meanwhile, I'm inclined to keep it as it is, this is likely going to change
again soon :)

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/html_cod...
File lib/src/codegen/html_codegen.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/html_cod...
lib/src/codegen/html_codegen.dart:88: final _log = new
Logger('ddc.src.codegen.html_codegen');
On 2015/03/03 02:24:21, John Messerly wrote:
> dev_compiler?

Good point - I'll send out a follow up CL with this one, so I change all of them
at once (together with the library renames)

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/js_codeg...
File lib/src/codegen/js_codegen.dart (left):

https://codereview.chromium.org/973433003/diff/40001/lib/src/codegen/js_codeg...
lib/src/codegen/js_codegen.dart:1801: SourceLocation _location(int offset) {
On 2015/03/03 02:24:21, John Messerly wrote:
> seems worth keeping this, but simplify impl?
> 
> _location(offset) => locationFor(unit, uri, offset);
>  

Done.

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

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:1: /// Tracks the shape of the import/export graph
and dependencies between files.
On 2015/03/03 02:24:21, John Messerly wrote:
> copyright?

Done.

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:34: AnalysisContext _context;
On 2015/03/03 02:24:22, John Messerly wrote:
> final?

Done.

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:111: Set<LibrarySourceNode> scripts = new
Set<LibrarySourceNode>();
On 2015/03/03 02:24:21, John Messerly wrote:
> final?

In this case no, we update the entire list every time we detect a structural
change in the file. We could replace the contents of the list, or make the field
read-only from outside this library, but I'm not sure that's worth it.

> 
> also, we are officially Java :)

sigh...

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:284: var apiChangeDetected = new
Set<SourceNode>();
On 2015/03/03 02:24:22, John Messerly wrote:
> by default Set is LinkedHashSet, which preserves iteration order as insertion
> order. Are you exploiting that in here? If not, it might be worth switching to
> `new HashSet` throughout.

I'm not depending on it, so I went ahead and did the change, but I sort of like
how the code reads by just doing `new Set`. Maybe because it feels odd to
specify a specific implementation if I don't rely on the implementation
internals?

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:311: if (seen == null) seen = new
Set<SourceNode>();
On 2015/03/03 02:24:21, John Messerly wrote:
> same comment on HashSet vs LinkedHashSet

Done.

https://codereview.chromium.org/973433003/diff/40001/lib/src/dependency_graph...
lib/src/dependency_graph.dart:312: if (seen.contains(node)) return;
On 2015/03/03 02:24:22, John Messerly wrote:
> you can combine the `contains` and `add` in one hash operation:
> 
> if (seen.add(node)) {
>   ... we found a new item ...
> }
> 
> (add returns `true` if it was added, false otherwise)

thx, Done

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart#newco...
lib/src/utils.dart:239: SourceLocation locationFor(CompilationUnit unit, Uri
uri, int offset) {
On 2015/03/03 02:24:22, John Messerly wrote:
> not sure the "For" adds much. "getSourceLocation"?

What do you think about locationForOffset?

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart#newco...
lib/src/utils.dart:253: var text = new StringBuffer();
On 2015/03/03 02:24:22, John Messerly wrote:
> I presume there is a method to this madness :-)
> 
> guessing here ... can we get back to the Source to actually just copy the
input
> text on demand?
> however we get at the text, it seems really important to do it lazily ...
spans
> should be O(1) to create not O(NumTokensInSpan)

yeah, this was more twisted than it needed to be :)

Normally you'd get this data by doing:

  unit.source.contents.data

however, the unit I'm getting here doesn't have a link to its source because I'm
using a digest parsing that only parses directives.

Of course, at the point were I used this I had the source available, so now I'm
just passing that in here.

https://codereview.chromium.org/973433003/diff/40001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/973433003/diff/40001/pubspec.yaml#newcode20
pubspec.yaml:20: shelf: ^0.5.7
On 2015/03/03 02:24:22, John Messerly wrote:
> trivial ... are we sorting this? because these would go a bit higher (and
> source_span is out of place)

Done.

https://codereview.chromium.org/973433003/diff/40001/test/checker/self_host_t...
File test/checker/self_host_test.dart (right):

https://codereview.chromium.org/973433003/diff/40001/test/checker/self_host_t...
test/checker/self_host_test.dart:24: new Compiler(options).runOnce();
On 2015/03/03 02:24:22, John Messerly wrote:
> I feel like "run()" would read better at most call sites
> 
> (usually when calling a method "once" is implied :-) )

sgtm :)

https://codereview.chromium.org/973433003/diff/40001/test/dependency_graph_te...
File test/dependency_graph_test.dart (right):

https://codereview.chromium.org/973433003/diff/40001/test/dependency_graph_te...
test/dependency_graph_test.dart:1: library ddc.test.dependency_graph_test;
On 2015/03/03 02:24:22, John Messerly wrote:
> copyright header?

Done.

[Jennifer Messerly, 2015-03-04 15:35:10]

lgtm

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart#newco...
lib/src/utils.dart:239: SourceLocation locationFor(CompilationUnit unit, Uri
uri, int offset) {
On 2015/03/04 04:44:24, Siggi Cherem (dart-lang) wrote:
> On 2015/03/03 02:24:22, John Messerly wrote:
> > not sure the "For" adds much. "getSourceLocation"?
> 
> What do you think about locationForOffset?

yeah that works too.
Just curious, is it intentional to avoid the "get" verb?

https://codereview.chromium.org/973433003/diff/60001/lib/devc.dart
File lib/devc.dart (right):

https://codereview.chromium.org/973433003/diff/60001/lib/devc.dart#newcode110
lib/devc.dart:110: bool _devCompilerRuntimeCopied = false;
since this is a field, maybe put up top? We seem to have that sort of informal
convention.
(except, I probably don't follow it entirely in js_codegen either :-) )

[Siggi Cherem (dart-lang), 2015-03-04 17:38:16]

Thanks John!

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/973433003/diff/40001/lib/src/utils.dart#newco...
lib/src/utils.dart:239: SourceLocation locationFor(CompilationUnit unit, Uri
uri, int offset) {
On 2015/03/04 15:35:10, John Messerly wrote:
> On 2015/03/04 04:44:24, Siggi Cherem (dart-lang) wrote:
> > On 2015/03/03 02:24:22, John Messerly wrote:
> > > not sure the "For" adds much. "getSourceLocation"?
> > 
> > What do you think about locationForOffset?
> 
> yeah that works too.
> Just curious, is it intentional to avoid the "get" verb?

funny - technically no, but every time I start naming something with `get` it
gets me thinking about whether there is a more descriptive way to name it.
Adding `get` in  `getLocationForOffset` seemed unnecessary. What's your
inclination on it?

https://codereview.chromium.org/973433003/diff/60001/lib/devc.dart
File lib/devc.dart (right):

https://codereview.chromium.org/973433003/diff/60001/lib/devc.dart#newcode110
lib/devc.dart:110: bool _devCompilerRuntimeCopied = false;
On 2015/03/04 15:35:10, John Messerly wrote:
> since this is a field, maybe put up top? We seem to have that sort of informal
> convention.
> (except, I probably don't follow it entirely in js_codegen either :-) )

Done.

[Siggi Cherem (dart-lang), 2015-03-04 17:40:11]

Committed patchset #3 (id:80001) manually as
ade0bf1b7e5321491e454fe9cdea6bfea84554d4 (presubmit successful).