Constrain type annotations with deferred types.

Constrain type annotations with deferred types.

This implements the constraint that a type imported with a deferred import
cannot be used in type annotations in the importing module.  They can still be
used in new-expressions. A violation will trigger a warning, and replace the
type with a MalformedType.

Was reverted by: Issue 185053005 r33277

R=floitsch@google.com, johnniwinther@google.com

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

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

[sigurdm, 2014-02-25 15:07:24]

Seems I was too quick posting for review - there are still some failing tests.

I will ping you again when I have fixed the issues.

[sigurdm, 2014-02-27 15:35:51]

It should be better now.

[floitsch, 2014-02-28 12:58:15]

LGTM with a few comments.
Wait for Johnni's OK.

Missing test: lib.Class.static  (at least I would like to see it next to the
others).

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:199: void
_computeImportDeferred(Import import, LibraryElement library) {
the name doesn't work for me.
maybe?
markIfDeferred

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:206: ==
deferredLibraryClass) {
please move the "==" into the previous line.

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:208: Element
maybePrefix = library.find(import.prefix.toString());
Isn't it an error if there is no prefix?

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/builder.dart (right):

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:55: case "\n" : return
"\\n";
use raw strings.
case "\n": return r"\n";

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:65: return new
LiteralString('"$escaped"');
LiteralString result = string(escaped);
// We don't escape ' under the assumption that the string is wrapped
// into ". Verify that assumption.
assert(result.value.codeUnitAt(0) == '"');
return result;

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1673:
Pair<Element, bool> resolveTypeName(Identifier prefixName,
I really don't like that we return a pair now.

maybe Johnni has a better suggestion?

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:4486:
deferredIsMalformed: false);
Why is it not an error here?

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
File tests/language/deferred_constraints_test.dart (right):

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:16: lib.C a = new lib.C(); ///
type_annotation1: runtime error, static type warning
these long names make it harder to review, because they don't fit into one line
anymore.

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:20: lib2.C a = new lib.C(); ///
type_annotation_non_deferred: continued
new lib2.C()?

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:22: lib.C a = new lib.C(); ///
type_annotation2: dynamic type error, static type warning
don't declare "a", "g" several times.

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:29: var g = new G2<lib.C>(); ///
new_generic2: static type warning
why is this not a dynamic error?

[sigurdm, 2014-03-03 13:18:49]

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:199: void
_computeImportDeferred(Import import, LibraryElement library) {
On 2014/02/28 12:58:15, floitsch wrote:
> the name doesn't work for me.
> maybe?
> markIfDeferred

Done.

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:206: ==
deferredLibraryClass) {
On 2014/02/28 12:58:15, floitsch wrote:
> please move the "==" into the previous line.

Done.

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:208: Element
maybePrefix = library.find(import.prefix.toString());
On 2014/02/28 12:58:15, floitsch wrote:
> Isn't it an error if there is no prefix?

Yes - but it is not fatal - we try compile as much as possible to report more
errors. Maybe this is wrong?

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/builder.dart (right):

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:55: case "\n" : return
"\\n";
On 2014/02/28 12:58:15, floitsch wrote:
> use raw strings.
> case "\n": return r"\n";

Done.

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:65: return new
LiteralString('"$escaped"');
On 2014/02/28 12:58:15, floitsch wrote:
> LiteralString result = string(escaped);
> // We don't escape ' under the assumption that the string is wrapped
> // into ". Verify that assumption.
> assert(result.value.codeUnitAt(0) == '"');
> return result;

Done.

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:65: return new
LiteralString('"$escaped"');
On 2014/02/28 12:58:15, floitsch wrote:
> LiteralString result = string(escaped);
> // We don't escape ' under the assumption that the string is wrapped
> // into ". Verify that assumption.
> assert(result.value.codeUnitAt(0) == '"');
> return result;

Done.

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1673:
Pair<Element, bool> resolveTypeName(Identifier prefixName,
On 2014/02/28 12:58:15, floitsch wrote:
> I really don't like that we return a pair now.
> 
> maybe Johnni has a better suggestion?

With Johnni's help I found a better way.

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:4486:
deferredIsMalformed: false);
On 2014/02/28 12:58:15, floitsch wrote:
> Why is it not an error here?

I added an explanatory comment.

The problem is that the resolution of the constructor-name in "new A<x>()" is
treating the constructor as a type-annotation and using that mechanism for
resolving it.
But because we want to allow new-expressions with a deferred constructor we
allow deferred types her.

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
File tests/language/deferred_constraints_test.dart (right):

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:16: lib.C a = new lib.C(); ///
type_annotation1: runtime error, static type warning
On 2014/02/28 12:58:15, floitsch wrote:
> these long names make it harder to review, because they don't fit into one
line
> anymore.

On the other hand the status files and test run messages are easier to review.
I can change them if you prefer.

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:20: lib2.C a = new lib.C(); ///
type_annotation_non_deferred: continued
On 2014/02/28 12:58:15, floitsch wrote:
> new lib2.C()?

Done.

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:22: lib.C a = new lib.C(); ///
type_annotation2: dynamic type error, static type warning
On 2014/02/28 12:58:15, floitsch wrote:
> don't declare "a", "g" several times.

Done.

https://codereview.chromium.org/177543002/diff/170001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:29: var g = new G2<lib.C>(); ///
new_generic2: static type warning
On 2014/02/28 12:58:15, floitsch wrote:
> why is this not a dynamic error?

That also confused me.
The type arguments of the constructor can be malformed, and the program will
still run without error in checked mode.
Like:

void main() {
  var a = new List<f>();
}

https://codereview.chromium.org/177543002/diff/190001/tests/language/deferred...
File tests/language/deferred_constraints_test.dart (right):

https://codereview.chromium.org/177543002/diff/190001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:35: int i = lib.C.staticMethod();
/// static_method: ok
I added a test of invoking a static method from a deferred class. Was that what
you meant?

[Johnni Winther, 2014-03-03 13:24:31]

TypeResolver LGTM.

[floitsch, 2014-03-03 14:18:05]

LGTM.

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/177543002/diff/170001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:208: Element
maybePrefix = library.find(import.prefix.toString());
On 2014/03/03 13:18:49, sigurdm wrote:
> On 2014/02/28 12:58:15, floitsch wrote:
> > Isn't it an error if there is no prefix?
> 
> Yes - but it is not fatal - we try compile as much as possible to report more
> errors. Maybe this is wrong?

At least add a comment.

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:200:
Link<MetadataAnnotation> metadatalist = import.metadata;
metadataList ?

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:205: if
(metadata.value.computeType(compiler).element ==
you element just one line above.

if (element == deferredLibraryClass) {

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:736: // Is true
if this prefix belongs to a deferred import.
Might as well make it a Dart comment. "///".

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1686:
compiler.deferredLoadTask.splitProgram) {
why the check for compiler.deferredLoadTast.splitProgram ?

Even if we don't split, it should still be an error/warning.

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1707: return
element;
nit: I preferred the individual returns. But keep as is now.

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1761: // Now
try to construct the type from the element.
-Now-

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/util/util.dart (right):

https://codereview.chromium.org/177543002/diff/190001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/util/util.dart:300: class Pair<S, T> {
Still needed?

https://codereview.chromium.org/177543002/diff/190001/tests/language/deferred...
File tests/language/deferred_constraints_test.dart (right):

https://codereview.chromium.org/177543002/diff/190001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:29: var g2 = new G2<lib.C>(); ///
new_generic2: static type warning
Add comment explaining why this is still (at runtime).

https://codereview.chromium.org/177543002/diff/190001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:35: int i = lib.C.staticMethod();
/// static_method: ok
On 2014/03/03 13:18:49, sigurdm wrote:
> I added a test of invoking a static method from a deferred class. Was that
what
> you meant?

yes.

[sigurdm, 2014-03-04 14:12:36]

Committed patchset #4 manually as r33273 (presubmit successful).

[floitsch, 2014-03-05 16:16:29]

What did you change since the last time you committed?

[floitsch, 2014-03-05 16:16:50]

On 2014/03/05 16:16:29, floitsch wrote:
> What did you change since the last time you committed?

Also: why didn't it stick?

[sigurdm, 2014-03-06 07:47:45]

Several things changed.

The "runtime error" negative tests did not work well in browsers.
So I changed them to Expect.throws positive tests. I think that is better.

The order of the else if-checks in members.dart was wrong - so the wrong warning
was printed.

I chose to mark the csp tests as skip instead of fail. I don't think they are
meaningful to run as we have no hope to have them working, (and moved that
section from language.status to language_dart2js.status).

Then the assert(result.codeUnitAt(0) == '"') test was wrong.

[floitsch, 2014-03-06 15:25:02]

Still LGTM.

https://codereview.chromium.org/177543002/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js/builder.dart (right):

https://codereview.chromium.org/177543002/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js/builder.dart:68:
assert(result.value.codeUnitAt(0) == '"'.codeUnitAt(0));
since this is an assert.
assert(result.value[0] == '"');

This is actually optimized by the VM.

https://codereview.chromium.org/177543002/diff/230001/tests/language/deferred...
File tests/language/deferred_constraints_test.dart (right):

https://codereview.chromium.org/177543002/diff/230001/tests/language/deferred...
tests/language/deferred_constraints_test.dart:35: var g3 = new lib.G<lib.C>();
/// new_generic3: static type warning
Not for this CL, but I think this is going to bite us.

https://codereview.chromium.org/177543002/diff/230001/tests/language/language...
File tests/language/language.status (right):

https://codereview.chromium.org/177543002/diff/230001/tests/language/language...
tests/language/language.status:24: # The vm and dart2dart does not support
deferred loading.
do not

https://codereview.chromium.org/177543002/diff/230001/tests/language/language...
tests/language/language.status:31: # The vm and dart2dart does not support
deferred loading.
do not

https://codereview.chromium.org/177543002/diff/230001/tests/language/language...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/177543002/diff/230001/tests/language/language...
tests/language/language_dart2js.status:300: deferred_constraints_test/*: Skip #
Issue 16898
Without skipping we know at least that it won't crash the compiler.

[sigurdm, 2014-03-07 10:04:57]

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