Extend incorrect_fn_null_checks to incorrect_non_null_checks

This extends the scope of the incorrect_fn_null_checks lint to
also references.

In theory, we could extend the check even further,
for example to NonNull, or Box, but these types can't be casted
via as casts to raw pointers so it would be a bit harder from
an implementation point of view.

Author: est31

compiler/rustc_lint/messages.ftl

@@ -215,9 +215,6 @@ lint_expectation = this lint expectation is unfulfilled
     .note = the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
     .rationale = {$rationale}
 
-lint_fn_null_check = function pointers are not nullable, so checking them for null will always return false
-    .help = wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value
-
 lint_for_loops_over_fallibles =
     for loop over {$article} `{$ty}`. This is more readably written as an `if let` statement
     .suggestion = consider using `if let` to clear intent
@@ -399,6 +396,9 @@ lint_non_fmt_panic_unused =
     }
     .add_fmt_suggestion = or add a "{"{"}{"}"}" format string to use the message literally
 
+lint_non_null_check = {$ty_desc}s can never be null, so checking them for null will always return false
+    .help = wrap the {$ty_desc} inside an `Option` and use `Option::is_none` to check for null pointer values
+
 lint_non_snake_case = {$sort} `{$name}` should have a snake case name
     .rename_or_convert_suggestion = rename the identifier or convert it to a snake case raw identifier
     .cannot_convert_note = `{$sc}` cannot be used as a raw identifier

compiler/rustc_lint/src/lib.rs

@@ -58,7 +58,6 @@ mod early;
 mod enum_intrinsics_non_enums;
 mod errors;
 mod expect;
-mod fn_null_check;
 mod for_loops_over_fallibles;
 pub mod hidden_unicode_codepoints;
 mod internal;
@@ -72,6 +71,7 @@ mod methods;
 mod multiple_supertrait_upcastable;
 mod non_ascii_idents;
 mod non_fmt_panic;
+mod non_null_check;
 mod nonstandard_style;
 mod noop_method_call;
 mod opaque_hidden_inferred_bound;
@@ -103,7 +103,6 @@ use cast_ref_to_mut::*;
 use deref_into_dyn_supertrait::*;
 use drop_forget_useless::*;
 use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums;
-use fn_null_check::*;
 use for_loops_over_fallibles::*;
 use hidden_unicode_codepoints::*;
 use internal::*;
@@ -114,6 +113,7 @@ use methods::*;
 use multiple_supertrait_upcastable::*;
 use non_ascii_idents::*;
 use non_fmt_panic::NonPanicFmt;
+use non_null_check::*;
 use nonstandard_style::*;
 use noop_method_call::*;
 use opaque_hidden_inferred_bound::*;
@@ -227,7 +227,7 @@ late_lint_methods!(
             // Depends on types used in type definitions
             MissingCopyImplementations: MissingCopyImplementations,
             // Depends on referenced function signatures in expressions
-            IncorrectFnNullChecks: IncorrectFnNullChecks,
+            IncorrectNonNullChecks: IncorrectNonNullChecks,
             MutableTransmutes: MutableTransmutes,
             TypeAliasBounds: TypeAliasBounds,
             TrivialConstraints: TrivialConstraints,

compiler/rustc_lint/src/lints.rs

@@ -1,5 +1,6 @@
 #![allow(rustc::untranslatable_diagnostic)]
 #![allow(rustc::diagnostic_outside_of_impl)]
+use std::borrow::Cow;
 use std::num::NonZeroU32;
 
 use crate::fluent_generated as fluent;
@@ -613,11 +614,13 @@ pub struct ExpectationNote {
     pub rationale: Symbol,
 }
 
-// fn_null_check.rs
+// non_null_check.rs
 #[derive(LintDiagnostic)]
-#[diag(lint_fn_null_check)]
+#[diag(lint_non_null_check)]
 #[help]
-pub struct FnNullCheckDiag;
+pub struct NonNullCheckDiag {
+    pub ty_desc: Cow<'static, str>,
+}
 
 // for_loops_over_fallibles.rs
 #[derive(LintDiagnostic)]

compiler/rustc_lint/src/fn_null_check.rs renamed to compiler/rustc_lint/src/non_null_check.rs

@@ -1,12 +1,13 @@
-use crate::{lints::FnNullCheckDiag, LateContext, LateLintPass, LintContext};
+use crate::{lints::NonNullCheckDiag, LateContext, LateLintPass, LintContext};
 use rustc_ast::LitKind;
 use rustc_hir::{BinOpKind, Expr, ExprKind, TyKind};
+use rustc_middle::ty::Ty;
 use rustc_session::{declare_lint, declare_lint_pass};
 use rustc_span::sym;
 
 declare_lint! {
-    /// The `incorrect_fn_null_checks` lint checks for expression that checks if a
-    /// function pointer is null.
+    /// The `incorrect_non_null_checks` lint checks for expressions that check if a
+    /// non-nullable type is null.
     ///
     /// ### Example
     ///
@@ -22,85 +23,108 @@ declare_lint! {
     ///
     /// ### Explanation
     ///
-    /// Function pointers are assumed to be non-null, checking them for null will always
-    /// return false.
-    INCORRECT_FN_NULL_CHECKS,
+    /// A non-nullable type is assumed to never be null, and therefore having an actual
+    /// non-null pointer is ub.
+    INCORRECT_NON_NULL_CHECKS,
     Warn,
-    "incorrect checking of null function pointer"
+    "incorrect checking of non null pointers"
 }
 
-declare_lint_pass!(IncorrectFnNullChecks => [INCORRECT_FN_NULL_CHECKS]);
+declare_lint_pass!(IncorrectNonNullChecks => [INCORRECT_NON_NULL_CHECKS]);
 
-fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+/// Is the cast to a nonnull type?
+/// If yes, return (ty, nullable_version) where former is the nonnull type while latter
+/// is a nullable version (e.g. (fn, Option<fn>) or (&u8, *const u8)).
+fn is_nonnull_cast<'a>(cx: &LateContext<'a>, expr: &Expr<'_>) -> Option<Ty<'a>> {
     let mut expr = expr.peel_blocks();
     let mut had_at_least_one_cast = false;
     while let ExprKind::Cast(cast_expr, cast_ty) = expr.kind
             && let TyKind::Ptr(_) = cast_ty.kind {
         expr = cast_expr.peel_blocks();
         had_at_least_one_cast = true;
     }
-    had_at_least_one_cast && cx.typeck_results().expr_ty_adjusted(expr).is_fn()
+    if !had_at_least_one_cast {
+        return None;
+    }
+    let ty = cx.typeck_results().expr_ty_adjusted(expr);
+    if ty.is_fn() || ty.is_ref() {
+        return Some(ty);
+    }
+    // Usually, references get coerced to pointers in a casting situation.
+    // Therefore, we give also give a look to the original type.
+    let ty_unadjusted = cx.typeck_results().expr_ty_opt(expr);
+    if let Some(ty_unadjusted) = ty_unadjusted && ty_unadjusted.is_ref() {
+        return Some(ty_unadjusted);
+    }
+    None
 }
 
-impl<'tcx> LateLintPass<'tcx> for IncorrectFnNullChecks {
+impl<'tcx> LateLintPass<'tcx> for IncorrectNonNullChecks {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         match expr.kind {
             // Catching:
-            // <*<const/mut> <ty>>::is_null(fn_ptr as *<const/mut> <ty>)
+            // <*<const/mut> <ty>>::is_null(test_ptr as *<const/mut> <ty>)
             ExprKind::Call(path, [arg])
                 if 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_const_is_null | sym::ptr_is_null)
                     )
-                    && is_fn_ptr_cast(cx, arg) =>
+                    && let Some(ty) = is_nonnull_cast(cx, arg) =>
             {
-                cx.emit_spanned_lint(INCORRECT_FN_NULL_CHECKS, expr.span, FnNullCheckDiag)
+                let diag = NonNullCheckDiag { ty_desc: ty.prefix_string(cx.tcx) };
+                cx.emit_spanned_lint(INCORRECT_NON_NULL_CHECKS, expr.span, diag)
             }
 
             // Catching:
-            // (fn_ptr as *<const/mut> <ty>).is_null()
+            // (test_ptr as *<const/mut> <ty>).is_null()
             ExprKind::MethodCall(_, receiver, _, _)
                 if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
                     && matches!(
                         cx.tcx.get_diagnostic_name(def_id),
                         Some(sym::ptr_const_is_null | sym::ptr_is_null)
                     )
-                    && is_fn_ptr_cast(cx, receiver) =>
+                    && let Some(ty) = is_nonnull_cast(cx, receiver) =>
             {
-                cx.emit_spanned_lint(INCORRECT_FN_NULL_CHECKS, expr.span, FnNullCheckDiag)
+                let diag = NonNullCheckDiag { ty_desc: ty.prefix_string(cx.tcx) };
+                cx.emit_spanned_lint(INCORRECT_NON_NULL_CHECKS, expr.span, diag)
             }
 
             ExprKind::Binary(op, left, right) if matches!(op.node, BinOpKind::Eq) => {
                 let to_check: &Expr<'_>;
-                if is_fn_ptr_cast(cx, left) {
+                let ty: Ty<'_>;
+                if let Some(ty_) = is_nonnull_cast(cx, left) {
                     to_check = right;
-                } else if is_fn_ptr_cast(cx, right) {
+                    ty = ty_;
+                } else if let Some(ty_) = is_nonnull_cast(cx, right) {
                     to_check = left;
+                    ty = ty_;
                 } else {
                     return;
                 }
 
                 match to_check.kind {
                     // Catching:
-                    // (fn_ptr as *<const/mut> <ty>) == (0 as <ty>)
+                    // (test_ptr as *<const/mut> <ty>) == (0 as <ty>)
                     ExprKind::Cast(cast_expr, _)
                         if let ExprKind::Lit(spanned) = cast_expr.kind
                             && let LitKind::Int(v, _) = spanned.node && v == 0 =>
                     {
-                        cx.emit_spanned_lint(INCORRECT_FN_NULL_CHECKS, expr.span, FnNullCheckDiag)
+                        let diag = NonNullCheckDiag { ty_desc: ty.prefix_string(cx.tcx) };
+                        cx.emit_spanned_lint(INCORRECT_NON_NULL_CHECKS, expr.span, diag)
                     },
 
                     // Catching:
-                    // (fn_ptr as *<const/mut> <ty>) == std::ptr::null()
+                    // (test_ptr as *<const/mut> <ty>) == std::ptr::null()
                     ExprKind::Call(path, [])
                         if let ExprKind::Path(ref qpath) = path.kind
                             && let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
                             && let Some(diag_item) = cx.tcx.get_diagnostic_name(def_id)
                             && (diag_item == sym::ptr_null || diag_item == sym::ptr_null_mut) =>
                     {
-                        cx.emit_spanned_lint(INCORRECT_FN_NULL_CHECKS, expr.span, FnNullCheckDiag)
+                        let diag = NonNullCheckDiag { ty_desc: ty.prefix_string(cx.tcx) };
+                        cx.emit_spanned_lint(INCORRECT_NON_NULL_CHECKS, expr.span, diag)
                     },
 
                     _ => {},

tests/ui/lint/fn_null_check.rs

@@ -0,0 +1,45 @@
+// check-pass
+
+fn main() {
+    let fn_ptr = main;
+
+    if (fn_ptr as *mut ()).is_null() {}
+    //~^ WARN can never be null, so checking
+    if (fn_ptr as *const u8).is_null() {}
+    //~^ WARN can never be null, so checking
+    if (fn_ptr as *const ()) == std::ptr::null() {}
+    //~^ WARN can never be null, so checking
+    if (fn_ptr as *mut ()) == std::ptr::null_mut() {}
+    //~^ WARN can never be null, so checking
+    if (fn_ptr as *const ()) == (0 as *const ()) {}
+    //~^ WARN can never be null, so checking
+    if <*const _>::is_null(fn_ptr as *const ()) {}
+    //~^ WARN can never be null, so checking
+    if (fn_ptr as *mut fn() as *const fn() as *const ()).is_null() {}
+    //~^ WARN can never be null, so checking
+    if (fn_ptr as fn() as *const ()).is_null() {}
+    //~^ WARN can never be null, so checking
+
+    const ZPTR: *const () = 0 as *const _;
+    const NOT_ZPTR: *const () = 1 as *const _;
+
+    // unlike the uplifted clippy::fn_null_check lint we do
+    // not lint on them
+    if (fn_ptr as *const ()) == ZPTR {}
+    if (fn_ptr as *const ()) == NOT_ZPTR {}
+
+    // Non fn pointers
+
+    let tup_ref: &_ = &(10u8, 10u8);
+    if (tup_ref as *const (u8, u8)).is_null() {}
+    //~^ WARN can never be null, so checking
+    if (&mut (10u8, 10u8) as *mut (u8, u8)).is_null() {}
+    //~^ WARN can never be null, so checking
+
+    // We could warn on these too, but don't:
+    if Box::into_raw(Box::new("hi")).is_null() {}
+
+    let ptr = &mut () as *mut ();
+    if core::ptr::NonNull::new(ptr).unwrap().as_ptr().is_null() {}
+
+}