Further work on 17522

Further work on 17522- deferred loading in the analyzer.  There are some additional error code that are needed (indicated by a TODO in CompileTimeErrorCode), but all of the valid language tests now pass.

R=brianwilkerson@google.com

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

[Brian Wilkerson, 2014-04-29 23:01:52]

LGTM. Comments can be addressed in a later CL.

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java
(right):

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java:194:
"'const' variables must be constant value, values from deferred libraries are
not evaluated to be constant"),
I'm not sure how clear this message will be. How about "Constant values from a
deferred library cannot be used to initialized a 'const' variable"? (I'm
assuming that the value is, in fact, constant. I'm not sure what errors we
produce for non-constants from a deferred library, but we probably don't need a
different error message for those cases.)

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

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:1062:
if (identifier.getName().equals(FunctionElement.LOAD_LIBRARY_NAME) &&
isDeferredPrefix(prefix)) {
Given that loadLibrary is a method, under what conditions does the parser
produce a prefixed identifier?

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
(right):

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:86:
private static boolean isDeferredLibraryReference(Expression expression) {
Does this method need to handle nested references, or are they handled in some
other way? For example, given

import 'foo.dart' deferred as foo;

const int X = 3 + foo.Y;

the method visitVariableDeclaration won't create an error (although it might be
created elsewhere. I'm wondering whether we need to add a method like
"hasDeferredReference" to Expression (which would answer your question in the
Javadoc).

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java
(right):

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:3808:
if (nameIdentifier instanceof PrefixedIdentifier) {
Does this cover the constructor case ("@prefix.ClassName.constructorName()"), or
do we also need to check for ConstructorName?

https://codereview.chromium.org/258393003/diff/20001/tests/language/language_...
File tests/language/language_analyzer.status (right):

https://codereview.chromium.org/258393003/diff/20001/tests/language/language_...
tests/language/language_analyzer.status:456: # Old syntax (@lazy) for deferred
libraries
We should open an issue for these tests (if you haven't already).

https://codereview.chromium.org/258393003/diff/20001/tests/language/language_...
tests/language/language_analyzer.status:461: # "The deferred types cannot be
used in a declaration" warning
We should create an issue for these tests (if you haven't already).

[jwren, 2014-04-29 23:07:59]

I will look at responding to the other comments (thank you by the way),
tomorrow.  Responded to one question below...

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

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:1062:
if (identifier.getName().equals(FunctionElement.LOAD_LIBRARY_NAME) &&
isDeferredPrefix(prefix)) {
On 2014/04/29 23:01:52, Brian Wilkerson wrote:
> Given that loadLibrary is a method, under what conditions does the parser
> produce a prefixed identifier?

var x = lib.loadLibrary;
...
x().then....

[Brian Wilkerson, 2014-04-29 23:18:12]

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

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ElementResolver.java:1062:
if (identifier.getName().equals(FunctionElement.LOAD_LIBRARY_NAME) &&
isDeferredPrefix(prefix)) {
> var x = lib.loadLibrary;
> ...
> x().then....

Is that legal? Can the loadLibrary function be torn off? (I wouldn't have
expected so, but I'll have to check the proposal.)

[jwren, 2014-04-30 22:42:13]

PTAL

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java
(right):

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java:194:
"'const' variables must be constant value, values from deferred libraries are
not evaluated to be constant"),
Done and correct.

On 2014/04/29 23:01:52, Brian Wilkerson wrote:
> I'm not sure how clear this message will be. How about "Constant values from a
> deferred library cannot be used to initialized a 'const' variable"? (I'm
> assuming that the value is, in fact, constant. I'm not sure what errors we
> produce for non-constants from a deferred library, but we probably don't need
a
> different error message for those cases.)

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
(right):

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:86:
private static boolean isDeferredLibraryReference(Expression expression) {
No.

Support and tests have now been added.

On 2014/04/29 23:01:52, Brian Wilkerson wrote:
> Does this method need to handle nested references, or are they handled in some
> other way? For example, given
> 
> import 'foo.dart' deferred as foo;
> 
> const int X = 3 + foo.Y;
> 
> the method visitVariableDeclaration won't create an error (although it might
be
> created elsewhere. I'm wondering whether we need to add a method like
> "hasDeferredReference" to Expression (which would answer your question in the
> Javadoc).

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java
(right):

https://codereview.chromium.org/258393003/diff/20001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:3808:
if (nameIdentifier instanceof PrefixedIdentifier) {
It is covered, but it isn't obvious by looking at the source.  The
node.getName() returns "prefix.ClassName", while node.getConstructorName()
returns "constructorName".

I added some tests for the @prefix.ClassName() and
@prefix.ClassName.constructorName() cases.



On 2014/04/29 23:01:52, Brian Wilkerson wrote:
> Does this cover the constructor case
(mailto:"@prefix.ClassName.constructorName()"), or
> do we also need to check for ConstructorName?

https://codereview.chromium.org/258393003/diff/20001/tests/language/language_...
File tests/language/language_analyzer.status (right):

https://codereview.chromium.org/258393003/diff/20001/tests/language/language_...
tests/language/language_analyzer.status:456: # Old syntax (@lazy) for deferred
libraries
https://code.google.com/p/dart/issues/detail?id=18562

On 2014/04/29 23:01:52, Brian Wilkerson wrote:
> We should open an issue for these tests (if you haven't already).

https://codereview.chromium.org/258393003/diff/20001/tests/language/language_...
tests/language/language_analyzer.status:461: # "The deferred types cannot be
used in a declaration" warning
https://code.google.com/p/dart/issues/detail?id=18562

On 2014/04/29 23:01:52, Brian Wilkerson wrote:
> We should create an issue for these tests (if you haven't already).

[Brian Wilkerson, 2014-04-30 23:29:29]

LGTM

https://codereview.chromium.org/258393003/diff/40001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java
(right):

https://codereview.chromium.org/258393003/diff/40001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java:679:
"Annotation can be only constant variable or constant constructor invocation,
values from deferred libraries are not evaluated to be constant"),
We might want to re-write the other messages similar to the one above. I'm
concerned that this will be confusing because it isn't that constants from
deferred libraries are not constants, just that they cannot be used as an
annotation.

https://codereview.chromium.org/258393003/diff/40001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
(right):

https://codereview.chromium.org/258393003/diff/40001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:262:
if (result instanceof ValidResult && referenceDetector.getResult()) {
Do we need to test "result instanceof ValidResult"? It looks like we will have
returned if this isn't true.

https://codereview.chromium.org/258393003/diff/40001/tests/language/language_...
File tests/language/language_analyzer.status (right):

https://codereview.chromium.org/258393003/diff/40001/tests/language/language_...
tests/language/language_analyzer.status:456: # Old syntax (@lazy) for deferred
libraries
It would be good to add the issue numbers for reference.

[jwren, 2014-05-01 17:20:57]

https://codereview.chromium.org/258393003/diff/40001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java
(right):

https://codereview.chromium.org/258393003/diff/40001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java:679:
"Annotation can be only constant variable or constant constructor invocation,
values from deferred libraries are not evaluated to be constant"),
On 2014/04/30 23:29:29, Brian Wilkerson wrote:
> We might want to re-write the other messages similar to the one above. I'm
> concerned that this will be confusing because it isn't that constants from
> deferred libraries are not constants, just that they cannot be used as an
> annotation.

Done.

https://codereview.chromium.org/258393003/diff/40001/editor/tools/plugins/com...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java
(right):

https://codereview.chromium.org/258393003/diff/40001/editor/tools/plugins/com...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:262:
if (result instanceof ValidResult && referenceDetector.getResult()) {
Removing the check.

https://codereview.chromium.org/258393003/diff/40001/tests/language/language_...
File tests/language/language_analyzer.status (right):

https://codereview.chromium.org/258393003/diff/40001/tests/language/language_...
tests/language/language_analyzer.status:456: # Old syntax (@lazy) for deferred
libraries
On 2014/04/30 23:29:29, Brian Wilkerson wrote:
> It would be good to add the issue numbers for reference.

Done.

[jwren, 2014-05-01 21:43:33]

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