Skip to content

Add detection of [Partial]Ord methods in the ambiguous_wide_pointer_comparisons lint #121268

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

Merged
merged 2 commits into from
Mar 29, 2024
Merged
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
14 changes: 8 additions & 6 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
@@ -1668,14 +1668,16 @@ pub enum AmbiguousWidePointerComparisonsAddrSuggestion<'a> {
Cast {
deref_left: &'a str,
deref_right: &'a str,
#[suggestion_part(code = "{deref_left}")]
paren_left: &'a str,
paren_right: &'a str,
#[suggestion_part(code = "({deref_left}")]
left_before: Option<Span>,
#[suggestion_part(code = " as *const ()")]
left: Span,
#[suggestion_part(code = "{deref_right}")]
#[suggestion_part(code = "{paren_left}.cast::<()>()")]
left_after: Span,
#[suggestion_part(code = "({deref_right}")]
right_before: Option<Span>,
#[suggestion_part(code = " as *const ()")]
right: Span,
#[suggestion_part(code = "{paren_right}.cast::<()>()")]
right_after: Span,
},
}

58 changes: 35 additions & 23 deletions compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
@@ -657,10 +657,16 @@ fn lint_nan<'tcx>(
cx.emit_span_lint(INVALID_NAN_COMPARISONS, e.span, lint);
}

#[derive(Debug, PartialEq)]
enum ComparisonOp {
BinOp(hir::BinOpKind),
Other,
}

fn lint_wide_pointer<'tcx>(
cx: &LateContext<'tcx>,
e: &'tcx hir::Expr<'tcx>,
binop: hir::BinOpKind,
cmpop: ComparisonOp,
l: &'tcx hir::Expr<'tcx>,
r: &'tcx hir::Expr<'tcx>,
) {
@@ -679,7 +685,7 @@ fn lint_wide_pointer<'tcx>(
}
};

// PartialEq::{eq,ne} takes references, remove any explicit references
// the left and right operands can have references, remove any explicit references
let l = l.peel_borrows();
let r = r.peel_borrows();

@@ -707,8 +713,8 @@ fn lint_wide_pointer<'tcx>(
);
};

let ne = if binop == hir::BinOpKind::Ne { "!" } else { "" };
let is_eq_ne = matches!(binop, hir::BinOpKind::Eq | hir::BinOpKind::Ne);
let ne = if cmpop == ComparisonOp::BinOp(hir::BinOpKind::Ne) { "!" } else { "" };
let is_eq_ne = matches!(cmpop, ComparisonOp::BinOp(hir::BinOpKind::Eq | hir::BinOpKind::Ne));
let is_dyn_comparison = l_inner_ty_is_dyn && r_inner_ty_is_dyn;

let left = e.span.shrink_to_lo().until(l_span.shrink_to_lo());
@@ -745,12 +751,12 @@ fn lint_wide_pointer<'tcx>(
AmbiguousWidePointerComparisonsAddrSuggestion::Cast {
deref_left,
deref_right,
// those two Options are required for correctness as having
// an empty span and an empty suggestion is not permitted
left_before: (l_ty_refs != 0).then_some(left),
right_before: (r_ty_refs != 0).then(|| r_span.shrink_to_lo()),
left: l_span.shrink_to_hi(),
right,
paren_left: if l_ty_refs != 0 { ")" } else { "" },
paren_right: if r_ty_refs != 0 { ")" } else { "" },
left_before: (l_ty_refs != 0).then_some(l_span.shrink_to_lo()),
left_after: l_span.shrink_to_hi(),
right_before: (r_ty_refs != 0).then_some(r_span.shrink_to_lo()),
right_after: r_span.shrink_to_hi(),
}
},
},
@@ -773,7 +779,7 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
cx.emit_span_lint(UNUSED_COMPARISONS, e.span, UnusedComparisons);
} else {
lint_nan(cx, e, binop, l, r);
lint_wide_pointer(cx, e, binop.node, l, r);
lint_wide_pointer(cx, e, ComparisonOp::BinOp(binop.node), l, r);
}
}
}
@@ -782,16 +788,16 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
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)
&& let Some(binop) = partialeq_binop(diag_item) =>
&& let Some(cmpop) = diag_item_cmpop(diag_item) =>
{
lint_wide_pointer(cx, e, binop, l, r);
lint_wide_pointer(cx, e, cmpop, 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) =>
&& let Some(cmpop) = diag_item_cmpop(diag_item) =>
{
lint_wide_pointer(cx, e, binop, l, r);
lint_wide_pointer(cx, e, cmpop, l, r);
}
_ => {}
};
@@ -876,14 +882,20 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
)
}

fn partialeq_binop(diag_item: Symbol) -> Option<hir::BinOpKind> {
if diag_item == sym::cmp_partialeq_eq {
Some(hir::BinOpKind::Eq)
} else if diag_item == sym::cmp_partialeq_ne {
Some(hir::BinOpKind::Ne)
} else {
None
}
fn diag_item_cmpop(diag_item: Symbol) -> Option<ComparisonOp> {
Some(match diag_item {
sym::cmp_ord_max => ComparisonOp::Other,
sym::cmp_ord_min => ComparisonOp::Other,
sym::ord_cmp_method => ComparisonOp::Other,
sym::cmp_partialeq_eq => ComparisonOp::BinOp(hir::BinOpKind::Eq),
sym::cmp_partialeq_ne => ComparisonOp::BinOp(hir::BinOpKind::Ne),
sym::cmp_partialord_cmp => ComparisonOp::Other,
sym::cmp_partialord_ge => ComparisonOp::BinOp(hir::BinOpKind::Ge),
sym::cmp_partialord_gt => ComparisonOp::BinOp(hir::BinOpKind::Gt),
sym::cmp_partialord_le => ComparisonOp::BinOp(hir::BinOpKind::Le),
sym::cmp_partialord_lt => ComparisonOp::BinOp(hir::BinOpKind::Lt),
_ => return None,
})
}
}
}
7 changes: 7 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
@@ -531,8 +531,15 @@ symbols! {
cmp,
cmp_max,
cmp_min,
cmp_ord_max,
cmp_ord_min,
cmp_partialeq_eq,
cmp_partialeq_ne,
cmp_partialord_cmp,
cmp_partialord_ge,
cmp_partialord_gt,
cmp_partialord_le,
cmp_partialord_lt,
cmpxchg16b_target_feature,
cmse_nonsecure_entry,
coerce_unsized,
7 changes: 7 additions & 0 deletions library/core/src/cmp.rs
Original file line number Diff line number Diff line change
@@ -848,6 +848,7 @@ pub trait Ord: Eq + PartialOrd<Self> {
#[stable(feature = "ord_max_min", since = "1.21.0")]
#[inline]
#[must_use]
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_ord_max")]
fn max(self, other: Self) -> Self
where
Self: Sized,
@@ -868,6 +869,7 @@ pub trait Ord: Eq + PartialOrd<Self> {
#[stable(feature = "ord_max_min", since = "1.21.0")]
#[inline]
#[must_use]
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_ord_min")]
fn min(self, other: Self) -> Self
where
Self: Sized,
@@ -1154,6 +1156,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
/// ```
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialord_cmp")]
fn partial_cmp(&self, other: &Rhs) -> Option<Ordering>;

/// This method tests less than (for `self` and `other`) and is used by the `<` operator.
@@ -1168,6 +1171,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
#[inline]
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialord_lt")]
fn lt(&self, other: &Rhs) -> bool {
matches!(self.partial_cmp(other), Some(Less))
}
@@ -1185,6 +1189,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
#[inline]
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialord_le")]
fn le(&self, other: &Rhs) -> bool {
matches!(self.partial_cmp(other), Some(Less | Equal))
}
@@ -1201,6 +1206,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
#[inline]
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialord_gt")]
fn gt(&self, other: &Rhs) -> bool {
matches!(self.partial_cmp(other), Some(Greater))
}
@@ -1218,6 +1224,7 @@ pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
#[inline]
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialord_ge")]
fn ge(&self, other: &Rhs) -> bool {
matches!(self.partial_cmp(other), Some(Greater | Equal))
}
1 change: 1 addition & 0 deletions library/core/src/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
@@ -1857,6 +1857,7 @@ impl<T: ?Sized> Ord for *const T {
#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> PartialOrd for *const T {
#[inline]
#[allow(ambiguous_wide_pointer_comparisons)]
fn partial_cmp(&self, other: &*const T) -> Option<Ordering> {
Some(self.cmp(other))
}
1 change: 1 addition & 0 deletions library/core/src/ptr/metadata.rs
Original file line number Diff line number Diff line change
@@ -258,6 +258,7 @@ impl<Dyn: ?Sized> PartialEq for DynMetadata<Dyn> {

impl<Dyn: ?Sized> Ord for DynMetadata<Dyn> {
#[inline]
#[allow(ambiguous_wide_pointer_comparisons)]
fn cmp(&self, other: &Self) -> crate::cmp::Ordering {
(self.vtable_ptr as *const VTable).cmp(&(other.vtable_ptr as *const VTable))
}
1 change: 1 addition & 0 deletions library/core/src/ptr/mut_ptr.rs
Original file line number Diff line number Diff line change
@@ -2275,6 +2275,7 @@ impl<T: ?Sized> Ord for *mut T {
#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> PartialOrd for *mut T {
#[inline(always)]
#[allow(ambiguous_wide_pointer_comparisons)]
fn partial_cmp(&self, other: &*mut T) -> Option<Ordering> {
Some(self.cmp(other))
}
2 changes: 2 additions & 0 deletions library/core/src/ptr/non_null.rs
Original file line number Diff line number Diff line change
@@ -1821,6 +1821,7 @@ impl<T: ?Sized> PartialEq for NonNull<T> {
#[stable(feature = "nonnull", since = "1.25.0")]
impl<T: ?Sized> Ord for NonNull<T> {
#[inline]
#[allow(ambiguous_wide_pointer_comparisons)]
fn cmp(&self, other: &Self) -> Ordering {
self.as_ptr().cmp(&other.as_ptr())
}
@@ -1829,6 +1830,7 @@ impl<T: ?Sized> Ord for NonNull<T> {
#[stable(feature = "nonnull", since = "1.25.0")]
impl<T: ?Sized> PartialOrd for NonNull<T> {
#[inline]
#[allow(ambiguous_wide_pointer_comparisons)]
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
self.as_ptr().partial_cmp(&other.as_ptr())
}
24 changes: 24 additions & 0 deletions tests/ui/lint/wide_pointer_comparisons.rs
Original file line number Diff line number Diff line change
@@ -37,6 +37,18 @@ fn main() {
//~^ WARN ambiguous wide pointer comparison
let _ = a.ne(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.cmp(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.partial_cmp(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.le(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.lt(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.ge(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.gt(&b);
//~^ WARN ambiguous wide pointer comparison

{
// &*const ?Sized
@@ -68,6 +80,18 @@ fn main() {
//~^ WARN ambiguous wide pointer comparison
let _ = a.ne(b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.cmp(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.partial_cmp(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.le(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.lt(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.ge(&b);
//~^ WARN ambiguous wide pointer comparison
let _ = a.gt(&b);
//~^ WARN ambiguous wide pointer comparison
}

let s = "" as *const str;
238 changes: 185 additions & 53 deletions tests/ui/lint/wide_pointer_comparisons.stderr

Large diffs are not rendered by default.