Remove scanBuiltinLibraries.

Remove scanBuiltinLibraries.

BUG=
R=karlklose@google.com

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

[karlklose, 2014-06-17 12:30:26]

LGTM.

https://codereview.chromium.org/339563002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/339563002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1005: /// This method is
called immediately after the [LibraryElement] [library] has
Move first part of the comments to the abstract class.

https://codereview.chromium.org/339563002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/339563002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:819:
Map<LibraryElement, LibraryDependencyNode> nodeMap =
+1.

https://codereview.chromium.org/339563002/diff/20001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/analyze_only_test.dart (right):

https://codereview.chromium.org/339563002/diff/20001/tests/compiler/dart2js/a...
tests/compiler/dart2js/analyze_only_test.dart:47: throw 'Compilation failed:
$e';
Do we need safeToString here?

https://codereview.chromium.org/339563002/diff/20001/tests/compiler/dart2js/m...
File tests/compiler/dart2js/memory_compiler.dart (right):

https://codereview.chromium.org/339563002/diff/20001/tests/compiler/dart2js/m...
tests/compiler/dart2js/memory_compiler.dart:233: Uri packageRoot =
Uri.base.resolveUri(
Why does this need a Uri.file and the libraryRoot not?

[Johnni Winther, 2014-06-17 12:52:28]

https://codereview.chromium.org/339563002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/339563002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1005: /// This method is
called immediately after the [LibraryElement] [library] has
On 2014/06/17 12:30:25, karlklose wrote:
> Move first part of the comments to the abstract class.

Done.

https://codereview.chromium.org/339563002/diff/20001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/analyze_only_test.dart (right):

https://codereview.chromium.org/339563002/diff/20001/tests/compiler/dart2js/a...
tests/compiler/dart2js/analyze_only_test.dart:47: throw 'Compilation failed:
$e';
On 2014/06/17 12:30:25, karlklose wrote:
> Do we need safeToString here?

Done.

https://codereview.chromium.org/339563002/diff/20001/tests/compiler/dart2js/m...
File tests/compiler/dart2js/memory_compiler.dart (right):

https://codereview.chromium.org/339563002/diff/20001/tests/compiler/dart2js/m...
tests/compiler/dart2js/memory_compiler.dart:233: Uri packageRoot =
Uri.base.resolveUri(
On 2014/06/17 12:30:26, karlklose wrote:
> Why does this need a Uri.file and the libraryRoot not?

[Platform.packageRoot] is platform specific and might contain C: and \

[Johnni Winther, 2014-06-17 13:00:22]

Committed patchset #3 manually as r37391 (presubmit successful).

[ahe, 2014-07-17 08:33:13]

https://codereview.chromium.org/339563002/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/339563002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1235: // when these are
handled as erroneous libraries/compilation units.
I guess it isn't clear how detrimental fatal errors are to incremental
compilation, and why that is.

When a fatal error occurs, the compiler is left in an inconsistent state. So if
the compiler instance was reused, it produces undefined results. In practice, I
have observed that fatal errors can lead to error that cannot go away, or take a
few attempts to go away. Or plain compiler crashes. I also have examples of the
compiler producing bad output.

So when a fatal error occurs, it is better to flush all state (discard the
compiler instance) and start over. This is why fatal errors are detrimental,
because the response to the user misspelling an import, or making a syntax error
results in the next compilation taking a really long time.

However, silently leaving the compiler in an inconsistent state is even worse.
If a fatal error had been reported, an exception would have been thrown, and the
compilation would have been aborted, and [compilerWasCancelled] would have been
true. The incremental compiler recognizes this situation and flushes state.

To support incremental compilation, the compiler needs to recover from all
errors without leaving itself in an inconsistent state, or let the incremental
compiler know that it cannot be reused. I think that can be stated in a simple
rule with one exception:

  Don't use reportFatalError, unless that would leave the compiler in an
inconsistent state.

I don't believe I have articulated the "unless" part clearly before.

IIUC, the bad state only occurs if a "builtin" library isn't available. If that
is really the case, then perhaps we can replace this check to see if
initializeCoreClasses and initializeHelperClasses hasn't been called.

[Johnni Winther, 2014-07-17 11:13:03]

https://codereview.chromium.org/339563002/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/339563002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:1235: // when these are
handled as erroneous libraries/compilation units.
On 2014/07/17 08:33:13, ahe wrote:
> I guess it isn't clear how detrimental fatal errors are to incremental
> compilation, and why that is.
> 
> When a fatal error occurs, the compiler is left in an inconsistent state. So
if
> the compiler instance was reused, it produces undefined results. In practice,
I
> have observed that fatal errors can lead to error that cannot go away, or take
a
> few attempts to go away. Or plain compiler crashes. I also have examples of
the
> compiler producing bad output.
> 
> So when a fatal error occurs, it is better to flush all state (discard the
> compiler instance) and start over. This is why fatal errors are detrimental,
> because the response to the user misspelling an import, or making a syntax
error
> results in the next compilation taking a really long time.
> 
> However, silently leaving the compiler in an inconsistent state is even worse.
> If a fatal error had been reported, an exception would have been thrown, and
the
> compilation would have been aborted, and [compilerWasCancelled] would have
been
> true. The incremental compiler recognizes this situation and flushes state.
> 
> To support incremental compilation, the compiler needs to recover from all
> errors without leaving itself in an inconsistent state, or let the incremental
> compiler know that it cannot be reused. I think that can be stated in a simple
> rule with one exception:
> 
>   Don't use reportFatalError, unless that would leave the compiler in an
> inconsistent state.
> 
> I don't believe I have articulated the "unless" part clearly before.
> 
> IIUC, the bad state only occurs if a "builtin" library isn't available. If
that
> is really the case, then perhaps we can replace this check to see if
> initializeCoreClasses and initializeHelperClasses hasn't been called.

Can you file a bug on this and put me in it?