Recompile unit with potential conflict/dependency on some top-level symbol.

Recompile unit with potential conflict/dependency on some top-level symbol.

1. Change dependency tracking from API version on unit to just unit last modified time.

2. Track sets of top-level and all declared symbols in units, recompile in case of possible conflict.

3. Track units with TypeErrorCode.CANNOT_BE_RESOLVED, recompile if any unit changes top-level symbols.

4. Better tests with source code directly in test method.


R=zundel@google.com
BUG=
TEST=

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

[zundel, 2012-01-12 17:11:47]

I didn't get very far, but wanted to go ahead and give you this feedback.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (left):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:519: String hash =
Integer.toString(depNode.computeHash());
Where did this test go?

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:1082: * Compiles the
source file which could be a single *.dart source file with
<code>#library</code>
I think the comment needs more revising.  What do you think of this?

Treat the sourceFile as the top level library and generate compiled output by
linking the dart source in this file with all libraries referenced with #import
statements.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompilerMainContext.java
(right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompilerMainContext.java:134: return
ImmutableList.of();
Collections.emptyList() might be more readable, I thought you were missing an
argument.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/LibraryDeps.java (right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:51: return
Objects.equal(libUri, dep.libUri) && Objects.equal(unitName, dep.unitName);
oh, I like Objects.equal.  I'll have to remember it.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:131: while (null !=
(unitName = buf.readLine())) {
the format of the library deps file changes in this CL.  Are folks upgrading the
compiler with an existing project going to get weird errors when trying to read
.deps files created with an old version of the compiler?

If this is an issue, you could solve this issue for everyone by making the first
line of the file contain a version number that represents the version of the
file format that can be incremented if the format changes again.

I'm pretty sure this problem is responsible for the exception I'm getting when
trying to recompile a test program under this patch:


$ dartc /tmp/t.dart
java.lang.NullPointerException
	at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:187)
	at com.google.common.base.Splitter.split(Splitter.java:364)
	at com.google.dart.compiler.LibraryDeps.fromReader(LibraryDeps.java:138)
	at com.google.dart.compiler.ast.LibraryUnit.getDeps(LibraryUnit.java:406)
	at
com.google.dart.compiler.DartCompiler$Compiler.parseOutOfDateFiles(DartCompiler.java:256)
	at
com.google.dart.compiler.DartCompiler$Compiler.updateAndResolve(DartCompiler.java:208)
	at
com.google.dart.compiler.DartCompiler$Compiler.compile(DartCompiler.java:173)
	at
com.google.dart.compiler.DartCompiler$Compiler.access$200(DartCompiler.java:125)
	at com.google.dart.compiler.DartCompiler.compileLib(DartCompiler.java:1192)
	at com.google.dart.compiler.DartCompiler.compileLib(DartCompiler.java:1169)
	at com.google.dart.compiler.DartCompiler.compileApp(DartCompiler.java:1110)
	at com.google.dart.compiler.DartCompiler.compilerMain(DartCompiler.java:1043)
	at com.google.dart.compiler.DartCompiler.compilerMain(DartCompiler.java:1030)
	at com.google.dart.compiler.DartCompiler.main(DartCompiler.java:990)

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:141: // Read all
symbols.
this terminology confuses me a bit.  We have top symbols, and we have all
symbols.  Is all symbols a superset of top symbols?  If so, there is redundant
information in the file and we could just combine the two together.  Otherwise,
maybe 'all symbols' could be renamed to 'nonTopSymbols' or 'scopedSymbols' or
something like that.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:172: private final
Map<String, Source> sources = Maps.newTreeMap();
nit.  While it is simpler to use a TreeMap to keep the keys in sorted order, if
you only need the keys sorted once, it is probably more efficient (probably
marginally in this case) to just sort the keys right before writing in write(). 
I'm thinking it will keep the in-memory data structure smaller and reduces the
cost of all other operations in the Map to O(1).

If I have missed another place where there is a need to keep the keys sorted
then you can probably disregard this comment.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:181: return
sources.keySet();
Maybe you should return this as a Set, or as immutableSet noting how this method
is used in DartCompiler.parseOutOfDateFiles().

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:195: return
sources.get(unitName);
nit. with TreeMap, this operation would turn from O(1) to O(log n)

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:221: // XXX
? you were saying ?

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:224: if
(error.getErrorCode() == TypeErrorCode.CANNOT_BE_RESOLVED) {
I'm a little leery of adding such a statement here that picks out a seemingly
arbitrary error code.  If we changed the error code to have multiple flavors for
different resolution problems, it would introduce a very subtle bug.

Maybe it would be better to add a flag into the DartErrorCode datastructure that
would communicate that this error makes the unit stale.  It would always be set
for ResovlerErrorCode, ParserErrorCode, and then conditionally for
TypeErrorCode.

[zundel, 2012-01-12 19:26:59]

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/LibraryUnit.java (right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:353: * Get an
unmodifiable collection of the to-level symbols in this library. You must call
to-level --> top-level

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:356: public
Set<String> getTopLevelNames() {
assert topLevelNodes != null?

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:364: public
Collection<DartNode> getTopLevelNodes() {
assert topLevelNodes != null

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/Elements.java (right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:565: new
MapMaker().weakKeys().makeMap();
reservation on this scheme in general - if we are going to keep a persistent
compiler around (we don't do this now), this is an odd place to store state.  

It might not be quite as quick, but maybe cleaner in terms of leaving state
lying around, to write a visitor to implement getDeclarationNames() instead of
storing them this way.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:573: &&
element.getNode().getRoot() instanceof DartUnit) {
out of curiosity, are there any nodes that aren't rooted in DartUnit?

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:574: DartUnit unit
= (DartUnit) element.getNode().getRoot();
nit, its somewhat expensive to call this twice (once in conditional, and again
to manipulate it), as it involves a walk up the tree.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:592: public static
Set<String> getDeclarationNames(DartUnit unit) {
nit. Does this method belong in Elements?  What does it have to do with the
class Element?  Maybe it should be a method on DartUnit.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:600: public static
Set<String> getTopDeclarationNames(DartUnit unit) {
nit. Does this method belong in Elements?  What does it have to do with the
class Element?  Maybe it should be a method on DartUnit.

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
File
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java
(right):

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:39:
private static final String TEST_APP = "my.app.dart";
The whole .app concept is out moded.  if you are writing a new test, might as
well get rid of it.  How about calling this file myApp.dart or something like
that?

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:84:
"#library('my.app');",
same comment, .app files are a holdover from an old specification of the
language.

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:132:
didNotWrite("B.dart", EXTENSION_JS);
I don't remember how we resolved our discussion on this point.  Shouldn't B be
recompiled in order to get the shadowing warnings (member A.not_hole, defined in
B.dart now shadows top level not_hole)?  (We have all the information in the
.deps file now to compute this)

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:277:
}
do you want to add a test where you start with B.Dart containing a top level,
then compile, then add A.dart with the same top-level and make sure that both
units return errors (not just A.dart). (just tried it myself, it works)

[scheglov, 2012-01-13 15:55:35]

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (left):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:519: String hash =
Integer.toString(depNode.computeHash());
On 2012/01/12 17:11:47, zundel wrote:
> Where did this test go?

We don't need it.
Now we use dependency on unit, not on structure.
So, we compare time stamps of dependency and unit on disc.
Do you want to compare hash codes of full unit content instead?
This seems a little unusual from what I saw in other compilers.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:1082: * Compiles the
source file which could be a single *.dart source file with
<code>#library</code>
On 2012/01/12 17:11:47, zundel wrote:
> I think the comment needs more revising.  What do you think of this?
> 
> Treat the sourceFile as the top level library and generate compiled output by
> linking the dart source in this file with all libraries referenced with
#import
> statements.

Done.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompilerMainContext.java
(right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompilerMainContext.java:134: return
ImmutableList.of();
On 2012/01/12 17:11:47, zundel wrote:
> Collections.emptyList() might be more readable, I thought you were missing an
> argument.

Done.

Well, I'd prefer to use Guava where possible, but I agree with you because
Collections.emptyList() is used in many places. Note that ImmutableList.of() is
also used, but not with empty argumetns list.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/LibraryDeps.java (right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:131: while (null !=
(unitName = buf.readLine())) {
OK, now we will write and check version.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:141: // Read all
symbols.
On 2012/01/12 17:11:47, zundel wrote:
> this terminology confuses me a bit.  We have top symbols, and we have all
> symbols.  Is all symbols a superset of top symbols?  If so, there is redundant
> information in the file and we could just combine the two together. 
Otherwise,
> maybe 'all symbols' could be renamed to 'nonTopSymbols' or 'scopedSymbols' or
> something like that.

Yes, these are "top" and "all" symbols.
"all" includes "top".
"top" is exactly as it says - top-level symbols, i.e. classes, top-level fields
and methods. This is not same as "api", i.e. does not include fields/ethids of
classes.
For check of conflict with top-level symbols we need exactly "all", plus to
separate "top" from "all" we need to do special step, and only to merge them
later in memory (or check two sets in conflict check).

So, I'd prefer to keep "top" and "all" as is.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:172: private final
Map<String, Source> sources = Maps.newTreeMap();
On 2012/01/12 17:11:47, zundel wrote:
> nit.  While it is simpler to use a TreeMap to keep the keys in sorted order,
if
> you only need the keys sorted once, it is probably more efficient (probably
> marginally in this case) to just sort the keys right before writing in
write(). 
> I'm thinking it will keep the in-memory data structure smaller and reduces the
> cost of all other operations in the Map to O(1).
> 
> If I have missed another place where there is a need to keep the keys sorted
> then you can probably disregard this comment.

OK, fixed.

I actually think that HashMap should be smaller, but slower.
I don't need sorting, so HashMap is better.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:181: return
sources.keySet();
On 2012/01/12 17:11:47, zundel wrote:
> Maybe you should return this as a Set, or as immutableSet noting how this
method
> is used in DartCompiler.parseOutOfDateFiles().

Done.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:195: return
sources.get(unitName);
On 2012/01/12 17:11:47, zundel wrote:
> nit. with TreeMap, this operation would turn from O(1) to O(log n)

Done.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:221: // XXX
On 2012/01/12 17:11:47, zundel wrote:
> ? you were saying ?

I put comments like this as placeholders for comment.
Fixed.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/LibraryDeps.java:224: if
(error.getErrorCode() == TypeErrorCode.CANNOT_BE_RESOLVED) {
On 2012/01/12 17:11:47, zundel wrote:
> I'm a little leery of adding such a statement here that picks out a seemingly
> arbitrary error code.  If we changed the error code to have multiple flavors
for
> different resolution problems, it would introduce a very subtle bug.
> 
> Maybe it would be better to add a flag into the DartErrorCode datastructure
that
> would communicate that this error makes the unit stale.  It would always be
set
> for ResovlerErrorCode, ParserErrorCode, and then conditionally for
> TypeErrorCode.

Done.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/ast/LibraryUnit.java (right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:353: * Get an
unmodifiable collection of the to-level symbols in this library. You must call
On 2012/01/12 19:26:59, zundel wrote:
> to-level --> top-level

This method was removed.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:356: public
Set<String> getTopLevelNames() {
On 2012/01/12 19:26:59, zundel wrote:
> assert topLevelNodes != null?

This method was removed.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:364: public
Collection<DartNode> getTopLevelNodes() {
On 2012/01/12 19:26:59, zundel wrote:
> assert topLevelNodes != null

This method was removed.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/Elements.java (right):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:565: new
MapMaker().weakKeys().makeMap();
On 2012/01/12 19:26:59, zundel wrote:
> reservation on this scheme in general - if we are going to keep a persistent
> compiler around (we don't do this now), this is an odd place to store state.  
> 
> It might not be quite as quick, but maybe cleaner in terms of leaving state
> lying around, to write a visitor to implement getDeclarationNames() instead of
> storing them this way.

Done.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:573: &&
element.getNode().getRoot() instanceof DartUnit) {
On 2012/01/12 19:26:59, zundel wrote:
> out of curiosity, are there any nodes that aren't rooted in DartUnit?

Dangling node, disconnected from DartUnit?
Method removed in any case.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:574: DartUnit unit
= (DartUnit) element.getNode().getRoot();
On 2012/01/12 19:26:59, zundel wrote:
> nit, its somewhat expensive to call this twice (once in conditional, and again
> to manipulate it), as it involves a walk up the tree.

This method was removed.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:592: public static
Set<String> getDeclarationNames(DartUnit unit) {
On 2012/01/12 19:26:59, zundel wrote:
> nit. Does this method belong in Elements?  What does it have to do with the
> class Element?  Maybe it should be a method on DartUnit.

Done.

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:600: public static
Set<String> getTopDeclarationNames(DartUnit unit) {
On 2012/01/12 19:26:59, zundel wrote:
> nit. Does this method belong in Elements?  What does it have to do with the
> class Element?  Maybe it should be a method on DartUnit.

Done.

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
File
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java
(right):

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:39:
private static final String TEST_APP = "my.app.dart";
On 2012/01/12 19:26:59, zundel wrote:
> The whole .app concept is out moded.  if you are writing a new test, might as
> well get rid of it.  How about calling this file myApp.dart or something like
> that?

Done.

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:84:
"#library('my.app');",
On 2012/01/12 19:26:59, zundel wrote:
> same comment, .app files are a holdover from an old specification of the
> language.

AFAIK we can use any name here.
Tweaked.

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:132:
didNotWrite("B.dart", EXTENSION_JS);
On 2012/01/12 19:26:59, zundel wrote:
> I don't remember how we resolved our discussion on this point.  Shouldn't B be
> recompiled in order to get the shadowing warnings (member A.not_hole, defined
in
> B.dart now shadows top level not_hole)?  (We have all the information in the
> .deps file now to compute this)
> 

Fixed recompilation, but without producing shadowing error.
Will be solved as issue 1159.

http://codereview.chromium.org/9148026/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:277:
}
On 2012/01/12 19:26:59, zundel wrote:
> do you want to add a test where you start with B.Dart containing a top level,
> then compile, then add A.dart with the same top-level and make sure that both
> units return errors (not just A.dart). (just tried it myself, it works)

Done.

[mmendez, 2012-01-13 16:05:23]

Konstantine, I'd like to take a detailed look at this CL as well before you
commit it, but there are few things I need to pop off the stack first.

[codefu, 2012-01-13 18:21:21]

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/DartCompiler.java (left):

http://codereview.chromium.org/9148026/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/DartCompiler.java:519: String hash =
Integer.toString(depNode.computeHash());
On 2012/01/13 15:55:37, scheglov wrote:
> On 2012/01/12 17:11:47, zundel wrote:
> > Where did this test go?
> 
> We don't need it.
> Now we use dependency on unit, not on structure.
> So, we compare time stamps of dependency and unit on disc.
> Do you want to compare hash codes of full unit content instead?
> This seems a little unusual from what I saw in other compilers.

They were trying to optimize the case where the API didn't change, but the body
of a block did, in which case don't recompile people that wont see a change.  If
we're getting ride of the hash codes, then the API files are now virtually
obsolete with regards to diet parsed code.

[scheglov, 2012-01-13 18:28:26]

> > 
> > We don't need it.
> > Now we use dependency on unit, not on structure.
> > So, we compare time stamps of dependency and unit on disc.
> > Do you want to compare hash codes of full unit content instead?
> > This seems a little unusual from what I saw in other compilers.
> 
> They were trying to optimize the case where the API didn't change, but the
body
> of a block did, in which case don't recompile people that wont see a change. 
If
> we're getting ride of the hash codes, then the API files are now virtually
> obsolete with regards to diet parsed code.

API files are still very useful, because if some unit does not depend on
anything changed, we need to parse (and analyze) only its API version of source,
i.e. much less and faster.

[codefu, 2012-01-13 18:32:11]

On 2012/01/13 18:28:26, scheglov wrote:
> > > 
> > > We don't need it.
> > > Now we use dependency on unit, not on structure.
> > > So, we compare time stamps of dependency and unit on disc.
> > > Do you want to compare hash codes of full unit content instead?
> > > This seems a little unusual from what I saw in other compilers.
> > 
> > They were trying to optimize the case where the API didn't change, but the
> body
> > of a block did, in which case don't recompile people that wont see a change.

> If
> > we're getting ride of the hash codes, then the API files are now virtually
> > obsolete with regards to diet parsed code.
> 
> API files are still very useful, because if some unit does not depend on
> anything changed, we need to parse (and analyze) only its API version of
source,
> i.e. much less and faster.

We can just run a diet parse of the larger file and skip {body} tokens.  I think
you'll find the reduction in complexity is much nicer to the small gains in
parsing the API file.

[zundel, 2012-01-18 15:12:52]

just a couple of random comments I had not sent out to you.

http://codereview.chromium.org/9148026/diff/7001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/9148026/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:543: /*for (String
unitName : deps.getUnitNames()) {
Could you delete this commented out code before you commit?

http://codereview.chromium.org/9148026/diff/7001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/DartCompilerErrorCode.java (right):

http://codereview.chromium.org/9148026/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompilerErrorCode.java:55: public
boolean needsRecompilation() {
Maybe this could default to getErrorSeverity().equals(ErrorSeverity.ERROR));

[scheglov, 2012-01-18 15:57:23]

http://codereview.chromium.org/9148026/diff/7001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/9148026/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompiler.java:543: /*for (String
unitName : deps.getUnitNames()) {
On 2012/01/18 15:12:52, zundel wrote:
> Could you delete this commented out code before you commit?

Done.

http://codereview.chromium.org/9148026/diff/7001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/DartCompilerErrorCode.java (right):

http://codereview.chromium.org/9148026/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/DartCompilerErrorCode.java:55: public
boolean needsRecompilation() {
On 2012/01/18 15:12:52, zundel wrote:
> Maybe this could default to getErrorSeverity().equals(ErrorSeverity.ERROR));

1. Enums can be compared using ==
2. I don't think that this is needed. All compiler errors are now
ErrorSeverity.ERROR.
3. We actually don't know if warning will need recompilation or not. For example
in ResolverErrorCode shadowing class name with local variable is warning, but we
want recompilation to may be solve it.

[codefu, 2012-01-19 21:48:54]

I'm sure I missed some stuff that the other guys will pick up on.  Here's a
short list of nits.

https://chromiumcodereview.appspot.com/9148026/diff/16007/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

https://chromiumcodereview.appspot.com/9148026/diff/16007/compiler/java/com/g...
compiler/java/com/google/dart/compiler/DartCompiler.java:270: DartUnit unit =
parse(dartSrc, lib.getPrefixes(),  false);
nit: ws + should this default to false to make it more clear that this isn't a
diet parse?

https://chromiumcodereview.appspot.com/9148026/diff/16007/compiler/java/com/g...
compiler/java/com/google/dart/compiler/DartCompiler.java:303: {
Nit: I'm use to doing random blocks in C so I could define variables at random,
but do we need all the code blocked out here and other places?

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/LibraryDepsVisitor.java (right):

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/LibraryDepsVisitor.java:49: // - Add
"holes" for fields and methods found in a superclass (see LibraryDeps for
further
Comment is invalid if we have no more holes.

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/ast/LibraryUnit.java (right):

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:249: try {
fromReader() doesn't throw, you can remove the try{}finally{}

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/javatests/...
File compiler/javatests/com/google/dart/compiler/common/ErrorExpectation.java
(right):

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/common/ErrorExpectation.java:37:
return new ErrorExpectation(null, errorCode, line, column, length);
Nit: could be "", which makes the code in assertErrors a small bit cleaner.

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/javatests/...
File
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java
(right):

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:65:
"// filler filler filler filler filler filler filler filler filler filler
filler",
I'm going to channel my inner-Mendez and ask what is "filler filler filler
filler" really mean?

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/javatests/...
File
compiler/javatests/com/google/dart/compiler/resolver/NegativeResolverTest.java
(right):

https://chromiumcodereview.appspot.com/9148026/diff/22001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/resolver/NegativeResolverTest.java:335:
errEx(ResolverErrorCode.DUPLICATE_TOP_LEVEL_DEFINITION, 2, 5, 3),
ws.

[scheglov, 2012-01-20 15:59:24]

http://codereview.chromium.org/9148026/diff/22001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/LibraryDepsVisitor.java (right):

http://codereview.chromium.org/9148026/diff/22001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/LibraryDepsVisitor.java:49: // - Add
"holes" for fields and methods found in a superclass (see LibraryDeps for
further
On 2012/01/19 21:48:54, codefu wrote:
> Comment is invalid if we have no more holes.

Done.

http://codereview.chromium.org/9148026/diff/22001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/LibraryUnit.java (right):

http://codereview.chromium.org/9148026/diff/22001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:249: try {
On 2012/01/19 21:48:54, codefu wrote:
> fromReader() doesn't throw, you can remove the try{}finally{}

Done.

http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/common/ErrorExpectation.java
(right):

http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/common/ErrorExpectation.java:37:
return new ErrorExpectation(null, errorCode, line, column, length);
On 2012/01/19 21:48:54, codefu wrote:
> Nit: could be "", which makes the code in assertErrors a small bit cleaner.

Done.

http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java
(right):

http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:65:
"// filler filler filler filler filler filler filler filler filler filler
filler",
On 2012/01/19 21:48:54, codefu wrote:
> I'm going to channel my inner-Mendez and ask what is "filler filler filler
> filler" really mean?

"filler" line makes invocation long enough to force splitting argument on
several lines after automatic formatting. So, you get same Dart source structure
as you would have in an actual file. And it is so 20th Century to format source
manually...

http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/resolver/NegativeResolverTest.java
(right):

http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/resolver/NegativeResolverTest.java:335:
errEx(ResolverErrorCode.DUPLICATE_TOP_LEVEL_DEFINITION, 2, 5, 3),
On 2012/01/19 21:48:54, codefu wrote:
> ws.

Done.

[mmendez, 2012-01-23 17:37:27]

To summarize a discussion that we had offline:

This patch removes the concept of a "hole".  A hole is a dependency that is
introduced when when a subtype contains an unqualified invocation of a super
type method.  By removing this it becomes impossible to know that the subtype
needs to be recompiled if a member with the same name is introduced at the
library scope.  In that case the binding in the subtype could be wrong.

Lets go back to the language specification and double check our assumptions on
the scoping rules, etc.  Then lets circle back and discuss offline.

On 2012/01/20 15:59:24, scheglov wrote:
>
http://codereview.chromium.org/9148026/diff/22001/compiler/java/com/google/da...
> File compiler/java/com/google/dart/compiler/LibraryDepsVisitor.java (right):
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/LibraryDepsVisitor.java:49: // - Add
> "holes" for fields and methods found in a superclass (see LibraryDeps for
> further
> On 2012/01/19 21:48:54, codefu wrote:
> > Comment is invalid if we have no more holes.
> 
> Done.
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/java/com/google/da...
> File compiler/java/com/google/dart/compiler/ast/LibraryUnit.java (right):
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/ast/LibraryUnit.java:249: try {
> On 2012/01/19 21:48:54, codefu wrote:
> > fromReader() doesn't throw, you can remove the try{}finally{}
> 
> Done.
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
> File compiler/javatests/com/google/dart/compiler/common/ErrorExpectation.java
> (right):
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
> compiler/javatests/com/google/dart/compiler/common/ErrorExpectation.java:37:
> return new ErrorExpectation(null, errorCode, line, column, length);
> On 2012/01/19 21:48:54, codefu wrote:
> > Nit: could be "", which makes the code in assertErrors a small bit cleaner.
> 
> Done.
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
> File
>
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java
> (right):
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
>
compiler/javatests/com/google/dart/compiler/end2end/inc/IncrementalCompilation2Test.java:65:
> "// filler filler filler filler filler filler filler filler filler filler
> filler",
> On 2012/01/19 21:48:54, codefu wrote:
> > I'm going to channel my inner-Mendez and ask what is "filler filler filler
> > filler" really mean?
> 
> "filler" line makes invocation long enough to force splitting argument on
> several lines after automatic formatting. So, you get same Dart source
structure
> as you would have in an actual file. And it is so 20th Century to format
source
> manually...
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
> File
> compiler/javatests/com/google/dart/compiler/resolver/NegativeResolverTest.java
> (right):
> 
>
http://codereview.chromium.org/9148026/diff/22001/compiler/javatests/com/goog...
>
compiler/javatests/com/google/dart/compiler/resolver/NegativeResolverTest.java:335:
> errEx(ResolverErrorCode.DUPLICATE_TOP_LEVEL_DEFINITION, 2, 5, 3),
> On 2012/01/19 21:48:54, codefu wrote:
> > ws.
> 
> Done.

[codefu, 2012-01-27 15:20:16]

I don't think the mintmaker files should be added (perhaps by accident?).  Other
than that, lgtm.

http://codereview.chromium.org/9148026/diff/32001/tests/stub-generator/src/Mi...
File tests/stub-generator/src/MintMakerFullyIsolatedTest-generatedTest.dart
(right):

http://codereview.chromium.org/9148026/diff/32001/tests/stub-generator/src/Mi...
tests/stub-generator/src/MintMakerFullyIsolatedTest-generatedTest.dart:7:
#import("../../../tests/isolate/src/TestFramework.dart");
I'm not certain the mintmaker templates should be updated.

http://codereview.chromium.org/9148026/diff/32001/tests/stub-generator/src/Mi...
File tests/stub-generator/src/MintMakerPromiseWithStubsTest-generatedTest.dart
(right):

http://codereview.chromium.org/9148026/diff/32001/tests/stub-generator/src/Mi...
tests/stub-generator/src/MintMakerPromiseWithStubsTest-generatedTest.dart:8:
#import("../../../tests/isolate/src/TestFramework.dart");
dito on this file.

[ahe, 2012-02-08 09:15:57]

With this change, it appears you stopped saving .api files. However, the rule
compile_systemlibrary in dart-compiler.gyp still assumes that they exist.

As far as I can tell, this is fixed by doing a global search and replace of .api
with .deps in dart-compiler.gyp. I have done so in CL 9353015.

Please let me know if I got this wrong.