Skip to content

Commit 6170394

Browse files
committed
Implement RFC3373 non local definitions lint
1 parent c114443 commit 6170394

File tree

10 files changed

+1241
-0
lines changed

10 files changed

+1241
-0
lines changed

Cargo.lock

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4164,6 +4164,7 @@ dependencies = [
41644164
"rustc_target",
41654165
"rustc_trait_selection",
41664166
"rustc_type_ir",
4167+
"smallvec",
41674168
"tracing",
41684169
"unicode-security",
41694170
]

compiler/rustc_lint/Cargo.toml

Lines changed: 1 addition & 0 deletions
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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,26 @@ lint_non_fmt_panic_unused =
411411
}
412412
.add_fmt_suggestion = or add a "{"{"}{"}"}" format string to use the message literally
413413
414+
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>
415+
416+
lint_non_local_definitions_impl = non-local `impl` definition, they should be avoided as they go against expectation
417+
.help =
418+
move this `impl` block outside the of the current {$body_kind_descr} {$depth ->
419+
[one] `{$body_name}`
420+
*[other] `{$body_name}` and up {$depth} bodies
421+
}
422+
.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
423+
.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
424+
425+
lint_non_local_definitions_macro_rules = non-local `macro_rules!` definition, they should be avoided as they go against expectation
426+
.help =
427+
remove the `#[macro_export]` or move this `macro_rules!` outside the of the current {$body_kind_descr} {$depth ->
428+
[one] `{$body_name}`
429+
*[other] `{$body_name}` and up {$depth} bodies
430+
}
431+
.non_local = a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute
432+
.exception = one exception to the rule are anon-const (`const _: () = {"{"} ... {"}"}`) at top-level module
433+
414434
lint_non_snake_case = {$sort} `{$name}` should have a snake case name
415435
.rename_or_convert_suggestion = rename the identifier or convert it to a snake case raw identifier
416436
.cannot_convert_note = `{$sc}` cannot be used as a raw identifier

compiler/rustc_lint/src/lib.rs

Lines changed: 3 additions & 0 deletions
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::*;
@@ -231,6 +233,7 @@ late_lint_methods!(
231233
MissingDebugImplementations: MissingDebugImplementations,
232234
MissingDoc: MissingDoc,
233235
AsyncFnInTrait: AsyncFnInTrait,
236+
NonLocalDefinitions: NonLocalDefinitions::default(),
234237
]
235238
]
236239
);

compiler/rustc_lint/src/lints.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,6 +1293,23 @@ pub struct SuspiciousDoubleRefCloneDiag<'a> {
12931293
pub ty: Ty<'a>,
12941294
}
12951295

1296+
// non_local_defs.rs
1297+
#[derive(LintDiagnostic)]
1298+
pub enum NonLocalDefinitionsDiag {
1299+
#[diag(lint_non_local_definitions_impl)]
1300+
#[help]
1301+
#[note(lint_non_local)]
1302+
#[note(lint_exception)]
1303+
#[note(lint_non_local_definitions_deprecation)]
1304+
Impl { depth: u32, body_kind_descr: &'static str, body_name: String },
1305+
#[diag(lint_non_local_definitions_macro_rules)]
1306+
#[help]
1307+
#[note(lint_non_local)]
1308+
#[note(lint_exception)]
1309+
#[note(lint_non_local_definitions_deprecation)]
1310+
MacroRules { depth: u32, body_kind_descr: &'static str, body_name: String },
1311+
}
1312+
12961313
// pass_by_value.rs
12971314
#[derive(LintDiagnostic)]
12981315
#[diag(lint_pass_by_value)]
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
use rustc_hir::{def::DefKind, Body, Item, ItemKind, Path, QPath, TyKind};
2+
use rustc_span::{def_id::DefId, sym, symbol::kw, MacroKind};
3+
4+
use smallvec::{smallvec, SmallVec};
5+
6+
use crate::{lints::NonLocalDefinitionsDiag, LateContext, LateLintPass, LintContext};
7+
8+
declare_lint! {
9+
/// The `non_local_definitions` lint checks for `impl` blocks and `#[macro_export]`
10+
/// macro inside bodies (functions, enum discriminant, ...).
11+
///
12+
/// ### Example
13+
///
14+
/// ```rust
15+
/// trait MyTrait {}
16+
/// struct MyStruct;
17+
///
18+
/// fn foo() {
19+
/// impl MyTrait for MyStruct {}
20+
/// }
21+
/// ```
22+
///
23+
/// {{produces}}
24+
///
25+
/// ### Explanation
26+
///
27+
/// Creating non-local definitions go against expectation and can create discrepancies
28+
/// in tooling. It should be avoided. It may become deny-by-default in edition 2024
29+
/// and higher, see see the tracking issue <https://github.com/rust-lang/rust/issues/120363>.
30+
///
31+
/// An `impl` definition is non-local if it is nested inside an item and neither
32+
/// the type nor the trait are at the same nesting level as the `impl` block.
33+
///
34+
/// All nested bodies (functions, enum discriminant, array length, consts) (expect for
35+
/// `const _: Ty = { ... }` in top-level module, which is still undecided) are checked.
36+
pub NON_LOCAL_DEFINITIONS,
37+
Warn,
38+
"checks for non-local definitions",
39+
report_in_external_macro
40+
}
41+
42+
#[derive(Default)]
43+
pub struct NonLocalDefinitions {
44+
body_depth: u32,
45+
}
46+
47+
impl_lint_pass!(NonLocalDefinitions => [NON_LOCAL_DEFINITIONS]);
48+
49+
// FIXME(Urgau): Figure out how to handle modules nested in bodies.
50+
// It's currently not handled by the current logic because modules are not bodies.
51+
// They don't even follow the correct order (check_body -> check_mod -> check_body_post)
52+
// instead check_mod is called after every body has been handled.
53+
54+
impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
55+
fn check_body(&mut self, _cx: &LateContext<'tcx>, _body: &'tcx Body<'tcx>) {
56+
self.body_depth += 1;
57+
}
58+
59+
fn check_body_post(&mut self, _cx: &LateContext<'tcx>, _body: &'tcx Body<'tcx>) {
60+
self.body_depth -= 1;
61+
}
62+
63+
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
64+
if self.body_depth == 0 {
65+
return;
66+
}
67+
68+
let parent = cx.tcx.parent(item.owner_id.def_id.into());
69+
let parent_def_kind = cx.tcx.def_kind(parent);
70+
let parent_opt_item_name = cx.tcx.opt_item_name(parent);
71+
72+
// Per RFC we (currently) ignore anon-const (`const _: Ty = ...`) in top-level module.
73+
if self.body_depth == 1
74+
&& parent_def_kind == DefKind::Const
75+
&& parent_opt_item_name == Some(kw::Underscore)
76+
{
77+
return;
78+
}
79+
80+
match item.kind {
81+
ItemKind::Impl(impl_) => {
82+
// The RFC states:
83+
//
84+
// > An item nested inside an expression-containing item (through any
85+
// > level of nesting) may not define an impl Trait for Type unless
86+
// > either the **Trait** or the **Type** is also nested inside the
87+
// > same expression-containing item.
88+
//
89+
// To achieve this we get try to get the paths of the _Trait_ and
90+
// _Type_, and we look inside thoses paths to try a find in one
91+
// of them a type whose parent is the same as the impl definition.
92+
//
93+
// If that's the case this means that this impl block declaration
94+
// is using local items and so we don't lint on it.
95+
96+
// We also ignore anon-const in item by including the anon-const
97+
// parent as well; and since it's quite uncommon, we use smallvec
98+
// to avoid unnecessary heap allocations.
99+
let local_parents: SmallVec<[DefId; 1]> = if parent_def_kind == DefKind::Const
100+
&& parent_opt_item_name == Some(kw::Underscore)
101+
{
102+
smallvec![parent, cx.tcx.parent(parent)]
103+
} else {
104+
smallvec![parent]
105+
};
106+
107+
let self_ty_has_local_parent = match impl_.self_ty.kind {
108+
TyKind::Path(QPath::Resolved(_, ty_path)) => {
109+
path_has_local_parent(ty_path, cx, &*local_parents)
110+
}
111+
TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => {
112+
path_has_local_parent(
113+
principle_poly_trait_ref.trait_ref.path,
114+
cx,
115+
&*local_parents,
116+
)
117+
}
118+
TyKind::TraitObject([], _, _)
119+
| TyKind::InferDelegation(_, _)
120+
| TyKind::Slice(_)
121+
| TyKind::Array(_, _)
122+
| TyKind::Ptr(_)
123+
| TyKind::Ref(_, _)
124+
| TyKind::BareFn(_)
125+
| TyKind::Never
126+
| TyKind::Tup(_)
127+
| TyKind::Path(_)
128+
| TyKind::AnonAdt(_)
129+
| TyKind::OpaqueDef(_, _, _)
130+
| TyKind::Typeof(_)
131+
| TyKind::Infer
132+
| TyKind::Err(_) => false,
133+
};
134+
135+
let of_trait_has_local_parent = impl_
136+
.of_trait
137+
.map(|of_trait| path_has_local_parent(of_trait.path, cx, &*local_parents))
138+
.unwrap_or(false);
139+
140+
// If none of them have a local parent (LOGICAL NOR) this means that
141+
// this impl definition is a non-local definition and so we lint on it.
142+
if !(self_ty_has_local_parent || of_trait_has_local_parent) {
143+
cx.emit_span_lint(
144+
NON_LOCAL_DEFINITIONS,
145+
item.span,
146+
NonLocalDefinitionsDiag::Impl {
147+
depth: self.body_depth,
148+
body_kind_descr: cx.tcx.def_kind_descr(parent_def_kind, parent),
149+
body_name: parent_opt_item_name
150+
.map(|s| s.to_ident_string())
151+
.unwrap_or_else(|| "<unnameable>".to_string()),
152+
},
153+
)
154+
}
155+
}
156+
ItemKind::Macro(_macro, MacroKind::Bang)
157+
if cx.tcx.has_attr(item.owner_id.def_id, sym::macro_export) =>
158+
{
159+
cx.emit_span_lint(
160+
NON_LOCAL_DEFINITIONS,
161+
item.span,
162+
NonLocalDefinitionsDiag::MacroRules {
163+
depth: self.body_depth,
164+
body_kind_descr: cx.tcx.def_kind_descr(parent_def_kind, parent),
165+
body_name: parent_opt_item_name
166+
.map(|s| s.to_ident_string())
167+
.unwrap_or_else(|| "<unnameable>".to_string()),
168+
},
169+
)
170+
}
171+
_ => {}
172+
}
173+
}
174+
}
175+
176+
/// Given a path and a parent impl def id, this checks if the if parent resolution
177+
/// def id correspond to the def id of the parent impl definition.
178+
///
179+
/// Given this path, we will look at the path (and ignore any generic args):
180+
///
181+
/// ```text
182+
/// std::convert::PartialEq<Foo<Bar>>
183+
/// ^^^^^^^^^^^^^^^^^^^^^^^
184+
/// ```
185+
fn path_has_local_parent(path: &Path<'_>, cx: &LateContext<'_>, local_parents: &[DefId]) -> bool {
186+
path.res.opt_def_id().is_some_and(|did| local_parents.contains(&cx.tcx.parent(did)))
187+
}

0 commit comments

Comments
 (0)