Fix type variables in closures.

Fix type variables in closures.

BUG=dartbug.com/11258
R=johnniwinther@google.com, ngeoffray@google.com

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

[Johnni Winther, 2013-06-18 06:00:09]

lgtm

https://codereview.chromium.org/17262003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/17262003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:512: } else if
(node.isIsCheck) {
Why not also [:node.isIsNotCheck:] ?

[karlklose, 2013-06-18 07:48:23]

https://codereview.chromium.org/17262003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/17262003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:512: } else if
(node.isIsCheck) {
Of course. Thanks for catching this.

[ngeoffray, 2013-06-18 08:00:43]

DBC

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:512: } else if
(node.isIsCheck || node.isIsNotCheck) {
Why isn't visiting the right hand side of the expression (that will be done in
node.visitChildren) enough?

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:554: if
(outermostElement.isConstructor()) {
Why is it only for constructor? Please add a comment.

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

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3257: // The type
variable is stored on the object and needs to be accessed
which object? The real "this" object?

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3259: return
readTypeVariable(member.getEnclosingClass(),
I would move this code in a if (isClosure) test line 3249. I think that would
read easier to understand what different scenarios can we be in.

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3260: type.element);
Fits in one line?

[karlklose, 2013-06-18 11:05:00]

Thanks for the comments, Nicolas.

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:512: } else if
(node.isIsCheck || node.isIsNotCheck) {
It is not enough because we only recorded the dependency in checked mode before.
 I changed the code in visitTypeAnnotation and removed the case here.

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:554: if
(outermostElement.isConstructor()) {
Added comment and changed to !isField.

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

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3257: // The type
variable is stored on the object and needs to be accessed
Yes.

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3259: return
readTypeVariable(member.getEnclosingClass(),
On 2013/06/18 08:00:43, ngeoffray wrote:
> I would move this code in a if (isClosure) test line 3249. I think that would
> read easier to understand what different scenarios can we be in.

Done.

https://codereview.chromium.org/17262003/diff/8001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3260: type.element);
On 2013/06/18 08:00:43, ngeoffray wrote:
> Fits in one line?

Done.

[ngeoffray, 2013-06-18 11:14:51]

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:474: // with type
variable use in field initializer (in both modes).
use -> used

Could you file a bug? Do you have a failing test case?

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:479: registerNeedsThis();
Thinking more about it, it looks my previous comment was wrong: how do you
differentiate between an is check and a type annotation here? A type annotation
only needs to register this in checked mode. It looks like you're
unconditionally registering it here.

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

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3249: bool
isConstructor =
How about calling this variable isInConstructorContext and add member.isField to
it?

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3257: } else if
(!member.isField()) {
Maybe use the positive form instead and shuffle the code below.

[karlklose, 2013-06-19 12:17:27]

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:474: // with type
variable use in field initializer (in both modes).
In fact it is not really a bug, we have to special case field initializers
because of the way they are generated.  This comment is obsolete because I
reverted the change, but I changed the comment at the place I copied this from.

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:479: registerNeedsThis();
On 2013/06/18 11:14:51, ngeoffray wrote:
> Thinking more about it, it looks my previous comment was wrong: how do you
> differentiate between an is check and a type annotation here? A type
annotation
> only needs to register this in checked mode. It looks like you're
> unconditionally registering it here.

Reverted it back.

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

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3249: bool
isConstructor =
On 2013/06/18 11:14:51, ngeoffray wrote:
> How about calling this variable isInConstructorContext and add member.isField
to
> it?

Done.

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3257: } else if
(!member.isField()) {
I am not sure it will be more readable.

[ngeoffray, 2013-06-19 20:03:57]

LGTM

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

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3257: } else if
(!member.isField()) {
On 2013/06/19 12:17:27, karlklose wrote:
> I am not sure it will be more readable.

What I don't like about negative forms is that everything else goes. In this
example everything else covers the !member.isField, but it's not explicit which
elements you actually cover.

But maybe that's just me, feel free to keep it as is.

https://codereview.chromium.org/17262003/diff/20001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/17262003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:483: // therefore the
closure needs a this-element, if it is not a field;
not a field -> not declared in a field initializer?

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

https://codereview.chromium.org/17262003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3249: bool
isConstructor =
Didn't you say you changed this to isInConstructorContext?

[karlklose, 2013-06-20 10:17:59]

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

https://codereview.chromium.org/17262003/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3257: } else if
(!member.isField()) {
I changed it to use the positive form and added an assert.

https://codereview.chromium.org/17262003/diff/20001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/17262003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:483: // therefore the
closure needs a this-element, if it is not a field;
On 2013/06/19 20:03:57, ngeoffray wrote:
> not a field -> not declared in a field initializer?

Done.

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

https://codereview.chromium.org/17262003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3249: bool
isConstructor =
Perhaps I did not stage or save the change. Done.

[karlklose, 2013-06-20 11:35:53]

Committed patchset #8 manually as r24217 (presubmit successful).