-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Add unsafeLifetime APIs and fix RawSpan initializer lifetime dependencies
#77912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@swift-ci test |
Contributor
Author
|
@swift-ci test |
Member
|
Be sure to underscore internal interface names. |
glessard
requested changes
Dec 3, 2024
Contributor
glessard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsafeLifetime should be _unsafeLifetime, but otherwise this is good. Thanks.
lorentey
reviewed
Dec 3, 2024
c963782 to
28fab3e
Compare
Contributor
Author
|
@swift-ci test |
Contributor
Author
|
@swift-ci test |
glessard
approved these changes
Dec 5, 2024
Unsafely discard any lifetime dependence on the `dependent` argument. Return a value identical to `dependent` with a new lifetime dependence on the `borrows` argument. This is required to enable lifetime enforcement in the standard library build.
Two are fixes needed in most of the `RawSpan` and `Span` initializers. For example:
```
let baseAddress = buffer.baseAddress
let span = RawSpan(_unchecked: baseAddress, byteCount: buffer.count)
// As a trivial value, 'baseAddress' does not formally depend on the
// lifetime of 'buffer'. Make the dependence explicit.
self = _overrideLifetime(span, borrowing: buffer)
```
Fix #1. baseAddress needs to be a variable
`span` has a lifetime dependence on `baseAddress` via its
initializer. Therefore, the lifetime of `baseAddress` needs to include the call
to `_overrideLifetime`. The override sets the lifetime dependency of its result,
not its argument. It's argument still needs to be non-escaping when it is passed
in.
Alternatives:
- Make the RawSpan initializer `@_unsafeNonescapableResult`.
Any occurrence of `@_unsafeNonescapableResult` actually signals a bug. We never
want to expose this annotation.
In addition to being gross, it would totally disable enforcement of the
initialized span. But we really don't want to side-step `_overrideLifetime`
where it makes sense. We want the library author to explicitly indicate that
they understand exactly which dependence is unsafe. And we do want to
eventually expose the `_overrideLifetime` API, which needs to be well
understood, supported, and tested.
- Add lifetime annotations to a bunch of `UnsafePointer`-family APIs so the
compiler can see that the resulting pointer is derived from self, where self is
an incoming `Unsafe[Buffer]Pointer`. This would create a massive lifetime
annotation burden on the `UnsafePointer`-family APIs, which don't really have
anything to do with lifetime dependence. It makes more sense for the author of
`Span`-like APIs to reason about pointer lifetimes.
Fix #2. `_overrideLifetime` changes the lifetime dependency of span to be on an
incoming argument rather than a local variable.
This makes it legal to escape the function (by assigning it to self). Remember
that self is implicitly returned, so the `@lifetime(borrow buffer)` tells the
compiler that `self` is valid within `buffer`'s borrow scope.
Also, fix the _overrideLifetime doc comments.
8e0dc96 to
0b7a9dd
Compare
Contributor
Author
|
@swift-ci test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit #1: Add unsafeLifetime APIsUnsafely discard any lifetime dependence on the
dependentargument. Returna value identical to
dependentwith a new lifetime dependence on theborrowsargument.This is required to enable lifetime enforcement in the standard library build.
Commit #2: Fix RawSpan initializer lifetime dependencies.Two fixes are needed in most of the
RawSpanandSpaninitializers. For example:Fix #1. baseAddress needs to be a variablespanhas a lifetime dependence onbaseAddressvia its initializer. Therefore, the lifetime ofbaseAddressneeds to include the call to_overrideLifetime. The override sets the lifetime dependency of its result, not its argument. It's argument still needs to be non-escaping when it is passed in.Alternatives:
Make the RawSpan initializer
@_unsafeNonescapableResult.Any occurrence of
@_unsafeNonescapableResultactually signals a bug. We never want to expose this annotation.In addition to being gross, it would totally disable enforcement of the initialized span. But we really don't want to side-step
_overrideLifetimewhere it makes sense. We want the library author to explicitly indicate that they understand exactly which dependence is unsafe. And we do want to eventually expose the_overrideLifetimeAPI, which needs to be well understood, supported, and tested.Add lifetime annotations to a bunch of
UnsafePointer-family APIs so the compiler can see that the resulting pointeris derived from self, where self is an incoming
Unsafe[Buffer]Pointer. This would create a massive lifetimeannotation burden on the
UnsafePointer-family APIs, which don't really have anything to do with lifetimedependence. It makes more sense for the author of
Span-like APIs to reason about pointer lifetimes.Fix #2:_overrideLifetimechanges the lifetime dependency of span to be on an incoming argument rather than a local variable. This makes it legal to escape the function (by assigning it to self). Remember that self is implicitly returned, so the@lifetime(borrow buffer)tells the compiler thatselfis valid withinbuffer's borrow scope.