-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Convention for constructing lifetime-bound values from raw pointers #556
Merged
Merged
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
eb65e39
Added RFC: Lifetime parameters for unsafe pointer conversions
mzabaluev f5872d5
Small rewordings
mzabaluev 8586136
A tab crept in
mzabaluev 6fd5aff
Small readability change
mzabaluev ab84b27
Cleared the placeholder text in the header
mzabaluev 839e6a3
Added a note about using `""` as the lifetime anchor
mzabaluev c9e5704
Described the alternative of not providing input lifetimes
mzabaluev b3c34db
Corrected the analysis on the anchor-less alternative
mzabaluev 4d67b84
Drop the anchors!
mzabaluev dcf608f
Credit where due
mzabaluev 184daee
Tiny editorial
mzabaluev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
- Start Date: 2015-01-06 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Provide more flexible, less error-prone lifetime support | ||
for unsafe pointer conversions throughout the libraries. | ||
|
||
The currently tedious: | ||
```rust | ||
let s = std::slice::from_raw_buf(&ptr, len) | ||
std::mem::copy_lifetime(self, s) | ||
``` | ||
Becomes: | ||
```rust | ||
std::slice::from_raw_buf_with_lifetime(ptr, len, self) | ||
``` | ||
Or, after a breaking change of the current API: | ||
```rust | ||
std::slice::from_raw_buf(ptr, len, self) | ||
``` | ||
|
||
Static lifetime can be easy too: | ||
```rust | ||
std::slice::from_raw_buf(ptr, len, std::mem::STATIC) | ||
``` | ||
|
||
# Motivation | ||
|
||
The current library convention on functions constructing lifetime-bound | ||
values from raw pointers has the pointer passed by reference, which | ||
reference's lifetime is carried over to the return value. | ||
Unfortunately, the lifetime of a raw pointer is often not indicative | ||
of the lifetime of the pointed-to data. This may lead to mistakes | ||
since the acquired reference crosses into the "safe" domain without | ||
much indication in the code, while the pointed-to value may become | ||
invalid at any time. | ||
|
||
A typical use case where the lifetime needs to be adjusted is | ||
in bindings to a foregn library, when returning a reference to an object's | ||
inner value (we know from the library's API contract that | ||
the inner data's lifetime is bound to the containing object): | ||
```rust | ||
impl Outer { | ||
fn inner_str(&self) -> &[u8] { | ||
unsafe { | ||
let p = ffi::outer_get_inner_str(&self.raw); | ||
let s = std::slice::from_raw_buf(p, libc::strlen(p)); | ||
std::mem::copy_lifetime(self, s) | ||
} | ||
} | ||
} | ||
``` | ||
|
||
And here's a plausible gotcha: | ||
```rust | ||
let foo = unsafe { ffi::new_foo() }; | ||
let s = unsafe { std::slice::from_raw_buf(&foo.data, foo.len) }; | ||
// s lives as long as foo | ||
|
||
// some lines later | ||
|
||
unsafe { ffi::free_foo(foo) }; | ||
|
||
// more lines later, in perfectly safe-looking code: | ||
|
||
let guess_what = s[0]; | ||
``` | ||
|
||
# Detailed design | ||
|
||
The signature of `from_raw*` constructors is changed: the raw pointer is | ||
passed by value, and a generic reference argument is appended to work as a | ||
lifetime anchor (the value can be anything and is ignored): | ||
|
||
```rust | ||
fn from_raw_buf_with_lifetime<'a, T, Sized? U>(ptr: *const T, len: uint, | ||
life_anchor: &'a U) | ||
-> &'a [T] | ||
``` | ||
```rust | ||
fn from_raw_mut_buf_with_lifetime<'a, T, Sized? U>(ptr: *mut T, len: uint, | ||
life_anchor: &'a U) | ||
-> &'a mut [T] | ||
``` | ||
|
||
The existing constructors can be deprecated, to open a migration | ||
path towards reusing their shorter names with the new signatures | ||
when the time to break the API is right. | ||
|
||
The current usage can be mechanically changed: | ||
```rust | ||
let s = std::slice::from_raw_buf(&ptr, len) | ||
``` | ||
to: | ||
```rust | ||
std::slice::from_raw_buf_with_lifetime(ptr, len, &ptr) | ||
``` | ||
However, it's better to try to find a more appropriate lifetime anchor | ||
for each use. | ||
|
||
## Fix copy_mut_lifetime | ||
|
||
While we are at it, the first parameter of `std::mem::copy_mut_lifetime` | ||
could be made a non-mutable reference. There is no reason for the lifetime | ||
anchor to be mutable: the pointer's mutability is usually the relevant | ||
question, and it's an unsafe function to begin with. This wart makes | ||
code tedious, mut-happy, or transmute-happy in cases when a container | ||
providing the lifetime for a mutable view into its contents is not itself | ||
necessarily mutable. | ||
|
||
## Anchor for static references | ||
|
||
To facilitate conversion of pointers to static references, an anchor constant | ||
is provided in `std::mem`: | ||
|
||
```rust | ||
pub const STATIC: &'static () = &(); | ||
``` | ||
|
||
# Drawbacks | ||
|
||
The proposal adds new functions to the library for something that is | ||
already doable, albeit with some inconvenience. Replacement of the existing | ||
constructors, if approved, is a breaking change. | ||
|
||
# Alternatives | ||
|
||
Not adding the lifetime-anchored conversion functions, continuing to use the | ||
current convention despite its problems and inconvenience. | ||
|
||
# Unresolved questions | ||
|
||
Should the existing by-reference constructors be deprecated and eventually | ||
removed? If yes, should this change be done before 1.0? |
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.
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.
nit: the indentation on this line is a tiny bit off
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.
Fixed, thanks.