[Review] SE-0176: Enforce Exclusive Access to Memory


(Ben Cohen) #1

Hello Swift community,

The review of revisions to SE-0176: Enforce Exclusive Access to Memory begins now and runs through May 17, 2017.

Most of this proposal was previously accepted. An implementation issue has been discovered with the use of dynamic enforcement on inout parameters. The proposal implementors suggest adopting a stronger rule governing the use of non-escaping closures which will also allow Swift to make firm guarantees about the use of static enforcement when a variable does not escape. The core team tentatively supports this new rule but believes it is a substantial enough revision that it requires a separate review period.
The proposal is available here: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md
Since this is a review of revisions only, you may find these two relevant commits easier:
https://github.com/apple/swift-evolution/commit/d61c07df2f02bee6c00528e73fbe33738288179a
https://github.com/apple/swift-evolution/commit/5205a61f9cdca918d896269521bf89cb11e4aa12

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:
https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md

Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,


(Christopher Kornher) #2

https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md
What is your evaluation of the proposal?
+1 It seems to be a carefully considered and balanced approach to simultaneous memory access in single threaded code.
Is the problem being addressed significant enough to warrant a change to Swift?
It sure seems to be.
Does this proposal fit well with the feel and direction of Swift?
Absolutely. Safety is one of the paramount goals of Swift and the ‘gotchas’ listed in the document can lead to subtle bugs, with potentially fatal consequences.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
variants of `memcpy()` and similar C functions. Complex manipulation of data structures in C, and C++ could lead to similar issues, I suppose, but the simple memory model of these languages puts developers on guard against simultaneous memory access. I certainly was not aware of these issues is Swift, but I am very cautious around mutating value types. I expect that many Swift developers coming from classical OO languages are similarly wary of mutating value types, or should be. Fixing these issues will give me more confidence when dealing with mutating value types.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I read through the proposal. It is very well written. While I did not carefully study each case, I feel that I have a good understanding of the problem and the philosophy behind the solution. I feel confident that this will be a big improvement to the safety of Swift.


(Howard Lovatt) #3

+1, the benefits outweigh the extra restrictions. Particularly since it is
easy to add an escaping annotation if you think the compiler is wrong.
However this will require careful documentation and good error messages so
that people know why it fails to type check and how to overcome that.

  -- Howard.

···

On 13 May 2017 at 12:29, Ben Cohen <ben_cohen@apple.com> wrote:

Hello Swift community,

The review of revisions to *SE-0176: **Enforce Exclusive Access to Memory* begins
now and runs through *May 17, 2017*.
Most of this proposal was previously *accepted*. An implementation issue
has been discovered with the use of dynamic enforcement on inout
parameters. The proposal implementors suggest adopting a stronger rule
governing the use of non-escaping closures which will also allow Swift to
make firm guarantees about the use of static enforcement when a variable
does not escape. The core team tentatively supports this new rule but
believes it is a substantial enough revision that it requires a separate
review period.

The proposal is available here: https://github.com/
apple/swift-evolution/blob/master/proposals/0176-enforce-
exclusive-access-to-memory.md
Since this is a review of revisions only, you may find these two relevant
commits easier:
https://github.com/apple/swift-evolution/commit/
d61c07df2f02bee6c00528e73fbe33738288179a
https://github.com/apple/swift-evolution/commit/
5205a61f9cdca918d896269521bf89cb11e4aa12

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at:

https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager. When replying, please try to keep the proposal link at the
top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/
proposals/0176-enforce-exclusive-access-to-memory.md

Reply text

Other replies

What goes into a review?

The goal of the review process is to improve the proposal under review
through constructive criticism and, eventually, determine the direction of
Swift. When writing your review, here are some questions you might want to
answer in your review:

   - What is your evaluation of the proposal?
   - Is the problem being addressed significant enough to warrant a
   change to Swift?
   - Does this proposal fit well with the feel and direction of Swift?
   - If you have used other languages or libraries with a similar
   feature, how do you feel that this proposal compares to those?
   - How much effort did you put into your review? A glance, a quick
   reading, or an in-depth study?

More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

_______________________________________________
swift-evolution-announce mailing list
swift-evolution-announce@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution-announce


(Florent Bruneau) #4

Hi,

I may simply have missed something, but I'm not sure to understand why the proposal adds user-facing restrictions instead of using a fallback from static to dynamic enforcement in case of violation of the NRR/NPCR rules (maybe with an additional warning).

···

Le 13 mai 2017 à 04:29, Ben Cohen <ben_cohen@apple.com> a écrit :

Hello Swift community,

The review of revisions to SE-0176: Enforce Exclusive Access to Memory begins now and runs through May 17, 2017.

Most of this proposal was previously accepted. An implementation issue has been discovered with the use of dynamic enforcement on inout parameters. The proposal implementors suggest adopting a stronger rule governing the use of non-escaping closures which will also allow Swift to make firm guarantees about the use of static enforcement when a variable does not escape. The core team tentatively supports this new rule but believes it is a substantial enough revision that it requires a separate review period.
The proposal is available here: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md

Since this is a review of revisions only, you may find these two relevant commits easier:
https://github.com/apple/swift-evolution/commit/d61c07df2f02bee6c00528e73fbe33738288179a
https://github.com/apple/swift-evolution/commit/5205a61f9cdca918d896269521bf89cb11e4aa12

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:
https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md

Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

_______________________________________________
swift-evolution-announce mailing list
swift-evolution-announce@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution-announce

--
Florent Bruneau


(Jordan Rose) #5

[Proposal: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md]

I have severe concerns about these revisions because they make withoutActuallyEscaping harder to reason about.

+ - Programmers using ``withoutActuallyEscaping`` should take
+ care not to allow the result to be recursively invoked.

+[…] if they are certain that their code will
+not violate the NRR, use ``withoutActuallyEscaping`` to disable
+the NPCR check.

I think this constitutes a violation of the user model for withoutActuallyEscaping, because withoutActuallyEscaping hasn't historically meant withoutBeingReentrant. The proposal "should take care" is a very mild way of saying we're introducing a new rule under which the compiler can silently do something different than what you wrote (with no "unsafe" in sight). Similarly, using withoutActuallyEscaping to mean withoutActuallyViolatingExclusivity seems like it'll come back to haunt us, the kind of thing where people ask on Stack Overflow why the Swift people didn't design something that said what it did.

When I first brought this up on an Apple-internal list, John let me know that we could introduce checking for it. This helps assuage my concerns quite a bit, but I'm not sure how we would do so without modifying the original closure, and if we can modify the original closure I'm not sure we've saved anything over proper dynamic checking. John, can you clarify for the list how we might check for this? Like withoutActuallyEscaping, it doesn't have to be something we implement right away as long as we have the ability to do so without changing the ABI.

Thanks,
Jordan

P.S. Devin and I had previously discussed an additional example that does not seem to be forbidden by these rules. Is that correct and will this program continue to print "2 2"?

func invoke(_ callback: /*nonescaping*/ () -> Void) {
  callback()
}
class Foo {
  var op: () -> Void = {}
  var prop = 0
  func test() {
    var x = 0
    self.op = { x = 1; self.prop = 1 }
    invoke { self.op(); x += 1; self.prop += 1 }
    print(x, self.prop)
  }
}
Foo().test()


(David Sweeris) #6

This subject is a bit over my head, but from what I do understand, +1

- Dave Sweeris

···

On May 12, 2017, at 19:29, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The review of revisions to SE-0176: Enforce Exclusive Access to Memory begins now and runs through May 17, 2017.

Most of this proposal was previously accepted. An implementation issue has been discovered with the use of dynamic enforcement on inout parameters. The proposal implementors suggest adopting a stronger rule governing the use of non-escaping closures which will also allow Swift to make firm guarantees about the use of static enforcement when a variable does not escape. The core team tentatively supports this new rule but believes it is a substantial enough revision that it requires a separate review period.
The proposal is available here: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md


(John McCall) #7

I may simply have missed something, but I'm not sure to understand why the proposal adds user-facing restrictions instead of using a fallback from static to dynamic enforcement in case of violation of the NRR/NPCR rules (maybe with an additional warning).

Three reasons.

The first is that it's much simpler for both users and the compiler if there is a bright-line rule guaranteeing that static enforcement will be used. A programmer trying to understand "why is this slower" or "why is this problem only checked at runtime" should not have to understand the language implementation well enough to examine their code and e.g. deduce that the problems is that they made an un-inlineable call during an access within a closure and the compiler couldn't prove that that didn't lead to a re-entrant access and so it fell back to dynamic enforcement. Paring back dynamic enforcement to the same cases where we need to allocate variables on the heap is far more straightforward to explain and understand.

The second is closely related, which is that it preserves the ability to have a subset of the language which doesn't need dynamic allocation or enforcement. This sort of performance and semantic guarantee is important in a number of situations, from audio programming to low-level systems work. You could imagine having a special scope, or even a compiler flag, that mandates staying within that subset. But you wouldn't want that flag to change the dynamic behavior of the program or rely for correctness on an assumption that the entire program, libraries included, are compiled with the flag. And that ties in with the next point.

The final reason is that, when compilers get conservative, they get very conservative. For example, we can only tell that the NPCR is being violated by looking at the function that takes the closures, not the one that creates them; but it's the latter function which decides what enforcement to use for its captured variables. Statically detecting NPCR violations, therefore, basically requires the ability to inline every function to which we passed them — we don't actually have to do the inlining, but we do have to be able to see their definitions, which in many cases is not possible. And most of the heuristics you might imagine we could use to statically prove that a closure can't be recursively invoked aren't actually valid. So conservatively falling back on dynamic enforcement in closures really means doing it pervasively.

When you take those reasons and balance them against the patterns that are being restricted here — and really, they're still legal, you just have to mark one of the closures as escaping —it's not really a difficult choice.

John.

···

On May 15, 2017, at 3:00 AM, Florent Bruneau via swift-evolution <swift-evolution@swift.org> wrote:

Le 13 mai 2017 à 04:29, Ben Cohen <ben_cohen@apple.com> a écrit :

Hello Swift community,

The review of revisions to SE-0176: Enforce Exclusive Access to Memory begins now and runs through May 17, 2017.

Most of this proposal was previously accepted. An implementation issue has been discovered with the use of dynamic enforcement on inout parameters. The proposal implementors suggest adopting a stronger rule governing the use of non-escaping closures which will also allow Swift to make firm guarantees about the use of static enforcement when a variable does not escape. The core team tentatively supports this new rule but believes it is a substantial enough revision that it requires a separate review period.
The proposal is available here: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md

Since this is a review of revisions only, you may find these two relevant commits easier:
https://github.com/apple/swift-evolution/commit/d61c07df2f02bee6c00528e73fbe33738288179a
https://github.com/apple/swift-evolution/commit/5205a61f9cdca918d896269521bf89cb11e4aa12

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:
https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md

Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

_______________________________________________
swift-evolution-announce mailing list
swift-evolution-announce@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution-announce

--
Florent Bruneau

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Florent Bruneau) #8

I may simply have missed something, but I'm not sure to understand why the proposal adds user-facing restrictions instead of using a fallback from static to dynamic enforcement in case of violation of the NRR/NPCR rules (maybe with an additional warning).

Three reasons.

The first is that it's much simpler for both users and the compiler if there is a bright-line rule guaranteeing that static enforcement will be used. A programmer trying to understand "why is this slower" or "why is this problem only checked at runtime" should not have to understand the language implementation well enough to examine their code and e.g. deduce that the problems is that they made an un-inlineable call during an access within a closure and the compiler couldn't prove that that didn't lead to a re-entrant access and so it fell back to dynamic enforcement. Paring back dynamic enforcement to the same cases where we need to allocate variables on the heap is far more straightforward to explain and understand.

The second is closely related, which is that it preserves the ability to have a subset of the language which doesn't need dynamic allocation or enforcement. This sort of performance and semantic guarantee is important in a number of situations, from audio programming to low-level systems work. You could imagine having a special scope, or even a compiler flag, that mandates staying within that subset. But you wouldn't want that flag to change the dynamic behavior of the program or rely for correctness on an assumption that the entire program, libraries included, are compiled with the flag. And that ties in with the next point.

The final reason is that, when compilers get conservative, they get very conservative. For example, we can only tell that the NPCR is being violated by looking at the function that takes the closures, not the one that creates them; but it's the latter function which decides what enforcement to use for its captured variables. Statically detecting NPCR violations, therefore, basically requires the ability to inline every function to which we passed them — we don't actually have to do the inlining, but we do have to be able to see their definitions, which in many cases is not possible. And most of the heuristics you might imagine we could use to statically prove that a closure can't be recursively invoked aren't actually valid. So conservatively falling back on dynamic enforcement in closures really means doing it pervasively.

When you take those reasons and balance them against the patterns that are being restricted here — and really, they're still legal, you just have to mark one of the closures as escaping —it's not really a difficult choice.

Thanks for the details, this really helps understanding the choice you made. I can only +1 the proposal.

···

Le 15 mai 2017 à 10:55, John McCall <rjmccall@apple.com> a écrit :

On May 15, 2017, at 3:00 AM, Florent Bruneau via swift-evolution <swift-evolution@swift.org> wrote:

Le 13 mai 2017 à 04:29, Ben Cohen <ben_cohen@apple.com> a écrit :

Hello Swift community,

The review of revisions to SE-0176: Enforce Exclusive Access to Memory begins now and runs through May 17, 2017.

Most of this proposal was previously accepted. An implementation issue has been discovered with the use of dynamic enforcement on inout parameters. The proposal implementors suggest adopting a stronger rule governing the use of non-escaping closures which will also allow Swift to make firm guarantees about the use of static enforcement when a variable does not escape. The core team tentatively supports this new rule but believes it is a substantial enough revision that it requires a separate review period.
The proposal is available here: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md

Since this is a review of revisions only, you may find these two relevant commits easier:
https://github.com/apple/swift-evolution/commit/d61c07df2f02bee6c00528e73fbe33738288179a
https://github.com/apple/swift-evolution/commit/5205a61f9cdca918d896269521bf89cb11e4aa12

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at:
https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md

Reply text

Other replies
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

_______________________________________________
swift-evolution-announce mailing list
swift-evolution-announce@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution-announce

--
Florent Bruneau

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

--
Florent Bruneau


(John McCall) #9

[Proposal: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md]

I have severe concerns about these revisions because they make withoutActuallyEscaping harder to reason about.

+ - Programmers using ``withoutActuallyEscaping`` should take
+ care not to allow the result to be recursively invoked.

+[…] if they are certain that their code will
+not violate the NRR, use ``withoutActuallyEscaping`` to disable
+the NPCR check.

I think this constitutes a violation of the user model for withoutActuallyEscaping, because withoutActuallyEscaping hasn't historically meant withoutBeingReentrant. The proposal "should take care" is a very mild way of saying we're introducing a new rule under which the compiler can silently do something different than what you wrote (with no "unsafe" in sight). Similarly, using withoutActuallyEscaping to mean withoutActuallyViolatingExclusivity seems like it'll come back to haunt us, the kind of thing where people ask on Stack Overflow why the Swift people didn't design something that said what it did.

You're right that it's a silent change in the semantics of withoutActuallyEscaping. I'm comfortable with that because of the ways in which I expect withoutActuallyEscaping to be used, but I can see why someone wouldn't be.

When I first brought this up on an Apple-internal list, John let me know that we could introduce checking for it. This helps assuage my concerns quite a bit, but I'm not sure how we would do so without modifying the original closure, and if we can modify the original closure I'm not sure we've saved anything over proper dynamic checking.

withoutActuallyEscaping does not actually promise to return exactly the original closure (which is not a user-detectable property of the closure value), just something semantically equivalent. In particular, we can wrap it in a thunk that (say) dynamically asserts that the closure is not called re-entrantly.

We may need withoutActuallyEscaping to add a thunk anyway, because I don't know that we actually want to guarantee that the context pointer of a non-escaping closure is retainable; it costs us unnecessary set-up code in the caller.

John, can you clarify for the list how we might check for this? Like withoutActuallyEscaping, it doesn't have to be something we implement right away as long as we have the ability to do so without changing the ABI.

Thanks,
Jordan

P.S. Devin and I had previously discussed an additional example that does not seem to be forbidden by these rules. Is that correct and will this program continue to print "2 2"?

func invoke(_ callback: /*nonescaping*/ () -> Void) {
  callback()
}
class Foo {
  var op: () -> Void = {}
  var prop = 0
  func test() {
    var x = 0
    self.op = { x = 1; self.prop = 1 }
    invoke { self.op(); x += 1; self.prop += 1 }
    print(x, self.prop)
  }
}
Foo().test()

Correct. The closure passed to 'invoke' does recurse into a closure that captures the same variable, but that closure is not non-escaping, so the NRR is not violated.

On a design-intent level, recursing into an escaping closure is fine because any variable captured in an escaping closure is escaping, and therefore the accesses to it are generally not statically analyzable and must use dynamic enforcement.

(I did just now notice the phrasing of the NRR in the proposal isn't clear about the restriction only forbidding *indirect* recursion, but your example doesn't violate the stronger rule either. This can be fixed later because it's just a weakening.)

John.

···

On May 15, 2017, at 9:00 PM, Jordan Rose <jordan_rose@apple.com> wrote:


(Jordan Rose) #10

[Proposal: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md]

I have severe concerns about these revisions because they make withoutActuallyEscaping harder to reason about.

+ - Programmers using ``withoutActuallyEscaping`` should take
+ care not to allow the result to be recursively invoked.

+[…] if they are certain that their code will
+not violate the NRR, use ``withoutActuallyEscaping`` to disable
+the NPCR check.

I think this constitutes a violation of the user model for withoutActuallyEscaping, because withoutActuallyEscaping hasn't historically meant withoutBeingReentrant. The proposal "should take care" is a very mild way of saying we're introducing a new rule under which the compiler can silently do something different than what you wrote (with no "unsafe" in sight). Similarly, using withoutActuallyEscaping to mean withoutActuallyViolatingExclusivity seems like it'll come back to haunt us, the kind of thing where people ask on Stack Overflow why the Swift people didn't design something that said what it did.

You're right that it's a silent change in the semantics of withoutActuallyEscaping. I'm comfortable with that because of the ways in which I expect withoutActuallyEscaping to be used, but I can see why someone wouldn't be.

When I first brought this up on an Apple-internal list, John let me know that we could introduce checking for it. This helps assuage my concerns quite a bit, but I'm not sure how we would do so without modifying the original closure, and if we can modify the original closure I'm not sure we've saved anything over proper dynamic checking.

withoutActuallyEscaping does not actually promise to return exactly the original closure (which is not a user-detectable property of the closure value), just something semantically equivalent. In particular, we can wrap it in a thunk that (say) dynamically asserts that the closure is not called re-entrantly.

We may need withoutActuallyEscaping to add a thunk anyway, because I don't know that we actually want to guarantee that the context pointer of a non-escaping closure is retainable; it costs us unnecessary set-up code in the caller.

I thought of this too, but it doesn't handle the case where you use withoutActuallyEscaping and use the original closure directly from within the block. Maybe we can forbid that, though—it seems extra-rare. Does it make sense to add that as an extra rule in this proposal?

John, can you clarify for the list how we might check for this? Like withoutActuallyEscaping, it doesn't have to be something we implement right away as long as we have the ability to do so without changing the ABI.

Thanks,
Jordan

P.S. Devin and I had previously discussed an additional example that does not seem to be forbidden by these rules. Is that correct and will this program continue to print "2 2"?

func invoke(_ callback: /*nonescaping*/ () -> Void) {
  callback()
}
class Foo {
  var op: () -> Void = {}
  var prop = 0
  func test() {
    var x = 0
    self.op = { x = 1; self.prop = 1 }
    invoke { self.op(); x += 1; self.prop += 1 }
    print(x, self.prop)
  }
}
Foo().test()

Correct. The closure passed to 'invoke' does recurse into a closure that captures the same variable, but that closure is not non-escaping, so the NRR is not violated.

On a design-intent level, recursing into an escaping closure is fine because any variable captured in an escaping closure is escaping, and therefore the accesses to it are generally not statically analyzable and must use dynamic enforcement.

(I did just now notice the phrasing of the NRR in the proposal isn't clear about the restriction only forbidding *indirect* recursion, but your example doesn't violate the stronger rule either. This can be fixed later because it's just a weakening.)

Thanks for the clarification!
Jordan

···

On May 15, 2017, at 20:29, John McCall <rjmccall@apple.com> wrote:

On May 15, 2017, at 9:00 PM, Jordan Rose <jordan_rose@apple.com <mailto:jordan_rose@apple.com>> wrote:


(John McCall) #11

[Proposal: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md]

I have severe concerns about these revisions because they make withoutActuallyEscaping harder to reason about.

+ - Programmers using ``withoutActuallyEscaping`` should take
+ care not to allow the result to be recursively invoked.

+[…] if they are certain that their code will
+not violate the NRR, use ``withoutActuallyEscaping`` to disable
+the NPCR check.

I think this constitutes a violation of the user model for withoutActuallyEscaping, because withoutActuallyEscaping hasn't historically meant withoutBeingReentrant. The proposal "should take care" is a very mild way of saying we're introducing a new rule under which the compiler can silently do something different than what you wrote (with no "unsafe" in sight). Similarly, using withoutActuallyEscaping to mean withoutActuallyViolatingExclusivity seems like it'll come back to haunt us, the kind of thing where people ask on Stack Overflow why the Swift people didn't design something that said what it did.

You're right that it's a silent change in the semantics of withoutActuallyEscaping. I'm comfortable with that because of the ways in which I expect withoutActuallyEscaping to be used, but I can see why someone wouldn't be.

When I first brought this up on an Apple-internal list, John let me know that we could introduce checking for it. This helps assuage my concerns quite a bit, but I'm not sure how we would do so without modifying the original closure, and if we can modify the original closure I'm not sure we've saved anything over proper dynamic checking.

withoutActuallyEscaping does not actually promise to return exactly the original closure (which is not a user-detectable property of the closure value), just something semantically equivalent. In particular, we can wrap it in a thunk that (say) dynamically asserts that the closure is not called re-entrantly.

We may need withoutActuallyEscaping to add a thunk anyway, because I don't know that we actually want to guarantee that the context pointer of a non-escaping closure is retainable; it costs us unnecessary set-up code in the caller.

I thought of this too, but it doesn't handle the case where you use withoutActuallyEscaping and use the original closure directly from within the block. Maybe we can forbid that, though—it seems extra-rare. Does it make sense to add that as an extra rule in this proposal?

Well, you'd have to integrate the check with some sort of record-keeping done by the original closures. I'm not entirely sure what that record-keeping would look like off-hand.

I'm not sure what you're proposing to disallow here. I don't think we should add any extra restrictions in order to avoid problems with unfortunate uses of withoutActuallyEscaping, though.

John.

···

On May 16, 2017, at 5:20 PM, Jordan Rose <jordan_rose@apple.com> wrote:

On May 15, 2017, at 20:29, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

On May 15, 2017, at 9:00 PM, Jordan Rose <jordan_rose@apple.com <mailto:jordan_rose@apple.com>> wrote:

John, can you clarify for the list how we might check for this? Like withoutActuallyEscaping, it doesn't have to be something we implement right away as long as we have the ability to do so without changing the ABI.

Thanks,
Jordan

P.S. Devin and I had previously discussed an additional example that does not seem to be forbidden by these rules. Is that correct and will this program continue to print "2 2"?

func invoke(_ callback: /*nonescaping*/ () -> Void) {
  callback()
}
class Foo {
  var op: () -> Void = {}
  var prop = 0
  func test() {
    var x = 0
    self.op = { x = 1; self.prop = 1 }
    invoke { self.op(); x += 1; self.prop += 1 }
    print(x, self.prop)
  }
}
Foo().test()

Correct. The closure passed to 'invoke' does recurse into a closure that captures the same variable, but that closure is not non-escaping, so the NRR is not violated.

On a design-intent level, recursing into an escaping closure is fine because any variable captured in an escaping closure is escaping, and therefore the accesses to it are generally not statically analyzable and must use dynamic enforcement.

(I did just now notice the phrasing of the NRR in the proposal isn't clear about the restriction only forbidding *indirect* recursion, but your example doesn't violate the stronger rule either. This can be fixed later because it's just a weakening.)

Thanks for the clarification!
Jordan


(Jordan Rose) #12

Here's a small, currently-legal, re-entrant case that only calls the wrapped function once.

var global: (() -> Void)?
func problem(_ fn: () -> Void) {
  withoutActuallyEscaping(fn) { wrappedFn in
    global = wrappedFn
    fn() // note: not wrappedFn
  }
}
func test() {
  var local = 0
  problem() {
    local += 1
    if let callback = global {
      global = nil
      callback()
    }
    print(local)
  }
}
test()

We could prevent this by saying it's not legal to refer to 'fn' inside the callback for withoutActuallyEscaping, and you have to use 'wrappedFn' instead.

Jordan

···

On May 16, 2017, at 17:37, John McCall <rjmccall@apple.com> wrote:

On May 16, 2017, at 5:20 PM, Jordan Rose <jordan_rose@apple.com <mailto:jordan_rose@apple.com>> wrote:

On May 15, 2017, at 20:29, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

On May 15, 2017, at 9:00 PM, Jordan Rose <jordan_rose@apple.com <mailto:jordan_rose@apple.com>> wrote:

[Proposal: https://github.com/apple/swift-evolution/blob/master/proposals/0176-enforce-exclusive-access-to-memory.md]

I have severe concerns about these revisions because they make withoutActuallyEscaping harder to reason about.

+ - Programmers using ``withoutActuallyEscaping`` should take
+ care not to allow the result to be recursively invoked.

+[…] if they are certain that their code will
+not violate the NRR, use ``withoutActuallyEscaping`` to disable
+the NPCR check.

I think this constitutes a violation of the user model for withoutActuallyEscaping, because withoutActuallyEscaping hasn't historically meant withoutBeingReentrant. The proposal "should take care" is a very mild way of saying we're introducing a new rule under which the compiler can silently do something different than what you wrote (with no "unsafe" in sight). Similarly, using withoutActuallyEscaping to mean withoutActuallyViolatingExclusivity seems like it'll come back to haunt us, the kind of thing where people ask on Stack Overflow why the Swift people didn't design something that said what it did.

You're right that it's a silent change in the semantics of withoutActuallyEscaping. I'm comfortable with that because of the ways in which I expect withoutActuallyEscaping to be used, but I can see why someone wouldn't be.

When I first brought this up on an Apple-internal list, John let me know that we could introduce checking for it. This helps assuage my concerns quite a bit, but I'm not sure how we would do so without modifying the original closure, and if we can modify the original closure I'm not sure we've saved anything over proper dynamic checking.

withoutActuallyEscaping does not actually promise to return exactly the original closure (which is not a user-detectable property of the closure value), just something semantically equivalent. In particular, we can wrap it in a thunk that (say) dynamically asserts that the closure is not called re-entrantly.

We may need withoutActuallyEscaping to add a thunk anyway, because I don't know that we actually want to guarantee that the context pointer of a non-escaping closure is retainable; it costs us unnecessary set-up code in the caller.

I thought of this too, but it doesn't handle the case where you use withoutActuallyEscaping and use the original closure directly from within the block. Maybe we can forbid that, though—it seems extra-rare. Does it make sense to add that as an extra rule in this proposal?

Well, you'd have to integrate the check with some sort of record-keeping done by the original closures. I'm not entirely sure what that record-keeping would look like off-hand.

I'm not sure what you're proposing to disallow here. I don't think we should add any extra restrictions in order to avoid problems with unfortunate uses of withoutActuallyEscaping, though.


(Devin Coughlin) #13

This rule seems quite reasonable to me and possible to check statically. It is only feasible to enforce the rule statically when the closure passed to the ‘do’ argument of withoutActuallyEscaping(_:do:) hasn’t itself escaped — but since ‘fn’ here is noescape it can’t be referred to inside a closure that escapes.

Devin

···

On May 16, 2017, at 5:50 PM, Jordan Rose via swift-evolution <swift-evolution@swift.org> wrote:

On May 16, 2017, at 17:37, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

I'm not sure what you're proposing to disallow here. I don't think we should add any extra restrictions in order to avoid problems with unfortunate uses of withoutActuallyEscaping, though.

Here's a small, currently-legal, re-entrant case that only calls the wrapped function once.

var global: (() -> Void)?
func problem(_ fn: () -> Void) {
  withoutActuallyEscaping(fn) { wrappedFn in
    global = wrappedFn
    fn() // note: not wrappedFn
  }
}
func test() {
  var local = 0
  problem() {
    local += 1
    if let callback = global {
      global = nil
      callback()
    }
    print(local)
  }
}
test()

We could prevent this by saying it's not legal to refer to 'fn' inside the callback for withoutActuallyEscaping, and you have to use 'wrappedFn' instead.