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

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

Merged
merged 2 commits into from
Mar 29, 2024
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
14 changes: 8 additions & 6 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}

Expand Down
58 changes: 35 additions & 23 deletions compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
) {
Expand All @@ -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();

Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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(),
}
},
},
Expand All @@ -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);
}
}
}
Expand All @@ -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);
}
_ => {}
};
Expand Down Expand Up @@ -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,
})
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions library/core/src/cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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.
Expand All @@ -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))
}
Expand All @@ -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))
}
Expand All @@ -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))
}
Expand All @@ -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))
}
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 @@ -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))
}
Expand Down
1 change: 1 addition & 0 deletions library/core/src/ptr/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
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 @@ -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))
}
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/ptr/non_null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand All @@ -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())
}
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/lint/wide_pointer_comparisons.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
Loading
Loading