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 const <*const T>::is_null #74939

Closed
oli-obk opened this issue Jul 30, 2020 · 23 comments · Fixed by #133116
Closed

Tracking issue for const <*const T>::is_null #74939

oli-obk opened this issue Jul 30, 2020 · 23 comments · Fixed by #133116
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Jul 30, 2020

This is tracking #[feature(const_ptr_is_null)].

impl<T: ?Sized> *const T {
    pub const unsafe fn as_ref<'a>(self) -> Option<&'a T>;
    pub const fn is_null(&self) -> bool;
}

impl<T: ?Sized> *mut T {
    pub const unsafe fn as_mut<'a>(self) -> Option<&'a mut T>;
    pub const unsafe fn as_ref<'a>(self) -> Option<&'a T>;
    pub const fn is_null(&self) -> bool;
}

Comparing pointers in const eval is dangerous business.

But checking whether a pointer is the null pointer is actually completely fine, as Rust does not support items being placed at the null address. Any otherwise created null pointers are supposed to return true for is_null anyway, so that's ok. Thus, we implement is_null as ptr.guaranteed_eq(ptr::null()), which returns true if it's guaranteed that ptr is null, and there are no cases where it will return false where it may be null, but we don't know.

@oli-obk oli-obk added A-const-fn C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Jul 30, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2020

and there are no cases where it will return false where it may be null, but we don't know.

That's not entirely correct. It will at some point be possible to create out-of-bounds raw pointers in const-eval (it might be possible already with enough hacks, I am not sure). Once that is possible, a pointer could be offset enough such that it is NULL at run-time but still non-NULL when checked during CTFE -- we cannot know if out-of-bounds pointers are NULL as that depends on their concrete base address.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 10, 2020

So, some examples that we ran reliably check:

  • std::ptr::null::<T>().is_null() will always be true
  • NonZeroUsize::new(x).unwrap().is_null() will always be false
  • `(&x as *const T).is_null() will always be false
  • ((&x as *const T as usize - &x as *const T as usize) as *const T).is_null() will always be true (if you get it or an equivalent to compile)

The problematic case (&x as *const T).wrapping_offset(496).is_null() will be false, but at runtime could be true. We could set it up so it would be true instead, but then is_null() can return true when at runtime it would return false.

Another alternative is to panic if no exact solution is known (this can be done in a way that it compiles away to nothing at runtime)

Basically the options are:

  • use ptr.guaranteed_eq(std::ptr::null()), but then you get false for out of bounds pointers that at runtime are null
  • use !ptr.guaranteed_ne(std::ptr::null()), but then you get true for all out of bounds pointers, even the ones that are not null at runtime.
  • use both variants and assert that they return the same thing.

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 17, 2020
Make `<*const T>::is_null` const fn

r? @RalfJung

cc @rust-lang/wg-const-eval

tracking issue: rust-lang#74939
@josephlr
Copy link
Contributor

  • use both variants and assert that they return the same thing.

As a user, this seems the most natural to me.

  • Integer pointer that's zero => return true
  • Integer pointer that's non-zero => return false
  • Real pointer that's in-bounds => return false
  • Real pointer that's out-of-bounds => panic, could be either true/false at runtime.

In general, panicking when doing something fundamentally non-const a cosnt fn seems fine.

@kupiakos
Copy link
Contributor

Is there anything blocking stabilization of this? This also blocks NonNull::new from stabilization.

@RalfJung
Copy link
Member

RalfJung commented Mar 22, 2023 via email

@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2024

Given in particular the use of is_null in NonNull::new/ptr::as_ref, I think the only possible interpretation it can have is "might be null": if we allow NonNull::new(ptr.wrapping_sub(0x120000)).unwrap() and later it turns out that pointer is at address 0x120000, we have a problem. Therefore, ptr.wrapping_sub(0x120000).is_null must be true.

Or of course we could panic and thus abort const-eval. We'll never panic at runtime so this may be acceptable.

@rust-lang/wg-const-eval any opinions on this?

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 9, 2024
const: make ptr.is_null() stop execution on ambiguity

This seems better than saying `false` -- saying `false` is in fact actively unsound if `NonNull` then uses this to permit putting this pointer inside of it, but at runtime it turns out to be null.

Part of rust-lang#74939
Cc `@rust-lang/wg-const-eval`
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 9, 2024
const: make ptr.is_null() stop execution on ambiguity

This seems better than saying `false` -- saying `false` is in fact actively unsound if `NonNull` then uses this to permit putting this pointer inside of it, but at runtime it turns out to be null.

Part of rust-lang#74939
Cc ``@rust-lang/wg-const-eval``
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 9, 2024
const: make ptr.is_null() stop execution on ambiguity

This seems better than saying `false` -- saying `false` is in fact actively unsound if `NonNull` then uses this to permit putting this pointer inside of it, but at runtime it turns out to be null.

Part of rust-lang#74939
Cc ```@rust-lang/wg-const-eval```
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 9, 2024
Rollup merge of rust-lang#130107 - RalfJung:const-ptr-is-null, r=oli-obk

const: make ptr.is_null() stop execution on ambiguity

This seems better than saying `false` -- saying `false` is in fact actively unsound if `NonNull` then uses this to permit putting this pointer inside of it, but at runtime it turns out to be null.

Part of rust-lang#74939
Cc ```@rust-lang/wg-const-eval```
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 10, 2024
move some const fn out of the const_ptr_as_ref feature

When a `const fn` is still `#[unstable]`, it should generally use the same feature to track its regular stability and const-stability. Then when that feature moves towards stabilization we can decide whether the const-ness can be stabilized as well, or whether it should be moved into a new feature.

Also, functions like `ptr::as_ref` (which returns an `Option<&mut T>`) require `is_null`, which is tricky and blocked on some design concerns (see rust-lang#74939). So move those to the is_null feature gate, as they should be stabilized together with `ptr.is_null()`.

Affects rust-lang#91822, rust-lang#122034, rust-lang#75402, rust-lang#74939
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 10, 2024
move some const fn out of the const_ptr_as_ref feature

When a `const fn` is still `#[unstable]`, it should generally use the same feature to track its regular stability and const-stability. Then when that feature moves towards stabilization we can decide whether the const-ness can be stabilized as well, or whether it should be moved into a new feature.

Also, functions like `ptr::as_ref` (which returns an `Option<&mut T>`) require `is_null`, which is tricky and blocked on some design concerns (see rust-lang#74939). So move those to the is_null feature gate, as they should be stabilized together with `ptr.is_null()`.

Affects rust-lang#91822, rust-lang#122034, rust-lang#75402, rust-lang#74939
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 10, 2024
Rollup merge of rust-lang#130164 - RalfJung:const_ptr_as_ref, r=dtolnay

move some const fn out of the const_ptr_as_ref feature

When a `const fn` is still `#[unstable]`, it should generally use the same feature to track its regular stability and const-stability. Then when that feature moves towards stabilization we can decide whether the const-ness can be stabilized as well, or whether it should be moved into a new feature.

Also, functions like `ptr::as_ref` (which returns an `Option<&mut T>`) require `is_null`, which is tricky and blocked on some design concerns (see rust-lang#74939). So move those to the is_null feature gate, as they should be stabilized together with `ptr.is_null()`.

Affects rust-lang#91822, rust-lang#122034, rust-lang#75402, rust-lang#74939
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 11, 2024
move some const fn out of the const_ptr_as_ref feature

When a `const fn` is still `#[unstable]`, it should generally use the same feature to track its regular stability and const-stability. Then when that feature moves towards stabilization we can decide whether the const-ness can be stabilized as well, or whether it should be moved into a new feature.

Also, functions like `ptr::as_ref` (which returns an `Option<&mut T>`) require `is_null`, which is tricky and blocked on some design concerns (see #74939). So move those to the is_null feature gate, as they should be stabilized together with `ptr.is_null()`.

Affects #91822, #122034, #75402, rust-lang/rust#74939
@workingjubilee workingjubilee added A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) I-libs-api-nominated Nominated for discussion during a libs-api team meeting. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 28, 2024
@D0liphin
Copy link

D0liphin commented Sep 29, 2024

Stabilization Report

Summary

Current tracking issue

Pointers can be definitely null:

std::ptr::null().is_null() // always `true`

Or definitely not null:

(&x as *const T).is_null() // always `false` 

Sometimes though, pointers are "maybe null", which is what makes stabilizing
this tricky.

(&x as *const T).wrapping_offset(-0x12345).is_null() // who knows?

Given that pointer comparison is a deterministic operation,
the "end-to-end behavior" must be the same for both the const fn and
fn. This rules out selecting either true or false consistently for the
maybe case; it could be different at runtime.

What we're left with then is no option but to abort const-evaluation if
the pointer may or may not be null. Note that this does not introduce
"observable behavior differences" between compile-time and run-time evaluation;
since compilation stops, there is "no compile-time behavior" so to speak.
Basically, this declares is_null to be supported in const contexts for some inputs
but not others, with a runtime check ensuring it only gets used on supported inputs.

Concretely, the supported inputs are:

  • all "integer" pointers (created via ptr::without_provenance or int-to-ptr cast)
  • all pointers that are inbounds or "at the end" of a current or former allocated object (i.e., the allocated object does not have to be live any more)

(also) Stabilize <*const T>.as_ref() as a const fn

This is currently blocked on const_ptr_is_null, so follows immediately
from stabilizing it. Both this and the mut variant can work now,
since const_mut_refs is stable.

(also) Stabilize NonNull::new() as a const fn

Current tracking issue

This follows immediately from stabilizing the above, but would need a separate stabilization process, I suppose?

Tests

  • const_nonnull_new is tested in a const context here.
  • const_ptr_is_null is tested in a const context hereven that

@workingjubilee
Copy link
Member

I have updated the tracking issue with all const fn listed under this feature flag.

@RalfJung RalfJung added I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 30, 2024
@rfcbot rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Oct 16, 2024
@traviscross
Copy link
Contributor

@rfcbot reviewed

@rustbot labels -I-lang-nominated

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 16, 2024
@RalfJung
Copy link
Member

RalfJung commented Nov 1, 2024

@Amanieu @BurntSushi @m-ou-se @nikomatsakis @pnkfelix @tmandry FCP checkbox reminder ping :)

@traviscross traviscross added the I-lang-nominated Nominated for discussion during a lang team meeting. label Nov 2, 2024
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 6, 2024
@rfcbot
Copy link

rfcbot commented Nov 6, 2024

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

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Nov 6, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 16, 2024
@rfcbot
Copy link

rfcbot commented Nov 16, 2024

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@RalfJung
Copy link
Member

Stabilization PR is up. :)
#133116

jhpratt added a commit to jhpratt/rust that referenced this issue Nov 17, 2024
stabilize const_ptr_is_null

FCP passed in rust-lang#74939.

The second commit cleans up const stability around UB checks a bit, now that everything they need (except for `const_eval_select`) is stable.

Fixes rust-lang#74939
@bors bors closed this as completed in af1c8be Nov 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 17, 2024
Rollup merge of rust-lang#133116 - RalfJung:const-null-ptr, r=dtolnay

stabilize const_ptr_is_null

FCP passed in rust-lang#74939.

The second commit cleans up const stability around UB checks a bit, now that everything they need (except for `const_eval_select`) is stable.

Fixes rust-lang#74939
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 21, 2024
theemathas added a commit to theemathas/rust that referenced this issue Dec 15, 2024
The "panic in const if CTFE doesn't know the answer" behavior was discussed to be the desired behavior in rust-lang#74939, and is currently how the function actually behaves.

I intentionally wrote this documentation to allow for the possibility that a panic might not occur even if the pointer is out of bounds, because of rust-lang#133700 and other potential changes in the future.
theemathas added a commit to theemathas/rust that referenced this issue Dec 19, 2024
The "panic in const if CTFE doesn't know the answer" behavior was discussed to be the desired behavior in rust-lang#74939, and is currently how the function actually behaves.

I intentionally wrote this documentation to allow for the possibility that a panic might not occur even if the pointer is out of bounds, because of rust-lang#133700 and other potential changes in the future.
theemathas added a commit to theemathas/rust that referenced this issue Dec 21, 2024
The "panic in const if CTFE doesn't know the answer" behavior was discussed to be the desired behavior in rust-lang#74939, and is currently how the function actually behaves.

I intentionally wrote this documentation to allow for the possibility that a panic might not occur even if the pointer is out of bounds, because of rust-lang#133700 and other potential changes in the future.
DianQK added a commit to DianQK/rust that referenced this issue Dec 21, 2024
Correctly document CTFE behavior of is_null and methods that call is_null.

The "panic in const if CTFE doesn't know the answer" behavior was discussed to be the desired behavior in rust-lang#74939, and is currently how the function actually behaves.

I intentionally wrote this documentation to allow for the possibility that a panic might not occur even if the pointer is out of bounds, because of rust-lang#133700 and other potential changes in the future.

This is beta-nominated since `const fn is_null` stabilization is in beta already but the docs there are wrong, and it seems better to have the docs be correct at the time of stabilization.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Dec 21, 2024
Correctly document CTFE behavior of is_null and methods that call is_null.

The "panic in const if CTFE doesn't know the answer" behavior was discussed to be the desired behavior in rust-lang#74939, and is currently how the function actually behaves.

I intentionally wrote this documentation to allow for the possibility that a panic might not occur even if the pointer is out of bounds, because of rust-lang#133700 and other potential changes in the future.

This is beta-nominated since `const fn is_null` stabilization is in beta already but the docs there are wrong, and it seems better to have the docs be correct at the time of stabilization.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 21, 2024
Correctly document CTFE behavior of is_null and methods that call is_null.

The "panic in const if CTFE doesn't know the answer" behavior was discussed to be the desired behavior in rust-lang#74939, and is currently how the function actually behaves.

I intentionally wrote this documentation to allow for the possibility that a panic might not occur even if the pointer is out of bounds, because of rust-lang#133700 and other potential changes in the future.

This is beta-nominated since `const fn is_null` stabilization is in beta already but the docs there are wrong, and it seems better to have the docs be correct at the time of stabilization.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 22, 2024
Rollup merge of rust-lang#134325 - theemathas:is_null-docs, r=RalfJung

Correctly document CTFE behavior of is_null and methods that call is_null.

The "panic in const if CTFE doesn't know the answer" behavior was discussed to be the desired behavior in rust-lang#74939, and is currently how the function actually behaves.

I intentionally wrote this documentation to allow for the possibility that a panic might not occur even if the pointer is out of bounds, because of rust-lang#133700 and other potential changes in the future.

This is beta-nominated since `const fn is_null` stabilization is in beta already but the docs there are wrong, and it seems better to have the docs be correct at the time of stabilization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

15 participants