Initial package indexing implementation.

Initial package indexing implementation.

More tests will be added in following CLs.

R=paulberry@google.com
BUG=

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

[Paul Berry, 2016-02-25 23:05:13]

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

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:16: * it to assemble a new
[PackageIndexBuilder].
It would be helpful to have a comment here explaining how this class is intended
to be used.  E.g. "Call [index] on each compilation unit to be indexed, then
call [assemble] to retrieve the complete index for the package."

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:21: final
Map<CompilationUnitElement, int> _elementUnitMap =
Please add doc comments for all these fields.  I've spent a lot of time cross
referencing the code and re-reading the IDL to figure out what they are for, and
I'm still not certain whether my conclusions are correct.

Documentation would be especially helpful for a field like this one, where it's
hard to infer what's going on from the type alone, because I don't know what the
int refers to.  (At first I guessed it was the index of the compilation unit in
[PackageIndex.units], but that turned out to be wrong).

E.g. "Map from a compilation unit to the location in
[PackageIndex.elementUnitUris] and [PackageIndex.elementLibraryUris] where we
will store information about the URIs of the compilation unit."

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:34: * Assemble a new
[PackageIndexBuilder] using the gathered information.
s/using the gathered information/using the information gathered by [index]/

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:37: List<_ElementInfo> elements =
_elementMap.values.toList();
Suggestion: rename this to "elementInfos" to avoid confusion (I keep thinking
it's a list of elements)

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:71: int
_getUnitElementId(CompilationUnitElement element) {
Documentation on these private methods would be helpful too.  E.g. "add
information about [element] to [_elementUnitUris] and [_elementLibraryUris] if
necessary, and return the location in those arrays representing [element]."

Suggestion: rename [element] to [compilationUnit] to make the above text easier
to understand.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:93: static CompilationUnitElement
getUnitElement(Element element) {
It looks like this is only used by other code in this file.  Can we rename it to
"_getUnitElement"?  That would help me understand that it's not intended to be
part of the public API to this file.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:114: final Element element;
It looks like this is only used by the [offset] getter below.  How about getting
rid of it and changing [offset] into a final field?

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:232: ClassElement element =
node.element;
There's a lot of common code between this method and the one above.  Consider
refactoring the common code into its own method.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:263: // field reference is write
here
I don't understand this comment.  Do you mean "right here" or "written here"?

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:281: element =
element.redirectedConstructor;
What if there are multiple levels of indirection?

What if the indirections form a loop?

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:447: } else if (element is
PrefixElement) {
This clause can be combined with the "if" clause above, since the bodies are the
same.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:489: void
_recordLibraryReference(UriBasedDirective node, LibraryElement library) {
All methods in this class are inherently private since the class is private. 
Consider renaming this to "recordLibraryReference" so that it sorts with the
other "record..." methods.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:580: * Information about a single
relation.
Add a comment explaining that [_RelationInfo] always exists inside of
[_UnitIndexAssembler.relations], so [offset] and [length] should be understood
within the context of the compilation unit pointed to by the
[_UnitIndexAssempler].

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:583: final _ElementInfo element;
Rename to "elementInfo".  Otherwise it's easy to be confused when reading line
612 and incorrectly conclude that it's sorting by [Element.id].

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:593: */
As with [PackageIndexAssembler], it would be helpful to have a comment
explaining how this is intended to be used.  I *think* the intention is:
- Call [addRelation] for each relation found in the compilation unit.
- Assign ids to all the _ElementInfo objects reachable from [relations].
- Call [assemble] to produce the final unit index.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:608: * Assemble a new
[UnitIndexBuilder] using the gathered information.
s/using the gathered information/using the information gathered by
[addRelation]/

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/index_unit_test.dart (right):

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/index_unit_test.dart:41: class LocationImpl {
What is this class for?  I don't see where it's used or what interface it's
implementing.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/index_unit_test.dart:49: class
PackageIndexAssemblerTest extends AbstractSingleUnitTest {
It seems like a lot of code in IndexContributor is not covered by this test. 
Are you planning to add more tests?

[scheglov, 2016-02-26 04:54:03]

PTAL

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

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:16: * it to assemble a new
[PackageIndexBuilder].
On 2016/02/25 23:05:13, Paul Berry wrote:
> It would be helpful to have a comment here explaining how this class is
intended
> to be used.  E.g. "Call [index] on each compilation unit to be indexed, then
> call [assemble] to retrieve the complete index for the package."

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:21: final
Map<CompilationUnitElement, int> _elementUnitMap =
On 2016/02/25 23:05:12, Paul Berry wrote:
> Please add doc comments for all these fields.  I've spent a lot of time cross
> referencing the code and re-reading the IDL to figure out what they are for,
and
> I'm still not certain whether my conclusions are correct.
> 
> Documentation would be especially helpful for a field like this one, where
it's
> hard to infer what's going on from the type alone, because I don't know what
the
> int refers to.  (At first I guessed it was the index of the compilation unit
in
> [PackageIndex.units], but that turned out to be wrong).
> 
> E.g. "Map from a compilation unit to the location in
> [PackageIndex.elementUnitUris] and [PackageIndex.elementLibraryUris] where we
> will store information about the URIs of the compilation unit."

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:34: * Assemble a new
[PackageIndexBuilder] using the gathered information.
On 2016/02/25 23:05:12, Paul Berry wrote:
> s/using the gathered information/using the information gathered by [index]/

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:37: List<_ElementInfo> elements =
_elementMap.values.toList();
On 2016/02/25 23:05:12, Paul Berry wrote:
> Suggestion: rename this to "elementInfos" to avoid confusion (I keep thinking
> it's a list of elements)

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:71: int
_getUnitElementId(CompilationUnitElement element) {
On 2016/02/25 23:05:12, Paul Berry wrote:
> Documentation on these private methods would be helpful too.  E.g. "add
> information about [element] to [_elementUnitUris] and [_elementLibraryUris] if
> necessary, and return the location in those arrays representing [element]."
> 
> Suggestion: rename [element] to [compilationUnit] to make the above text
easier
> to understand.

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:93: static CompilationUnitElement
getUnitElement(Element element) {
On 2016/02/25 23:05:13, Paul Berry wrote:
> It looks like this is only used by other code in this file.  Can we rename it
to
> "_getUnitElement"?  That would help me understand that it's not intended to be
> part of the public API to this file.

It is used in tests.
And it also will be used in index clients.
Although the location is not ideal, I don't know whether making it a top-level
function is much better.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:114: final Element element;
On 2016/02/25 23:05:13, Paul Berry wrote:
> It looks like this is only used by the [offset] getter below.  How about
getting
> rid of it and changing [offset] into a final field?

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:232: ClassElement element =
node.element;
On 2016/02/25 23:05:12, Paul Berry wrote:
> There's a lot of common code between this method and the one above.  Consider
> refactoring the common code into its own method.

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:263: // field reference is write
here
On 2016/02/25 23:05:12, Paul Berry wrote:
> I don't understand this comment.  Do you mean "right here" or "written here"?

Initially the code was using IndexRelationKind.IS_WRITTEN_BY.
But then I removed IS_WRITTEN_BY altogether - it was mostly useful for local
variables and parameters, which we don't record into index anymore. And a field,
if we record it into index (this.field and constructor field initializer) is
always written. And in the most cases getter/setter elements are referenced, not
fields.

I removed the comment.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:281: element =
element.redirectedConstructor;
On 2016/02/25 23:05:13, Paul Berry wrote:
> What if there are multiple levels of indirection?
> 
> What if the indirections form a loop?

I will add TODO and test/fix it in the next CL.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:447: } else if (element is
PrefixElement) {
On 2016/02/25 23:05:13, Paul Berry wrote:
> This clause can be combined with the "if" clause above, since the bodies are
the
> same.

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:489: void
_recordLibraryReference(UriBasedDirective node, LibraryElement library) {
On 2016/02/25 23:05:13, Paul Berry wrote:
> All methods in this class are inherently private since the class is private. 
> Consider renaming this to "recordLibraryReference" so that it sorts with the
> other "record..." methods.

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:580: * Information about a single
relation.
On 2016/02/25 23:05:12, Paul Berry wrote:
> Add a comment explaining that [_RelationInfo] always exists inside of
> [_UnitIndexAssembler.relations], so [offset] and [length] should be understood
> within the context of the compilation unit pointed to by the
> [_UnitIndexAssempler].

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:583: final _ElementInfo element;
On 2016/02/25 23:05:13, Paul Berry wrote:
> Rename to "elementInfo".  Otherwise it's easy to be confused when reading line
> 612 and incorrectly conclude that it's sorting by [Element.id].

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:593: */
On 2016/02/25 23:05:13, Paul Berry wrote:
> As with [PackageIndexAssembler], it would be helpful to have a comment
> explaining how this is intended to be used.  I *think* the intention is:
> - Call [addRelation] for each relation found in the compilation unit.
> - Assign ids to all the _ElementInfo objects reachable from [relations].
> - Call [assemble] to produce the final unit index.

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/index_unit.dart:608: * Assemble a new
[UnitIndexBuilder] using the gathered information.
On 2016/02/25 23:05:12, Paul Berry wrote:
> s/using the gathered information/using the information gathered by
> [addRelation]/

Done.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/index_unit_test.dart (right):

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/index_unit_test.dart:41: class LocationImpl {
On 2016/02/25 23:05:13, Paul Berry wrote:
> What is this class for?  I don't see where it's used or what interface it's
> implementing.

Removed.

https://codereview.chromium.org/1735243003/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/index_unit_test.dart:49: class
PackageIndexAssemblerTest extends AbstractSingleUnitTest {
On 2016/02/25 23:05:13, Paul Berry wrote:
> It seems like a lot of code in IndexContributor is not covered by this test. 
> Are you planning to add more tests?

Yes, of course!
Just not in this CL.

[Paul Berry, 2016-02-26 16:17:27]

lgtm, thanks!

[scheglov, 2016-02-26 16:23:39]

Description was changed from

==========
Initial package indexing implementation.

More tests will be added in following CLs.

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

to

==========
Initial package indexing implementation.

More tests will be added in following CLs.

R=paulberry@google.com
BUG=

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

[scheglov, 2016-02-26 16:23:40]

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