diff options
| -rw-r--r-- | compiler/rustc_lint/src/non_local_def.rs | 205 | ||||
| -rw-r--r-- | tests/ui/lint/non_local_definitions.rs | 88 | ||||
| -rw-r--r-- | tests/ui/lint/non_local_definitions.stderr | 99 |
3 files changed, 326 insertions, 66 deletions
diff --git a/compiler/rustc_lint/src/non_local_def.rs b/compiler/rustc_lint/src/non_local_def.rs index 7c4d92d3ce0..80e2c655203 100644 --- a/compiler/rustc_lint/src/non_local_def.rs +++ b/compiler/rustc_lint/src/non_local_def.rs @@ -1,6 +1,18 @@ -use rustc_hir::{def::DefKind, Body, Item, ItemKind, Node, Path, QPath, TyKind}; +use rustc_hir::{def::DefKind, Body, Item, ItemKind, Node, TyKind}; +use rustc_hir::{Path, QPath}; +use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind}; +use rustc_infer::infer::InferCtxt; +use rustc_infer::traits::{Obligation, ObligationCause}; +use rustc_middle::query::Key; +use rustc_middle::ty::{self, Binder, Ty, TyCtxt, TypeFoldable, TypeFolder}; +use rustc_middle::ty::{EarlyBinder, TraitRef, TypeSuperFoldable}; use rustc_span::def_id::{DefId, LOCAL_CRATE}; +use rustc_span::Span; use rustc_span::{sym, symbol::kw, ExpnKind, MacroKind}; +use rustc_trait_selection::infer::TyCtxtInferExt; +use rustc_trait_selection::traits::error_reporting::ambiguity::{ + compute_applicable_impls_for_diagnostics, CandidateSource, +}; use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag}; use crate::{LateContext, LateLintPass, LintContext}; @@ -66,7 +78,8 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { return; } - let parent = cx.tcx.parent(item.owner_id.def_id.into()); + let def_id = item.owner_id.def_id.into(); + let parent = cx.tcx.parent(def_id); let parent_def_kind = cx.tcx.def_kind(parent); let parent_opt_item_name = cx.tcx.opt_item_name(parent); @@ -121,6 +134,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { None }; + // 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) @@ -150,41 +164,70 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { | TyKind::Err(_) => false, }; + if self_ty_has_local_parent { + return; + } + + // Part 2: Is the Trait local? let of_trait_has_local_parent = impl_ .of_trait .map(|of_trait| path_has_local_parent(of_trait.path, cx, parent, parent_parent)) .unwrap_or(false); - // If none of them have a local parent (LOGICAL NOR) this means that - // this impl definition is a non-local definition and so we lint on it. - if !(self_ty_has_local_parent || of_trait_has_local_parent) { - let const_anon = if self.body_depth == 1 - && parent_def_kind == DefKind::Const - && parent_opt_item_name != Some(kw::Underscore) - && let Some(parent) = parent.as_local() - && let Node::Item(item) = cx.tcx.hir_node_by_def_id(parent) - && let ItemKind::Const(ty, _, _) = item.kind - && let TyKind::Tup(&[]) = ty.kind - { - Some(item.ident.span) - } else { - None - }; - - cx.emit_span_lint( - NON_LOCAL_DEFINITIONS, - item.span, - NonLocalDefinitionsDiag::Impl { - depth: self.body_depth, - body_kind_descr: cx.tcx.def_kind_descr(parent_def_kind, parent), - body_name: parent_opt_item_name - .map(|s| s.to_ident_string()) - .unwrap_or_else(|| "<unnameable>".to_string()), - cargo_update: cargo_update(), - const_anon, - }, - ) + if of_trait_has_local_parent { + return; + } + + // Part 3: Is the impl definition leaking outside it's defining scope? + // + // We always consider inherent impls to be leaking. + let impl_has_enough_non_local_candidates = cx + .tcx + .impl_trait_ref(def_id) + .map(|binder| { + impl_trait_ref_has_enough_non_local_candidates( + cx.tcx, + item.span, + def_id, + binder, + |did| did_has_local_parent(did, cx.tcx, parent, parent_parent), + ) + }) + .unwrap_or(false); + + if impl_has_enough_non_local_candidates { + return; } + + // Get the span of the parent const item ident (if it's a not a const anon). + // + // Used to suggest changing the const item to a const anon. + let span_for_const_anon_suggestion = if self.body_depth == 1 + && parent_def_kind == DefKind::Const + && parent_opt_item_name != Some(kw::Underscore) + && let Some(parent) = parent.as_local() + && let Node::Item(item) = cx.tcx.hir_node_by_def_id(parent) + && let ItemKind::Const(ty, _, _) = item.kind + && let TyKind::Tup(&[]) = ty.kind + { + Some(item.ident.span) + } else { + None + }; + + cx.emit_span_lint( + NON_LOCAL_DEFINITIONS, + item.span, + NonLocalDefinitionsDiag::Impl { + depth: self.body_depth, + body_kind_descr: cx.tcx.def_kind_descr(parent_def_kind, parent), + body_name: parent_opt_item_name + .map(|s| s.to_ident_string()) + .unwrap_or_else(|| "<unnameable>".to_string()), + cargo_update: cargo_update(), + const_anon: span_for_const_anon_suggestion, + }, + ) } ItemKind::Macro(_macro, MacroKind::Bang) if cx.tcx.has_attr(item.owner_id.def_id, sym::macro_export) => @@ -207,6 +250,81 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { } } +// Detecting if the impl definition is leaking outside of it's defining scope. +// +// Rule: for each impl, instantiate all local types with inference vars and +// then assemble candidates for that goal, if there are more than 1 (non-private +// impls), it does not leak. +// +// https://github.com/rust-lang/rust/issues/121621#issuecomment-1976826895 +fn impl_trait_ref_has_enough_non_local_candidates<'tcx>( + tcx: TyCtxt<'tcx>, + infer_span: Span, + trait_def_id: DefId, + binder: EarlyBinder<TraitRef<'tcx>>, + mut did_has_local_parent: impl FnMut(DefId) -> bool, +) -> bool { + let infcx = tcx.infer_ctxt().build(); + let trait_ref = binder.instantiate(tcx, infcx.fresh_args_for_item(infer_span, trait_def_id)); + + let trait_ref = trait_ref.fold_with(&mut ReplaceLocalTypesWithInfer { + infcx: &infcx, + infer_span, + did_has_local_parent: &mut did_has_local_parent, + }); + + let poly_trait_obligation = Obligation::new( + tcx, + ObligationCause::dummy(), + ty::ParamEnv::empty(), + Binder::dummy(trait_ref), + ); + + let ambiguities = compute_applicable_impls_for_diagnostics(&infcx, &poly_trait_obligation); + + let mut it = ambiguities.iter().filter(|ambi| match ambi { + CandidateSource::DefId(did) => !did_has_local_parent(*did), + CandidateSource::ParamEnv(_) => unreachable!(), + }); + + let _ = it.next(); + it.next().is_some() +} + +/// Replace every local type by inference variable. +/// +/// ```text +/// <Global<Local> as std::cmp::PartialEq<Global<Local>>> +/// to +/// <Global<_> as std::cmp::PartialEq<Global<_>>> +/// ``` +struct ReplaceLocalTypesWithInfer<'a, 'tcx, F: FnMut(DefId) -> bool> { + infcx: &'a InferCtxt<'tcx>, + did_has_local_parent: F, + infer_span: Span, +} + +impl<'a, 'tcx, F: FnMut(DefId) -> bool> TypeFolder<TyCtxt<'tcx>> + for ReplaceLocalTypesWithInfer<'a, 'tcx, F> +{ + fn interner(&self) -> TyCtxt<'tcx> { + self.infcx.tcx + } + + fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> { + if let Some(ty_did) = t.ty_def_id() + && (self.did_has_local_parent)(ty_did) + { + self.infcx.next_ty_var(TypeVariableOrigin { + kind: TypeVariableOriginKind::TypeInference, + span: self.infer_span, + }) + } else { + t.super_fold_with(self) + } + } +} + /// 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. /// @@ -216,16 +334,29 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { /// std::convert::PartialEq<Foo<Bar>> /// ^^^^^^^^^^^^^^^^^^^^^^^ /// ``` +#[inline] fn path_has_local_parent( path: &Path<'_>, cx: &LateContext<'_>, impl_parent: DefId, impl_parent_parent: Option<DefId>, ) -> bool { - path.res.opt_def_id().is_some_and(|did| { - did.is_local() && { - let res_parent = cx.tcx.parent(did); - res_parent == impl_parent || Some(res_parent) == impl_parent_parent - } - }) + path.res + .opt_def_id() + .is_some_and(|did| did_has_local_parent(did, cx.tcx, impl_parent, impl_parent_parent)) +} + +/// Given a def id and a parent impl def id, this checks if the parent +/// def id correspond to the def id of the parent impl definition. +#[inline] +fn did_has_local_parent( + did: DefId, + tcx: TyCtxt<'_>, + impl_parent: DefId, + impl_parent_parent: Option<DefId>, +) -> bool { + did.is_local() && { + let res_parent = tcx.parent(did); + res_parent == impl_parent || Some(res_parent) == impl_parent_parent + } } diff --git a/tests/ui/lint/non_local_definitions.rs b/tests/ui/lint/non_local_definitions.rs index eee582a6f11..4c4f19dd88d 100644 --- a/tests/ui/lint/non_local_definitions.rs +++ b/tests/ui/lint/non_local_definitions.rs @@ -282,7 +282,6 @@ struct Cat; fn meow() { impl From<Cat> for () { - //~^ WARN non-local `impl` definition fn from(_: Cat) -> () { todo!() } @@ -374,6 +373,72 @@ fn rawr() { todo!() } } + + #[derive(Debug)] + struct Elephant; + + impl From<Wrap<Wrap<Elephant>>> for () { + //~^ WARN non-local `impl` definition + fn from(_: Wrap<Wrap<Elephant>>) -> Self { + todo!() + } + } +} + +pub trait StillNonLocal {} + +impl StillNonLocal for &str {} + +fn only_global() { + struct Foo; + impl StillNonLocal for &Foo {} + //~^ WARN non-local `impl` definition +} + +struct GlobalSameFunction; + +fn same_function() { + struct Local1(GlobalSameFunction); + impl From<Local1> for GlobalSameFunction { + //~^ WARN non-local `impl` definition + fn from(x: Local1) -> GlobalSameFunction { + x.0 + } + } + + struct Local2(GlobalSameFunction); + impl From<Local2> for GlobalSameFunction { + //~^ WARN non-local `impl` definition + fn from(x: Local2) -> GlobalSameFunction { + x.0 + } + } +} + +struct GlobalDifferentFunction; + +fn diff_foo() { + struct Local(GlobalDifferentFunction); + + impl From<Local> for GlobalDifferentFunction { + // FIXME(Urgau): Should warn but doesn't since we currently consider + // the other impl to be "global", but that's not the case for the type-system + fn from(x: Local) -> GlobalDifferentFunction { + x.0 + } + } +} + +fn diff_bar() { + struct Local(GlobalDifferentFunction); + + impl From<Local> for GlobalDifferentFunction { + // FIXME(Urgau): Should warn but doesn't since we currently consider + // the other impl to be "global", but that's not the case for the type-system + fn from(x: Local) -> GlobalDifferentFunction { + x.0 + } + } } macro_rules! m { @@ -404,3 +469,24 @@ fn bitflags() { impl Flags {} }; } + +// https://github.com/rust-lang/rust/issues/121621#issuecomment-1976826895 +fn commonly_reported() { + struct Local(u8); + impl From<Local> for u8 { + fn from(x: Local) -> u8 { + x.0 + } + } +} + +// https://github.com/rust-lang/rust/issues/121621#issue-2153187542 +pub trait Serde {} + +impl Serde for &[u8] {} +impl Serde for &str {} + +fn serde() { + struct Thing; + impl Serde for &Thing {} +} diff --git a/tests/ui/lint/non_local_definitions.stderr b/tests/ui/lint/non_local_definitions.stderr index ef74e262f9d..9bfdec4e588 100644 --- a/tests/ui/lint/non_local_definitions.stderr +++ b/tests/ui/lint/non_local_definitions.stderr @@ -480,23 +480,7 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:284:5 - | -LL | / impl From<Cat> for () { -LL | | -LL | | fn from(_: Cat) -> () { -LL | | todo!() -LL | | } -LL | | } - | |_____^ - | - = help: move this `impl` block outside the of the current function `meow` - = note: 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 - = note: 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 - = note: 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> - -warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:293:5 + --> $DIR/non_local_definitions.rs:292:5 | LL | / impl AsRef<Cat> for () { LL | | @@ -510,7 +494,7 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:304:5 + --> $DIR/non_local_definitions.rs:303:5 | LL | / impl PartialEq<B> for G { LL | | @@ -526,7 +510,7 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:321:5 + --> $DIR/non_local_definitions.rs:320:5 | LL | / impl PartialEq<Dog> for &Dog { LL | | @@ -542,7 +526,7 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:328:5 + --> $DIR/non_local_definitions.rs:327:5 | LL | / impl PartialEq<()> for Dog { LL | | @@ -558,7 +542,7 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:335:5 + --> $DIR/non_local_definitions.rs:334:5 | LL | / impl PartialEq<()> for &Dog { LL | | @@ -574,7 +558,7 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:342:5 + --> $DIR/non_local_definitions.rs:341:5 | LL | / impl PartialEq<Dog> for () { LL | | @@ -590,7 +574,7 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:364:5 + --> $DIR/non_local_definitions.rs:363:5 | LL | / impl From<Wrap<Wrap<Lion>>> for () { LL | | @@ -606,7 +590,7 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:371:5 + --> $DIR/non_local_definitions.rs:370:5 | LL | / impl From<()> for Wrap<Lion> { LL | | @@ -622,7 +606,66 @@ LL | | } = note: 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> warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:384:13 + --> $DIR/non_local_definitions.rs:380:5 + | +LL | / impl From<Wrap<Wrap<Elephant>>> for () { +LL | | +LL | | fn from(_: Wrap<Wrap<Elephant>>) -> Self { +LL | | todo!() +LL | | } +LL | | } + | |_____^ + | + = help: move this `impl` block outside the of the current function `rawr` + = note: 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 + = note: 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 + = note: 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> + +warning: non-local `impl` definition, they should be avoided as they go against expectation + --> $DIR/non_local_definitions.rs:394:5 + | +LL | impl StillNonLocal for &Foo {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: move this `impl` block outside the of the current function `only_global` + = note: 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 + = note: 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 + = note: 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> + +warning: non-local `impl` definition, they should be avoided as they go against expectation + --> $DIR/non_local_definitions.rs:402:5 + | +LL | / impl From<Local1> for GlobalSameFunction { +LL | | +LL | | fn from(x: Local1) -> GlobalSameFunction { +LL | | x.0 +LL | | } +LL | | } + | |_____^ + | + = help: move this `impl` block outside the of the current function `same_function` + = note: 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 + = note: 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 + = note: 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> + +warning: non-local `impl` definition, they should be avoided as they go against expectation + --> $DIR/non_local_definitions.rs:410:5 + | +LL | / impl From<Local2> for GlobalSameFunction { +LL | | +LL | | fn from(x: Local2) -> GlobalSameFunction { +LL | | x.0 +LL | | } +LL | | } + | |_____^ + | + = help: move this `impl` block outside the of the current function `same_function` + = note: 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 + = note: 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 + = note: 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> + +warning: non-local `impl` definition, they should be avoided as they go against expectation + --> $DIR/non_local_definitions.rs:449:13 | LL | impl MacroTrait for OutsideStruct {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -637,7 +680,7 @@ LL | m!(); = note: this warning originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info) warning: non-local `impl` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:394:1 + --> $DIR/non_local_definitions.rs:459:1 | LL | non_local_macro::non_local_impl!(CargoUpdate); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -650,7 +693,7 @@ LL | non_local_macro::non_local_impl!(CargoUpdate); = note: this warning originates in the macro `non_local_macro::non_local_impl` (in Nightly builds, run with -Z macro-backtrace for more info) warning: non-local `macro_rules!` definition, they should be avoided as they go against expectation - --> $DIR/non_local_definitions.rs:397:1 + --> $DIR/non_local_definitions.rs:462:1 | LL | non_local_macro::non_local_macro_rules!(my_macro); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -662,5 +705,5 @@ LL | non_local_macro::non_local_macro_rules!(my_macro); = note: the macro `non_local_macro::non_local_macro_rules` may come from an old version of the `non_local_macro` crate, try updating your dependency with `cargo update -p non_local_macro` = note: this warning originates in the macro `non_local_macro::non_local_macro_rules` (in Nightly builds, run with -Z macro-backtrace for more info) -warning: 52 warnings emitted +warning: 55 warnings emitted |