Use SummarySdkAnalysisContext if the SDK has the analysis_summary file.

Use SummarySdkAnalysisContext if the SDK has the analysis_summary file.

This file does not exist yet, so it should be safe to have incomplete
implementation for now.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/22b044ca20e31e600988dd53bc8a9c6c70893181

[Brian Wilkerson, 2016-01-05 18:46:19]

LGTM

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

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/sdk_io.dart:550: if (file.existsSync()) {
Should we catch IO exceptions here and return `null`, or do we want the
exception to propagate (and if the latter, how far)?

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/summary_sdk.dart (right):

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:48: if (resynthesizer == null) {
Is there any reason to do this lazily? It seems like we will always create a
resynthesizer, so why not do it once up front and avoid the test?

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:80: print(target.source.uri);
Remove?

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:82: return true;
Should this return `false` for now until we can correctly compute the kind?

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:84: throw new
UnimplementedError('$result of $target');
Is this for debugging? If so, perhaps comment it out (or at least add a TODO).

[Paul Berry, 2016-01-05 19:33:43]

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

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/sdk_io.dart:550: if (file.existsSync()) {
On 2016/01/05 18:46:18, Brian Wilkerson wrote:
> Should we catch IO exceptions here and return `null`, or do we want the
> exception to propagate (and if the latter, how far)?

I hope we can propagate (or at least report) the exception in such a way that if
it causes unit tests to fail, it will be obvious what has gone wrong.

Historically we have had enough defensive coding surrounding the code that
accesses core types in the SDK that when I've introduced a bug that interferes
with core types, the exception I got had no obvious relationship to the root
cause of the problem, and it was really difficult to figure out what I had done
wrong.  I'd like to avoid creating a similar situation here.

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/summary_sdk.dart (right):

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:68: result == LIBRARY_ELEMENT) {
Is it going to be a maintenance burden to keep this chain of equality tests
updated when we modify the element model?  Maybe there should be a field in
ResultDescriptor expressing the relationship between all of the various
LIBRARY_ELEMENT*s (e.g. maybe a ".whenComplete" field that expresses what the
result will look like when it's been completely built), so we could instead say
something like:

if (result.whenComplete == LIBRARY_ELEMENT)

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:79: // TODO(scheglov) not every
source is a library
I think the only way this could be reached is if the code being analyzed tries
to refer to one of the SDK's part files directly, e.g.:

import "dart:core/bool.dart";

Maybe we should implement just enough logic to ensure that the correct error
messages will get generated, and then add some unit tests to verify that they
work properly.

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:106: throw new StateError('Unable
to find prelinked summary for $uri');
Is there a danger of this code getting triggered if the user tries to import an
SDK library that doesn't exist, e.g.:

import "dart:macaroons";

and causing a crash rather than generating the appropriate error?

[Paul Berry, 2016-01-05 19:48:41]

lgtm

[scheglov, 2016-01-05 20:27:57]

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

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/sdk_io.dart:550: if (file.existsSync()) {
I will catch all exceptions here and report them with
AnalysisEngine.instance.logger.logError().

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/summary_sdk.dart (right):

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:48: if (resynthesizer == null) {
On 2016/01/05 18:46:18, Brian Wilkerson wrote:
> Is there any reason to do this lazily? It seems like we will always create a
> resynthesizer, so why not do it once up front and avoid the test?

The property "sourceFactory" is set after the context is created.
There is also a circular dependency between DartSdk.context and
PartitionManager.forSdk().
So, we need to create the context instance before setting "sourceFactory".

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:68: result == LIBRARY_ELEMENT) {
On 2016/01/05 19:33:43, Paul Berry wrote:
> Is it going to be a maintenance burden to keep this chain of equality tests
> updated when we modify the element model?  Maybe there should be a field in
> ResultDescriptor expressing the relationship between all of the various
> LIBRARY_ELEMENT*s (e.g. maybe a ".whenComplete" field that expresses what the
> result will look like when it's been completely built), so we could instead
say
> something like:
> 
> if (result.whenComplete == LIBRARY_ELEMENT)

Sounds interesting.

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:79: // TODO(scheglov) not every
source is a library
On 2016/01/05 19:33:43, Paul Berry wrote:
> I think the only way this could be reached is if the code being analyzed tries
> to refer to one of the SDK's part files directly, e.g.:
> 
> import "dart:core/bool.dart";
> 
> Maybe we should implement just enough logic to ensure that the correct error
> messages will get generated, and then add some unit tests to verify that they
> work properly.

Yes.
For now it is just a placeholder.
Once we will start adding tests, we will provide better implementation.

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:80: print(target.source.uri);
On 2016/01/05 18:46:18, Brian Wilkerson wrote:
> Remove?

Done.

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:82: return true;
On 2016/01/05 18:46:18, Brian Wilkerson wrote:
> Should this return `false` for now until we can correctly compute the kind?

Returning `false` mean that we cannot provide this information.
We can provide it, but we do it incorrectly for now :-)
We don't want to read/scan/parse the source.

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:84: throw new
UnimplementedError('$result of $target');
On 2016/01/05 18:46:18, Brian Wilkerson wrote:
> Is this for debugging? If so, perhaps comment it out (or at least add a TODO).

Done.

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:106: throw new StateError('Unable
to find prelinked summary for $uri');
On 2016/01/05 19:33:43, Paul Berry wrote:
> Is there a danger of this code getting triggered if the user tries to import
an
> SDK library that doesn't exist, e.g.:
> 
> import "dart:macaroons";
> 
> and causing a crash rather than generating the appropriate error?

No, if a URI is not resolved to an existing Source, we don't even ask for its
elements.

There is not much what we could do here - we're asked for the summary, and we
need to return something. Returning `null` seems worse than providing
information in exception.

[scheglov, 2016-01-05 20:31:55]

Description was changed from

==========
Use SummarySdkAnalysisContext if the SDK has the analysis_summary file.

This file does not exist yet, so it should be safe to have incomplete
implementation for now.

R=brianwilkerson@google.com, paulberry@google.com
BUG=
==========

to

==========
Use SummarySdkAnalysisContext if the SDK has the analysis_summary file.

This file does not exist yet, so it should be safe to have incomplete
implementation for now.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/22b044ca20e31e600988dd53bc8a9c6c70893181
==========

[scheglov, 2016-01-05 20:31:56]

Committed patchset #2 (id:20001) manually as
22b044ca20e31e600988dd53bc8a9c6c70893181 (presubmit successful).

[Brian Wilkerson, 2016-01-05 20:42:51]

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/summary_sdk.dart (right):

https://codereview.chromium.org/1555093005/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/summary_sdk.dart:82: return true;
> Returning `false` mean that we cannot provide this information.
> We can provide it, but we do it incorrectly for now :-)
> We don't want to read/scan/parse the source.

Could we implement it correctly by just testing whether
"sourceFactory.dartSdk.uris.contains(target.source.uri)"?