Skip to content

Commit 8a37655

Browse files
borsbors
committed
Auto merge of #117758 - Urgau:lint_pointer_trait_comparisons, r=davidtwco
Add lint against ambiguous wide pointer comparisons This PR is the resolution of #106447 decided in #117717 by T-lang. ## `ambiguous_wide_pointer_comparisons` *warn-by-default* The `ambiguous_wide_pointer_comparisons` lint checks comparison of `*const/*mut ?Sized` as the operands. ### Example ```rust let ab = (A, B); let a = &ab.0 as *const dyn T; let b = &ab.1 as *const dyn T; let _ = a == b; ``` ### Explanation The comparison includes metadata which may not be expected. ------- This PR also drops `clippy::vtable_address_comparisons` which is superseded by this one. ~~One thing: is the current naming right? `invalid` seems a bit too much.~~ Fixes #117717
2 parents ff2c563 + 5e1bfb5 commit 8a37655

25 files changed

+933
-254
lines changed

‎compiler/rustc_codegen_cranelift/example/mini_core.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
thread_local
1212
)]
1313
#![no_core]
14-
#![allow(dead_code, internal_features)]
14+
#![allow(dead_code, internal_features, ambiguous_wide_pointer_comparisons)]
1515

1616
#[lang = "sized"]
1717
pub trait Sized {}

‎compiler/rustc_lint/messages.ftl

+4
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
lint_ambiguous_wide_pointer_comparisons = ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
2+
.addr_metadata_suggestion = use explicit `std::ptr::eq` method to compare metadata and addresses
3+
.addr_suggestion = use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
4+
15
lint_array_into_iter =
26
this method call resolves to `<&{$target} as IntoIterator>::into_iter` (due to backwards compatibility), but will resolve to <{$target} as IntoIterator>::into_iter in Rust 2021
37
.use_iter_suggestion = use `.iter()` instead of `.into_iter()` to avoid ambiguity

‎compiler/rustc_lint/src/lints.rs

+70
Original file line numberDiff line numberDiff line change
@@ -1574,6 +1574,76 @@ pub enum InvalidNanComparisonsSuggestion {
15741574
Spanless,
15751575
}
15761576

1577+
#[derive(LintDiagnostic)]
1578+
pub enum AmbiguousWidePointerComparisons<'a> {
1579+
#[diag(lint_ambiguous_wide_pointer_comparisons)]
1580+
Spanful {
1581+
#[subdiagnostic]
1582+
addr_suggestion: AmbiguousWidePointerComparisonsAddrSuggestion<'a>,
1583+
#[subdiagnostic]
1584+
addr_metadata_suggestion: Option<AmbiguousWidePointerComparisonsAddrMetadataSuggestion<'a>>,
1585+
},
1586+
#[diag(lint_ambiguous_wide_pointer_comparisons)]
1587+
#[help(lint_addr_metadata_suggestion)]
1588+
#[help(lint_addr_suggestion)]
1589+
Spanless,
1590+
}
1591+
1592+
#[derive(Subdiagnostic)]
1593+
#[multipart_suggestion(
1594+
lint_addr_metadata_suggestion,
1595+
style = "verbose",
1596+
applicability = "machine-applicable"
1597+
)]
1598+
pub struct AmbiguousWidePointerComparisonsAddrMetadataSuggestion<'a> {
1599+
pub ne: &'a str,
1600+
pub deref_left: &'a str,
1601+
pub deref_right: &'a str,
1602+
#[suggestion_part(code = "{ne}std::ptr::eq({deref_left}")]
1603+
pub left: Span,
1604+
#[suggestion_part(code = ", {deref_right}")]
1605+
pub middle: Span,
1606+
#[suggestion_part(code = ")")]
1607+
pub right: Span,
1608+
}
1609+
1610+
#[derive(Subdiagnostic)]
1611+
pub enum AmbiguousWidePointerComparisonsAddrSuggestion<'a> {
1612+
#[multipart_suggestion(
1613+
lint_addr_suggestion,
1614+
style = "verbose",
1615+
applicability = "machine-applicable"
1616+
)]
1617+
AddrEq {
1618+
ne: &'a str,
1619+
deref_left: &'a str,
1620+
deref_right: &'a str,
1621+
#[suggestion_part(code = "{ne}std::ptr::addr_eq({deref_left}")]
1622+
left: Span,
1623+
#[suggestion_part(code = ", {deref_right}")]
1624+
middle: Span,
1625+
#[suggestion_part(code = ")")]
1626+
right: Span,
1627+
},
1628+
#[multipart_suggestion(
1629+
lint_addr_suggestion,
1630+
style = "verbose",
1631+
applicability = "machine-applicable"
1632+
)]
1633+
Cast {
1634+
deref_left: &'a str,
1635+
deref_right: &'a str,
1636+
#[suggestion_part(code = "{deref_left}")]
1637+
left_before: Option<Span>,
1638+
#[suggestion_part(code = " as *const ()")]
1639+
left: Span,
1640+
#[suggestion_part(code = "{deref_right}")]
1641+
right_before: Option<Span>,
1642+
#[suggestion_part(code = " as *const ()")]
1643+
right: Span,
1644+
},
1645+
}
1646+
15771647
pub struct ImproperCTypes<'a> {
15781648
pub ty: Ty<'a>,
15791649
pub desc: &'a str,

‎compiler/rustc_lint/src/types.rs

+172-8
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
use crate::{
22
fluent_generated as fluent,
33
lints::{
4-
AtomicOrderingFence, AtomicOrderingLoad, AtomicOrderingStore, ImproperCTypes,
5-
InvalidAtomicOrderingDiag, InvalidNanComparisons, InvalidNanComparisonsSuggestion,
6-
OnlyCastu8ToChar, OverflowingBinHex, OverflowingBinHexSign, OverflowingBinHexSignBitSub,
7-
OverflowingBinHexSub, OverflowingInt, OverflowingIntHelp, OverflowingLiteral,
8-
OverflowingUInt, RangeEndpointOutOfRange, UnusedComparisons, UseInclusiveRange,
9-
VariantSizeDifferencesDiag,
4+
AmbiguousWidePointerComparisons, AmbiguousWidePointerComparisonsAddrMetadataSuggestion,
5+
AmbiguousWidePointerComparisonsAddrSuggestion, AtomicOrderingFence, AtomicOrderingLoad,
6+
AtomicOrderingStore, ImproperCTypes, InvalidAtomicOrderingDiag, InvalidNanComparisons,
7+
InvalidNanComparisonsSuggestion, OnlyCastu8ToChar, OverflowingBinHex,
8+
OverflowingBinHexSign, OverflowingBinHexSignBitSub, OverflowingBinHexSub, OverflowingInt,
9+
OverflowingIntHelp, OverflowingLiteral, OverflowingUInt, RangeEndpointOutOfRange,
10+
UnusedComparisons, UseInclusiveRange, VariantSizeDifferencesDiag,
1011
},
1112
};
1213
use crate::{LateContext, LateLintPass, LintContext};
@@ -17,17 +18,18 @@ use rustc_errors::DiagnosticMessage;
1718
use rustc_hir as hir;
1819
use rustc_hir::{is_range_literal, Expr, ExprKind, Node};
1920
use rustc_middle::ty::layout::{IntegerExt, LayoutOf, SizeSkeleton};
20-
use rustc_middle::ty::GenericArgsRef;
2121
use rustc_middle::ty::{
2222
self, AdtKind, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt,
2323
};
24+
use rustc_middle::ty::{GenericArgsRef, TypeAndMut};
2425
use rustc_span::def_id::LocalDefId;
2526
use rustc_span::source_map;
2627
use rustc_span::symbol::sym;
2728
use rustc_span::{Span, Symbol};
2829
use rustc_target::abi::{Abi, Size, WrappingRange};
2930
use rustc_target::abi::{Integer, TagEncoding, Variants};
3031
use rustc_target::spec::abi::Abi as SpecAbi;
32+
use rustc_type_ir::DynKind;
3133

3234
use std::iter;
3335
use std::ops::ControlFlow;
@@ -136,6 +138,37 @@ declare_lint! {
136138
"detects invalid floating point NaN comparisons"
137139
}
138140

141+
declare_lint! {
142+
/// The `ambiguous_wide_pointer_comparisons` lint checks comparison
143+
/// of `*const/*mut ?Sized` as the operands.
144+
///
145+
/// ### Example
146+
///
147+
/// ```rust
148+
/// # struct A;
149+
/// # struct B;
150+
///
151+
/// # trait T {}
152+
/// # impl T for A {}
153+
/// # impl T for B {}
154+
///
155+
/// let ab = (A, B);
156+
/// let a = &ab.0 as *const dyn T;
157+
/// let b = &ab.1 as *const dyn T;
158+
///
159+
/// let _ = a == b;
160+
/// ```
161+
///
162+
/// {{produces}}
163+
///
164+
/// ### Explanation
165+
///
166+
/// The comparison includes metadata which may not be expected.
167+
AMBIGUOUS_WIDE_POINTER_COMPARISONS,
168+
Warn,
169+
"detects ambiguous wide pointer comparisons"
170+
}
171+
139172
#[derive(Copy, Clone)]
140173
pub struct TypeLimits {
141174
/// Id of the last visited negated expression
@@ -144,7 +177,12 @@ pub struct TypeLimits {
144177
negated_expr_span: Option<Span>,
145178
}
146179

147-
impl_lint_pass!(TypeLimits => [UNUSED_COMPARISONS, OVERFLOWING_LITERALS, INVALID_NAN_COMPARISONS]);
180+
impl_lint_pass!(TypeLimits => [
181+
UNUSED_COMPARISONS,
182+
OVERFLOWING_LITERALS,
183+
INVALID_NAN_COMPARISONS,
184+
AMBIGUOUS_WIDE_POINTER_COMPARISONS
185+
]);
148186

149187
impl TypeLimits {
150188
pub fn new() -> TypeLimits {
@@ -620,6 +658,106 @@ fn lint_nan<'tcx>(
620658
cx.emit_spanned_lint(INVALID_NAN_COMPARISONS, e.span, lint);
621659
}
622660

661+
fn lint_wide_pointer<'tcx>(
662+
cx: &LateContext<'tcx>,
663+
e: &'tcx hir::Expr<'tcx>,
664+
binop: hir::BinOpKind,
665+
l: &'tcx hir::Expr<'tcx>,
666+
r: &'tcx hir::Expr<'tcx>,
667+
) {
668+
let ptr_unsized = |mut ty: Ty<'tcx>| -> Option<(usize, bool)> {
669+
let mut refs = 0;
670+
// here we remove any "implicit" references and count the number
671+
// of them to correctly suggest the right number of deref
672+
while let ty::Ref(_, inner_ty, _) = ty.kind() {
673+
ty = *inner_ty;
674+
refs += 1;
675+
}
676+
match ty.kind() {
677+
ty::RawPtr(TypeAndMut { mutbl: _, ty }) => (!ty.is_sized(cx.tcx, cx.param_env))
678+
.then(|| (refs, matches!(ty.kind(), ty::Dynamic(_, _, DynKind::Dyn)))),
679+
_ => None,
680+
}
681+
};
682+
683+
// PartialEq::{eq,ne} takes references, remove any explicit references
684+
let l = l.peel_borrows();
685+
let r = r.peel_borrows();
686+
687+
let Some(l_ty) = cx.typeck_results().expr_ty_opt(l) else {
688+
return;
689+
};
690+
let Some(r_ty) = cx.typeck_results().expr_ty_opt(r) else {
691+
return;
692+
};
693+
694+
let Some((l_ty_refs, l_inner_ty_is_dyn)) = ptr_unsized(l_ty) else {
695+
return;
696+
};
697+
let Some((r_ty_refs, r_inner_ty_is_dyn)) = ptr_unsized(r_ty) else {
698+
return;
699+
};
700+
701+
let (Some(l_span), Some(r_span)) =
702+
(l.span.find_ancestor_inside(e.span), r.span.find_ancestor_inside(e.span))
703+
else {
704+
return cx.emit_spanned_lint(
705+
AMBIGUOUS_WIDE_POINTER_COMPARISONS,
706+
e.span,
707+
AmbiguousWidePointerComparisons::Spanless,
708+
);
709+
};
710+
711+
let ne = if binop == hir::BinOpKind::Ne { "!" } else { "" };
712+
let is_eq_ne = matches!(binop, hir::BinOpKind::Eq | hir::BinOpKind::Ne);
713+
let is_dyn_comparison = l_inner_ty_is_dyn && r_inner_ty_is_dyn;
714+
715+
let left = e.span.shrink_to_lo().until(l_span.shrink_to_lo());
716+
let middle = l_span.shrink_to_hi().until(r_span.shrink_to_lo());
717+
let right = r_span.shrink_to_hi().until(e.span.shrink_to_hi());
718+
719+
let deref_left = &*"*".repeat(l_ty_refs);
720+
let deref_right = &*"*".repeat(r_ty_refs);
721+
722+
cx.emit_spanned_lint(
723+
AMBIGUOUS_WIDE_POINTER_COMPARISONS,
724+
e.span,
725+
AmbiguousWidePointerComparisons::Spanful {
726+
addr_metadata_suggestion: (is_eq_ne && !is_dyn_comparison).then(|| {
727+
AmbiguousWidePointerComparisonsAddrMetadataSuggestion {
728+
ne,
729+
deref_left,
730+
deref_right,
731+
left,
732+
middle,
733+
right,
734+
}
735+
}),
736+
addr_suggestion: if is_eq_ne {
737+
AmbiguousWidePointerComparisonsAddrSuggestion::AddrEq {
738+
ne,
739+
deref_left,
740+
deref_right,
741+
left,
742+
middle,
743+
right,
744+
}
745+
} else {
746+
AmbiguousWidePointerComparisonsAddrSuggestion::Cast {
747+
deref_left,
748+
deref_right,
749+
// those two Options are required for correctness as having
750+
// an empty span and an empty suggestion is not permitted
751+
left_before: (l_ty_refs != 0).then_some(left),
752+
right_before: (r_ty_refs != 0).then(|| r_span.shrink_to_lo()),
753+
left: l_span.shrink_to_hi(),
754+
right,
755+
}
756+
},
757+
},
758+
);
759+
}
760+
623761
impl<'tcx> LateLintPass<'tcx> for TypeLimits {
624762
fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx hir::Expr<'tcx>) {
625763
match e.kind {
@@ -636,10 +774,26 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
636774
cx.emit_spanned_lint(UNUSED_COMPARISONS, e.span, UnusedComparisons);
637775
} else {
638776
lint_nan(cx, e, binop, l, r);
777+
lint_wide_pointer(cx, e, binop.node, l, r);
639778
}
640779
}
641780
}
642781
hir::ExprKind::Lit(lit) => lint_literal(cx, self, e, lit),
782+
hir::ExprKind::Call(path, [l, r])
783+
if let ExprKind::Path(ref qpath) = path.kind
784+
&& let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
785+
&& let Some(diag_item) = cx.tcx.get_diagnostic_name(def_id)
786+
&& let Some(binop) = partialeq_binop(diag_item) =>
787+
{
788+
lint_wide_pointer(cx, e, binop, l, r);
789+
}
790+
hir::ExprKind::MethodCall(_, l, [r], _)
791+
if let Some(def_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
792+
&& let Some(diag_item) = cx.tcx.get_diagnostic_name(def_id)
793+
&& let Some(binop) = partialeq_binop(diag_item) =>
794+
{
795+
lint_wide_pointer(cx, e, binop, l, r);
796+
}
643797
_ => {}
644798
};
645799

@@ -722,6 +876,16 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
722876
| hir::BinOpKind::Gt
723877
)
724878
}
879+
880+
fn partialeq_binop(diag_item: Symbol) -> Option<hir::BinOpKind> {
881+
if diag_item == sym::cmp_partialeq_eq {
882+
Some(hir::BinOpKind::Eq)
883+
} else if diag_item == sym::cmp_partialeq_ne {
884+
Some(hir::BinOpKind::Ne)
885+
} else {
886+
None
887+
}
888+
}
725889
}
726890
}
727891

‎compiler/rustc_span/src/symbol.rs

+2
Original file line numberDiff line numberDiff line change
@@ -520,6 +520,8 @@ symbols! {
520520
cmp,
521521
cmp_max,
522522
cmp_min,
523+
cmp_partialeq_eq,
524+
cmp_partialeq_ne,
523525
cmpxchg16b_target_feature,
524526
cmse_nonsecure_entry,
525527
coerce_unsized,

‎library/alloc/tests/vec.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1980,7 +1980,7 @@ fn vec_macro_repeating_null_raw_fat_pointer() {
19801980

19811981
let vec = vec![null_raw_dyn; 1];
19821982
dbg!(ptr_metadata(vec[0]));
1983-
assert!(vec[0] == null_raw_dyn);
1983+
assert!(std::ptr::eq(vec[0], null_raw_dyn));
19841984

19851985
// Polyfill for https://github.com/rust-lang/rfcs/pull/2580
19861986

‎library/core/src/cmp.rs

+2
Original file line numberDiff line numberDiff line change
@@ -230,13 +230,15 @@ pub trait PartialEq<Rhs: ?Sized = Self> {
230230
/// by `==`.
231231
#[must_use]
232232
#[stable(feature = "rust1", since = "1.0.0")]
233+
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialeq_eq")]
233234
fn eq(&self, other: &Rhs) -> bool;
234235

235236
/// This method tests for `!=`. The default implementation is almost always
236237
/// sufficient, and should not be overridden without very good reason.
237238
#[inline]
238239
#[must_use]
239240
#[stable(feature = "rust1", since = "1.0.0")]
241+
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialeq_ne")]
240242
fn ne(&self, other: &Rhs) -> bool {
241243
!self.eq(other)
242244
}

0 commit comments

Comments
 (0)