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

Tracking issue for unsafe operations in const fn #55607

Closed
Centril opened this Issue Nov 2, 2018 · 31 comments

Comments

Projects
None yet
7 participants
@Centril
Copy link
Contributor

Centril commented Nov 2, 2018

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)]:

  1. Constructing types (e.g. NonZero) with #[rustc_layout_scalar_valid_range_start] becomes unsafe. This is an internal bug-fix that has no user facing consequences. A motivation is given in #55607 (comment) and in #55607 (comment).
  2. Calling const unsafe fn functions inside const fn functions inside an unsafe { ... } block.
  3. Calling const unsafe fn functions inside const unsafe fn functions.

Non-exhaustive lists of things that don't become allowed with #![feature(min_const_unsafe_fn)]:

  1. Calling const unsafe fn functions directly inside other const 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 in unsafe fn and fn. Thus, for now, we want to avoid making the situation worse in const unsafe fn. We can lift the restriction later if we want to.

    EDIT: This restriction has been removed.

  2. Calling ptr::read, mem::transmute or other functions that can't be written as const unsafe fn in user code (see discussion below...).

  3. Defererencing raw pointers; Tracked in #51911.

  4. Union field accesses; Tracked in #51909.

  5. Casting raw pointers to integers

  6. Taking references to fields of packed structs

  7. accessing extern statics

Things to be done before stabilizing:

Unresolved questions:

None.

Vocabulary:


cc #24111.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Nov 2, 2018

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Nov 2, 2018

Details can be found via rust-rfcs/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.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 2, 2018

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".

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Nov 2, 2018

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:

  1. Is there an exhaustive list of the unconst-behavior-triggering operations anywhere (these will be needed for the stabilization report of allowing unsafe { .. }
  2. mem::transmute(_copy) -- can we simply get away with not making this unsafe const fn for the time being and see how much we can constify without it?
  3. Would there be any ways beside the currently gated ops and transmute to do unconst behavior on stable?
  4. 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 stably const fn or that src 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,
so ptr::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 unsafe min_const_fn is fine, because their bodies are
restricted to the same rules.

With this said, I think we can / should do the following:

  1. Stabilize the calling of const unsafe fn inside const fn in an unsafe { ... } block.
  2. Stabilize the calling of const unsafe fn inside const unsafe fn in an unsafe { ... } block.
    • This is an intentional restriction; we don't permit:
      const unsafe fn foo() {}
      const unsafe fn foo() { bar(); }
      We impose this restriction because @RalfJung has noted that this is not a good thing in unsafe fn and fn.
      We can lift the restriction later if we want to.
  3. Keep the unconst things unconst.
  4. In particular wrt. 3, we don't allow ptr::read and mem::transmute(_copy) to be callable inside unsafe { ... } inside const unsafe fn / const fn + unsafe { ... }.
  5. 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.
@oli-obk

This comment was marked as outdated.

Copy link
Contributor

oli-obk commented Nov 2, 2018

I think you swapped const fn and const unsafe fn in 1.

@Centril

This comment was marked as outdated.

Copy link
Contributor Author

Centril commented Nov 2, 2018

@oli-obk Oops, fixed. =P

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Nov 3, 2018

@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):

  1. Constructing types (e.g. NonZero) with #[rustc_layout_scalar_valid_range_start] becomes unsafe. This is an internal bug-fix that has no user facing consequences. A motivation is given in #55607 (comment) and in #55607 (comment).
  2. Calling const unsafe fn functions inside const fn functions inside an unsafe { ... } block.
  3. Calling const unsafe fn functions inside const 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):

  1. Calling const unsafe fn functions directly inside other const 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 in unsafe fn and fn. Thus, for now, we want to avoid making the situation worse in const unsafe fn. We can lift the restriction later if we want to.

    EDIT: This restriction has been removed.

  2. Calling ptr::read, mem::transmute or other functions that can't be written as const unsafe fn in user code (see discussion above...).

  3. Defererencing raw pointers; Tracked in #51911.

  4. Union field accesses; Tracked in #51909.

  5. Casting raw pointers to integers

  6. Taking references to fields of packed structs

  7. accessing extern statics


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:

@rfcbot concern implementation
@rfcbot concern tests

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 3, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

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.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 3, 2018

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.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 3, 2018

@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?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Nov 3, 2018

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

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Nov 3, 2018

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. :)

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 fns, 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 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.

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.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 3, 2018

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.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 3, 2018

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.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Nov 3, 2018

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

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 3, 2018

We could at least lint cases like that, since they're abusing UB.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 3, 2018

@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.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Nov 3, 2018

@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?

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 4, 2018

We impose this restriction because @RalfJung has noted that this is not a good thing in unsafe fn and fn. Thus, for now, we want to avoid making the situation worse in const unsafe fn. We can lift the restriction later if we want to.

I have now submitted this as an RFC for non-const.

@Centril Centril added I-nominated and removed I-nominated labels Nov 7, 2018

bors added a commit that referenced this issue Dec 4, 2018

Auto merge of #55635 - oli-obk:min_const_unsafe_fn, r=nikomatsakis
Allow calling `const unsafe fn` in `const fn` behind a feature gate

cc #55607

r? @Centril

bors added a commit that referenced this issue Dec 6, 2018

Auto merge of #55635 - oli-obk:min_const_unsafe_fn, r=nikomatsakis
Allow calling `const unsafe fn` in `const fn` behind a feature gate

cc #55607

r? @Centril
@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Dec 6, 2018

@rfcbot resolve implementation
@rfcbot resolve tests

Both done in #55635.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 10, 2018

@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).

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Dec 10, 2018

@withoutboats

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?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 10, 2018

@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.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Dec 10, 2018

@withoutboats

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).

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 10, 2018

@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.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 10, 2018

@rfcbot resolve unsafe-in-unsafe

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 11, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 11, 2018

(FWIW, I agree with @withoutboats and had considered raising the same objection.)

Centril added a commit to Centril/rust that referenced this issue Dec 16, 2018

Rollup merge of rust-lang#56706 - oli-obk:const_unsafe_fn, r=Centril
Make `const unsafe fn` bodies `unsafe`

r? @Centril

Updated for tracking issue discussion rust-lang#55607 (comment)

Centril added a commit to Centril/rust that referenced this issue Dec 16, 2018

Rollup merge of rust-lang#56706 - oli-obk:const_unsafe_fn, r=Centril
Make `const unsafe fn` bodies `unsafe`

r? @Centril

Updated for tracking issue discussion rust-lang#55607 (comment)
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 21, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Dec 22, 2018

Filed stabilization PR: #57067

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Dec 23, 2018

Filed reference issue for documentation: rust-lang-nursery/reference#482

Centril added a commit to Centril/rust that referenced this issue Dec 23, 2018

Rollup merge of rust-lang#57067 - Centril:stabilize-min_const_unsafe_…
…fn, r=oli-obk

Stabilize min_const_unsafe_fn in 1.33

Fixes rust-lang#55607

r? @oli-obk

Centril added a commit to Centril/rust that referenced this issue Dec 23, 2018

Rollup merge of rust-lang#57067 - Centril:stabilize-min_const_unsafe_…
…fn, r=oli-obk

Stabilize min_const_unsafe_fn in 1.33

Fixes rust-lang#55607

r? @oli-obk

Centril added a commit to Centril/rust that referenced this issue Dec 23, 2018

Rollup merge of rust-lang#57067 - Centril:stabilize-min_const_unsafe_…
…fn, r=oli-obk

Stabilize min_const_unsafe_fn in 1.33

Fixes rust-lang#55607

r? @oli-obk

@bors bors closed this in #57067 Dec 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.