From ab0e72781f6bccb1c033a7c79edaebc6124ac2b5 Mon Sep 17 00:00:00 2001 From: Urgau Date: Thu, 13 Jun 2024 17:08:04 +0200 Subject: [PATCH] Suggest standalone doctest for non-local impl defs --- compiler/rustc_lint/messages.ftl | 1 + compiler/rustc_lint/src/lints.rs | 5 ++ compiler/rustc_lint/src/non_local_def.rs | 84 ++++++++++--------- .../rustdoc-ui/doctest/auxiliary/pub_trait.rs | 1 + .../rustdoc-ui/doctest/non-local-defs-impl.rs | 31 +++++++ .../doctest/non-local-defs-impl.stdout | 37 ++++++++ 6 files changed, 121 insertions(+), 38 deletions(-) create mode 100644 tests/rustdoc-ui/doctest/auxiliary/pub_trait.rs create mode 100644 tests/rustdoc-ui/doctest/non-local-defs-impl.rs create mode 100644 tests/rustdoc-ui/doctest/non-local-defs-impl.stdout diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index 733c73bc3d0..7430f1fc13a 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -548,6 +548,7 @@ lint_non_local_definitions_impl = non-local `impl` definition, `impl` blocks sho .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 + .doctest = make this doc-test a standalone test with its own `fn main() {"{"} ... {"}"}` .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 diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs index b377da31a58..6cc0a81aa4f 100644 --- a/compiler/rustc_lint/src/lints.rs +++ b/compiler/rustc_lint/src/lints.rs @@ -1358,6 +1358,7 @@ pub enum NonLocalDefinitionsDiag { cargo_update: Option, const_anon: Option>, move_to: Option<(Span, Vec)>, + doctest: bool, may_remove: Option<(Span, String)>, has_trait: bool, self_ty_str: String, @@ -1383,6 +1384,7 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag { cargo_update, const_anon, move_to, + doctest, may_remove, has_trait, self_ty_str, @@ -1411,6 +1413,9 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag { } diag.span_help(ms, fluent::lint_non_local_definitions_impl_move_help); } + if doctest { + diag.help(fluent::lint_doctest); + } if let Some((span, part)) = may_remove { diag.arg("may_remove_part", part); diff --git a/compiler/rustc_lint/src/non_local_def.rs b/compiler/rustc_lint/src/non_local_def.rs index d7ffc34d824..423862dcdba 100644 --- a/compiler/rustc_lint/src/non_local_def.rs +++ b/compiler/rustc_lint/src/non_local_def.rs @@ -111,6 +111,12 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { } }; + // determining if we are in a doctest context can't currently be determined + // by the code itself (there are no specific attributes), but fortunately rustdoc + // sets a perma-unstable env var for libtest so we just reuse that for now + let is_at_toplevel_doctest = + || self.body_depth == 2 && std::env::var("UNSTABLE_RUSTDOC_TEST_PATH").is_ok(); + match item.kind { ItemKind::Impl(impl_) => { // The RFC states: @@ -191,29 +197,6 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { None }; - let mut collector = PathCollector { paths: Vec::new() }; - collector.visit_ty(&impl_.self_ty); - if let Some(of_trait) = &impl_.of_trait { - collector.visit_trait_ref(of_trait); - } - collector.visit_generics(&impl_.generics); - - let mut may_move: Vec = collector - .paths - .into_iter() - .filter_map(|path| { - if let Some(did) = path.res.opt_def_id() - && did_has_local_parent(did, cx.tcx, parent, parent_parent) - { - Some(cx.tcx.def_span(did)) - } else { - None - } - }) - .collect(); - may_move.sort(); - may_move.dedup(); - let const_anon = matches!(parent_def_kind, DefKind::Const | DefKind::Static { .. }) .then_some(span_for_const_anon_suggestion); @@ -248,14 +231,44 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { } else { None }; - let move_to = if may_move.is_empty() { - ms.push_span_label( - cx.tcx.def_span(parent), - fluent::lint_non_local_definitions_impl_move_help, - ); - None + + let (doctest, move_to) = if is_at_toplevel_doctest() { + (true, None) } else { - Some((cx.tcx.def_span(parent), may_move)) + let mut collector = PathCollector { paths: Vec::new() }; + collector.visit_ty(&impl_.self_ty); + if let Some(of_trait) = &impl_.of_trait { + collector.visit_trait_ref(of_trait); + } + collector.visit_generics(&impl_.generics); + + let mut may_move: Vec = collector + .paths + .into_iter() + .filter_map(|path| { + if let Some(did) = path.res.opt_def_id() + && did_has_local_parent(did, cx.tcx, parent, parent_parent) + { + Some(cx.tcx.def_span(did)) + } else { + None + } + }) + .collect(); + may_move.sort(); + may_move.dedup(); + + let move_to = if may_move.is_empty() { + ms.push_span_label( + cx.tcx.def_span(parent), + fluent::lint_non_local_definitions_impl_move_help, + ); + None + } else { + Some((cx.tcx.def_span(parent), may_move)) + }; + + (false, move_to) }; cx.emit_span_lint( @@ -272,6 +285,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { self_ty_str, of_trait_str, move_to, + doctest, may_remove, has_trait: impl_.of_trait.is_some(), }, @@ -280,12 +294,6 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { ItemKind::Macro(_macro, MacroKind::Bang) if cx.tcx.has_attr(item.owner_id.def_id, sym::macro_export) => { - // determining we if are in a doctest context can't currently be determined - // by the code it-self (no specific attrs), but fortunatly rustdoc sets a - // perma-unstable env for libtest so we just re-use that env for now - let is_at_toplevel_doctest = - self.body_depth == 2 && std::env::var("UNSTABLE_RUSTDOC_TEST_PATH").is_ok(); - cx.emit_span_lint( NON_LOCAL_DEFINITIONS, item.span, @@ -296,8 +304,8 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions { .map(|s| s.to_ident_string()) .unwrap_or_else(|| "".to_string()), cargo_update: cargo_update(), - help: (!is_at_toplevel_doctest).then_some(()), - doctest_help: is_at_toplevel_doctest.then_some(()), + help: (!is_at_toplevel_doctest()).then_some(()), + doctest_help: is_at_toplevel_doctest().then_some(()), }, ) } diff --git a/tests/rustdoc-ui/doctest/auxiliary/pub_trait.rs b/tests/rustdoc-ui/doctest/auxiliary/pub_trait.rs new file mode 100644 index 00000000000..0a47fdc74d7 --- /dev/null +++ b/tests/rustdoc-ui/doctest/auxiliary/pub_trait.rs @@ -0,0 +1 @@ +pub trait Trait {} diff --git a/tests/rustdoc-ui/doctest/non-local-defs-impl.rs b/tests/rustdoc-ui/doctest/non-local-defs-impl.rs new file mode 100644 index 00000000000..c984e097c04 --- /dev/null +++ b/tests/rustdoc-ui/doctest/non-local-defs-impl.rs @@ -0,0 +1,31 @@ +//@ check-fail +//@ edition:2018 +//@ failure-status: 101 +//@ aux-build:pub_trait.rs +//@ compile-flags: --test --test-args --test-threads=1 +//@ normalize-stdout-test: "tests/rustdoc-ui/doctest" -> "$$DIR" +//@ normalize-stdout-test "finished in \d+\.\d+s" -> "finished in $$TIME" + +#![doc(test(attr(deny(non_local_definitions))))] +#![doc(test(attr(allow(dead_code))))] + +/// This will produce a warning: +/// ```rust,no_run +/// # extern crate pub_trait; +/// # use pub_trait::Trait; +/// +/// struct Local; +/// impl Trait for &Local {} +/// ``` +/// +/// But this shoudln't produce a warning: +/// ```rust,no_run +/// # extern crate pub_trait; +/// # use pub_trait::Trait; +/// +/// struct Local; +/// impl Trait for &Local {} +/// +/// # fn main() {} +/// ``` +pub fn doctest() {} diff --git a/tests/rustdoc-ui/doctest/non-local-defs-impl.stdout b/tests/rustdoc-ui/doctest/non-local-defs-impl.stdout new file mode 100644 index 00000000000..27797e22f8e --- /dev/null +++ b/tests/rustdoc-ui/doctest/non-local-defs-impl.stdout @@ -0,0 +1,37 @@ + +running 2 tests +test $DIR/non-local-defs-impl.rs - doctest (line 13) - compile ... FAILED +test $DIR/non-local-defs-impl.rs - doctest (line 22) - compile ... ok + +failures: + +---- $DIR/non-local-defs-impl.rs - doctest (line 13) stdout ---- +error: non-local `impl` definition, `impl` blocks should be written at the same level as their item + --> $DIR/non-local-defs-impl.rs:18:1 + | +LL | impl Trait for &Local {} + | ^^^^^-----^^^^^------ + | | | + | | `&'_ Local` is not local + | | help: remove `&` to make the `impl` local + | `Trait` is not 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` + = help: make this doc-test a standalone test with its own `fn main() { ... }` + = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue +note: the lint level is defined here + --> $DIR/non-local-defs-impl.rs:11:9 + | +LL | #![deny(non_local_definitions)] + | ^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 1 previous error + +Couldn't compile the test. + +failures: + $DIR/non-local-defs-impl.rs - doctest (line 13) + +test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME +