Remove Element.getNode() method

Remove Element.getNode() method

1. Use NodeElement in CompletionEngine

2. Don't use Element.getNode() in DartElementLocator

3. New tests for DartElementLocator

4. Clean up DartC tests to use NodeElement


R=brianwilkerson@google.com
BUG=
TEST=

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

[Brian Wilkerson, 2012-03-16 15:04:51]

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/completion/CompletionEngine.java
(right):

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/completion/CompletionEngine.java:1384:
String returnTypeName = method.getReturnType().getElement().getName();
Do methods always have a return type? How does the compiler handle a return type
of "var"? (My concern here is whether this can result in a NPE.)

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/utilities/ast/NameOccurrencesFinder.java
(right):

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/utilities/ast/NameOccurrencesFinder.java:16:
public class NameOccurrencesFinder extends ASTVisitor<Void> {
I'm not suggesting that you need to change this, but I can't help but think that
this class is poorly written. If we have an AST structure within which we are
searching, we ought to be able to visit the structure looking for identifiers
that resolve to the same element as the one passed in without asking the element
for its node.

In fact, asking the element for it's node is the wrong thing to do, even if we
wanted to keep the node reference. What if the element is defined in a different
compilation unit?

And what happens if there is no element defined for a particular identifier? I
think we want to find all identifiers with the same name that also do not have
any element that they resolved to. (Which means this class is also incomplete.)

But the important point is that the reference to getNode needs to go away, as do
NodeElement and all it's subtypes. (But not in this CL.)

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
File editor/tools/plugins/com.google.dart.tools.core_test/META-INF/MANIFEST.MF
(right):

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core_test/META-INF/MANIFEST.MF:15:
org.eclipse.ui,
The core tests have to be able to run in a headless environment, so we can't add
this dependency. I looked at the test case and I'm not sure why you need this
reference anyway, but if the tests won't work without it we'll have to move the
ones with this dependency to the com.google.dart.tools.ui_test plug-in. Those
tests don't get run on the build bots so they can have UI dependencies.

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/utilities/ast/TestProject.java
(right):

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/utilities/ast/TestProject.java:35:
public class TestProject {
Not that it's necessarily better, but there an alternative that you might not be
aware of. TestUtilities.loadPluginRelativeProject lets you create the project
structure you want in the test_data directory and then load it when you need it.
Like I said, it's not necessarily better, so I'm not asking you to change
anything.

[scheglov, 2012-03-16 18:25:02]

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/completion/CompletionEngine.java
(right):

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/completion/CompletionEngine.java:1384:
String returnTypeName = method.getReturnType().getElement().getName();
On 2012/03/16 15:04:51, Brian Wilkerson wrote:
> Do methods always have a return type? How does the compiler handle a return
type
> of "var"? (My concern here is whether this can result in a NPE.)

Yes, if return type is not specified, it is Dynamic.
I've added ypeAnalyzerCompilerTest.test_methodReturnTypes() to show this.

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/utilities/ast/NameOccurrencesFinder.java
(right):

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/utilities/ast/NameOccurrencesFinder.java:16:
public class NameOccurrencesFinder extends ASTVisitor<Void> {
On 2012/03/16 15:04:51, Brian Wilkerson wrote:
> I'm not suggesting that you need to change this, but I can't help but think
that
> this class is poorly written. If we have an AST structure within which we are
> searching, we ought to be able to visit the structure looking for identifiers
> that resolve to the same element as the one passed in without asking the
element
> for its node.
> 
> In fact, asking the element for it's node is the wrong thing to do, even if we
> wanted to keep the node reference. What if the element is defined in a
different
> compilation unit?
> 
> And what happens if there is no element defined for a particular identifier? I
> think we want to find all identifiers with the same name that also do not have
> any element that they resolved to. (Which means this class is also
incomplete.)

  I can not say that I understand 100% how this class is used, but I see that
findTargetContainer() is used just as optimization.
  If you have VariableElement, then it is part of some method, so no need to
visit full AST.
  And if this is DartParameter, then... well, this code looks invalid in any
case.
  I will remove findTargetContainer() and keep visiting full AST for now.
  Hm... strange that it passes tests, it should not work...

  I believe that this class should return only _local_ references on element.
  Something like "Mark Occurences" in Java editor.
  

> 
> But the important point is that the reference to getNode needs to go away, as
do
> NodeElement and all it's subtypes. (But not in this CL.)

  I think that we may be will remove NodeElement, and all its subtypes.
  However I'm still not sure if we want do this completely.
  Technically, we can afford ourself to use it in Resolver and TypeAnalyzer,
because we have Nodes.
  At least in some places it is easier, because Element has already prepared
information.

  May be in other places we don't have choice than use getNode(), but may be in
theory all can be removed.
  Take as example CompileTimeConstantAnalyzer.
  When we found reference on some Field, if this is external field, then we know
that FieldElement.getConstantType() will return its type. But if this is Field
in the same Unit, then may be we've not visited this Field yet, so its
inferred/constant type is not filled yet. Can we solve this? Yes. Easy? Probably
not.

  But if we stive for purity, I can try to remove all of them.

  Alternative solution is to keep Element.getNode(), but after resolving
everything, just "seal" Element and clear its node, so getNode() will throw
special exception.

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
File editor/tools/plugins/com.google.dart.tools.core_test/META-INF/MANIFEST.MF
(right):

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core_test/META-INF/MANIFEST.MF:15:
org.eclipse.ui,
On 2012/03/16 15:04:51, Brian Wilkerson wrote:
> The core tests have to be able to run in a headless environment, so we can't
add
> this dependency. I looked at the test case and I'm not sure why you need this
> reference anyway, but if the tests won't work without it we'll have to move
the
> ones with this dependency to the com.google.dart.tools.ui_test plug-in. Those
> tests don't get run on the build bots so they can have UI dependencies.

Ah... OK

I use this code to close all extra views/editors an put Eclipse window into
location where it does not hide JUnit view and Console.
I like to see test progress.
I will move this code to UI tests.

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/utilities/ast/TestProject.java
(right):

https://chromiumcodereview.appspot.com/9703096/diff/1/editor/tools/plugins/co...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/utilities/ast/TestProject.java:35:
public class TestProject {
On 2012/03/16 15:04:51, Brian Wilkerson wrote:
> Not that it's necessarily better, but there an alternative that you might not
be
> aware of. TestUtilities.loadPluginRelativeProject lets you create the project
> structure you want in the test_data directory and then load it when you need
it.
> Like I said, it's not necessarily better, so I'm not asking you to change
> anything.

  Yes, I saw this alternative.

  Using _external_ test data is THE WORST approach for testing.
  Look on existing test for DartElementLocatorTest.
  What happens at position 418?
  Where to find source?
  How to calculate position for other element?

  Doing so is the best way to create unmanageable, fragile tests.
  I'm not surprised that there was only 3 tests.

  I've written 11000+ tests for WindowBuilder and GWT Designer.
  Keeping _minimal_ input and output for test directly in test itself is the
only viable approach.

[Brian Wilkerson, 2012-03-16 18:49:14]

LGTM

>   I can not say that I understand 100% how this class is used...

I believe that it's only used for mark occurrences, but I might be mistaken.

>   I think that we may be will remove NodeElement, and all its subtypes.
>   However I'm still not sure if we want do this completely.

Definitely a long term discussion, but I think that any information we need from
the node probably ought to be in the element itself. But you have, as usual,
some good points.

>   Keeping _minimal_ input and output for test directly in test itself is the
> only viable approach.

I'll keep that in mind.