Skip to content

Commit b0d3e04

Browse files
committed
Auto merge of rust-lang#120393 - Urgau:rfc3373-non-local-defs, r=WaffleLapkin
Implement RFC 3373: Avoid non-local definitions in functions This PR implements [RFC 3373: Avoid non-local definitions in functions](rust-lang#120363).
2 parents 8c0b1fc + 63469ab commit b0d3e04

File tree

59 files changed

+1518
-92
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

59 files changed

+1518
-92
lines changed

Cargo.lock

+1
Original file line numberDiff line numberDiff line change
@@ -4101,6 +4101,7 @@ dependencies = [
41014101
"rustc_target",
41024102
"rustc_trait_selection",
41034103
"rustc_type_ir",
4104+
"smallvec",
41044105
"tracing",
41054106
"unicode-security",
41064107
]

compiler/rustc_hir_analysis/src/coherence/inherent_impls_overlap.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ struct InherentOverlapChecker<'tcx> {
2424
tcx: TyCtxt<'tcx>,
2525
}
2626

27+
rustc_index::newtype_index! {
28+
#[orderable]
29+
pub struct RegionId {}
30+
}
31+
2732
impl<'tcx> InherentOverlapChecker<'tcx> {
2833
/// Checks whether any associated items in impls 1 and 2 share the same identifier and
2934
/// namespace.
@@ -205,11 +210,6 @@ impl<'tcx> InherentOverlapChecker<'tcx> {
205210
// This is advantageous to running the algorithm over the
206211
// entire graph when there are many connected regions.
207212

208-
rustc_index::newtype_index! {
209-
#[orderable]
210-
pub struct RegionId {}
211-
}
212-
213213
struct ConnectedRegion {
214214
idents: SmallVec<[Symbol; 8]>,
215215
impl_blocks: FxHashSet<usize>,

compiler/rustc_lint/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ rustc_span = { path = "../rustc_span" }
2323
rustc_target = { path = "../rustc_target" }
2424
rustc_trait_selection = { path = "../rustc_trait_selection" }
2525
rustc_type_ir = { path = "../rustc_type_ir" }
26+
smallvec = { version = "1.8.1", features = ["union", "may_dangle"] }
2627
tracing = "0.1"
2728
unicode-security = "0.1.0"
2829
# tidy-alphabetical-end

compiler/rustc_lint/messages.ftl

+23
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,29 @@ lint_non_fmt_panic_unused =
414414
}
415415
.add_fmt_suggestion = or add a "{"{"}{"}"}" format string to use the message literally
416416
417+
lint_non_local_definitions_cargo_update = the {$macro_kind} `{$macro_name}` may come from an old version of the `{$crate_name}` crate, try updating your dependency with `cargo update -p {$crate_name}`
418+
419+
lint_non_local_definitions_deprecation = this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <https://github.com/rust-lang/rust/issues/120363>
420+
421+
lint_non_local_definitions_impl = non-local `impl` definition, they should be avoided as they go against expectation
422+
.help =
423+
move this `impl` block outside the of the current {$body_kind_descr} {$depth ->
424+
[one] `{$body_name}`
425+
*[other] `{$body_name}` and up {$depth} bodies
426+
}
427+
.non_local = an `impl` definition is non-local if it is nested inside an item and neither the type nor the trait are at the same nesting level as the `impl` block
428+
.exception = one exception to the rule are anon-const (`const _: () = {"{"} ... {"}"}`) at top-level module and anon-const at the same nesting as the trait or type
429+
.const_anon = use a const-anon item to suppress this lint
430+
431+
lint_non_local_definitions_macro_rules = non-local `macro_rules!` definition, they should be avoided as they go against expectation
432+
.help =
433+
remove the `#[macro_export]` or move this `macro_rules!` outside the of the current {$body_kind_descr} {$depth ->
434+
[one] `{$body_name}`
435+
*[other] `{$body_name}` and up {$depth} bodies
436+
}
437+
.non_local = a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute
438+
.exception = one exception to the rule are anon-const (`const _: () = {"{"} ... {"}"}`) at top-level module
439+
417440
lint_non_snake_case = {$sort} `{$name}` should have a snake case name
418441
.rename_or_convert_suggestion = rename the identifier or convert it to a snake case raw identifier
419442
.cannot_convert_note = `{$sc}` cannot be used as a raw identifier

compiler/rustc_lint/src/lib.rs

+3
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ mod methods;
7070
mod multiple_supertrait_upcastable;
7171
mod non_ascii_idents;
7272
mod non_fmt_panic;
73+
mod non_local_def;
7374
mod nonstandard_style;
7475
mod noop_method_call;
7576
mod opaque_hidden_inferred_bound;
@@ -105,6 +106,7 @@ use methods::*;
105106
use multiple_supertrait_upcastable::*;
106107
use non_ascii_idents::*;
107108
use non_fmt_panic::NonPanicFmt;
109+
use non_local_def::*;
108110
use nonstandard_style::*;
109111
use noop_method_call::*;
110112
use opaque_hidden_inferred_bound::*;
@@ -229,6 +231,7 @@ late_lint_methods!(
229231
MissingDebugImplementations: MissingDebugImplementations,
230232
MissingDoc: MissingDoc,
231233
AsyncFnInTrait: AsyncFnInTrait,
234+
NonLocalDefinitions: NonLocalDefinitions::default(),
232235
]
233236
]
234237
);

compiler/rustc_lint/src/lints.rs

+39
Original file line numberDiff line numberDiff line change
@@ -1334,6 +1334,45 @@ pub struct SuspiciousDoubleRefCloneDiag<'a> {
13341334
pub ty: Ty<'a>,
13351335
}
13361336

1337+
// non_local_defs.rs
1338+
#[derive(LintDiagnostic)]
1339+
pub enum NonLocalDefinitionsDiag {
1340+
#[diag(lint_non_local_definitions_impl)]
1341+
#[help]
1342+
#[note(lint_non_local)]
1343+
#[note(lint_exception)]
1344+
#[note(lint_non_local_definitions_deprecation)]
1345+
Impl {
1346+
depth: u32,
1347+
body_kind_descr: &'static str,
1348+
body_name: String,
1349+
#[subdiagnostic]
1350+
cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
1351+
#[suggestion(lint_const_anon, code = "_", applicability = "machine-applicable")]
1352+
const_anon: Option<Span>,
1353+
},
1354+
#[diag(lint_non_local_definitions_macro_rules)]
1355+
#[help]
1356+
#[note(lint_non_local)]
1357+
#[note(lint_exception)]
1358+
#[note(lint_non_local_definitions_deprecation)]
1359+
MacroRules {
1360+
depth: u32,
1361+
body_kind_descr: &'static str,
1362+
body_name: String,
1363+
#[subdiagnostic]
1364+
cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
1365+
},
1366+
}
1367+
1368+
#[derive(Subdiagnostic)]
1369+
#[note(lint_non_local_definitions_cargo_update)]
1370+
pub struct NonLocalDefinitionsCargoUpdateNote {
1371+
pub macro_kind: &'static str,
1372+
pub macro_name: Symbol,
1373+
pub crate_name: Symbol,
1374+
}
1375+
13371376
// pass_by_value.rs
13381377
#[derive(LintDiagnostic)]
13391378
#[diag(lint_pass_by_value)]
+222
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
use rustc_hir::{def::DefKind, Body, Item, ItemKind, Node, Path, QPath, TyKind};
2+
use rustc_span::def_id::{DefId, LOCAL_CRATE};
3+
use rustc_span::{sym, symbol::kw, ExpnKind, MacroKind};
4+
5+
use smallvec::{smallvec, SmallVec};
6+
7+
use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag};
8+
use crate::{LateContext, LateLintPass, LintContext};
9+
10+
declare_lint! {
11+
/// The `non_local_definitions` lint checks for `impl` blocks and `#[macro_export]`
12+
/// macro inside bodies (functions, enum discriminant, ...).
13+
///
14+
/// ### Example
15+
///
16+
/// ```rust
17+
/// trait MyTrait {}
18+
/// struct MyStruct;
19+
///
20+
/// fn foo() {
21+
/// impl MyTrait for MyStruct {}
22+
/// }
23+
/// ```
24+
///
25+
/// {{produces}}
26+
///
27+
/// ### Explanation
28+
///
29+
/// Creating non-local definitions go against expectation and can create discrepancies
30+
/// in tooling. It should be avoided. It may become deny-by-default in edition 2024
31+
/// and higher, see see the tracking issue <https://github.com/rust-lang/rust/issues/120363>.
32+
///
33+
/// An `impl` definition is non-local if it is nested inside an item and neither
34+
/// the type nor the trait are at the same nesting level as the `impl` block.
35+
///
36+
/// All nested bodies (functions, enum discriminant, array length, consts) (expect for
37+
/// `const _: Ty = { ... }` in top-level module, which is still undecided) are checked.
38+
pub NON_LOCAL_DEFINITIONS,
39+
Warn,
40+
"checks for non-local definitions",
41+
report_in_external_macro
42+
}
43+
44+
#[derive(Default)]
45+
pub struct NonLocalDefinitions {
46+
body_depth: u32,
47+
}
48+
49+
impl_lint_pass!(NonLocalDefinitions => [NON_LOCAL_DEFINITIONS]);
50+
51+
// FIXME(Urgau): Figure out how to handle modules nested in bodies.
52+
// It's currently not handled by the current logic because modules are not bodies.
53+
// They don't even follow the correct order (check_body -> check_mod -> check_body_post)
54+
// instead check_mod is called after every body has been handled.
55+
56+
impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
57+
fn check_body(&mut self, _cx: &LateContext<'tcx>, _body: &'tcx Body<'tcx>) {
58+
self.body_depth += 1;
59+
}
60+
61+
fn check_body_post(&mut self, _cx: &LateContext<'tcx>, _body: &'tcx Body<'tcx>) {
62+
self.body_depth -= 1;
63+
}
64+
65+
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
66+
if self.body_depth == 0 {
67+
return;
68+
}
69+
70+
let parent = cx.tcx.parent(item.owner_id.def_id.into());
71+
let parent_def_kind = cx.tcx.def_kind(parent);
72+
let parent_opt_item_name = cx.tcx.opt_item_name(parent);
73+
74+
// Per RFC we (currently) ignore anon-const (`const _: Ty = ...`) in top-level module.
75+
if self.body_depth == 1
76+
&& parent_def_kind == DefKind::Const
77+
&& parent_opt_item_name == Some(kw::Underscore)
78+
{
79+
return;
80+
}
81+
82+
let cargo_update = || {
83+
let oexpn = item.span.ctxt().outer_expn_data();
84+
if let Some(def_id) = oexpn.macro_def_id
85+
&& let ExpnKind::Macro(macro_kind, macro_name) = oexpn.kind
86+
&& def_id.krate != LOCAL_CRATE
87+
&& std::env::var_os("CARGO").is_some()
88+
{
89+
Some(NonLocalDefinitionsCargoUpdateNote {
90+
macro_kind: macro_kind.descr(),
91+
macro_name,
92+
crate_name: cx.tcx.crate_name(def_id.krate),
93+
})
94+
} else {
95+
None
96+
}
97+
};
98+
99+
match item.kind {
100+
ItemKind::Impl(impl_) => {
101+
// The RFC states:
102+
//
103+
// > An item nested inside an expression-containing item (through any
104+
// > level of nesting) may not define an impl Trait for Type unless
105+
// > either the **Trait** or the **Type** is also nested inside the
106+
// > same expression-containing item.
107+
//
108+
// To achieve this we get try to get the paths of the _Trait_ and
109+
// _Type_, and we look inside thoses paths to try a find in one
110+
// of them a type whose parent is the same as the impl definition.
111+
//
112+
// If that's the case this means that this impl block declaration
113+
// is using local items and so we don't lint on it.
114+
115+
// We also ignore anon-const in item by including the anon-const
116+
// parent as well; and since it's quite uncommon, we use smallvec
117+
// to avoid unnecessary heap allocations.
118+
let local_parents: SmallVec<[DefId; 1]> = if parent_def_kind == DefKind::Const
119+
&& parent_opt_item_name == Some(kw::Underscore)
120+
{
121+
smallvec![parent, cx.tcx.parent(parent)]
122+
} else {
123+
smallvec![parent]
124+
};
125+
126+
let self_ty_has_local_parent = match impl_.self_ty.kind {
127+
TyKind::Path(QPath::Resolved(_, ty_path)) => {
128+
path_has_local_parent(ty_path, cx, &*local_parents)
129+
}
130+
TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => {
131+
path_has_local_parent(
132+
principle_poly_trait_ref.trait_ref.path,
133+
cx,
134+
&*local_parents,
135+
)
136+
}
137+
TyKind::TraitObject([], _, _)
138+
| TyKind::InferDelegation(_, _)
139+
| TyKind::Slice(_)
140+
| TyKind::Array(_, _)
141+
| TyKind::Ptr(_)
142+
| TyKind::Ref(_, _)
143+
| TyKind::BareFn(_)
144+
| TyKind::Never
145+
| TyKind::Tup(_)
146+
| TyKind::Path(_)
147+
| TyKind::AnonAdt(_)
148+
| TyKind::OpaqueDef(_, _, _)
149+
| TyKind::Typeof(_)
150+
| TyKind::Infer
151+
| TyKind::Err(_) => false,
152+
};
153+
154+
let of_trait_has_local_parent = impl_
155+
.of_trait
156+
.map(|of_trait| path_has_local_parent(of_trait.path, cx, &*local_parents))
157+
.unwrap_or(false);
158+
159+
// If none of them have a local parent (LOGICAL NOR) this means that
160+
// this impl definition is a non-local definition and so we lint on it.
161+
if !(self_ty_has_local_parent || of_trait_has_local_parent) {
162+
let const_anon = if self.body_depth == 1
163+
&& parent_def_kind == DefKind::Const
164+
&& parent_opt_item_name != Some(kw::Underscore)
165+
&& let Some(parent) = parent.as_local()
166+
&& let Node::Item(item) = cx.tcx.hir_node_by_def_id(parent)
167+
&& let ItemKind::Const(ty, _, _) = item.kind
168+
&& let TyKind::Tup(&[]) = ty.kind
169+
{
170+
Some(item.ident.span)
171+
} else {
172+
None
173+
};
174+
175+
cx.emit_span_lint(
176+
NON_LOCAL_DEFINITIONS,
177+
item.span,
178+
NonLocalDefinitionsDiag::Impl {
179+
depth: self.body_depth,
180+
body_kind_descr: cx.tcx.def_kind_descr(parent_def_kind, parent),
181+
body_name: parent_opt_item_name
182+
.map(|s| s.to_ident_string())
183+
.unwrap_or_else(|| "<unnameable>".to_string()),
184+
cargo_update: cargo_update(),
185+
const_anon,
186+
},
187+
)
188+
}
189+
}
190+
ItemKind::Macro(_macro, MacroKind::Bang)
191+
if cx.tcx.has_attr(item.owner_id.def_id, sym::macro_export) =>
192+
{
193+
cx.emit_span_lint(
194+
NON_LOCAL_DEFINITIONS,
195+
item.span,
196+
NonLocalDefinitionsDiag::MacroRules {
197+
depth: self.body_depth,
198+
body_kind_descr: cx.tcx.def_kind_descr(parent_def_kind, parent),
199+
body_name: parent_opt_item_name
200+
.map(|s| s.to_ident_string())
201+
.unwrap_or_else(|| "<unnameable>".to_string()),
202+
cargo_update: cargo_update(),
203+
},
204+
)
205+
}
206+
_ => {}
207+
}
208+
}
209+
}
210+
211+
/// Given a path and a parent impl def id, this checks if the if parent resolution
212+
/// def id correspond to the def id of the parent impl definition.
213+
///
214+
/// Given this path, we will look at the path (and ignore any generic args):
215+
///
216+
/// ```text
217+
/// std::convert::PartialEq<Foo<Bar>>
218+
/// ^^^^^^^^^^^^^^^^^^^^^^^
219+
/// ```
220+
fn path_has_local_parent(path: &Path<'_>, cx: &LateContext<'_>, local_parents: &[DefId]) -> bool {
221+
path.res.opt_def_id().is_some_and(|did| local_parents.contains(&cx.tcx.parent(did)))
222+
}

compiler/rustc_macros/src/diagnostics/mod.rs

+6-3
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ use synstructure::Structure;
5555
///
5656
/// See rustc dev guide for more examples on using the `#[derive(Diagnostic)]`:
5757
/// <https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html>
58-
pub fn session_diagnostic_derive(s: Structure<'_>) -> TokenStream {
58+
pub fn session_diagnostic_derive(mut s: Structure<'_>) -> TokenStream {
59+
s.underscore_const(true);
5960
DiagnosticDerive::new(s).into_tokens()
6061
}
6162

@@ -101,7 +102,8 @@ pub fn session_diagnostic_derive(s: Structure<'_>) -> TokenStream {
101102
///
102103
/// See rustc dev guide for more examples on using the `#[derive(LintDiagnostic)]`:
103104
/// <https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html#reference>
104-
pub fn lint_diagnostic_derive(s: Structure<'_>) -> TokenStream {
105+
pub fn lint_diagnostic_derive(mut s: Structure<'_>) -> TokenStream {
106+
s.underscore_const(true);
105107
LintDiagnosticDerive::new(s).into_tokens()
106108
}
107109

@@ -151,6 +153,7 @@ pub fn lint_diagnostic_derive(s: Structure<'_>) -> TokenStream {
151153
///
152154
/// diag.subdiagnostic(RawIdentifierSuggestion { span, applicability, ident });
153155
/// ```
154-
pub fn session_subdiagnostic_derive(s: Structure<'_>) -> TokenStream {
156+
pub fn session_subdiagnostic_derive(mut s: Structure<'_>) -> TokenStream {
157+
s.underscore_const(true);
155158
SubdiagnosticDeriveBuilder::new().into_tokens(s)
156159
}

0 commit comments

Comments
 (0)