From 905e092b6a603387f65f25de7b84a7223d1858e8 Mon Sep 17 00:00:00 2001 From: est31 Date: Thu, 13 Jul 2023 07:31:30 +0200 Subject: [PATCH] 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. --- compiler/rustc_lint/messages.ftl | 6 +- compiler/rustc_lint/src/lib.rs | 6 +- compiler/rustc_lint/src/lints.rs | 9 +- .../{fn_null_check.rs => non_null_check.rs} | 70 +++++++++++----- tests/ui/lint/fn_null_check.rs | 30 ------- tests/ui/lint/fn_null_check.stderr | 67 --------------- tests/ui/lint/non_null_check.rs | 45 ++++++++++ tests/ui/lint/non_null_check.stderr | 83 +++++++++++++++++++ 8 files changed, 187 insertions(+), 129 deletions(-) rename compiler/rustc_lint/src/{fn_null_check.rs => non_null_check.rs} (51%) delete mode 100644 tests/ui/lint/fn_null_check.rs delete mode 100644 tests/ui/lint/fn_null_check.stderr create mode 100644 tests/ui/lint/non_null_check.rs create mode 100644 tests/ui/lint/non_null_check.stderr diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index 2c92277b50d21..e0eb4106d9056 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/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 diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index beb38dbb94c32..59525f6f3cd5d 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/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, diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs index 1dea758bb294b..fc25805ec84c8 100644 --- a/compiler/rustc_lint/src/lints.rs +++ b/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)] diff --git a/compiler/rustc_lint/src/fn_null_check.rs b/compiler/rustc_lint/src/non_null_check.rs similarity index 51% rename from compiler/rustc_lint/src/fn_null_check.rs rename to compiler/rustc_lint/src/non_null_check.rs index e3b33463ccfbc..3b67270266d80 100644 --- a/compiler/rustc_lint/src/fn_null_check.rs +++ b/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,16 +23,19 @@ 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) or (&u8, *const u8)). +fn is_nonnull_cast<'a>(cx: &LateContext<'a>, expr: &Expr<'_>) -> Option> { let mut expr = expr.peel_blocks(); let mut had_at_least_one_cast = false; while let ExprKind::Cast(cast_expr, cast_ty) = expr.kind @@ -39,14 +43,27 @@ fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { 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: - // <* >::is_null(fn_ptr as * ) + // <* >::is_null(test_ptr as * ) 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() @@ -54,53 +71,60 @@ impl<'tcx> LateLintPass<'tcx> for IncorrectFnNullChecks { 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 * ).is_null() + // (test_ptr as * ).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 * ) == (0 as ) + // (test_ptr as * ) == (0 as ) 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 * ) == std::ptr::null() + // (test_ptr as * ) == 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) }, _ => {}, diff --git a/tests/ui/lint/fn_null_check.rs b/tests/ui/lint/fn_null_check.rs deleted file mode 100644 index 7f01f2c428395..0000000000000 --- a/tests/ui/lint/fn_null_check.rs +++ /dev/null @@ -1,30 +0,0 @@ -// check-pass - -fn main() { - let fn_ptr = main; - - if (fn_ptr as *mut ()).is_null() {} - //~^ WARN function pointers are not nullable - if (fn_ptr as *const u8).is_null() {} - //~^ WARN function pointers are not nullable - if (fn_ptr as *const ()) == std::ptr::null() {} - //~^ WARN function pointers are not nullable - if (fn_ptr as *mut ()) == std::ptr::null_mut() {} - //~^ WARN function pointers are not nullable - if (fn_ptr as *const ()) == (0 as *const ()) {} - //~^ WARN function pointers are not nullable - if <*const _>::is_null(fn_ptr as *const ()) {} - //~^ WARN function pointers are not nullable - if (fn_ptr as *mut fn() as *const fn() as *const ()).is_null() {} - //~^ WARN function pointers are not nullable - if (fn_ptr as fn() as *const ()).is_null() {} - //~^ WARN function pointers are not nullable - - 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 {} -} diff --git a/tests/ui/lint/fn_null_check.stderr b/tests/ui/lint/fn_null_check.stderr deleted file mode 100644 index 0398c0da50feb..0000000000000 --- a/tests/ui/lint/fn_null_check.stderr +++ /dev/null @@ -1,67 +0,0 @@ -warning: function pointers are not nullable, so checking them for null will always return false - --> $DIR/fn_null_check.rs:6:8 - | -LL | if (fn_ptr as *mut ()).is_null() {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value - = note: `#[warn(incorrect_fn_null_checks)]` on by default - -warning: function pointers are not nullable, so checking them for null will always return false - --> $DIR/fn_null_check.rs:8:8 - | -LL | if (fn_ptr as *const u8).is_null() {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value - -warning: function pointers are not nullable, so checking them for null will always return false - --> $DIR/fn_null_check.rs:10:8 - | -LL | if (fn_ptr as *const ()) == std::ptr::null() {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value - -warning: function pointers are not nullable, so checking them for null will always return false - --> $DIR/fn_null_check.rs:12:8 - | -LL | if (fn_ptr as *mut ()) == std::ptr::null_mut() {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value - -warning: function pointers are not nullable, so checking them for null will always return false - --> $DIR/fn_null_check.rs:14:8 - | -LL | if (fn_ptr as *const ()) == (0 as *const ()) {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value - -warning: function pointers are not nullable, so checking them for null will always return false - --> $DIR/fn_null_check.rs:16:8 - | -LL | if <*const _>::is_null(fn_ptr as *const ()) {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value - -warning: function pointers are not nullable, so checking them for null will always return false - --> $DIR/fn_null_check.rs:18:8 - | -LL | if (fn_ptr as *mut fn() as *const fn() as *const ()).is_null() {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value - -warning: function pointers are not nullable, so checking them for null will always return false - --> $DIR/fn_null_check.rs:20:8 - | -LL | if (fn_ptr as fn() as *const ()).is_null() {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: wrap the function pointer inside an `Option` and use `Option::is_none` to check for null pointer value - -warning: 8 warnings emitted - diff --git a/tests/ui/lint/non_null_check.rs b/tests/ui/lint/non_null_check.rs new file mode 100644 index 0000000000000..829f4ff9cc03d --- /dev/null +++ b/tests/ui/lint/non_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() {} + +} diff --git a/tests/ui/lint/non_null_check.stderr b/tests/ui/lint/non_null_check.stderr new file mode 100644 index 0000000000000..bab8ecd16eb2b --- /dev/null +++ b/tests/ui/lint/non_null_check.stderr @@ -0,0 +1,83 @@ +warning: fn pointers can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:6:8 + | +LL | if (fn_ptr as *mut ()).is_null() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the fn pointer inside an `Option` and use `Option::is_none` to check for null pointer values + = note: `#[warn(incorrect_non_null_checks)]` on by default + +warning: fn pointers can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:8:8 + | +LL | if (fn_ptr as *const u8).is_null() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the fn pointer inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: fn pointers can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:10:8 + | +LL | if (fn_ptr as *const ()) == std::ptr::null() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the fn pointer inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: fn pointers can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:12:8 + | +LL | if (fn_ptr as *mut ()) == std::ptr::null_mut() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the fn pointer inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: fn pointers can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:14:8 + | +LL | if (fn_ptr as *const ()) == (0 as *const ()) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the fn pointer inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: fn pointers can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:16:8 + | +LL | if <*const _>::is_null(fn_ptr as *const ()) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the fn pointer inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: fn pointers can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:18:8 + | +LL | if (fn_ptr as *mut fn() as *const fn() as *const ()).is_null() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the fn pointer inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: fn pointers can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:20:8 + | +LL | if (fn_ptr as fn() as *const ()).is_null() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the fn pointer inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: references can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:34:8 + | +LL | if (tup_ref as *const (u8, u8)).is_null() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the reference inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: mutable references can never be null, so checking them for null will always return false + --> $DIR/non_null_check.rs:36:8 + | +LL | if (&mut (10u8, 10u8) as *mut (u8, u8)).is_null() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: wrap the mutable reference inside an `Option` and use `Option::is_none` to check for null pointer values + +warning: 10 warnings emitted +