Skip to content

Commit

Permalink
Rollup merge of rust-lang#125089 - Urgau:non_local_def-suggestions, r…
Browse files Browse the repository at this point in the history 
…=estebank

Improve diagnostic output the `non_local_definitions` lint

This PR improves (or at least tries to improve) the diagnostic output the `non_local_definitions` lint, by simplifying the wording, by adding a "sort of" explanation of bounds interaction that leak the impl...

This PR is best reviewed commit by commit and is voluntarily made a bit vague as to have a starting point to improve on.

Related to https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/non_local_defs.20wording.20improvements

Fixes rust-lang#125068
Fixes rust-lang#124396
cc `@workingjubilee`
r? `@estebank`
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored May 28, 2024
2 parents d86e122 + c7d3004 commit 3f4e64c
Show file tree
Hide file tree
Showing 19 changed files with 1,049 additions and 499 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4062,6 +4062,7 @@ dependencies = [
"rustc_feature",
"rustc_fluent_macro",
"rustc_hir",
"rustc_hir_pretty",
"rustc_index",
"rustc_infer",
"rustc_macros",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ rustc_errors = { path = "../rustc_errors" }
rustc_feature = { path = "../rustc_feature" }
rustc_fluent_macro = { path = "../rustc_fluent_macro" }
rustc_hir = { path = "../rustc_hir" }
rustc_hir_pretty = { path = "../rustc_hir_pretty" }
rustc_index = { path = "../rustc_index" }
rustc_infer = { path = "../rustc_infer" }
rustc_macros = { path = "../rustc_macros" }
Expand Down
29 changes: 19 additions & 10 deletions compiler/rustc_lint/messages.ftl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,17 +542,21 @@ lint_non_local_definitions_cargo_update = the {$macro_kind} `{$macro_name}` may
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>
lint_non_local_definitions_impl = non-local `impl` definition, they should be avoided as they go against expectation
.help =
move this `impl` block outside the of the current {$body_kind_descr} {$depth ->
[one] `{$body_name}`
*[other] `{$body_name}` and up {$depth} bodies
}
.non_local = an `impl` definition is non-local if it is nested inside an item and may impact type checking outside of that item. This can be the case if neither the trait or the self type are at the same nesting level as the `impl`
.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
lint_non_local_definitions_impl = non-local `impl` definition, `impl` blocks should be written at the same level as their item
.remove_help = remove `{$may_remove_part}` to make the `impl` local
.without_trait = methods and associated constants are still usable outside the current expression, only `impl Local` and `impl dyn Local` can ever be private, and only if the type is nested in the same item as the `impl`
.with_trait = an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
.bounds = `impl` may be usable in bounds, etc. from outside the expression, which might e.g. make something constructible that previously wasn't, because it's still on a publicly-visible type
.exception = items in an anonymous const item (`const _: () = {"{"} ... {"}"}`) are treated as in the same scope as the anonymous const's declaration
.const_anon = use a const-anon item to suppress this lint
lint_non_local_definitions_macro_rules = non-local `macro_rules!` definition, they should be avoided as they go against expectation
lint_non_local_definitions_impl_move_help =
move the `impl` block outside of this {$body_kind_descr} {$depth ->
[one] `{$body_name}`
*[other] `{$body_name}` and up {$depth} bodies
}
lint_non_local_definitions_macro_rules = non-local `macro_rules!` definition, `#[macro_export]` macro should be written at top level module
.help =
remove the `#[macro_export]` or move this `macro_rules!` outside the of the current {$body_kind_descr} {$depth ->
[one] `{$body_name}`
Expand All @@ -561,7 +565,12 @@ lint_non_local_definitions_macro_rules = non-local `macro_rules!` definition, th
.help_doctest =
remove the `#[macro_export]` or make this doc-test a standalone test with its own `fn main() {"{"} ... {"}"}`
.non_local = a `macro_rules!` definition is non-local if it is nested inside an item and has a `#[macro_export]` attribute
.exception = one exception to the rule are anon-const (`const _: () = {"{"} ... {"}"}`) at top-level module
lint_non_local_definitions_may_move = may need to be moved as well
lint_non_local_definitions_of_trait_not_local = `{$of_trait_str}` is not local
lint_non_local_definitions_self_ty_not_local = `{$self_ty_str}` is not local
lint_non_snake_case = {$sort} `{$name}` should have a snake case name
.rename_or_convert_suggestion = rename the identifier or convert it to a snake case raw identifier
Expand Down
122 changes: 104 additions & 18 deletions compiler/rustc_lint/src/lints.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::errors::RequestedLevel;
use crate::fluent_generated as fluent;
use rustc_errors::{
codes::*, Applicability, Diag, DiagArgValue, DiagMessage, DiagStyledString,
ElidedLifetimeInPathSubdiag, EmissionGuarantee, LintDiagnostic, SubdiagMessageOp,
ElidedLifetimeInPathSubdiag, EmissionGuarantee, LintDiagnostic, MultiSpan, SubdiagMessageOp,
Subdiagnostic, SuggestionStyle,
};
use rustc_hir::{def::Namespace, def_id::DefId};
Expand Down Expand Up @@ -1329,40 +1329,126 @@ pub struct SuspiciousDoubleRefCloneDiag<'a> {
}

// non_local_defs.rs
#[derive(LintDiagnostic)]
pub enum NonLocalDefinitionsDiag {
#[diag(lint_non_local_definitions_impl)]
#[help]
#[note(lint_non_local)]
#[note(lint_exception)]
#[note(lint_non_local_definitions_deprecation)]
Impl {
depth: u32,
body_kind_descr: &'static str,
body_name: String,
#[subdiagnostic]
cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
#[suggestion(lint_const_anon, code = "_", applicability = "machine-applicable")]
const_anon: Option<Span>,
const_anon: Option<Option<Span>>,
move_to: Option<(Span, Vec<Span>)>,
may_remove: Option<(Span, String)>,
has_trait: bool,
self_ty_str: String,
of_trait_str: Option<String>,
},
#[diag(lint_non_local_definitions_macro_rules)]
MacroRules {
depth: u32,
body_kind_descr: &'static str,
body_name: String,
#[help]
help: Option<()>,
#[help(lint_help_doctest)]
doctest_help: Option<()>,
#[note(lint_non_local)]
#[note(lint_exception)]
#[note(lint_non_local_definitions_deprecation)]
notes: (),
#[subdiagnostic]
cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
},
}

impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
fn decorate_lint<'b>(self, diag: &'b mut Diag<'a, ()>) {
match self {
NonLocalDefinitionsDiag::Impl {
depth,
body_kind_descr,
body_name,
cargo_update,
const_anon,
move_to,
may_remove,
has_trait,
self_ty_str,
of_trait_str,
} => {
diag.primary_message(fluent::lint_non_local_definitions_impl);
diag.arg("depth", depth);
diag.arg("body_kind_descr", body_kind_descr);
diag.arg("body_name", body_name);
diag.arg("self_ty_str", self_ty_str);
if let Some(of_trait_str) = of_trait_str {
diag.arg("of_trait_str", of_trait_str);
}

if has_trait {
diag.note(fluent::lint_bounds);
diag.note(fluent::lint_with_trait);
} else {
diag.note(fluent::lint_without_trait);
}

if let Some((move_help, may_move)) = move_to {
let mut ms = MultiSpan::from_span(move_help);
for sp in may_move {
ms.push_span_label(sp, fluent::lint_non_local_definitions_may_move);
}
diag.span_help(ms, fluent::lint_non_local_definitions_impl_move_help);
}

if let Some((span, part)) = may_remove {
diag.arg("may_remove_part", part);
diag.span_suggestion(
span,
fluent::lint_remove_help,
"",
Applicability::MaybeIncorrect,
);
}

if let Some(cargo_update) = cargo_update {
diag.subdiagnostic(&diag.dcx, cargo_update);
}
if let Some(const_anon) = const_anon {
diag.note(fluent::lint_exception);
if let Some(const_anon) = const_anon {
diag.span_suggestion(
const_anon,
fluent::lint_const_anon,
"_",
Applicability::MachineApplicable,
);
}
}

diag.note(fluent::lint_non_local_definitions_deprecation);
}
NonLocalDefinitionsDiag::MacroRules {
depth,
body_kind_descr,
body_name,
help,
doctest_help,
cargo_update,
} => {
diag.primary_message(fluent::lint_non_local_definitions_macro_rules);
diag.arg("depth", depth);
diag.arg("body_kind_descr", body_kind_descr);
diag.arg("body_name", body_name);

if let Some(()) = help {
diag.help(fluent::lint_help);
}
if let Some(()) = doctest_help {
diag.help(fluent::lint_help_doctest);
}

diag.note(fluent::lint_non_local);
diag.note(fluent::lint_non_local_definitions_deprecation);

if let Some(cargo_update) = cargo_update {
diag.subdiagnostic(&diag.dcx, cargo_update);
}
}
}
}
}

#[derive(Subdiagnostic)]
#[note(lint_non_local_definitions_cargo_update)]
pub struct NonLocalDefinitionsCargoUpdateNote {
Expand Down
Loading

0 comments on commit 3f4e64c

Please sign in to comment.