Support type variables on redirecting factory constructors.

Support type variables on redirecting factory constructors.

R=ngeoffray@google.com

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

[ahe, 2013-01-29 09:30:58]

DBC

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3236: // factory
constructors.
This should be done by the resolver. At least add a TODO.

[ngeoffray, 2013-01-29 09:37:22]

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3236: // factory
constructors.
On 2013/01/29 09:30:58, ahe wrote:
> This should be done by the resolver. At least add a TODO.

What would it take to move it to the resolver? Karl, any reason why you're doing
this here?

[ngeoffray, 2013-01-29 10:23:35]

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (left):

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3235: constructor =
functionElement.redirectionTarget;
@Peter: Why are we changing constructor here? The builder can do the general
inlining for you, we don't need to explicitly do it.

[ahe, 2013-01-29 10:57:48]

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (left):

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3235: constructor =
functionElement.redirectionTarget;
On 2013/01/29 10:23:35, ngeoffray wrote:
> @Peter: Why are we changing constructor here? The builder can do the general
> inlining for you, we don't need to explicitly do it.

Because I didn't know better.

[ngeoffray, 2013-01-29 11:03:19]

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (left):

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3235: constructor =
functionElement.redirectionTarget;
On 2013/01/29 10:57:48, ahe wrote:
> On 2013/01/29 10:23:35, ngeoffray wrote:
> > @Peter: Why are we changing constructor here? The builder can do the general
> > inlining for you, we don't need to explicitly do it.
> 
> Because I didn't know better.

After talking to Karl, it looks like we would still need to special case
something: the AST for the forwarding factory is pretty special:
Return(TypeAnnotation)

Compared to a regular factory:
Return(New(TypeAnnotation))

[karlklose, 2013-01-29 12:37:00]

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12093019/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3236: // factory
constructors.
Why do you think this belongs in the resolver (and what parts)?

This code is only computing the type arguments for the short-cut call to the
redirection target using already resolved types/elements.

[karlklose, 2013-02-01 07:37:44]

Ping.

[karlklose, 2013-02-09 07:12:36]

Ping.

[ngeoffray, 2013-02-18 09:44:28]

LGTM

https://codereview.chromium.org/12093019/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12093019/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3235: // Compute the
right target and type for redirecting generative and
What's a redirecting generative constructor?

https://codereview.chromium.org/12093019/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3238: if
(functionElement.redirectionTarget != functionElement) {
Shouldn't you get the redirectionTarget until it's null?

https://codereview.chromium.org/12093019/diff/9001/tests/language/language_da...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/12093019/diff/9001/tests/language/language_da...
tests/language/language_dart2js.status:91: factory_redirection_test/14: Fail #
redirecing to redirecting factory not implemented.
redirecing -> redirecting

[karlklose, 2013-02-19 13:20:27]

Thanks for the comments.

https://codereview.chromium.org/12093019/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12093019/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3235: // Compute the
right target and type for redirecting generative and
One that uses this(...), but they never actually end here, the comment was
wrong. Removed it.

https://codereview.chromium.org/12093019/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3238: if
(functionElement.redirectionTarget != functionElement) {
Yes. It does not matter currently, because we bail out for multiple
redirections.

https://codereview.chromium.org/12093019/diff/9001/tests/language/language_da...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/12093019/diff/9001/tests/language/language_da...
tests/language/language_dart2js.status:91: factory_redirection_test/14: Fail #
redirecing to redirecting factory not implemented.
On 2013/02/18 09:44:28, ngeoffray wrote:
> redirecing -> redirecting

Done.

[karlklose, 2013-05-29 13:53:48]

PTAL, I have rewritten the CL.

[ngeoffray, 2013-05-30 08:23:14]

LGTM

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2504:
compiler.backend.registerThrowNoSuchMethod(mapping);
Do you really need to make it explicit in both cases? I'd understand the backend
needs the helper if it is unresolved, but what about the if it's not a
constructor? What do we emit?

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2522:
compiler.backend.registerThrowNoSuchMethod(mapping);
I think you can remove the registering here.

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3385: bool
isRedirected = functionElement.redirectionTarget != functionElement;
Add this logic to the FunctionElement class or Elements.

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3403:
compiler.codegenWorld.instantiatedTypes.add(type);
Why is that needed here now, even in the case it's not a redirecting factory?

[karlklose, 2013-05-30 11:44:23]

Thanks for the review, Nicolas!

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2504:
compiler.backend.registerThrowNoSuchMethod(mapping);
The test is not necessary, I removed it.

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2522:
compiler.backend.registerThrowNoSuchMethod(mapping);
I have added logic to test whether we need this.

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3385: bool
isRedirected = functionElement.redirectionTarget != functionElement;
On 2013/05/30 08:23:14, ngeoffray wrote:
> Add this logic to the FunctionElement class or Elements.

Done.

https://codereview.chromium.org/12093019/diff/23005/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3403:
compiler.codegenWorld.instantiatedTypes.add(type);
This was a leftover from the old state of the builder in the original CL,
removed.

[ngeoffray, 2013-05-30 11:57:22]

LGTM!

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:738: bool get
isRedirection;
isRedirection -> isRedirectingFactory ?

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1111: for
(Element namedParameter in signature.orderedOptionalParameters) {
I don't think you need the ordered version.

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1117: if
(requiredParameterCount > signature.requiredParameterCount ||
Please add a comment how it is ok to have more parameters in the presence of
non-named optional parameters.

[karlklose, 2013-05-30 12:24:41]

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:738: bool get
isRedirection;
On 2013/05/30 11:57:23, ngeoffray wrote:
> isRedirection -> isRedirectingFactory ?

Done.

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1111: for
(Element namedParameter in signature.orderedOptionalParameters) {
Yes, changed to optionalParameters.

https://codereview.chromium.org/12093019/diff/39001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1117: if
(requiredParameterCount > signature.requiredParameterCount ||
On 2013/05/30 11:57:23, ngeoffray wrote:
> Please add a comment how it is ok to have more parameters in the presence of
> non-named optional parameters.

Done.

[karlklose, 2013-05-30 13:14:52]

Committed patchset #10 manually as r23407 (presubmit successful).