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

Uplift clippy::fn_null_check lint #111717

Merged
merged 3 commits into from
Jul 11, 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
3 changes: 3 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,9 @@ 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
Expand Down
112 changes: 112 additions & 0 deletions compiler/rustc_lint/src/fn_null_check.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
use crate::{lints::FnNullCheckDiag, LateContext, LateLintPass, LintContext};
use rustc_ast::LitKind;
use rustc_hir::{BinOpKind, Expr, ExprKind, TyKind};
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.
///
/// ### Example
///
/// ```rust
/// # fn test() {}
/// let fn_ptr: fn() = /* somehow obtained nullable function pointer */
/// # test;
///
/// if (fn_ptr as *const ()).is_null() { /* ... */ }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Function pointers are assumed to be non-null, checking them for null will always
/// return false.
INCORRECT_FN_NULL_CHECKS,
Warn,
"incorrect checking of null function pointer"
}

declare_lint_pass!(IncorrectFnNullChecks => [INCORRECT_FN_NULL_CHECKS]);

fn is_fn_ptr_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
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()
}

impl<'tcx> LateLintPass<'tcx> for IncorrectFnNullChecks {
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>)
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) =>
{
cx.emit_spanned_lint(INCORRECT_FN_NULL_CHECKS, expr.span, FnNullCheckDiag)
}

// Catching:
// (fn_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) =>
{
cx.emit_spanned_lint(INCORRECT_FN_NULL_CHECKS, expr.span, FnNullCheckDiag)
}

ExprKind::Binary(op, left, right) if matches!(op.node, BinOpKind::Eq) => {
let to_check: &Expr<'_>;
if is_fn_ptr_cast(cx, left) {
to_check = right;
} else if is_fn_ptr_cast(cx, right) {
to_check = left;
} else {
return;
}

match to_check.kind {
// Catching:
// (fn_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)
},

// Catching:
// (fn_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)
},

_ => {},
}
}
_ => {}
}
}
}
3 changes: 3 additions & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ 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;
Expand Down Expand Up @@ -102,6 +103,7 @@ 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::*;
Expand Down Expand Up @@ -225,6 +227,7 @@ late_lint_methods!(
// Depends on types used in type definitions
MissingCopyImplementations: MissingCopyImplementations,
// Depends on referenced function signatures in expressions
IncorrectFnNullChecks: IncorrectFnNullChecks,
MutableTransmutes: MutableTransmutes,
TypeAliasBounds: TypeAliasBounds,
TrivialConstraints: TrivialConstraints,
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,12 @@ pub struct ExpectationNote {
pub rationale: Symbol,
}

// fn_null_check.rs
#[derive(LintDiagnostic)]
#[diag(lint_fn_null_check)]
#[help]
pub struct FnNullCheckDiag;

// for_loops_over_fallibles.rs
#[derive(LintDiagnostic)]
#[diag(lint_for_loops_over_fallibles)]
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1151,8 +1151,10 @@ symbols! {
profiler_runtime,
ptr,
ptr_cast_mut,
ptr_const_is_null,
ptr_from_ref,
ptr_guaranteed_cmp,
ptr_is_null,
ptr_mask,
ptr_null,
ptr_null_mut,
Expand Down
1 change: 1 addition & 0 deletions library/core/src/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ impl<T: ?Sized> *const T {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_ptr_is_null", issue = "74939")]
#[rustc_diagnostic_item = "ptr_const_is_null"]
#[inline]
pub const fn is_null(self) -> bool {
#[inline]
Expand Down
1 change: 1 addition & 0 deletions library/core/src/ptr/mut_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ impl<T: ?Sized> *mut T {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_ptr_is_null", issue = "74939")]
#[rustc_diagnostic_item = "ptr_is_null"]
#[inline]
pub const fn is_null(self) -> bool {
#[inline]
Expand Down
1 change: 0 additions & 1 deletion src/tools/clippy/clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::float_literal::LOSSY_FLOAT_LITERAL_INFO,
crate::floating_point_arithmetic::IMPRECISE_FLOPS_INFO,
crate::floating_point_arithmetic::SUBOPTIMAL_FLOPS_INFO,
crate::fn_null_check::FN_NULL_CHECK_INFO,
crate::format::USELESS_FORMAT_INFO,
crate::format_args::FORMAT_IN_FORMAT_ARGS_INFO,
crate::format_args::TO_STRING_IN_FORMAT_ARGS_INFO,
Expand Down
102 changes: 0 additions & 102 deletions src/tools/clippy/clippy_lints/src/fn_null_check.rs

This file was deleted.

2 changes: 0 additions & 2 deletions src/tools/clippy/clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ mod extra_unused_type_parameters;
mod fallible_impl_from;
mod float_literal;
mod floating_point_arithmetic;
mod fn_null_check;
Urgau marked this conversation as resolved.
Show resolved Hide resolved
mod format;
mod format_args;
mod format_impl;
Expand Down Expand Up @@ -1003,7 +1002,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
semicolon_outside_block_ignore_multiline,
))
});
store.register_late_pass(|_| Box::new(fn_null_check::FnNullCheck));
store.register_late_pass(|_| Box::new(permissions_set_readonly_false::PermissionsSetReadonlyFalse));
store.register_late_pass(|_| Box::new(size_of_ref::SizeOfRef));
store.register_late_pass(|_| Box::new(multiple_unsafe_ops_per_block::MultipleUnsafeOpsPerBlock));
Expand Down
1 change: 1 addition & 0 deletions src/tools/clippy/clippy_lints/src/renamed_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
("clippy::for_loops_over_fallibles", "for_loops_over_fallibles"),
("clippy::forget_copy", "forgetting_copy_types"),
("clippy::forget_ref", "forgetting_references"),
("clippy::fn_null_check", "incorrect_fn_null_checks"),
("clippy::into_iter_on_array", "array_into_iter"),
("clippy::invalid_atomic_ordering", "invalid_atomic_ordering"),
("clippy::invalid_ref", "invalid_value"),
Expand Down
22 changes: 0 additions & 22 deletions src/tools/clippy/tests/ui/fn_null_check.rs

This file was deleted.

43 changes: 0 additions & 43 deletions src/tools/clippy/tests/ui/fn_null_check.stderr

This file was deleted.

Loading