Improve diagnostics and add tests for function calls

rust-lang · Aug 6, 2023 · 4b1bc27 · 4b1bc27
1 parent 8faac74
commit 4b1bc27
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 44 deletions.
diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl
@@ -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}`
 

diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs
@@ -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

diff --git a/compiler/rustc_lint/src/ptr_nulls.rs b/compiler/rustc_lint/src/ptr_nulls.rs
@@ -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;
@@ -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()
-            && cx.tcx.has_attr(def_id, sym::rustc_never_returns_null_ptr) {
-            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 {

diff --git a/tests/ui/lint/ptr_null_checks.rs b/tests/ui/lint/ptr_null_checks.rs
@@ -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;
@@ -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 _;

diff --git a/tests/ui/lint/ptr_null_checks.stderr b/tests/ui/lint/ptr_null_checks.stderr
@@ -117,13 +117,11 @@ LL |     if (&mut 8 as *mut i32).is_null() {}
    |         |
    |         expression has type `&mut i32`
 
-warning: references are not nullable, so checking them for null will always return false
+warning: returned pointer of `from_mut` call is never null, so checking it for null will always return false
   --> $DIR/ptr_null_checks.rs:40:8
    |
 LL |     if ptr::from_mut(&mut 8).is_null() {}
-   |        ^^^^^^^^^^^^^^------^^^^^^^^^^^
-   |                      |
-   |                      expression has type `&mut i32`
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 warning: references are not nullable, so checking them for null will always return false
   --> $DIR/ptr_null_checks.rs:42:8
@@ -133,29 +131,23 @@ LL |     if (&8 as *const i32).is_null() {}
    |         |
    |         expression has type `&i32`
 
-warning: references are not nullable, so checking them for null will always return false
+warning: returned pointer of `from_ref` call is never null, so checking it for null will always return false
   --> $DIR/ptr_null_checks.rs:44:8
    |
 LL |     if ptr::from_ref(&8).is_null() {}
-   |        ^^^^^^^^^^^^^^--^^^^^^^^^^^
-   |                      |
-   |                      expression has type `&i32`
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-warning: references are not nullable, so checking them for null will always return false
+warning: returned pointer of `from_ref` call is never null, so checking it for null will always return false
   --> $DIR/ptr_null_checks.rs:46:8
    |
 LL |     if ptr::from_ref(&8).cast_mut().is_null() {}
-   |        ^^^^^^^^^^^^^^--^^^^^^^^^^^^^^^^^^^^^^
-   |                      |
-   |                      expression has type `&i32`
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-warning: references are not nullable, so checking them for null will always return false
+warning: returned pointer of `from_ref` call is never null, so checking it for null will always return false
   --> $DIR/ptr_null_checks.rs:48:8
    |
 LL |     if (ptr::from_ref(&8).cast_mut() as *mut i32).is_null() {}
-   |        ^^^^^^^^^^^^^^^--^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |                       |
-   |                       expression has type `&i32`
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 warning: references are not nullable, so checking them for null will always return false
   --> $DIR/ptr_null_checks.rs:50:8
@@ -221,5 +213,17 @@ LL |     if (&*{ static_i32() } as *const i32).is_null() {}
    |         |
    |         expression has type `&i32`
 
-warning: 25 warnings emitted
+warning: returned pointer of `as_ptr` call is never null, so checking it for null will always return false
+  --> $DIR/ptr_null_checks.rs:69:8
+   |
+LL |     if ptr::NonNull::new(&mut 8).unwrap().as_ptr().is_null() {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+warning: returned pointer of `as_ptr` call is never null, so checking it for null will always return false
+  --> $DIR/ptr_null_checks.rs:71:8
+   |
+LL |     if ptr::NonNull::<u8>::dangling().as_ptr().is_null() {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+warning: 27 warnings emitted