Initial implementation of MemoryIndexStoreImpl and IndexContributor.

Initial implementation of MemoryIndexStoreImpl and IndexContributor.

Also added getElement() to several nodes.
Added ASTNodes for utilities.

For now it uses ElementProxy, depending on your opinion I will change it to ElementHandle or we will keep ElementProxy.


R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2013-01-07 21:27:18]

DBC

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ASTNode.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ASTNode.java:104:
public ASTNode getParent() {
Why did you remove the "final" modifier?

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/CompilationUnit.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/CompilationUnit.java:129:
* Return the {@link CompilationUnitElement} associated with this compilation
unit, or
My personal preference (which means you can ignore it) is to not use type names
in doc comments. The type is already in the signature, so duplicating it doesn't
help, and I think the text reads much better when we use English rather than
pseudo-code.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/IndexContributor.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/IndexContributor.java:106:
public Void visitClassDeclaration(ClassDeclaration node) {
This will probably need to be extended to handle mixins (so we should add a
TODO).

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/IndexContributor.java:177:
throw new UnsupportedOperationException();
I assume we will support this in the future. If so, please add a TODO.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/IndexContributor.java:227:
String prefix = null;
Is this defined because it might someday be non-null? If so, please create a
TODO. If not, let's just in-line the "null" below.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/MemoryIndexStoreImpl.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/MemoryIndexStoreImpl.java:91:
public Location[] getRelationships(ElementProxy element, Relationship
relationship) {
I think this would be a better API if it took an Element rather than an
ElementProxy. I expect most clients will have an element and I don't want us to
force them to create a proxy before making a query.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:124:
public String getId() {
nit: I would much prefer getIdentifier. I don't like abbreviations :-)

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:125:
// TODO(scheglov) use canonical path, but to get it we need to handle
IOException
Unless we always use canonical paths (that is, the file's path is already
canonical), in which case we can just return the absolute path.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/Source.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/Source.java:78:
* @return the unique identifier for this source in its {@link AnalysisContext}.
This seems to imply that the unique identifier for a source could change from
context to context. Is that what you're thinking?

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/utilities/ast/ASTNodes.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/utilities/ast/ASTNodes.java:25:
public class ASTNodes {
I don't like creating utility classes for methods when I control the class of
the argument. Is there a reason why we shouldn't define these methods on
Identifier or SimpleIdentifier (or ASTNode if it really makes sense to ask
question for arbitrary nodes)?

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/utilities/ast/ASTNodes.java:31:
if (node.getParent() instanceof BinaryExpression) {
The new AST structure uses AssignmentExpression rather than BinaryExpression to
represent an assignment.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/utilities/ast/ASTNodes.java:51:
if (node.getParent() instanceof BinaryExpression) {
Ditto.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/IndexContributorTest.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/IndexContributorTest.java:137:
private static <T extends ASTNode> NodeList<T> mockNodeList(ASTNode owner, T...
nodes) {
This isn't a mock, it's a real node list, so the name might want to be changed.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/IndexContributorTest.java:215:
public void test_ClassDeclaration() throws Exception {
A more consistent naming scheme might be good. Some tests are named by the
method being invoked, some by the relationship being tested for, and some by the
class being visited. I understand why and I don't have a suggestion for which to
use, but it seems unfortunate.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/IndexContributorTest.java:234:
// class A extends B {}   class B {}
You certainly don't have to, but my inclination would be to make each of these
cases be a separate test. I find it to be easier to debug if all of the tests
get run and I can see which ones break rather than have the first one stop the
others from breaking.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/MemoryIndexStoreImplTest.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/MemoryIndexStoreImplTest.java:134:
// XXX
What does this comment mean?

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/utilities/ast/ASTNodesTest.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/utilities/ast/ASTNodesTest.java:31:
BinaryExpression parent = mock(BinaryExpression.class);
Using mocks here feels like overkill. AST nodes are light weight; perhaps we
should just use ASTFactory to create real nodes...

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/utilities/ast/ASTNodesTest.java:31:
BinaryExpression parent = mock(BinaryExpression.class);
This should be an AssignmentExpression.

[scheglov, 2013-01-07 22:45:41]

PTAL

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ASTNode.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/ASTNode.java:104:
public ASTNode getParent() {
To be able to mock it.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/CompilationUnit.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/CompilationUnit.java:129:
* Return the {@link CompilationUnitElement} associated with this compilation
unit, or
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> My personal preference (which means you can ignore it) is to not use type
names
> in doc comments. The type is already in the signature, so duplicating it
doesn't
> help, and I think the text reads much better when we use English rather than
> pseudo-code.

Well, I prefer to use class names for terms which we modeled as classes instead
of just words.
I.e. use DSL-ed English :-)

Helps when you have to rename.
And such text looks more structured - you see not just several loose words
"compilation unit element", but single term.

But I should keep code style.
I'll change it to the existing (your) style.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/IndexContributor.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/IndexContributor.java:106:
public Void visitClassDeclaration(ClassDeclaration node) {
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> This will probably need to be extended to handle mixins (so we should add a
> TODO).

Done.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/IndexContributor.java:177:
throw new UnsupportedOperationException();
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> I assume we will support this in the future. If so, please add a TODO.

Done.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/IndexContributor.java:227:
String prefix = null;
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> Is this defined because it might someday be non-null? If so, please create a
> TODO. If not, let's just in-line the "null" below.

Yes, it may be not null some day.
Done.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/MemoryIndexStoreImpl.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/index/MemoryIndexStoreImpl.java:91:
public Location[] getRelationships(ElementProxy element, Relationship
relationship) {
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> I think this would be a better API if it took an Element rather than an
> ElementProxy. I expect most clients will have an element and I don't want us
to
> force them to create a proxy before making a query.

There is a reasons why ElementProxy exists.
And one of them - performance.
Sometimes you don't need to load full Element, for example and just want to know
names of types in hierarchy. So, Location returns ElementProxy and you send it
back to IndexStore to get new set of subclasses.

May be on SearchEngine level we will provide both interfaces.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/FileBasedSource.java:125:
// TODO(scheglov) use canonical path, but to get it we need to handle
IOException
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> Unless we always use canonical paths (that is, the file's path is already
> canonical), in which case we can just return the absolute path.

Do we need to return not-canonical File from FileBasedSource.getFile() because
we use it later to resolve relative URIs?

In any case, removed for now.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/Source.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/source/Source.java:78:
* @return the unique identifier for this source in its {@link AnalysisContext}.
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> This seems to imply that the unique identifier for a source could change from
> context to context. Is that what you're thinking?

I believe this comment is not correct.
I will remove getId() from Source for now.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/utilities/ast/ASTNodes.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/utilities/ast/ASTNodes.java:25:
public class ASTNodes {
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> I don't like creating utility classes for methods when I control the class of
> the argument. Is there a reason why we shouldn't define these methods on
> Identifier or SimpleIdentifier (or ASTNode if it really makes sense to ask
> question for arbitrary nodes)?

OK, I will move these methods to SimpleIdentifier.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/IndexContributorTest.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/IndexContributorTest.java:137:
private static <T extends ASTNode> NodeList<T> mockNodeList(ASTNode owner, T...
nodes) {
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> This isn't a mock, it's a real node list, so the name might want to be
changed.

Done.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/IndexContributorTest.java:215:
public void test_ClassDeclaration() throws Exception {
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> A more consistent naming scheme might be good. Some tests are named by the
> method being invoked, some by the relationship being tested for, and some by
the
> class being visited. I understand why and I don't have a suggestion for which
to
> use, but it seems unfortunate.

Agree.
Actually after breaking this big test into smaller we can use relationship name
scheme everywhere.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/IndexContributorTest.java:234:
// class A extends B {}   class B {}
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> You certainly don't have to, but my inclination would be to make each of these
> cases be a separate test. I find it to be easier to debug if all of the tests
> get run and I can see which ones break rather than have the first one stop the
> others from breaking.

Done.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/MemoryIndexStoreImplTest.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/index/MemoryIndexStoreImplTest.java:134:
// XXX
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> What does this comment mean?

Done.

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/utilities/ast/ASTNodesTest.java
(right):

https://codereview.chromium.org/11787017/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/utilities/ast/ASTNodesTest.java:31:
BinaryExpression parent = mock(BinaryExpression.class);
On 2013/01/07 21:27:18, Brian Wilkerson wrote:
> Using mocks here feels like overkill. AST nodes are light weight; perhaps we
> should just use ASTFactory to create real nodes...

Agree.
Tests for SimpleIdentifier will use ASTFactory.

[Brian Wilkerson, 2013-01-07 23:10:10]

LGTM

https://codereview.chromium.org/11787017/diff/1049/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/SimpleIdentifier.java
(right):

https://codereview.chromium.org/11787017/diff/1049/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/SimpleIdentifier.java:100:
return true;
Doesn't this need to check that the identifier is on the left-hand side? In the
expression "a = b" I would expect the node for "b" to return false.

[scheglov, 2013-01-07 23:21:35]

https://codereview.chromium.org/11787017/diff/1049/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/SimpleIdentifier.java
(right):

https://codereview.chromium.org/11787017/diff/1049/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/ast/SimpleIdentifier.java:100:
return true;
On 2013/01/07 23:10:10, Brian Wilkerson wrote:
> Doesn't this need to check that the identifier is on the left-hand side? In
the
> expression "a = b" I would expect the node for "b" to return false.

Done.