Added hint to report invocations of members (methods, getters, setters, operators, ...) that are de…

Added hint to report invocations of members (methods, getters, setters, operators, ...) that are declared as @deprecated or @Deprecated('...').

R=brianwilkerson@google.com, scheglov@google.com

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

[scheglov, 2013-11-01 22:38:00]

lgtm

[Brian Wilkerson, 2013-11-01 22:40:12]

LGTM

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/HintCode.java
(right):

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/HintCode.java:45:
* The {@code @deprecated} annotation should not be used.
The comment still needs to be updated.

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

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/hint/BestPracticesVerifier.java:84:
* TODO Do we want to move logic into a new method "isDeprecated", on Element?
Sort of. I would define "Element.isDeprecated()" as
/**
 * Return {@code true} if this element has an annotation of the form
'@deprecated' or '@Deprecated()', with or without arguments.
 *
 * @return {@code true} if this element is deprecated
 */
That's not quite what this method is doing. (This method could, and probably
should, be moved to "ElementAnnotation.isDeprecatedAnnotation()".)

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/hint/BestPracticesVerifier.java:325:
// TODO(jwren) should we modify ConstructorElement.getDisplayName instead of
doing this logic here?
It seems like there ought to be a single representation for a constructor that
is shown to the user, so the answer is very likely to be "yes".

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/hint/BestPracticesVerifier.java:333:
foundHint = true;
If you don't add isDeprecated to Element, this should be "return true;" in order
to avoid testing any remaining annotations. (In which case you don't need the
local variable.)

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/HintCodeTest.java
(right):

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/HintCodeTest.java:23:
// TODO investigate: this code works fine in the Dart Editor, but in the test
framework, the
That's because 'deprecated' isn't defined in the library
'deprecated_library.dart'. It's defined in a library that exports it, but that
doesn't make it visible in the exported library.

That said, you shouldn't need to import the declarations in 'annotations.dart'
because they are (or should be) defined in "dart:core". My guess is that you
need to add these elements to the model of core created by
AnalysisContextImpl.createAnalysisContext().

[jwren, 2013-11-04 07:58:31]

PTAL.

Brian- I originally intended to land this CL on Friday and then follow up with a
second  CL cleanup the tests and add of Element.isDeprecated.  That said, I
didn't have enough time to wait for all the bots for any side effects, so the
changes are now included in this CL.

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/HintCode.java
(right):

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/HintCode.java:45:
* The {@code @deprecated} annotation should not be used.
On 2013/11/01 22:40:12, Brian Wilkerson wrote:
> The comment still needs to be updated.

Done.

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

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/hint/BestPracticesVerifier.java:84:
* TODO Do we want to move logic into a new method "isDeprecated", on Element?
Done with one exception.  @Deprecated() is not valid- it produces a compile time
error, the argument is required.

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/hint/BestPracticesVerifier.java:325:
// TODO(jwren) should we modify ConstructorElement.getDisplayName instead of
doing this logic here?
Agreed, we should revisit this, I don't want to change getDisplayName for
constructors in this CL, and miss changes that could surface in the UI.

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/hint/BestPracticesVerifier.java:333:
foundHint = true;
Added Element.isDeprecated().

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/HintCodeTest.java
(right):

https://codereview.chromium.org/56083004/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/HintCodeTest.java:23:
// TODO investigate: this code works fine in the Dart Editor, but in the test
framework, the
I missed the missing import.

Updated CL to include code to modify the mocked out dart.core library.



On 2013/11/01 22:40:12, Brian Wilkerson wrote:
> That's because 'deprecated' isn't defined in the library
> 'deprecated_library.dart'. It's defined in a library that exports it, but that
> doesn't make it visible in the exported library.
> 
> That said, you shouldn't need to import the declarations in 'annotations.dart'
> because they are (or should be) defined in "dart:core". My guess is that you
> need to add these elements to the model of core created by
> AnalysisContextImpl.createAnalysisContext().

[Brian Wilkerson, 2013-11-04 15:09:43]

LGTM

https://codereview.chromium.org/56083004/diff/110002/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ElementImpl.java
(left):

https://codereview.chromium.org/56083004/diff/110002/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ElementImpl.java:40:

nit: Why did you get rid of the blank lines?

https://codereview.chromium.org/56083004/diff/110002/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/MultiplyDefinedElementImpl.java
(right):

https://codereview.chromium.org/56083004/diff/110002/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/MultiplyDefinedElementImpl.java:203:
public boolean isDeprecated() {
We might actually want to reverse the logic of this method in order to reduce
the number of false positives (that is, a multiply defined element is only
considered to be deprecated if all of the elements are deprecated).

Alternatively, just return false under the theory that there is already an error
marker for such references and a hint will never be useful until the error is
resolved.

[jwren, 2013-11-04 18:05:14]

Committed patchset #5 manually as r29828 (presubmit successful).

[jwren, 2013-11-04 18:06:32]

https://codereview.chromium.org/56083004/diff/110002/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ElementImpl.java
(left):

https://codereview.chromium.org/56083004/diff/110002/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/ElementImpl.java:40:

Not intended- fixed.

On 2013/11/04 15:09:43, Brian Wilkerson wrote:
> nit: Why did you get rid of the blank lines?

https://codereview.chromium.org/56083004/diff/110002/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/MultiplyDefinedElementImpl.java
(right):

https://codereview.chromium.org/56083004/diff/110002/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/MultiplyDefinedElementImpl.java:203:
public boolean isDeprecated() {
Changed to just return false.

On 2013/11/04 15:09:43, Brian Wilkerson wrote:
> We might actually want to reverse the logic of this method in order to reduce
> the number of false positives (that is, a multiply defined element is only
> considered to be deprecated if all of the elements are deprecated).
> 
> Alternatively, just return false under the theory that there is already an
error
> marker for such references and a hint will never be useful until the error is
> resolved.