Skip to content

Commit 7622c0f

Browse files
committed
Auto merge of rust-lang#120393 - Urgau:rfc3373-non-local-defs, r=<try>
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 5518eaa + a6faefc commit 7622c0f

File tree

59 files changed

+1351
-94
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

+1351
-94
lines changed

Cargo.lock

+1
Original file line numberDiff line numberDiff line change
@@ -4070,6 +4070,7 @@ dependencies = [
40704070
"rustc_target",
40714071
"rustc_trait_selection",
40724072
"rustc_type_ir",
4073+
"smallvec",
40734074
"tracing",
40744075
"unicode-security",
40754076
]

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

+19
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,25 @@ lint_non_fmt_panic_unused =
406406
}
407407
.add_fmt_suggestion = or add a "{"{"}{"}"}" format string to use the message literally
408408
409+
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>
410+
411+
lint_non_local_definitions_impl = non-local `impl` definition, they should be avoided as they go against expectation
412+
.help =
413+
move this `impl` block outside the of the current {$body_kind_descr} {$depth ->
414+
[one] `{$body_name}`
415+
*[other] `{$body_name}` and up {$depth} bodies
416+
}
417+
.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
418+
.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>
419+
420+
lint_non_local_definitions_macro_rules = non-local `macro_rules!` definition, they should be avoided as they go against expectation
421+
.help =
422+
reove the `#[macro_export]` or move this `macro_rules!` outside the of the current {$body_kind_descr} {$depth ->
423+
[one] `{$body_name}`
424+
*[other] `{$body_name}` and up {$depth} bodies
425+
}
426+
.non_local = a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute
427+
409428
lint_non_snake_case = {$sort} `{$name}` should have a snake case name
410429
.rename_or_convert_suggestion = rename the identifier or convert it to a snake case raw identifier
411430
.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
@@ -72,6 +72,7 @@ mod methods;
7272
mod multiple_supertrait_upcastable;
7373
mod non_ascii_idents;
7474
mod non_fmt_panic;
75+
mod non_local_def;
7576
mod nonstandard_style;
7677
mod noop_method_call;
7778
mod opaque_hidden_inferred_bound;
@@ -107,6 +108,7 @@ use methods::*;
107108
use multiple_supertrait_upcastable::*;
108109
use non_ascii_idents::*;
109110
use non_fmt_panic::NonPanicFmt;
111+
use non_local_def::*;
110112
use nonstandard_style::*;
111113
use noop_method_call::*;
112114
use opaque_hidden_inferred_bound::*;
@@ -233,6 +235,7 @@ late_lint_methods!(
233235
MissingDebugImplementations: MissingDebugImplementations,
234236
MissingDoc: MissingDoc,
235237
AsyncFnInTrait: AsyncFnInTrait,
238+
NonLocalDefinitions: NonLocalDefinitions::default(),
236239
]
237240
]
238241
);

compiler/rustc_lint/src/lints.rs

+15
Original file line numberDiff line numberDiff line change
@@ -1305,6 +1305,21 @@ pub struct SuspiciousDoubleRefCloneDiag<'a> {
13051305
pub ty: Ty<'a>,
13061306
}
13071307

1308+
// non_local_defs.rs
1309+
#[derive(LintDiagnostic)]
1310+
pub enum NonLocalDefinitionsDiag {
1311+
#[diag(lint_non_local_definitions_impl)]
1312+
#[help]
1313+
#[note(lint_non_local)]
1314+
#[note(lint_non_local_definitions_deprecation)]
1315+
Impl { depth: u32, body_kind_descr: &'static str, body_name: String },
1316+
#[diag(lint_non_local_definitions_macro_rules)]
1317+
#[help]
1318+
#[note(lint_non_local)]
1319+
#[note(lint_non_local_definitions_deprecation)]
1320+
MacroRules { depth: u32, body_kind_descr: &'static str, body_name: String },
1321+
}
1322+
13081323
// pass_by_value.rs
13091324
#[derive(LintDiagnostic)]
13101325
#[diag(lint_pass_by_value)]
+198
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
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_impl = cx.tcx.parent(item.owner_id.def_id.into());
69+
let parent_impl_def_kind = cx.tcx.def_kind(parent_impl);
70+
71+
// Per RFC we (currently) ignore anon-const (`const _: Ty = ...`) in top-level module.
72+
if self.body_depth == 1
73+
&& parent_impl_def_kind == DefKind::Const
74+
&& cx.tcx.opt_item_name(parent_impl) == Some(kw::Underscore)
75+
{
76+
return;
77+
}
78+
79+
match item.kind {
80+
ItemKind::Impl(impl_) => {
81+
// The RFC states:
82+
//
83+
// > An item nested inside an expression-containing item (through any
84+
// > level of nesting) may not define an impl Trait for Type unless
85+
// > either the **Trait** or the **Type** is also nested inside the
86+
// > same expression-containing item.
87+
//
88+
// To achieve this we get try to get the paths of the _Trait_ and
89+
// _Type_, and we look inside thoses paths to try a find in one
90+
// of them a type whose parent is the same as the impl definition.
91+
//
92+
// If that's the case this means that this impl block decleration
93+
// is using local items and so we don't lint on it.
94+
95+
// We also ignore anon-const in item by including the anon-const
96+
// parent as well; and since it's quite uncommon, we use smallvec
97+
// to avoid unnecessary heap allocations.
98+
let local_parents: SmallVec<[DefId; 1]> = if parent_impl_def_kind == DefKind::Const
99+
&& cx.tcx.opt_item_name(parent_impl) == Some(kw::Underscore)
100+
{
101+
smallvec![parent_impl, cx.tcx.parent(parent_impl)]
102+
} else {
103+
smallvec![parent_impl]
104+
};
105+
106+
let self_ty_has_local_parent = match impl_.self_ty.kind {
107+
TyKind::Path(QPath::Resolved(_, ty_path)) => {
108+
path_has_local_parent(ty_path, cx, &*local_parents)
109+
}
110+
TyKind::TraitObject(poly_trait_refs, _, _) => {
111+
poly_trait_refs.iter().any(|poly_trait_ref| {
112+
path_has_local_parent(
113+
poly_trait_ref.trait_ref.path,
114+
cx,
115+
&*local_parents,
116+
)
117+
})
118+
}
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::OpaqueDef(_, _, _)
129+
| TyKind::Typeof(_)
130+
| TyKind::Infer
131+
| TyKind::Err(_) => false,
132+
};
133+
134+
let of_trait_has_local_parent = impl_
135+
.of_trait
136+
.map(|of_trait| path_has_local_parent(of_trait.path, cx, &*local_parents))
137+
.unwrap_or(false);
138+
139+
// If none of them have a local parent (LOGICAL NOR) this means that
140+
// this impl definition is a non-local definition and so we lint on it.
141+
if !(self_ty_has_local_parent || of_trait_has_local_parent) {
142+
cx.emit_span_lint(
143+
NON_LOCAL_DEFINITIONS,
144+
item.span,
145+
NonLocalDefinitionsDiag::Impl {
146+
depth: self.body_depth,
147+
body_kind_descr: cx
148+
.tcx
149+
.def_kind_descr(parent_impl_def_kind, parent_impl),
150+
body_name: cx
151+
.tcx
152+
.opt_item_name(parent_impl)
153+
.map(|s| s.to_ident_string())
154+
.unwrap_or_else(|| "<unnameable>".to_string()),
155+
},
156+
)
157+
}
158+
}
159+
ItemKind::Macro(_macro, MacroKind::Bang)
160+
if cx.tcx.has_attr(item.owner_id.def_id, sym::macro_export) =>
161+
{
162+
cx.emit_span_lint(
163+
NON_LOCAL_DEFINITIONS,
164+
item.span,
165+
NonLocalDefinitionsDiag::MacroRules {
166+
depth: self.body_depth,
167+
body_kind_descr: cx.tcx.def_kind_descr(parent_impl_def_kind, parent_impl),
168+
body_name: cx
169+
.tcx
170+
.opt_item_name(parent_impl)
171+
.map(|s| s.to_ident_string())
172+
.unwrap_or_else(|| "<unnameable>".to_string()),
173+
},
174+
)
175+
}
176+
_ => {}
177+
}
178+
}
179+
}
180+
181+
/// Given a path and a parent impl def id, this checks if the if parent resolution
182+
/// def id correspond to the def id of the parent impl definition.
183+
///
184+
/// Given this path, we will look at the path (and ignore any generic args):
185+
///
186+
/// ```text
187+
/// std::convert::PartialEq<Foo<Bar>>
188+
/// ^^^^^^^^^^^^^^^^^^^^^^^
189+
/// ```
190+
fn path_has_local_parent(path: &Path<'_>, cx: &LateContext<'_>, local_parents: &[DefId]) -> bool {
191+
if let Some(did) = path.res.opt_def_id()
192+
&& local_parents.contains(&cx.tcx.parent(did))
193+
{
194+
return true;
195+
}
196+
197+
false
198+
}

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
}

compiler/rustc_macros/src/hash_stable.rs

+2
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ fn hash_stable_derive_with_mode(
7474
HashStableMode::Generic | HashStableMode::NoContext => parse_quote!(__CTX),
7575
};
7676

77+
s.underscore_const(true);
78+
7779
// no_context impl is able to derive by-field, which is closer to a perfect derive.
7880
s.add_bounds(match mode {
7981
HashStableMode::Normal | HashStableMode::Generic => synstructure::AddBounds::Generics,

compiler/rustc_macros/src/lift.rs

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use syn::parse_quote;
44
pub fn lift_derive(mut s: synstructure::Structure<'_>) -> proc_macro2::TokenStream {
55
s.add_bounds(synstructure::AddBounds::Generics);
66
s.bind_with(|_| synstructure::BindStyle::Move);
7+
s.underscore_const(true);
78

89
let tcx: syn::Lifetime = parse_quote!('tcx);
910
let newtcx: syn::GenericParam = parse_quote!('__lifted);

0 commit comments

Comments
 (0)