Question about implementation of closure capture lists


(Greg Titus) #1

Hi all,

I thought I’d take a look at SR-153 "Bad fix suggestion for changing value of capture list constants" <https://bugs.swift.org/browse/SR-153>, and I’d really appreciate it if someone more familiar with the code could check my thoughts, rather than me jumping straight to submitting a pull request that might be going in the wrong direction.

First, an entry in a capture list should always be semantically a constant, correct? Right now, the VarDecl ‘isLet’ flag is being set to ownershipKind != Ownership::Weak, which means that a capture of [a] is treated as constant by the type checker, but a capture of [weak a] is not. Thus, this code compiles without error (and prints “nil” and “A" when run), which is certainly unexpected behavior to me:

class A {}
var a = A()
let f = {
    [weak a] in
    a = A()
    print(a)
}
f()
print(a)

I can imagine there being a good reason why a weak var shouldn’t be marked as isLet for optimization purposes, etc, since you can’t assume that it won’t change, but surely the programmer shouldn’t be explicitly using it as an lvalue, right?

I think the solution here is to add a VarDecl flag bit for InClosureCaptureList, set that when setting up the VarDecl in ParseExpr, and then return false from isSettable() (to fix the issue shown here), and early return from emitLetToVarNoteIfSimple() (to fix the original issue 153). These would be similar and in the same places as the existing isa<ParamDecl> checks.

Does all that sound okay?

Thanks!
  - Greg


(Chris Lattner) #2

Hi all,

I thought I’d take a look at SR-153 "Bad fix suggestion for changing value of capture list constants" <https://bugs.swift.org/browse/SR-153>, and I’d really appreciate it if someone more familiar with the code could check my thoughts, rather than me jumping straight to submitting a pull request that might be going in the wrong direction.

First, an entry in a capture list should always be semantically a constant, correct? Right now, the VarDecl ‘isLet’ flag is being set to ownershipKind != Ownership::Weak, which means that a capture of [a] is treated as constant by the type checker, but a capture of [weak a] is not. Thus, this code compiles without error (and prints “nil” and “A" when run), which is certainly unexpected behavior to me:

class A {}
var a = A()
let f = {
   [weak a] in
   a = A()
   print(a)
}
f()
print(a)

Yes, this is not the right behavior. We have to do this, because weak values can actually be mutated at runtime (by the pointee being deallocated, and thus dropping to nil).

The issue here is that (at a pretty deep level) the compiler has conflated two different concepts into the single “isLet” bit: 1) can be mutated at runtime, and 2) can be assigned to. Two examples that violate this: "lazy lets” and “weak lets”. Both of these things can be mutated at the “bits” level, but neither can be assigned to within their current scope.

I can imagine there being a good reason why a weak var shouldn’t be marked as isLet for optimization purposes, etc, since you can’t assume that it won’t change, but surely the programmer shouldn’t be explicitly using it as an lvalue, right?

Right.

I think the solution here is to add a VarDecl flag bit for InClosureCaptureList, set that when setting up the VarDecl in ParseExpr, and then return false from isSettable() (to fix the issue shown here), and early return from emitLetToVarNoteIfSimple() (to fix the original issue 153). These would be similar and in the same places as the existing isa<ParamDecl> checks.

This isn’t specific to capture lists. I’d rather see the isLet bit generalized in one of two ways. Either:

1) Turn it into a three state enum, capturing the ideas of “var” and “let, but can change at runtime” and “let”.
2) Separate these two concepts into two bools.

-Chris

···

On Dec 9, 2015, at 8:57 AM, Greg Titus via swift-dev <swift-dev@swift.org> wrote:


(Greg Titus) #3

Hi all,

I thought I’d take a look at SR-153 "Bad fix suggestion for changing value of capture list constants" <https://bugs.swift.org/browse/SR-153>, and I’d really appreciate it if someone more familiar with the code could check my thoughts, rather than me jumping straight to submitting a pull request that might be going in the wrong direction.

First, an entry in a capture list should always be semantically a constant, correct? Right now, the VarDecl ‘isLet’ flag is being set to ownershipKind != Ownership::Weak, which means that a capture of [a] is treated as constant by the type checker, but a capture of [weak a] is not. Thus, this code compiles without error (and prints “nil” and “A" when run), which is certainly unexpected behavior to me:

class A {}
var a = A()
let f = {
  [weak a] in
  a = A()
  print(a)
}
f()
print(a)

Yes, this is not the right behavior. We have to do this, because weak values can actually be mutated at runtime (by the pointee being deallocated, and thus dropping to nil).

The issue here is that (at a pretty deep level) the compiler has conflated two different concepts into the single “isLet” bit: 1) can be mutated at runtime, and 2) can be assigned to. Two examples that violate this: "lazy lets” and “weak lets”. Both of these things can be mutated at the “bits” level, but neither can be assigned to within their current scope.

Is there a way that I’m not seeing to get these sorts of constructs right now, or are these constructs planned? Both “lazy" and “weak” yield compiler errors if they are used with “let” instead of “var” at the moment.

I can imagine there being a good reason why a weak var shouldn’t be marked as isLet for optimization purposes, etc, since you can’t assume that it won’t change, but surely the programmer shouldn’t be explicitly using it as an lvalue, right?

Right.

I think the solution here is to add a VarDecl flag bit for InClosureCaptureList, set that when setting up the VarDecl in ParseExpr, and then return false from isSettable() (to fix the issue shown here), and early return from emitLetToVarNoteIfSimple() (to fix the original issue 153). These would be similar and in the same places as the existing isa<ParamDecl> checks.

This isn’t specific to capture lists. I’d rather see the isLet bit generalized in one of two ways. Either:

1) Turn it into a three state enum, capturing the ideas of “var” and “let, but can change at runtime” and “let”.
2) Separate these two concepts into two bools.

Makes sense. I’ve gone with what I think is the lowest impact change for now, which is to add a second bool named “IsUserAssignable":

“var": IsLet = false, IsUserAssignable = true
"let but can change at runtime” i.e. mutable but not by the programmer: IsLet = false, IsUserAssignable = false
“let": IsLet = true, IsUserAssignable = true (since the user can assign to it exactly once)

Submitted as <https://github.com/apple/swift/pull/386>.

Thanks for the input!
  - Greg

···

On Dec 9, 2015, at 2:39 PM, Chris Lattner <clattner@apple.com> wrote:

On Dec 9, 2015, at 8:57 AM, Greg Titus via swift-dev <swift-dev@swift.org> wrote:

-Chris