Tracking issue for unsafe operations in const fn
Centril opened this issue ยท 31 comments
This is a tracking issue for the RFC "Const functions and inherent methods" (rust-lang/rfcs#911).
This issue only tracks a subset of the proposal in 911 that we are (hopefully) comfortable with stabilizing. To opt into the minimal subset, use #![feature(min_const_unsafe_fn)]
. To use the more expansive feature set, you can continue using #![feature(const_fn)]
and other associated feature gates.
Currently, while you can write unsafe {}
inside a const fn
/ unsafe const fn
, it is not possible to actually possible to call any unsafe operations inside the block. This makes it impossible to implement safe const fn
abstractions such as Vec::new
. This issue builds upon #53555 by allowing you to use unsafe
operations inside const fn
so that we can make more abstractions const fn
.
Exhaustive list of features supported in const fn
with #![feature(min_const_unsafe_fn)]
:
- Constructing types (e.g.
NonZero
) with#[rustc_layout_scalar_valid_range_start]
becomesunsafe
. This is an internal bug-fix that has no user facing consequences. A motivation is given in #55607 (comment) and in #55607 (comment). - Calling
const unsafe fn
functions insideconst fn
functions inside anunsafe { ... }
block. - Calling
const unsafe fn
functions insideconst unsafe fn
functions.
Non-exhaustive lists of things that don't become allowed with #![feature(min_const_unsafe_fn)]
:
-
Callingconst unsafe fn
functions directly inside otherconst unsafe fn
functions.
For example:const unsafe fn foo() {} const unsafe fn foo() { bar(); // <-- ERROR! You must write `unsafe { bar(); }`. }
We impose this restriction because @RalfJung has noted that this is not a good thing inunsafe fn
andfn
. Thus, for now, we want to avoid making the situation worse inconst unsafe fn
. We can lift the restriction later if we want to.EDIT: This restriction has been removed.
-
Calling
ptr::read
,mem::transmute
or other functions that can't be written asconst unsafe fn
in user code (see discussion below...). -
Defererencing raw pointers; Tracked in #51911.
-
Union field accesses; Tracked in #51909.
-
Casting raw pointers to integers
-
Taking references to fields of packed structs
-
accessing
extern static
s
Things to be done before stabilizing:
- Implement the
min_const_unsafe_fn
feature gate. (#55635) - Ensure that the above restrictions apply.
- Adjust documentation (see instructions on forge)
- Stabilization PR (see instructions on forge)
Unresolved questions:
None.
Vocabulary:
cc #24111.
Details can be found via rust-lang/const-eval#14
the TLDR is that if we allow e.g. transmute
as a const fn and be called within const fn by using unsafe
, we might be producing values that are fine at runtime but not at compile-time.
A prominent example is transmute::<&T, usize>(&whatev) / 2
. Dividing an address by two is useless but legal at runtime. At compile-time this value cannot be known (because we don't know at which address llvm and the OS will place any objects) and we thus bail out.
To elaborate on what @oli-obk said, imagine someone wrote
const fn totally_safe_fn(x: &i32) {
unsafe { transmute::<&i32, usize>(x) / 2 }
}
How do we communicate and teach that this is NOT okay? As usual there is a proof obligation that the unsafety is properly encapsulated within this safe function; it's just that adding const
changes the proof obligation. Without const
the example above would be safe to call; with const
it is not. IOW, the function is "unconst".
Me and @oli-obk briefly discussed the matter further on Discord.
Here is the conversation in full (edited for clarity):
@Centril wrote:
So having read yours and Ralf's replies and also
const-eval/const_safety.md
, I have thoughts:
- Is there an exhaustive list of the unconst-behavior-triggering operations anywhere (these will be needed for the stabilization report of allowing
unsafe { .. }
mem::transmute(_copy)
-- can we simply get away with not making thisunsafe const fn
for the time being and see how much we can constify without it?- Would there be any ways beside the currently gated ops and
transmute
to do unconst behavior on stable?- We have:
pub unsafe fn transmute_copy<T, U>(src: &T) -> U { ptr::read(src as *const T as *const U) }so we then need to ensure that
ptr::read
is also not stablyconst fn
or thatsrc as *const T as *const U
isn't (it is already, https://play.rust-lang.org/?version=nightly&mode=debug&edition=2015&gist=c7957a6eecccae81ebb6d0289f91484a)@oli-obk wrote:
Yeah, everything that can be problematic is behind extra feature gates.
I'll come up with the exhaustive list.
You can't take back casting things to raw pointers, because you can do that in constants,
soptr::read
needs to stay unstable, yes.
The list is very short: any unsafe operation, ptr to int casts, ptr operators (e.g. equality).
Without UCG it feels like it makes little sense to state which unsafe ops are fine.
What we can say is that just allowing calling unsafemin_const_fn
is fine, because their bodies are
restricted to the same rules.
With this said, I think we can / should do the following:
- Stabilize the calling of
const unsafe fn
insideconst fn
in anunsafe { ... }
block. - Stabilize the calling of
const unsafe fn
insideconst unsafe fn
in anunsafe { ... }
block.- This is an intentional restriction; we don't permit:
We impose this restriction because @RalfJung has noted that this is not a good thing in
const unsafe fn foo() {} const unsafe fn foo() { bar(); }
unsafe fn
andfn
.
We can lift the restriction later if we want to.
- This is an intentional restriction; we don't permit:
- Keep the unconst things unconst.
- In particular wrt. 3, we don't allow
ptr::read
andmem::transmute(_copy)
to be callable insideunsafe { ... }
insideconst unsafe fn
/const fn
+unsafe { ... }
. - We see how much of important standard library / ecosystem pieces such as
Vec::new
we can constify -- and evaluate what needs to be done wrt. 3. and 4. after that.
I think you swapped const fn and const unsafe fn in 1.
@rfcbot merge
I propose that we extend the stable const unsafe? fn
fragment of the language marginally, as provided for by rust-lang/rfcs#911 and in particular rust-lang/rfcs#1245, with (as noted in the issue description):
- Constructing types (e.g.
NonZero
) with#[rustc_layout_scalar_valid_range_start]
becomesunsafe
. This is an internal bug-fix that has no user facing consequences. A motivation is given in #55607 (comment) and in #55607 (comment). - Calling
const unsafe fn
functions insideconst fn
functions inside anunsafe { ... }
block. - Calling
const unsafe fn
functions insideconst unsafe fn
functions.
The points 1-2 are very conservative (in particular, 3. makes it even more conservative...) and provides a natural evolution of the piecemeal stabilization of const fn
that we started with in #53555.
Among other things we will not allow (as noted in the issue description):
-
Callingconst unsafe fn
functions directly inside otherconst unsafe fn
functions.
For example:const unsafe fn foo() {} const unsafe fn foo() { bar(); // <-- ERROR! You must write `unsafe { bar(); }`. }
We impose this restriction because @RalfJung has noted that this is not a good thing inunsafe fn
andfn
. Thus, for now, we want to avoid making the situation worse inconst unsafe fn
. We can lift the restriction later if we want to.EDIT: This restriction has been removed.
-
Calling
ptr::read
,mem::transmute
or other functions that can't be written asconst unsafe fn
in user code (see discussion above...). -
Defererencing raw pointers; Tracked in #51911.
-
Union field accesses; Tracked in #51909.
-
Casting raw pointers to integers
-
Taking references to fields of packed structs
-
accessing
extern static
s
The implementation and tests for it is pending in #55635.
While that is not yet merged, let's wait for that to happen, and so therefore:
Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:
- @Centril
- @aturon
- @cramertj
- @eddyb
- @joshtriplett
- @nikomatsakis
- @nrc
- @pnkfelix
- @scottmcm
- @withoutboats
Concerns:
implementationresolved by #55607 (comment)testsresolved by #55607 (comment)unsafe-in-unsaferesolved by #55607 (comment)
Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
So what is this allowing? If unsafe
blocks in const fn
don't get any special powers aside from calling unsafe const fn
, that wouldn't let us do anything new.
You mentioned Vec::new
, so I assume this is about the NonNull
constructor? Why is that not mentioned more explicitly. :)
The way I see it, we have a (non-critical) hole in our safety checks: The following code should not be accepted in libcore:
pub fn new_unchecked_safe(ptr: *mut T) -> Self {
NonNull { pointer: NonZero(ptr as _) }
}
This is calling a constructor of a rustc_layout_scalar_valid_range_start
type. That should be an unsafe operation. Can we do that, first? This attribute is forever unstable, so we can change its rules at a whim.
I think one consequence of doing that first will be that this proposal is useless, right? To make this useful, we would have to say that we also allow constructing rustc_layout_scalar_valid_range_start
types in const unsafe
context. Which is a fair thing to ask, but I'd prefer if we could decide that explicitly rather than doing it implicitly like I think is being proposed here.
@Centril your list does not include "taking references to fields of a packed struct", but I assume that also won't be allowed in const unsafe
blocks?
Is there anything else that unsafe
lets us do outside const
? IOW, is the intention of your list to say that unsafe
in const
does not let you do anything except for calling const unsafe fn
?
IOW, is the intention of your list to say that unsafe in const does not let you do anything except for calling const unsafe fn?
that is the intention. The disallowed list is by definition not exhaustive, because we are whitelisting allowed behavior, and thus don't really care about "forbidding" things. I added references to fields of packed structs to the list though, good to keep it as complete as we can
So what is this allowing? If
unsafe
blocks inconst fn
don't get any special powers aside from callingunsafe const fn
, that wouldn't let us do anything new.You mentioned
Vec::new
, so I assume this is about theNonNull
constructor? Why is that not mentioned more explicitly. :)
I sort of implicitly assumed since that is the only stable const unsafe fn
that you get access to it so APIs like Vec::new
and dependents can become stable const fn
s, but that is of course up to T-libs. ;)
The way I see it, we have a (non-critical) hole in our safety checks: The following code should not be accepted in libcore:
pub fn new_unchecked_safe(ptr: *mut T) -> Self { NonNull { pointer: NonZero(ptr as _) } }This is calling a constructor of a
rustc_layout_scalar_valid_range_start
type. That should be an unsafe operation. Can we do that, first? This attribute is forever unstable, so we can change its rules at a whim.I think one consequence of doing that first will be that this proposal is useless, right? To make this useful, we would have to say that we also allow constructing
rustc_layout_scalar_valid_range_start
types inconst unsafe
context. Which is a fair thing to ask, but I'd prefer if we could decide that explicitly rather than doing it implicitly like I think is being proposed here.
We discussed this further on Discord.
The conclusion was that we should make construction of rustc_layout_scalar_valid_range_start
types (e.g. NonZero
, which leads to NonNull
...) const unsafe
. This is essentially a sanity bug-fix that has no impact on behavior exposed to users because the attribute rustc_layout_scalar_valid_range_start
is unstable (as Ralf noted...). For technical reasons, we sort of need to make this change in the same PR as implements the min_const_unsafe_fn
gate, otherwise, we break NonNull::new_unchecked
which is stably const unsafe fn
.
The disallowed list is by definition not exhaustive, because we are whitelisting allowed behavior
That wasn't clear from reading the description here.
So the take-away from the Discord discussion is that this PR allows two kinds of unsafe operations in "min const fn":
- Constructing
rustc_layout_scalar_valid_range_start
types. - Calling
unsafe const fn
.
The latter on its own gives no additional power because the functions thus called would be subject to the same restriction. The former, however, is already allowed because we forgot to account for such types to be unsafe to construct...
Overall then this allows const fn
to construct "bad" inhabitants of NonNull
and similar types. The hope, from what @Centril said, is that this will not (in an obvious way at least) get us anywhere near "unconst" territory, i.e., anywhere near stuff that is "okay at run-time but not okay during CTFE". This is mostly about casting pointers to integers, and by extension transmute, and by extension loading from raw pointers. I agree that there is no obvious way to get anywhere near that with a bad NonNull
.
This might also be a good time to consider if we ever want to perform the kind of "validity check on every use" that miri does: Whenever data is memcpy
'd, since that generally happens with a type, we check that the data matches the type's invariant. With such a check, calling NonNull::new_unchecked(0)
would immediately stop CTFE, no chance for anything strange to happen.
I think we want this check, the question is just whether we are willing to pay the performance price this will incur.
Such a check would be backwards incompatible:
const F: Option<NonNull<i32>> = Some(unsafe { NonNull::new_unchecked(std::ptr::null_mut()) });
works on stable since 1.25 so it even predates miri
We could at least lint cases like that, since they're abusing UB.
@oli-obk We don't usually promise backwards compatibility for unsound code. If you write the same code outside const
context, it may work now and break in the future and we are totally in our right to do that. I think the same is true for const
context.
@RalfJung is technically right I think. However, a crater run might be in order to see what the extent of the breakage might be and what sort of roll out plan we'd like here if any?
@rfcbot concern unsafe-in-unsafe
I don't really have an opinion on whether or not the decision to allow calling unsafe functions inside of other unsafe functions was right or not, but I don't think the behavior should diverge for const fn. const
and unsafe
are orthogonal modifiers and users should be able to expect the same behavior for unsafe in both const and non-const items.
If we want to change the behavior of unsafe
, that should be done across the board (probably through an edition change).
I agree that we should aim for consistent behavior. My idea here was that since there's an outstanding RFC in rust-lang/rfcs#2585 to change the behavior of unsafe fn
gradually (and probably through an edition change when we come to that...) we don't want to make the situation, from the perspective of that RFC, worse.
If at the end of reviewing RFC 2585 we decide that we don't want to make any changes to unsafe fn
, then I think we should immediately change const unsafe fn
to have an implicit unsafe { .. }
body as well... Does that make sense?
@Centril I don't agree that the proposal in this thread would ameliorate anything with regard to RFC 2585, since it applies to only a tiny minority of expressions that would be impacted by the lint proposed there. Even in a state of transition, I think its more important that we should keep orthogonal constructs independent of one another.
EDIT: To be clear, I understood your reasoning in the initial proposal (and I understand your most recent post as reiterating the reasoning). I understand, but I don't agree.
Even in a state of transition, I think its more important that we should keep orthogonal constructs independent of one another.
I've changed the proposal accordingly (@rust-lang/lang: point 3. is now stricken and const unsafe fn foo() { my_const_unsafe_fn() }
is now allowed).
@rfbot resolve unsafe-in-unsafe
@Centril Thanks. As an anecdote, I was just reading an update about an unrelated issue in which someone asked about how two unrelated language features interact - my answer in brief would be "they don't." It's really nice to maintain this property for simplifying decision making down the road.
@rfcbot resolve unsafe-in-unsafe
๐ This is now entering its final comment period, as per the review above. ๐
(FWIW, I agree with @withoutboats and had considered raising the same objection.)
The final comment period, with a disposition to merge, as per the review above, is now complete.
Filed reference issue for documentation: rust-lang/reference#482