Skip to content
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

Clarify story on *T (unsafe pointers) #7362

Closed
metajack opened this issue Jun 24, 2013 · 12 comments
Closed

Clarify story on *T (unsafe pointers) #7362

metajack opened this issue Jun 24, 2013 · 12 comments
Labels
A-FFI Area: Foreign function interface (FFI)
Milestone

Comments

@metajack
Copy link
Contributor

Currently we automatically coerce from &Foo to *Foo, which makes interop much nicer. However, for C APIs that have some calls taking *mut Foo and some *Foo (getters and setters on an opaque structure for example), you must currently do a lot of &*foo to turn *mut Foo into *Foo. It would be much more convenient to coerce to immutable automatically.

I feel like the current situation punishes correct type annotation of FFI calls.

@nikomatsakis mentioned in IRC that *mut Foo might just go away, but if it's here to stay, then more coersion would be nice.

@metajack
Copy link
Contributor Author

nominating production ready

@brson
Copy link
Contributor

brson commented Aug 28, 2013

I'm becoming more inclined to just remove *mut

@catamorphism
Copy link
Contributor

Repurposing this issue to, more generally, talk about clarifying the unsafe pointer story. Milestone 1, well-defined

@huonw
Copy link
Member

huonw commented Feb 26, 2014

I'm very strongly against removing *mut: using only * would make FFI much harder to get right, because there would then be no distinction between C functions taking const T* and those taking T*. This means that one (a) has to remember to check the C definition* (there's no information in the Rust code, other than possibly a comment) to see whether an argument is mutated, and (b) can start mutating variables that are theoretically immutable.

Example of this I fixed recently, where a non-mut value was passed into a C function via &res, but the C function modified it. 4cc723d#diff-f0ba70706ebd16d4c6868bbfc6f68dcaR45

If we ever start informing LLVM of mutability/immutability (assuming this is possible), then doing the above would be undefined behaviour.

One might argue "FFI is unsafe anyway", but *mut is a tiny feature that makes it much harder to screw up. (Especially with #2124, which will make many FFI definitions "automatically" correct about const T* <-> *T and T* <-> *mut T, which isn't currently the case with our libc bindings.)

@huonw
Copy link
Member

huonw commented Feb 26, 2014

(To be clear: I'm strongly against removing *mut without replacing it with some equivalent way to encode in Rust (an approximation to) C's T* vs. const T* distinction.)

@nikomatsakis
Copy link
Contributor

Example of this I fixed recently, where a non-mut value was passed into a C function via &res, but the C function modified it. 4cc723d#diff-f0ba70706ebd16d4c6868bbfc6f68dcaR45

@huonw It seems to me that this rather argues for the change than against it. As I wrote recently in an e-mail thread, my concern with *mut is that it does not exist in C and it's meaning is unintuitive. I think in practice most people write * and then transmute as needed, or else pass a * into C functions that mutate its referent (effectively the same thing -- and precisely what happened here, it sounds like). I'd rather have a *T type with precisely the same meaning as C (well, modulo placement of the
*).

I could imagine adding *const T (with precisely the same meaning as C), but only if it helps with optimization somewhere. (Note that I would not favor adding const anywhere but unsafe pointers; it would invalidate all the progress [1] we've been making on &mut and aliasing.)

I think that the only sensible interpretation of the current pointer types is that *T means what const T* means in C and and *mut T means what T* means in C. So I could also imagine having no *T,
but just *const T and *mut T.

@thestinger
Copy link
Contributor

The const pointer annotation doesn't help with optimization, but it certainly helps with writing correct code. You'll get compiler errors in many cases when you're getting mutability wrong. It doesn't offer a full safety solution but it's far more useful than most compiler warnings.

@nikomatsakis
Copy link
Contributor

Thinking more on the issue, I'm pretty sure that our current types are wrong, but I'm not yet sure of the best fix. It would be nice if the bug @huonw mentions could not occur so easily. The fundamental problem seems to be that *T means one thing in Rust (read-only) but something else in C (mutable), so people who are transcribing fn signatures tend to use the wrong sigil. Then in Rust code they write &x (which also means one thing in Rust and another in C), rely on the coercion from &T to *T, and get a wacky result. If *T meant mutable, and we only coerced from &mut T to *T, this would be solved -- but if *T were the only type, it would be annoying to require coercion from &mut T only, since many pointers are read-only. This seems to argue that we do require distinct read-only and mutable pointer types, if we hope to avoid this bug, though I continue to think that calling them *T and *mut T is not a good idea. It's inconsistent with other Rust pointer types (where *T is immutable, not read-only, which we can't hope to enforce here) and inconsistent with C (where *T is mutable).

@erickt you may have an opinion here, I know that safety of * has been a longtime concern of yours.

@huonw
Copy link
Member

huonw commented Feb 26, 2014

Note that I would not favor adding const anywhere but unsafe pointers; it would invalidate all the progress [1] we've been making on &mut and aliasing

(You're missing the link.)

@huonw
Copy link
Member

huonw commented Feb 26, 2014

people who are transcribing fn signatures tend to use the wrong sigil

Much of this problem would be addressed with #2124, e.g., resulting in #[header="stdio.h"] extern mod stdio; instead of this (incorrect) extern block (under our current scheme pretty much all those *FILEs should be *mut FILE, according to their C definitions (although it doesn't matter that much in this case, because a let f: FILE directly isn't possible, i.e. the pointers are black boxes to Rust); the ptr args to f{s,g}etpos should be mut too).

That said, I do agree that *T matching the C syntax but not the semantics can be confusing.

This seems to argue that we do require distinct read-only and mutable pointer types

I think this is the crux of what I'm saying.

FWIW, I'm personally happy with pretty much any syntax as long as we have the distinction in some form or another, to assist people writing FFI/unsafe code as much as is reasonable (in particular, I have no attachment to the current *mut/* beyond the semantics of having two).

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jun 16, 2014
This does not yet change the compiler and libraries from `*T` to `*const T` as
it will require a snapshot to do so.

cc rust-lang#7362
bors added a commit that referenced this issue Jun 24, 2014
This does not yet change the compiler and libraries from `*T` to `*const T` as
it will require a snapshot to do so.

cc #7362

---

Note that the corresponding RFC, rust-lang/rfcs#68, has not yet been accepted. It was [discussed at the last meeting](https://github.com/rust-lang/rust/wiki/Meeting-weekly-2014-06-10#rfc-pr-68-unsafe-pointers-rename-t-to-const-t) and decided to be accepted, however. I figured I'd get started on the preliminary work for the RFC that will be required regardless.
@pcwalton
Copy link
Contributor

Now that #15208 is closed, nominating for closing.

@pnkfelix
Copy link
Member

pnkfelix commented Jul 3, 2014

Closing, yay!

@pnkfelix pnkfelix closed this as completed Jul 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI)
Projects
None yet
Development

No branches or pull requests

8 participants