Skip to content

Commit

Permalink
Auto merge of rust-lang#118833 - Urgau:lint_function_pointer_comparis…
Browse files Browse the repository at this point in the history
…ons, r=<try>

Add lint against function pointer comparisons

This is kind of a follow-up to rust-lang#117758 where we added a lint against wide pointer comparisons for being ambiguous and unreliable; well function pointer comparisons are also unreliable. We should IMO follow a similar logic and warn people about it.

-----

## `unpredictable_function_pointer_comparisons`

*warn-by-default*

The `unpredictable_function_pointer_comparisons` lint checks comparison of function pointer as the operands.

### Example

```rust
fn foo() {}
let a = foo as fn();

let _ = a == foo;
```

### Explanation

Function pointers comparisons do not produce meaningful result since they are never guaranteed to be unique and could vary between different code generation units. Furthermore different function could have the same address after being merged together.

----

This PR also uplift the very similar `clippy::fn_address_comparisons` lint, which only linted on if one of the operand was an `ty::FnDef` while this PR lints proposes to lint on all `ty::FnPtr` and `ty::FnDef`.

`@rustbot` labels +I-lang-nominated
  • Loading branch information
bors committed Dec 13, 2023
2 parents c3def26 + 6c5c546 commit 2959657
Show file tree
Hide file tree
Showing 26 changed files with 260 additions and 163 deletions.
4 changes: 4 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,10 @@ lint_unknown_lint =
lint_unknown_tool_in_scoped_lint = unknown tool name `{$tool_name}` found in scoped lint: `{$tool_name}::{$lint_name}`
.help = add `#![register_tool({$tool_name})]` to the crate root
lint_unpredictable_fn_pointer_comparisons = function pointer comparisons do not produce meaningful results since their addresses are not guaranteed to be unique
.note_duplicated_fn = the address of the same function can very between different codegen units
.note_deduplicated_fn = furthermore, different functions could have the same address after being merged together
lint_unsupported_group = `{$lint_group}` lint group is not supported with ´--force-warn´
lint_untranslatable_diag = diagnostics should be created using translatable messages
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 @@ -1644,6 +1644,12 @@ pub enum AmbiguousWidePointerComparisonsAddrSuggestion<'a> {
},
}

#[derive(LintDiagnostic)]
#[diag(lint_unpredictable_fn_pointer_comparisons)]
#[note(lint_note_duplicated_fn)]
#[note(lint_note_deduplicated_fn)]
pub struct UnpredictableFunctionPointerComparisons;

pub struct ImproperCTypes<'a> {
pub ty: Ty<'a>,
pub desc: &'a str,
Expand Down
59 changes: 57 additions & 2 deletions compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use crate::{
InvalidNanComparisonsSuggestion, OnlyCastu8ToChar, OverflowingBinHex,
OverflowingBinHexSign, OverflowingBinHexSignBitSub, OverflowingBinHexSub, OverflowingInt,
OverflowingIntHelp, OverflowingLiteral, OverflowingUInt, RangeEndpointOutOfRange,
UnusedComparisons, UseInclusiveRange, VariantSizeDifferencesDiag,
UnpredictableFunctionPointerComparisons, UnusedComparisons, UseInclusiveRange,
VariantSizeDifferencesDiag,
},
};
use crate::{LateContext, LateLintPass, LintContext};
Expand Down Expand Up @@ -169,6 +170,32 @@ declare_lint! {
"detects ambiguous wide pointer comparisons"
}

declare_lint! {
/// The `unpredictable_function_pointer_comparisons` lint checks comparison
/// of function pointer as the operands.
///
/// ### Example
///
/// ```rust
/// fn foo() {}
/// # let a = foo as fn();
///
/// let _ = a == foo;
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Function pointers comparisons do not produce meaningful result since
/// they are never guaranteed to be unique and could vary between different
/// code generation units. Furthermore, different functions could have the
/// same address after being merged together.
UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS,
Deny,
"detects unpredictable function pointer comparisons"
}

#[derive(Copy, Clone)]
pub struct TypeLimits {
/// Id of the last visited negated expression
Expand All @@ -181,7 +208,8 @@ impl_lint_pass!(TypeLimits => [
UNUSED_COMPARISONS,
OVERFLOWING_LITERALS,
INVALID_NAN_COMPARISONS,
AMBIGUOUS_WIDE_POINTER_COMPARISONS
AMBIGUOUS_WIDE_POINTER_COMPARISONS,
UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS
]);

impl TypeLimits {
Expand Down Expand Up @@ -758,6 +786,30 @@ fn lint_wide_pointer<'tcx>(
);
}

fn lint_fn_pointer<'tcx>(
cx: &LateContext<'tcx>,
e: &'tcx hir::Expr<'tcx>,
_binop: hir::BinOpKind,
l: &'tcx hir::Expr<'tcx>,
r: &'tcx hir::Expr<'tcx>,
) {
let Some(l_ty) = cx.typeck_results().expr_ty_opt(l) else { return };
let Some(r_ty) = cx.typeck_results().expr_ty_opt(r) else { return };

let l_ty = l_ty.peel_refs();
let r_ty = r_ty.peel_refs();

if !l_ty.is_fn() || !r_ty.is_fn() {
return;
}

cx.emit_spanned_lint(
UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS,
e.span,
UnpredictableFunctionPointerComparisons,
);
}

impl<'tcx> LateLintPass<'tcx> for TypeLimits {
fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx hir::Expr<'tcx>) {
match e.kind {
Expand All @@ -775,6 +827,7 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
} else {
lint_nan(cx, e, binop, l, r);
lint_wide_pointer(cx, e, binop.node, l, r);
lint_fn_pointer(cx, e, binop.node, l, r);
}
}
}
Expand All @@ -786,13 +839,15 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
&& let Some(binop) = partialeq_binop(diag_item) =>
{
lint_wide_pointer(cx, e, binop, l, r);
lint_fn_pointer(cx, e, binop, l, r);
}
hir::ExprKind::MethodCall(_, l, [r], _)
if let Some(def_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
&& let Some(diag_item) = cx.tcx.get_diagnostic_name(def_id)
&& let Some(binop) = partialeq_binop(diag_item) =>
{
lint_wide_pointer(cx, e, binop, l, r);
lint_fn_pointer(cx, e, binop, l, r);
}
_ => {}
};
Expand Down
1 change: 1 addition & 0 deletions library/core/tests/ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ fn test_const_nonnull_new() {
#[test]
#[cfg(unix)] // printf may not be available on other platforms
#[allow(deprecated)] // For SipHasher
#[cfg_attr(not(bootstrap), allow(unpredictable_function_pointer_comparisons))]
pub fn test_variadic_fnptr() {
use core::ffi;
use core::hash::{Hash, SipHasher};
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 @@ -683,7 +683,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::unit_types::LET_UNIT_VALUE_INFO,
crate::unit_types::UNIT_ARG_INFO,
crate::unit_types::UNIT_CMP_INFO,
crate::unnamed_address::FN_ADDRESS_COMPARISONS_INFO,
crate::unnecessary_box_returns::UNNECESSARY_BOX_RETURNS_INFO,
crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO,
crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO,
Expand Down
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 @@ -328,7 +328,6 @@ mod unicode;
mod uninit_vec;
mod unit_return_expecting_ord;
mod unit_types;
mod unnamed_address;
mod unnecessary_box_returns;
mod unnecessary_map_on_constructor;
mod unnecessary_owned_empty_strings;
Expand Down Expand Up @@ -862,7 +861,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_early_pass(|| Box::new(option_env_unwrap::OptionEnvUnwrap));
store.register_late_pass(move |_| Box::new(wildcard_imports::WildcardImports::new(warn_on_all_wildcard_imports)));
store.register_late_pass(|_| Box::<redundant_pub_crate::RedundantPubCrate>::default());
store.register_late_pass(|_| Box::new(unnamed_address::UnnamedAddress));
store.register_late_pass(|_| Box::<dereference::Dereferencing<'_>>::default());
store.register_late_pass(|_| Box::new(option_if_let_else::OptionIfLetElse));
store.register_late_pass(|_| Box::new(future_not_send::FutureNotSend));
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 @@ -59,4 +59,5 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
("clippy::unknown_clippy_lints", "unknown_lints"),
("clippy::unused_label", "unused_labels"),
("clippy::vtable_address_comparisons", "ambiguous_wide_pointer_comparisons"),
("clippy::fn_address_comparisons", "unpredictable_function_pointer_comparisons"),
];
60 changes: 0 additions & 60 deletions src/tools/clippy/clippy_lints/src/unnamed_address.rs

This file was deleted.

23 changes: 0 additions & 23 deletions src/tools/clippy/tests/ui/fn_address_comparisons.rs

This file was deleted.

17 changes: 0 additions & 17 deletions src/tools/clippy/tests/ui/fn_address_comparisons.stderr

This file was deleted.

2 changes: 2 additions & 0 deletions src/tools/clippy/tests/ui/rename.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#![allow(unknown_lints)]
#![allow(unused_labels)]
#![allow(ambiguous_wide_pointer_comparisons)]
#![allow(unpredictable_function_pointer_comparisons)]
#![warn(clippy::almost_complete_range)]
#![warn(clippy::disallowed_names)]
#![warn(clippy::blocks_in_if_conditions)]
Expand Down Expand Up @@ -109,5 +110,6 @@
#![warn(unknown_lints)]
#![warn(unused_labels)]
#![warn(ambiguous_wide_pointer_comparisons)]
#![warn(unpredictable_function_pointer_comparisons)]

fn main() {}
2 changes: 2 additions & 0 deletions src/tools/clippy/tests/ui/rename.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#![allow(unknown_lints)]
#![allow(unused_labels)]
#![allow(ambiguous_wide_pointer_comparisons)]
#![allow(unpredictable_function_pointer_comparisons)]
#![warn(clippy::almost_complete_letter_range)]
#![warn(clippy::blacklisted_name)]
#![warn(clippy::block_in_if_condition_expr)]
Expand Down Expand Up @@ -109,5 +110,6 @@
#![warn(clippy::unknown_clippy_lints)]
#![warn(clippy::unused_label)]
#![warn(clippy::vtable_address_comparisons)]
#![warn(clippy::fn_address_comparisons)]

fn main() {}
Loading

0 comments on commit 2959657

Please sign in to comment.