Report warnings and non-fatal errors for shadowing declaration and usage.

Report warnings and non-fatal errors for shadowing declaration and usage.

Fix for Type references in declaration.

Make Back a default button if there is Error in refactoring status.



R=messick@google.com
BUG=
TEST=

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

[messick, 2012-04-23 15:30:53]

LGTM with one caveat: I'm not sure one error message is correct.

https://chromiumcodereview.appspot.com/10162026/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java
(right):

https://chromiumcodereview.appspot.com/10162026/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:40:
this.expression = expression;
Please explain why this is needed.

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/internal/corext/refactoring/RefactoringCoreMessages.properties
(right):

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/internal/corext/refactoring/RefactoringCoreMessages.properties:422:
RenameProcessor_typeMemberUsage_shadowedBy_topLevel=Usage of {0} ''{1}.{2}''
will be shadowed by top-level {3} ''{4}'' from ''{5}'' in library ''{6}''
This seems backward. I thought top-level functions could be shadowed. The
Scoping section of the spec says that in a general way. I could not find a more
specific description of top-level shadowing. If there is one, please let me
know.

Also, what will that print if the top-level element is defined in an application
that has no #library directive?

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/internal/corext/refactoring/rename/RenameAnalyzeUtil.java
(right):

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/internal/corext/refactoring/rename/RenameAnalyzeUtil.java:197:
* @return the references on the given {@link DartElement}, may be empty {@link
List}, but not
on => to

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src/com/google/dart/tools/ui/refactoring/RenameFieldProcessorTest.java
(right):

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui_test/src/com/google/dart/tools/ui/refactoring/RenameFieldProcessorTest.java:498:
"Usage of field 'A.test' will be shadowed by top-level type 'newName' from
'Test/Test.dart' in library 'Test'",
I mentioned this error message in the file that defines it. I think this is
backward; inner scope names shadow outer scope names. If this is defined
differently in the spec please point me to it.

[scheglov, 2012-04-23 17:53:01]

https://chromiumcodereview.appspot.com/10162026/diff/1/compiler/java/com/goog...
File compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java
(right):

https://chromiumcodereview.appspot.com/10162026/diff/1/compiler/java/com/goog...
compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:40:
this.expression = expression;
On 2012/04/23 15:30:53, messick wrote:
> Please explain why this is needed.

DartParameterizedTypeNode is hack.
It is used to wrap type name and type parameters, and created in
ClassElementImplementation.
It should not take ownership of Name, because DartClass returns name as child,
and at same time parent of Name is not DartClass, but DartParameterizedTypeNode,
which itself has no parent...

We need to have this structure sane to check in
com.google.dart.tools.core.internal.index.contributor.IndexContributor.isNameInDeclaration(DartIdentifier)
that Name is part of declaration, or is reference on Type.

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/internal/corext/refactoring/rename/RenameAnalyzeUtil.java
(right):

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/internal/corext/refactoring/rename/RenameAnalyzeUtil.java:197:
* @return the references on the given {@link DartElement}, may be empty {@link
List}, but not
On 2012/04/23 15:30:53, messick wrote:
> on => to

Done.

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui_test/src/com/google/dart/tools/ui/refactoring/RenameFieldProcessorTest.java
(right):

https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui_test/src/com/google/dart/tools/ui/refactoring/RenameFieldProcessorTest.java:498:
"Usage of field 'A.test' will be shadowed by top-level type 'newName' from
'Test/Test.dart' in library 'Test'",
On 2012/04/23 15:30:53, messick wrote:
> I mentioned this error message in the file that defines it. I think this is
> backward; inner scope names shadow outer scope names. If this is defined
> differently in the spec please point me to it.

Short answer: it depends.
In the class which defines field, this field will shadow top-level.
In the class which inherits from class which defines field, top-level will
shadow field.

http://code.google.com/p/dart/issues/detail?id=1598
http://code.google.com/p/dart/issues/detail?id=1180

[messick, 2012-04-23 18:30:44]

On 2012/04/23 17:53:01, scheglov wrote:
>
https://chromiumcodereview.appspot.com/10162026/diff/1/compiler/java/com/goog...
> File compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java
> (right):
> 
>
https://chromiumcodereview.appspot.com/10162026/diff/1/compiler/java/com/goog...
> compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:40:
> this.expression = expression;
> On 2012/04/23 15:30:53, messick wrote:
> > Please explain why this is needed.
> 
> DartParameterizedTypeNode is hack.
> It is used to wrap type name and type parameters, and created in
> ClassElementImplementation.
> It should not take ownership of Name, because DartClass returns name as child,
> and at same time parent of Name is not DartClass, but
DartParameterizedTypeNode,
> which itself has no parent...
> 
> We need to have this structure sane to check in
>
com.google.dart.tools.core.internal.index.contributor.IndexContributor.isNameInDeclaration(DartIdentifier)
> that Name is part of declaration, or is reference on Type.
> 
>
https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
> File
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/internal/corext/refactoring/rename/RenameAnalyzeUtil.java
> (right):
> 
>
https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
>
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/internal/corext/refactoring/rename/RenameAnalyzeUtil.java:197:
> * @return the references on the given {@link DartElement}, may be empty {@link
> List}, but not
> On 2012/04/23 15:30:53, messick wrote:
> > on => to
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
> File
>
editor/tools/plugins/com.google.dart.tools.ui_test/src/com/google/dart/tools/ui/refactoring/RenameFieldProcessorTest.java
> (right):
> 
>
https://chromiumcodereview.appspot.com/10162026/diff/1/editor/tools/plugins/c...
>
editor/tools/plugins/com.google.dart.tools.ui_test/src/com/google/dart/tools/ui/refactoring/RenameFieldProcessorTest.java:498:
> "Usage of field 'A.test' will be shadowed by top-level type 'newName' from
> 'Test/Test.dart' in library 'Test'",
> On 2012/04/23 15:30:53, messick wrote:
> > I mentioned this error message in the file that defines it. I think this is
> > backward; inner scope names shadow outer scope names. If this is defined
> > differently in the spec please point me to it.
> 
> Short answer: it depends.
> In the class which defines field, this field will shadow top-level.
> In the class which inherits from class which defines field, top-level will
> shadow field.
> 
> http://code.google.com/p/dart/issues/detail?id=1598
> http://code.google.com/p/dart/issues/detail?id=1180

I think it is worth adding a comment with a link to
http://code.google.com/p/dart/issues/detail?id=1180
somewhere in the code, as this situation is confusing.
Thanks for the follow-up!

LGTM-er