Change the error for importing a deferred library with a top-level 'loadLibrary' to being a hint. A…

Change the error for importing a deferred library with a top-level 'loadLibrary' to being a hint. Also, the logic for checking for the 'loadLibrary' from the element model into the hints phase (BestPracticesVerifier).

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

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

[scheglov, 2014-04-28 17:46:43]

LGTM

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

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/hint/BestPracticesVerifier.java:97:
CompilationUnitElement[] parts = libraryElement.getParts();
You could use getUnits() which includes the defining CU.

[jwren, 2014-04-28 18:07:02]

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

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/hint/BestPracticesVerifier.java:97:
CompilationUnitElement[] parts = libraryElement.getParts();
On 2014/04/28 17:46:43, scheglov wrote:
> You could use getUnits() which includes the defining CU.

Done.

[Brian Wilkerson, 2014-04-28 19:38:33]

LGTM once the utility methods and constants have been moved back.

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/element/CompilationUnitElement.java
(left):

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/element/CompilationUnitElement.java:107:
public boolean hasLoadLibraryFunction();
Is the hint really the only place that will ever want to check this? I could
imagine a "Make deferred" refactoring wanting to know whether the import can
safely be converted, or other tools that help a user decide how to optimize
their download size.

In general, I'd rather have a utility method that everyone can use even if it's
only used once, rather than require all potential clients to write the same code
over and over. I think we should leave this method here.

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LibraryElementImpl.java
(right):

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LibraryElementImpl.java:261:
FunctionElementImpl function = new
FunctionElementImpl(ElementResolver.LOAD_LIBRARY_NAME, -1);
The constant should definitely stay on FunctionElement.

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java
(right):

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:354:
public static final String CALL_METHOD_NAME = "call"; //$NON-NLS-1$
These should be moved to FunctionElement as well.

[jwren, 2014-04-28 20:08:55]

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/element/CompilationUnitElement.java
(left):

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/element/CompilationUnitElement.java:107:
public boolean hasLoadLibraryFunction();
Done.  I thought we talked about removing the utility.

If we think it might be used by others we should definitely leave it in.



On 2014/04/28 19:38:34, Brian Wilkerson wrote:
> Is the hint really the only place that will ever want to check this? I could
> imagine a "Make deferred" refactoring wanting to know whether the import can
> safely be converted, or other tools that help a user decide how to optimize
> their download size.
> 
> In general, I'd rather have a utility method that everyone can use even if
it's
> only used once, rather than require all potential clients to write the same
code
> over and over. I think we should leave this method here.

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LibraryElementImpl.java
(right):

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/element/LibraryElementImpl.java:261:
FunctionElementImpl function = new
FunctionElementImpl(ElementResolver.LOAD_LIBRARY_NAME, -1);
Done

On 2014/04/28 19:38:34, Brian Wilkerson wrote:
> The constant should definitely stay on FunctionElement.

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java
(right):

https://codereview.chromium.org/253493010/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:354:
public static final String CALL_METHOD_NAME = "call"; //$NON-NLS-1$
Done

On 2014/04/28 19:38:34, Brian Wilkerson wrote:
> These should be moved to FunctionElement as well.

[jwren, 2014-04-28 20:14:09]

Committed patchset #2 manually as r35495 (presubmit successful).