Redirection type of a redirecting factory can be deferred

Redirection type of a redirecting factory can be deferred


BUG=23408
R=regis@google.com

Committed: https://github.com/dart-lang/sdk/commit/ce917c5160dbefc71091f0b172c0a589e6cb2763

[hausner, 2015-06-30 18:01:15]

hausner@google.com changed reviewers:
+ regis@google.com

[regis, 2015-06-30 18:16:31]

LGTM, but I suspect you are not done yet. See my comment.

https://codereview.chromium.org/1219023002/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1219023002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:12718: // an onloaded deferred prefix, mark this function
as depending
onloaded -> unloaded

https://codereview.chromium.org/1219023002/diff/1/tests/language/regress_2340...
File tests/language/regress_23408_test.dart (right):

https://codereview.chromium.org/1219023002/diff/1/tests/language/regress_2340...
tests/language/regress_23408_test.dart:14: factory C.c() = lib.C;
You should add a test where the redirection type is generic with a deferred type
argument, something like:

factory C.c() = List<lib.C>;

I suspect that the code is not yet able to handle that case correctly.

[hausner, 2015-06-30 21:51:00]

https://codereview.chromium.org/1219023002/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1219023002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:12718: // an onloaded deferred prefix, mark this function
as depending
On 2015/06/30 18:16:31, regis wrote:
> onloaded -> unloaded

Done.

https://codereview.chromium.org/1219023002/diff/1/tests/language/regress_2340...
File tests/language/regress_23408_test.dart (right):

https://codereview.chromium.org/1219023002/diff/1/tests/language/regress_2340...
tests/language/regress_23408_test.dart:14: factory C.c() = lib.C;
On 2015/06/30 18:16:31, regis wrote:
> You should add a test where the redirection type is generic with a deferred
type
> argument, something like:
> 
> factory C.c() = List<lib.C>;
> 
> I suspect that the code is not yet able to handle that case correctly.

What is the behavior you expect? Using an unresolved type as a type parameter is
not a runtime error. The type argument is converted to dynamic in that case.

var x = new List<NoSuchType>();

The above creates a List<dynamic>.

var x = new List<UnloadedPrefix.C>();

This also creates a List<dynamic>.

Therefore, a factory forwarding to List<UnloadedPrefix.C> forwards to
List<dynamic>.

Discussing this with Gilad, we concluded that the Spec is not very clear on the
semantics of using a deferred type as a type argument. The intention is that the
type stays malformed and the argument is mapped to dynamic even after the
deferred type is loaded. 

Fortunately, this is what the VM implements :-) We don't allow deferred types in
type arguments and simply map them to dynamic.

I added a test case.

[regis, 2015-06-30 21:57:06]

On 2015/06/30 21:51:00, hausner wrote:
> https://codereview.chromium.org/1219023002/diff/1/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/1219023002/diff/1/runtime/vm/parser.cc#newcod...
> runtime/vm/parser.cc:12718: // an onloaded deferred prefix, mark this function
> as depending
> On 2015/06/30 18:16:31, regis wrote:
> > onloaded -> unloaded
> 
> Done.
> 
>
https://codereview.chromium.org/1219023002/diff/1/tests/language/regress_2340...
> File tests/language/regress_23408_test.dart (right):
> 
>
https://codereview.chromium.org/1219023002/diff/1/tests/language/regress_2340...
> tests/language/regress_23408_test.dart:14: factory C.c() = lib.C;
> On 2015/06/30 18:16:31, regis wrote:
> > You should add a test where the redirection type is generic with a deferred
> type
> > argument, something like:
> > 
> > factory C.c() = List<lib.C>;
> > 
> > I suspect that the code is not yet able to handle that case correctly.
> 
> What is the behavior you expect? Using an unresolved type as a type parameter
is
> not a runtime error. The type argument is converted to dynamic in that case.
> 
> var x = new List<NoSuchType>();
> 
> The above creates a List<dynamic>.
> 
> var x = new List<UnloadedPrefix.C>();
> 
> This also creates a List<dynamic>.
> 
> Therefore, a factory forwarding to List<UnloadedPrefix.C> forwards to
> List<dynamic>.
> 
> Discussing this with Gilad, we concluded that the Spec is not very clear on
the
> semantics of using a deferred type as a type argument. The intention is that
the
> type stays malformed and the argument is mapped to dynamic even after the
> deferred type is loaded. 
> 
> Fortunately, this is what the VM implements :-) We don't allow deferred types
in
> type arguments and simply map them to dynamic.
> 
> I added a test case.

Thanks for checking and adding a test.
I would have expected the same behavior for a type argument as for a type, i.e.
the caller being invalidated and recompiled once the deferred type argument is
loaded. But the code does not do that, that's the reason I asked to add a test.
Now, if Gilad says the expected behavior is different, then it's all good.

Thanks,
Regis

[hausner, 2015-06-30 21:57:23]

Committed patchset #2 (id:20001) manually as
ce917c5160dbefc71091f0b172c0a589e6cb2763 (presubmit successful).