Skip to content

Commit 3694e40

Browse files
authored
Rollup merge of #97389 - m-ou-se:memory-ordering-diagnostics, r=estebank
Improve memory ordering diagnostics Before: ![image](https://user-images.githubusercontent.com/783247/170234545-891cac30-eaa2-4186-847b-35cd51e00f2b.png) After: ![image](https://user-images.githubusercontent.com/783247/170239684-645f186f-5a02-4eb9-8651-2e5fe9591352.png) --- Before this change, the compiler suggests the failure ordering is too strong and suggests choosing a weaker ordering. After this change, it instead suggests the success ordering is not strong enough, and suggests chosing a stronger one. This is more likely to be correct. Also, before this change, the compiler suggested downgrading an invalid AcqRel failure ordering to Relaxed, without mentioning Acquire as an option.
2 parents 7702ae1 + f107923 commit 3694e40

7 files changed

+312
-310
lines changed

compiler/rustc_lint/src/types.rs

+66-82
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ use rustc_attr as attr;
44
use rustc_data_structures::fx::FxHashSet;
55
use rustc_errors::Applicability;
66
use rustc_hir as hir;
7-
use rustc_hir::def_id::DefId;
87
use rustc_hir::{is_range_literal, Expr, ExprKind, Node};
98
use rustc_middle::ty::layout::{IntegerExt, LayoutOf, SizeSkeleton};
109
use rustc_middle::ty::subst::SubstsRef;
@@ -1483,39 +1482,32 @@ impl InvalidAtomicOrdering {
14831482
None
14841483
}
14851484

1486-
fn matches_ordering(cx: &LateContext<'_>, did: DefId, orderings: &[Symbol]) -> bool {
1485+
fn match_ordering(cx: &LateContext<'_>, ord_arg: &Expr<'_>) -> Option<Symbol> {
1486+
let ExprKind::Path(ref ord_qpath) = ord_arg.kind else { return None };
1487+
let did = cx.qpath_res(ord_qpath, ord_arg.hir_id).opt_def_id()?;
14871488
let tcx = cx.tcx;
14881489
let atomic_ordering = tcx.get_diagnostic_item(sym::Ordering);
1489-
orderings.iter().any(|ordering| {
1490-
tcx.item_name(did) == *ordering && {
1491-
let parent = tcx.parent(did);
1492-
Some(parent) == atomic_ordering
1493-
// needed in case this is a ctor, not a variant
1494-
|| tcx.opt_parent(parent) == atomic_ordering
1495-
}
1496-
})
1497-
}
1498-
1499-
fn opt_ordering_defid(cx: &LateContext<'_>, ord_arg: &Expr<'_>) -> Option<DefId> {
1500-
if let ExprKind::Path(ref ord_qpath) = ord_arg.kind {
1501-
cx.qpath_res(ord_qpath, ord_arg.hir_id).opt_def_id()
1502-
} else {
1503-
None
1504-
}
1490+
let name = tcx.item_name(did);
1491+
let parent = tcx.parent(did);
1492+
[sym::Relaxed, sym::Release, sym::Acquire, sym::AcqRel, sym::SeqCst].into_iter().find(
1493+
|&ordering| {
1494+
name == ordering
1495+
&& (Some(parent) == atomic_ordering
1496+
// needed in case this is a ctor, not a variant
1497+
|| tcx.opt_parent(parent) == atomic_ordering)
1498+
},
1499+
)
15051500
}
15061501

15071502
fn check_atomic_load_store(cx: &LateContext<'_>, expr: &Expr<'_>) {
1508-
use rustc_hir::def::{DefKind, Res};
1509-
use rustc_hir::QPath;
15101503
if let Some((method, args)) = Self::inherent_atomic_method_call(cx, expr, &[sym::load, sym::store])
15111504
&& let Some((ordering_arg, invalid_ordering)) = match method {
15121505
sym::load => Some((&args[1], sym::Release)),
15131506
sym::store => Some((&args[2], sym::Acquire)),
15141507
_ => None,
15151508
}
1516-
&& let ExprKind::Path(QPath::Resolved(_, path)) = ordering_arg.kind
1517-
&& let Res::Def(DefKind::Ctor(..), ctor_id) = path.res
1518-
&& Self::matches_ordering(cx, ctor_id, &[invalid_ordering, sym::AcqRel])
1509+
&& let Some(ordering) = Self::match_ordering(cx, ordering_arg)
1510+
&& (ordering == invalid_ordering || ordering == sym::AcqRel)
15191511
{
15201512
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, ordering_arg.span, |diag| {
15211513
if method == sym::load {
@@ -1537,9 +1529,7 @@ impl InvalidAtomicOrdering {
15371529
&& let ExprKind::Path(ref func_qpath) = func.kind
15381530
&& let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id()
15391531
&& matches!(cx.tcx.get_diagnostic_name(def_id), Some(sym::fence | sym::compiler_fence))
1540-
&& let ExprKind::Path(ref ordering_qpath) = &args[0].kind
1541-
&& let Some(ordering_def_id) = cx.qpath_res(ordering_qpath, args[0].hir_id).opt_def_id()
1542-
&& Self::matches_ordering(cx, ordering_def_id, &[sym::Relaxed])
1532+
&& Self::match_ordering(cx, &args[0]) == Some(sym::Relaxed)
15431533
{
15441534
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, args[0].span, |diag| {
15451535
diag.build("memory fences cannot have `Relaxed` ordering")
@@ -1550,62 +1540,56 @@ impl InvalidAtomicOrdering {
15501540
}
15511541

15521542
fn check_atomic_compare_exchange(cx: &LateContext<'_>, expr: &Expr<'_>) {
1553-
if let Some((method, args)) = Self::inherent_atomic_method_call(cx, expr, &[sym::fetch_update, sym::compare_exchange, sym::compare_exchange_weak])
1554-
&& let Some((success_order_arg, failure_order_arg)) = match method {
1555-
sym::fetch_update => Some((&args[1], &args[2])),
1556-
sym::compare_exchange | sym::compare_exchange_weak => Some((&args[3], &args[4])),
1557-
_ => None,
1558-
}
1559-
&& let Some(fail_ordering_def_id) = Self::opt_ordering_defid(cx, failure_order_arg)
1560-
{
1561-
// Helper type holding on to some checking and error reporting data. Has
1562-
// - (success ordering,
1563-
// - list of failure orderings forbidden by the success order,
1564-
// - suggestion message)
1565-
type OrdLintInfo = (Symbol, &'static [Symbol], &'static str);
1566-
const RELAXED: OrdLintInfo = (sym::Relaxed, &[sym::SeqCst, sym::Acquire], "ordering mode `Relaxed`");
1567-
const ACQUIRE: OrdLintInfo = (sym::Acquire, &[sym::SeqCst], "ordering modes `Acquire` or `Relaxed`");
1568-
const SEQ_CST: OrdLintInfo = (sym::SeqCst, &[], "ordering modes `Acquire`, `SeqCst` or `Relaxed`");
1569-
const RELEASE: OrdLintInfo = (sym::Release, RELAXED.1, RELAXED.2);
1570-
const ACQREL: OrdLintInfo = (sym::AcqRel, ACQUIRE.1, ACQUIRE.2);
1571-
const SEARCH: [OrdLintInfo; 5] = [RELAXED, ACQUIRE, SEQ_CST, RELEASE, ACQREL];
1572-
1573-
let success_lint_info = Self::opt_ordering_defid(cx, success_order_arg)
1574-
.and_then(|success_ord_def_id| -> Option<OrdLintInfo> {
1575-
SEARCH
1576-
.iter()
1577-
.copied()
1578-
.find(|(ordering, ..)| {
1579-
Self::matches_ordering(cx, success_ord_def_id, &[*ordering])
1580-
})
1581-
});
1582-
if Self::matches_ordering(cx, fail_ordering_def_id, &[sym::Release, sym::AcqRel]) {
1583-
// If we don't know the success order is, use what we'd suggest
1584-
// if it were maximally permissive.
1585-
let suggested = success_lint_info.unwrap_or(SEQ_CST).2;
1586-
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, failure_order_arg.span, |diag| {
1587-
let msg = format!(
1588-
"{}'s failure ordering may not be `Release` or `AcqRel`",
1589-
method,
1590-
);
1591-
diag.build(&msg)
1592-
.help(&format!("consider using {} instead", suggested))
1593-
.emit();
1594-
});
1595-
} else if let Some((success_ord, bad_ords_given_success, suggested)) = success_lint_info {
1596-
if Self::matches_ordering(cx, fail_ordering_def_id, bad_ords_given_success) {
1597-
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, failure_order_arg.span, |diag| {
1598-
let msg = format!(
1599-
"{}'s failure ordering may not be stronger than the success ordering of `{}`",
1600-
method,
1601-
success_ord,
1602-
);
1603-
diag.build(&msg)
1604-
.help(&format!("consider using {} instead", suggested))
1605-
.emit();
1606-
});
1607-
}
1608-
}
1543+
let Some((method, args)) = Self::inherent_atomic_method_call(cx, expr, &[sym::fetch_update, sym::compare_exchange, sym::compare_exchange_weak])
1544+
else {return };
1545+
1546+
let (success_order_arg, fail_order_arg) = match method {
1547+
sym::fetch_update => (&args[1], &args[2]),
1548+
sym::compare_exchange | sym::compare_exchange_weak => (&args[3], &args[4]),
1549+
_ => return,
1550+
};
1551+
1552+
let Some(fail_ordering) = Self::match_ordering(cx, fail_order_arg) else { return };
1553+
1554+
if matches!(fail_ordering, sym::Release | sym::AcqRel) {
1555+
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, fail_order_arg.span, |diag| {
1556+
diag.build(&format!(
1557+
"`{method}`'s failure ordering may not be `Release` or `AcqRel`, \
1558+
since a failed `{method}` does not result in a write",
1559+
))
1560+
.span_label(fail_order_arg.span, "invalid failure ordering")
1561+
.help("consider using `Acquire` or `Relaxed` failure ordering instead")
1562+
.emit();
1563+
});
1564+
}
1565+
1566+
let Some(success_ordering) = Self::match_ordering(cx, success_order_arg) else { return };
1567+
1568+
if matches!(
1569+
(success_ordering, fail_ordering),
1570+
(sym::Relaxed | sym::Release, sym::Acquire)
1571+
| (sym::Relaxed | sym::Release | sym::Acquire | sym::AcqRel, sym::SeqCst)
1572+
) {
1573+
let success_suggestion =
1574+
if success_ordering == sym::Release && fail_ordering == sym::Acquire {
1575+
sym::AcqRel
1576+
} else {
1577+
fail_ordering
1578+
};
1579+
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, success_order_arg.span, |diag| {
1580+
diag.build(&format!(
1581+
"`{method}`'s success ordering must be at least as strong as its failure ordering"
1582+
))
1583+
.span_label(fail_order_arg.span, format!("`{fail_ordering}` failure ordering"))
1584+
.span_label(success_order_arg.span, format!("`{success_ordering}` success ordering"))
1585+
.span_suggestion_short(
1586+
success_order_arg.span,
1587+
format!("consider using `{success_suggestion}` success ordering instead"),
1588+
format!("std::sync::atomic::Ordering::{success_suggestion}"),
1589+
Applicability::MaybeIncorrect,
1590+
)
1591+
.emit();
1592+
});
16091593
}
16101594
}
16111595
}

src/test/ui/lint/lint-invalid-atomic-ordering-exchange-weak.rs

+16-16
Original file line numberDiff line numberDiff line change
@@ -20,43 +20,43 @@ fn main() {
2020

2121
// AcqRel is always forbidden as a failure ordering
2222
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Relaxed, Ordering::AcqRel);
23-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
23+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
2424
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Acquire, Ordering::AcqRel);
25-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
25+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
2626
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Release, Ordering::AcqRel);
27-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
27+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
2828
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::AcqRel, Ordering::AcqRel);
29-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
29+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
3030
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::SeqCst, Ordering::AcqRel);
31-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
31+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
3232

3333
// Release is always forbidden as a failure ordering
3434
let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Relaxed, Ordering::Release);
35-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
35+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
3636
let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Acquire, Ordering::Release);
37-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
37+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
3838
let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Release, Ordering::Release);
39-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
39+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
4040
let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::AcqRel, Ordering::Release);
41-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
41+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
4242
let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::SeqCst, Ordering::Release);
43-
//~^ ERROR compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
43+
//~^ ERROR `compare_exchange_weak`'s failure ordering may not be `Release` or `AcqRel`
4444

4545
// Release success order forbids failure order of Acquire or SeqCst
4646
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Release, Ordering::Acquire);
47-
//~^ ERROR compare_exchange_weak's failure ordering may not be stronger
47+
//~^ ERROR `compare_exchange_weak`'s success ordering must be at least as strong as
4848
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Release, Ordering::SeqCst);
49-
//~^ ERROR compare_exchange_weak's failure ordering may not be stronger
49+
//~^ ERROR `compare_exchange_weak`'s success ordering must be at least as strong as
5050

5151
// Relaxed success order also forbids failure order of Acquire or SeqCst
5252
let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Relaxed, Ordering::SeqCst);
53-
//~^ ERROR compare_exchange_weak's failure ordering may not be stronger
53+
//~^ ERROR `compare_exchange_weak`'s success ordering must be at least as strong as
5454
let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Relaxed, Ordering::Acquire);
55-
//~^ ERROR compare_exchange_weak's failure ordering may not be stronger
55+
//~^ ERROR `compare_exchange_weak`'s success ordering must be at least as strong as
5656

5757
// Acquire/AcqRel forbids failure order of SeqCst
5858
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Acquire, Ordering::SeqCst);
59-
//~^ ERROR compare_exchange_weak's failure ordering may not be stronger
59+
//~^ ERROR `compare_exchange_weak`'s success ordering must be at least as strong as
6060
let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::AcqRel, Ordering::SeqCst);
61-
//~^ ERROR compare_exchange_weak's failure ordering may not be stronger
61+
//~^ ERROR `compare_exchange_weak`'s success ordering must be at least as strong as
6262
}

0 commit comments

Comments
 (0)