Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Misc improvements to non local defs lint implementation #121813

Merged
merged 4 commits into from
Mar 9, 2024
Merged

Misc improvements to non local defs lint implementation #121813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c87ca2
Add some weird test cases to the non_local_definitions lint tests
Urgau Feb 29, 2024
20200f6
Remove useless smallvec dependency in rustc_lint::non_local_def
Urgau Feb 29, 2024
6c4eadd
Add early-return when checking if a path is local
Urgau Feb 29, 2024
98dbe9a
Use was_invoked_from_cargo method instead of hand-written one
Urgau Feb 29, 2024
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
1 change: 0 additions & 1 deletion Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4133,7 +4133,6 @@ dependencies = [
"rustc_target",
"rustc_trait_selection",
"rustc_type_ir",
"smallvec",
"tracing",
"unicode-security",
]
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_lint/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ rustc_span = { path = "../rustc_span" }
rustc_target = { path = "../rustc_target" }
rustc_trait_selection = { path = "../rustc_trait_selection" }
rustc_type_ir = { path = "../rustc_type_ir" }
smallvec = { version = "1.8.1", features = ["union", "may_dangle"] }
tracing = "0.1"
unicode-security = "0.1.0"
# tidy-alphabetical-end
34 changes: 21 additions & 13 deletions compiler/rustc_lint/src/non_local_def.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ use rustc_hir::{def::DefKind, Body, Item, ItemKind, Node, Path, QPath, TyKind};
use rustc_span::def_id::{DefId, LOCAL_CRATE};
use rustc_span::{sym, symbol::kw, ExpnKind, MacroKind};

use smallvec::{smallvec, SmallVec};

use crate::lints::{NonLocalDefinitionsCargoUpdateNote, NonLocalDefinitionsDiag};
use crate::{LateContext, LateLintPass, LintContext};

Expand Down Expand Up @@ -85,7 +83,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
if let Some(def_id) = oexpn.macro_def_id
&& let ExpnKind::Macro(macro_kind, macro_name) = oexpn.kind
&& def_id.krate != LOCAL_CRATE
&& std::env::var_os("CARGO").is_some()
&& rustc_session::utils::was_invoked_from_cargo()
{
Some(NonLocalDefinitionsCargoUpdateNote {
macro_kind: macro_kind.descr(),
Expand Down Expand Up @@ -114,25 +112,25 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
// is using local items and so we don't lint on it.

// We also ignore anon-const in item by including the anon-const
// parent as well; and since it's quite uncommon, we use smallvec
// to avoid unnecessary heap allocations.
let local_parents: SmallVec<[DefId; 1]> = if parent_def_kind == DefKind::Const
// parent as well.
let parent_parent = if parent_def_kind == DefKind::Const
&& parent_opt_item_name == Some(kw::Underscore)
{
smallvec![parent, cx.tcx.parent(parent)]
Some(cx.tcx.parent(parent))
} else {
smallvec![parent]
None
};

let self_ty_has_local_parent = match impl_.self_ty.kind {
TyKind::Path(QPath::Resolved(_, ty_path)) => {
path_has_local_parent(ty_path, cx, &*local_parents)
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,
&*local_parents,
parent,
parent_parent,
)
}
TyKind::TraitObject([], _, _)
Expand All @@ -154,7 +152,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {

let of_trait_has_local_parent = impl_
.of_trait
.map(|of_trait| path_has_local_parent(of_trait.path, cx, &*local_parents))
.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
Expand Down Expand Up @@ -218,6 +216,16 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
/// std::convert::PartialEq<Foo<Bar>>
/// ^^^^^^^^^^^^^^^^^^^^^^^
/// ```
fn path_has_local_parent(path: &Path<'_>, cx: &LateContext<'_>, local_parents: &[DefId]) -> bool {
path.res.opt_def_id().is_some_and(|did| local_parents.contains(&cx.tcx.parent(did)))
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
}
})
}
22 changes: 21 additions & 1 deletion tests/ui/lint/non_local_definitions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//@ check-pass
//@ edition:2021
//@ aux-build:non_local_macro.rs
//@ rustc-env:CARGO=/usr/bin/cargo
//@ rustc-env:CARGO_CRATE_NAME=non_local_def

#![feature(inline_const)]
#![warn(non_local_definitions)]
Expand Down Expand Up @@ -245,6 +245,26 @@ fn bad() {
//~^ WARN non-local `impl` definition
}

trait Uto9 {}
trait Uto10 {}
const _: u32 = {
let _a = || {
impl Uto9 for Test {}
//~^ WARN non-local `impl` definition

1
};

type A = [u32; {
impl Uto10 for Test {}
//~^ WARN non-local `impl` definition

1
}];

1
};

struct UwU<T>(T);

fn fun() {
Expand Down
50 changes: 36 additions & 14 deletions tests/ui/lint/non_local_definitions.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,29 @@ LL | impl<T> Uto8 for T {}
= 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:253:5
--> $DIR/non_local_definitions.rs:252:9
|
LL | impl Uto9 for Test {}
| ^^^^^^^^^^^^^^^^^^^^^
|
= help: move this `impl` block outside the of the current closure `<unnameable>` and up 2 bodies
= 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:259:9
|
LL | impl Uto10 for Test {}
| ^^^^^^^^^^^^^^^^^^^^^^
|
= help: move this `impl` block outside the of the current constant expression `<unnameable>` and up 2 bodies
= 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:273:5
|
LL | / impl Default for UwU<OwO> {
LL | |
Expand All @@ -458,7 +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:264:5
--> $DIR/non_local_definitions.rs:284:5
|
LL | / impl From<Cat> for () {
LL | |
Expand All @@ -474,7 +496,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:273:5
--> $DIR/non_local_definitions.rs:293:5
|
LL | / impl AsRef<Cat> for () {
LL | |
Expand All @@ -488,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:284:5
--> $DIR/non_local_definitions.rs:304:5
|
LL | / impl PartialEq<B> for G {
LL | |
Expand All @@ -504,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:301:5
--> $DIR/non_local_definitions.rs:321:5
|
LL | / impl PartialEq<Dog> for &Dog {
LL | |
Expand All @@ -520,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:308:5
--> $DIR/non_local_definitions.rs:328:5
|
LL | / impl PartialEq<()> for Dog {
LL | |
Expand All @@ -536,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:315:5
--> $DIR/non_local_definitions.rs:335:5
|
LL | / impl PartialEq<()> for &Dog {
LL | |
Expand All @@ -552,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:322:5
--> $DIR/non_local_definitions.rs:342:5
|
LL | / impl PartialEq<Dog> for () {
LL | |
Expand All @@ -568,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:344:5
--> $DIR/non_local_definitions.rs:364:5
|
LL | / impl From<Wrap<Wrap<Lion>>> for () {
LL | |
Expand All @@ -584,7 +606,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:351:5
--> $DIR/non_local_definitions.rs:371:5
|
LL | / impl From<()> for Wrap<Lion> {
LL | |
Expand All @@ -600,7 +622,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:13
--> $DIR/non_local_definitions.rs:384:13
|
LL | impl MacroTrait for OutsideStruct {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -615,7 +637,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:374:1
--> $DIR/non_local_definitions.rs:394:1
|
LL | non_local_macro::non_local_impl!(CargoUpdate);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -628,7 +650,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:377:1
--> $DIR/non_local_definitions.rs:397:1
|
LL | non_local_macro::non_local_macro_rules!(my_macro);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -640,5 +662,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: 50 warnings emitted
warning: 52 warnings emitted

Loading