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

Make useless_ptr_null_checks smarter about some std functions #114494

Merged
merged 4 commits into from
Sep 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,10 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
rustc_pass_by_value, Normal, template!(Word), ErrorFollowing,
"#[rustc_pass_by_value] is used to mark types that must be passed by value instead of reference."
),
rustc_attr!(
rustc_never_returns_null_ptr, Normal, template!(Word), ErrorFollowing,
"#[rustc_never_returns_null_ptr] is used to mark functions returning non-null pointers."
),
rustc_attr!(
rustc_coherence_is_core, AttributeType::CrateLevel, template!(Word), ErrorFollowing, @only_local: true,
"#![rustc_coherence_is_core] allows inherent methods on builtin types, only intended to be used in `core`."
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,8 @@ lint_ptr_null_checks_fn_ptr = function pointers are not nullable, so checking th
.help = wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value
.label = expression has type `{$orig_ty}`

lint_ptr_null_checks_fn_ret = returned pointer of `{$fn_name}` call is never null, so checking it for null will always return false

lint_ptr_null_checks_ref = references are not nullable, so checking them for null will always return false
.label = expression has type `{$orig_ty}`

Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,8 @@ pub enum PtrNullChecksDiag<'a> {
#[label]
label: Span,
},
#[diag(lint_ptr_null_checks_fn_ret)]
FnRet { fn_name: Ident },
}

// for_loops_over_fallibles.rs
Expand Down
52 changes: 29 additions & 23 deletions compiler/rustc_lint/src/ptr_nulls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,30 @@ declare_lint! {

declare_lint_pass!(PtrNullChecks => [USELESS_PTR_NULL_CHECKS]);

/// This function detects and returns the original expression from a series of consecutive casts,
/// ie. `(my_fn as *const _ as *mut _).cast_mut()` would return the expression for `my_fn`.
fn ptr_cast_chain<'a>(cx: &'a LateContext<'_>, mut e: &'a Expr<'a>) -> Option<&'a Expr<'a>> {
/// This function checks if the expression is from a series of consecutive casts,
/// ie. `(my_fn as *const _ as *mut _).cast_mut()` and whether the original expression is either
/// a fn ptr, a reference, or a function call whose definition is
/// annotated with `#![rustc_never_returns_null_ptr]`.
/// If this situation is present, the function returns the appropriate diagnostic.
fn incorrect_check<'a, 'tcx: 'a>(
cx: &'a LateContext<'tcx>,
mut e: &'a Expr<'a>,
) -> Option<PtrNullChecksDiag<'tcx>> {
let mut had_at_least_one_cast = false;
loop {
e = e.peel_blocks();
if let ExprKind::MethodCall(_, _expr, [], _) = e.kind
&& let Some(def_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
&& cx.tcx.has_attr(def_id, sym::rustc_never_returns_null_ptr)
&& let Some(fn_name) = cx.tcx.opt_item_ident(def_id) {
return Some(PtrNullChecksDiag::FnRet { fn_name });
} else if let ExprKind::Call(path, _args) = e.kind
&& let ExprKind::Path(ref qpath) = path.kind
&& let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
&& cx.tcx.has_attr(def_id, sym::rustc_never_returns_null_ptr)
&& let Some(fn_name) = cx.tcx.opt_item_ident(def_id) {
return Some(PtrNullChecksDiag::FnRet { fn_name });
}
e = if let ExprKind::Cast(expr, t) = e.kind
&& let TyKind::Ptr(_) = t.kind {
had_at_least_one_cast = true;
Expand All @@ -46,33 +64,21 @@ fn ptr_cast_chain<'a>(cx: &'a LateContext<'_>, mut e: &'a Expr<'a>) -> Option<&'
&& matches!(cx.tcx.get_diagnostic_name(def_id), Some(sym::ptr_cast | sym::ptr_cast_mut)) {
had_at_least_one_cast = true;
expr
} else if let ExprKind::Call(path, [arg]) = e.kind
&& let ExprKind::Path(ref qpath) = path.kind
&& let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
&& matches!(cx.tcx.get_diagnostic_name(def_id), Some(sym::ptr_from_ref | sym::ptr_from_mut)) {
had_at_least_one_cast = true;
arg
} else if had_at_least_one_cast {
return Some(e);
let orig_ty = cx.typeck_results().expr_ty(e);
return if orig_ty.is_fn() {
Some(PtrNullChecksDiag::FnPtr { orig_ty, label: e.span })
} else if orig_ty.is_ref() {
Some(PtrNullChecksDiag::Ref { orig_ty, label: e.span })
} else {
None
};
} else {
return None;
};
}
}

fn incorrect_check<'a>(cx: &LateContext<'a>, expr: &Expr<'_>) -> Option<PtrNullChecksDiag<'a>> {
let expr = ptr_cast_chain(cx, expr)?;

let orig_ty = cx.typeck_results().expr_ty(expr);
if orig_ty.is_fn() {
Some(PtrNullChecksDiag::FnPtr { orig_ty, label: expr.span })
} else if orig_ty.is_ref() {
Some(PtrNullChecksDiag::Ref { orig_ty, label: expr.span })
} else {
None
}
}

impl<'tcx> LateLintPass<'tcx> for PtrNullChecks {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
match expr.kind {
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_passes/src/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ impl CheckAttrVisitor<'_> {
self.check_rustc_std_internal_symbol(&attr, span, target)
}
sym::naked => self.check_naked(hir_id, attr, span, target),
sym::rustc_never_returns_null_ptr => {
self.check_applied_to_fn_or_method(hir_id, attr, span, target)
}
sym::rustc_legacy_const_generics => {
self.check_rustc_legacy_const_generics(hir_id, &attr, span, target, item)
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1159,7 +1159,6 @@ symbols! {
ptr_cast,
ptr_cast_mut,
ptr_const_is_null,
ptr_from_mut,
ptr_from_ref,
ptr_guaranteed_cmp,
ptr_is_null,
Expand Down Expand Up @@ -1311,6 +1310,7 @@ symbols! {
rustc_main,
rustc_mir,
rustc_must_implement_one_of,
rustc_never_returns_null_ptr,
rustc_nonnull_optimization_guaranteed,
rustc_nounwind,
rustc_object_lifetime_default,
Expand Down
2 changes: 2 additions & 0 deletions library/alloc/src/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1304,6 +1304,7 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
/// assert_eq!(unsafe { &*x_ptr }, "hello");
/// ```
#[stable(feature = "rc_raw", since = "1.17.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub fn into_raw(this: Self) -> *const T {
let ptr = Self::as_ptr(&this);
mem::forget(this);
Expand All @@ -1327,6 +1328,7 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
/// assert_eq!(unsafe { &*x_ptr }, "hello");
/// ```
#[stable(feature = "weak_into_raw", since = "1.45.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub fn as_ptr(this: &Self) -> *const T {
let ptr: *mut RcBox<T> = NonNull::as_ptr(this.ptr);

Expand Down
2 changes: 2 additions & 0 deletions library/alloc/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1455,6 +1455,7 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
/// ```
#[must_use = "losing the pointer will leak memory"]
#[stable(feature = "rc_raw", since = "1.17.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub fn into_raw(this: Self) -> *const T {
let ptr = Self::as_ptr(&this);
mem::forget(this);
Expand All @@ -1479,6 +1480,7 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
/// ```
#[must_use]
#[stable(feature = "rc_as_ptr", since = "1.45.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub fn as_ptr(this: &Self) -> *const T {
let ptr: *mut ArcInner<T> = NonNull::as_ptr(this.ptr);

Expand Down
2 changes: 2 additions & 0 deletions library/alloc/src/vec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,7 @@ impl<T, A: Allocator> Vec<T, A> {
///
/// [`as_mut_ptr`]: Vec::as_mut_ptr
#[stable(feature = "vec_as_ptr", since = "1.37.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
#[inline]
pub fn as_ptr(&self) -> *const T {
// We shadow the slice method of the same name to avoid going through
Expand Down Expand Up @@ -1266,6 +1267,7 @@ impl<T, A: Allocator> Vec<T, A> {
/// assert_eq!(&*x, &[0, 1, 2, 3]);
/// ```
#[stable(feature = "vec_as_ptr", since = "1.37.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
#[inline]
pub fn as_mut_ptr(&mut self) -> *mut T {
// We shadow the slice method of the same name to avoid going through
Expand Down
4 changes: 4 additions & 0 deletions library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ impl<T: ?Sized> Cell<T> {
#[inline]
#[stable(feature = "cell_as_ptr", since = "1.12.0")]
#[rustc_const_stable(feature = "const_cell_as_ptr", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn as_ptr(&self) -> *mut T {
self.value.get()
}
Expand Down Expand Up @@ -1076,6 +1077,7 @@ impl<T: ?Sized> RefCell<T> {
/// ```
#[inline]
#[stable(feature = "cell_as_ptr", since = "1.12.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub fn as_ptr(&self) -> *mut T {
self.value.get()
}
Expand Down Expand Up @@ -2071,6 +2073,7 @@ impl<T: ?Sized> UnsafeCell<T> {
#[inline(always)]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "const_unsafecell_get", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn get(&self) -> *mut T {
// We can just cast the pointer from `UnsafeCell<T>` to `T` because of
// #[repr(transparent)]. This exploits std's special status, there is
Expand Down Expand Up @@ -2213,6 +2216,7 @@ impl<T: ?Sized> SyncUnsafeCell<T> {
/// when casting to `&mut T`, and ensure that there are no mutations
/// or mutable aliases going on when casting to `&T`
#[inline]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn get(&self) -> *mut T {
self.value.get()
}
Expand Down
1 change: 1 addition & 0 deletions library/core/src/ffi/c_str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,7 @@ impl CStr {
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "const_str_as_ptr", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn as_ptr(&self) -> *const c_char {
self.inner.as_ptr()
}
Expand Down
3 changes: 2 additions & 1 deletion library/core/src/ptr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ where
#[inline(always)]
#[must_use]
#[unstable(feature = "ptr_from_ref", issue = "106116")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
#[rustc_diagnostic_item = "ptr_from_ref"]
pub const fn from_ref<T: ?Sized>(r: &T) -> *const T {
r
Expand All @@ -710,7 +711,7 @@ pub const fn from_ref<T: ?Sized>(r: &T) -> *const T {
#[inline(always)]
#[must_use]
#[unstable(feature = "ptr_from_ref", issue = "106116")]
#[rustc_diagnostic_item = "ptr_from_mut"]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn from_mut<T: ?Sized>(r: &mut T) -> *mut T {
r
}
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/ptr/non_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ impl<T: ?Sized> NonNull<T> {
/// ```
#[stable(feature = "nonnull", since = "1.25.0")]
#[rustc_const_stable(feature = "const_nonnull_as_ptr", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
#[must_use]
#[inline(always)]
pub const fn as_ptr(self) -> *mut T {
Expand Down Expand Up @@ -579,6 +580,7 @@ impl<T> NonNull<[T]> {
#[must_use]
#[unstable(feature = "slice_ptr_get", issue = "74265")]
#[rustc_const_unstable(feature = "slice_ptr_get", issue = "74265")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn as_mut_ptr(self) -> *mut T {
self.as_non_null_ptr().as_ptr()
}
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,7 @@ impl<T> [T] {
/// [`as_mut_ptr`]: slice::as_mut_ptr
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "const_slice_as_ptr", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
#[inline(always)]
#[must_use]
pub const fn as_ptr(&self) -> *const T {
Expand Down Expand Up @@ -760,6 +761,7 @@ impl<T> [T] {
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
#[rustc_allow_const_fn_unstable(const_mut_refs)]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
#[inline(always)]
#[must_use]
pub const fn as_mut_ptr(&mut self) -> *mut T {
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/str/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ impl str {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_stable(feature = "rustc_str_as_ptr", since = "1.32.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
#[must_use]
#[inline(always)]
pub const fn as_ptr(&self) -> *const u8 {
Expand All @@ -402,6 +403,7 @@ impl str {
/// It is your responsibility to make sure that the string slice only gets
/// modified in a way that it remains valid UTF-8.
#[stable(feature = "str_as_mut_ptr", since = "1.36.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
#[must_use]
#[inline(always)]
pub fn as_mut_ptr(&mut self) -> *mut u8 {
Expand Down
3 changes: 3 additions & 0 deletions library/core/src/sync/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,7 @@ impl AtomicBool {
#[inline]
#[stable(feature = "atomic_as_ptr", since = "1.70.0")]
#[rustc_const_stable(feature = "atomic_as_ptr", since = "1.70.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn as_ptr(&self) -> *mut bool {
self.v.get().cast()
}
Expand Down Expand Up @@ -1953,6 +1954,7 @@ impl<T> AtomicPtr<T> {
#[inline]
#[stable(feature = "atomic_as_ptr", since = "1.70.0")]
#[rustc_const_stable(feature = "atomic_as_ptr", since = "1.70.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn as_ptr(&self) -> *mut *mut T {
self.p.get()
}
Expand Down Expand Up @@ -2891,6 +2893,7 @@ macro_rules! atomic_int {
#[inline]
#[stable(feature = "atomic_as_ptr", since = "1.70.0")]
#[rustc_const_stable(feature = "atomic_as_ptr", since = "1.70.0")]
#[cfg_attr(not(bootstrap), rustc_never_returns_null_ptr)]
pub const fn as_ptr(&self) -> *mut $int_type {
self.v.get()
}
Expand Down
14 changes: 10 additions & 4 deletions tests/ui/lint/ptr_null_checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,15 @@ fn main() {
if (&mut 8 as *mut i32).is_null() {}
//~^ WARN references are not nullable
if ptr::from_mut(&mut 8).is_null() {}
//~^ WARN references are not nullable
//~^ WARN call is never null
if (&8 as *const i32).is_null() {}
//~^ WARN references are not nullable
if ptr::from_ref(&8).is_null() {}
//~^ WARN references are not nullable
//~^ WARN call is never null
if ptr::from_ref(&8).cast_mut().is_null() {}
//~^ WARN references are not nullable
//~^ WARN call is never null
if (ptr::from_ref(&8).cast_mut() as *mut i32).is_null() {}
//~^ WARN references are not nullable
//~^ WARN call is never null
if (&8 as *const i32) == std::ptr::null() {}
//~^ WARN references are not nullable
let ref_num = &8;
Expand All @@ -65,6 +65,12 @@ fn main() {
if (&*{ static_i32() } as *const i32).is_null() {}
//~^ WARN references are not nullable

// ---------------- Functions -------------------
if ptr::NonNull::new(&mut 8).unwrap().as_ptr().is_null() {}
//~^ WARN call is never null
if ptr::NonNull::<u8>::dangling().as_ptr().is_null() {}
//~^ WARN call is never null

// ----------------------------------------------
const ZPTR: *const () = 0 as *const _;
const NOT_ZPTR: *const () = 1 as *const _;
Expand Down
Loading