Skip to content

Improve diagnostic output of non_local_definitions lint #125089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 28, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
non_local_defs: suggest removing leading ref/ptr to make the impl local
  • Loading branch information
Urgau committed May 27, 2024

Verified

This commit was signed with the committer’s verified signature.
commit b71952904df8b48eb78ba54a1706680c722cb2cb
3 changes: 2 additions & 1 deletion compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
@@ -543,11 +543,12 @@ 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, `impl` blocks should be written at the same level as their item
.help =
.move_help =
move this `impl` block outside of the current {$body_kind_descr} {$depth ->
[one] `{$body_name}`
*[other] `{$body_name}` and up {$depth} bodies
}
.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
14 changes: 13 additions & 1 deletion compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
@@ -1338,6 +1338,7 @@ pub enum NonLocalDefinitionsDiag {
const_anon: Option<Option<Span>>,
move_help: Span,
may_move: Vec<Span>,
may_remove: Option<(Span, String)>,
has_trait: bool,
},
MacroRules {
@@ -1361,6 +1362,7 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
const_anon,
move_help,
may_move,
may_remove,
has_trait,
} => {
diag.primary_message(fluent::lint_non_local_definitions_impl);
@@ -1379,7 +1381,17 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
for sp in may_move {
ms.push_span_label(sp, fluent::lint_non_local_definitions_may_move);
}
diag.span_help(ms, fluent::lint_help);
diag.span_help(ms, fluent::lint_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);
80 changes: 51 additions & 29 deletions compiler/rustc_lint/src/non_local_def.rs
Original file line number Diff line number Diff line change
@@ -136,35 +136,8 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
};

// Part 1: Is the Self type local?
let self_ty_has_local_parent = match impl_.self_ty.kind {
TyKind::Path(QPath::Resolved(_, ty_path)) => {
path_has_local_parent(ty_path, cx, parent, parent_parent)
}
TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => {
path_has_local_parent(
principle_poly_trait_ref.trait_ref.path,
cx,
parent,
parent_parent,
)
}
TyKind::TraitObject([], _, _)
| TyKind::InferDelegation(_, _)
| TyKind::Slice(_)
| TyKind::Array(_, _)
| TyKind::Ptr(_)
| TyKind::Ref(_, _)
| TyKind::BareFn(_)
| TyKind::Never
| TyKind::Tup(_)
| TyKind::Path(_)
| TyKind::Pat(..)
| TyKind::AnonAdt(_)
| TyKind::OpaqueDef(_, _, _)
| TyKind::Typeof(_)
| TyKind::Infer
| TyKind::Err(_) => false,
};
let self_ty_has_local_parent =
ty_has_local_parent(&impl_.self_ty.kind, cx, parent, parent_parent);

if self_ty_has_local_parent {
return;
@@ -242,6 +215,18 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
let const_anon = matches!(parent_def_kind, DefKind::Const | DefKind::Static { .. })
.then_some(span_for_const_anon_suggestion);

let may_remove = match &impl_.self_ty.kind {
TyKind::Ptr(mut_ty) | TyKind::Ref(_, mut_ty)
if ty_has_local_parent(&mut_ty.ty.kind, cx, parent, parent_parent) =>
{
let type_ =
if matches!(impl_.self_ty.kind, TyKind::Ptr(_)) { "*" } else { "&" };
let part = format!("{}{}", type_, mut_ty.mutbl.prefix_str());
Some((impl_.self_ty.span.shrink_to_lo().until(mut_ty.ty.span), part))
}
_ => None,
};

cx.emit_span_lint(
NON_LOCAL_DEFINITIONS,
item.span.shrink_to_lo().to(impl_.self_ty.span),
@@ -255,6 +240,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
cargo_update: cargo_update(),
const_anon,
may_move,
may_remove,
has_trait: impl_.of_trait.is_some(),
},
)
@@ -384,6 +370,42 @@ impl<'tcx> Visitor<'tcx> for PathCollector<'tcx> {
}
}

/// Given a `Ty` we check if the (outermost) type is local.
fn ty_has_local_parent(
ty_kind: &TyKind<'_>,
cx: &LateContext<'_>,
impl_parent: DefId,
impl_parent_parent: Option<DefId>,
) -> bool {
match ty_kind {
TyKind::Path(QPath::Resolved(_, ty_path)) => {
path_has_local_parent(ty_path, cx, impl_parent, impl_parent_parent)
}
TyKind::TraitObject([principle_poly_trait_ref, ..], _, _) => path_has_local_parent(
principle_poly_trait_ref.trait_ref.path,
cx,
impl_parent,
impl_parent_parent,
),
TyKind::TraitObject([], _, _)
| TyKind::InferDelegation(_, _)
| TyKind::Slice(_)
| TyKind::Array(_, _)
| TyKind::Ptr(_)
| TyKind::Ref(_, _)
| TyKind::BareFn(_)
| TyKind::Never
| TyKind::Tup(_)
| TyKind::Path(_)
| TyKind::Pat(..)
| TyKind::AnonAdt(_)
| TyKind::OpaqueDef(_, _, _)
| TyKind::Typeof(_)
| TyKind::Infer
| TyKind::Err(_) => false,
}
}

/// Given a path and a parent impl def id, this checks if the if parent resolution
/// def id correspond to the def id of the parent impl definition.
///
4 changes: 3 additions & 1 deletion tests/ui/lint/non-local-defs/exhaustive.stderr
Original file line number Diff line number Diff line change
@@ -189,7 +189,9 @@ warning: non-local `impl` definition, `impl` blocks should be written at the sam
--> $DIR/exhaustive.rs:58:5
|
LL | impl Trait for *mut InsideMain {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^-----^^^^^^^^^^
| |
| help: remove `*mut ` to make the `impl` local
|
= note: `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
= note: 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`
4 changes: 3 additions & 1 deletion tests/ui/lint/non-local-defs/from-local-for-global.stderr
Original file line number Diff line number Diff line change
@@ -46,7 +46,9 @@ warning: non-local `impl` definition, `impl` blocks should be written at the sam
--> $DIR/from-local-for-global.rs:32:5
|
LL | impl StillNonLocal for &Foo {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^-^^^
| |
| help: remove `&` to make the `impl` local
|
= note: `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
= note: 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`
Original file line number Diff line number Diff line change
@@ -2,7 +2,9 @@ warning: non-local `impl` definition, `impl` blocks should be written at the sam
--> $DIR/trait-solver-overflow-123573.rs:12:5
|
LL | impl Test for &Local {}
| ^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^-^^^^^
| |
| help: remove `&` to make the `impl` local
|
= note: `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
= note: 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`