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

Suggest using a standalone doctest for non-local impl defs #126422

Merged
merged 2 commits into from
Jun 19, 2024
Merged

Suggest using a standalone doctest for non-local impl defs #126422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab0e727
Suggest standalone doctest for non-local impl defs
Urgau Jun 13, 2024
94c2821
Also simplify macro_rules doctest code
Urgau Jun 13, 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: 1 addition & 0 deletions compiler/rustc_lint/messages.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
18 changes: 10 additions & 8 deletions compiler/rustc_lint/src/lints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1358,6 +1358,7 @@ pub enum NonLocalDefinitionsDiag {
cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
const_anon: Option<Option<Span>>,
move_to: Option<(Span, Vec<Span>)>,
doctest: bool,
may_remove: Option<(Span, String)>,
has_trait: bool,
self_ty_str: String,
Expand All @@ -1367,8 +1368,7 @@ pub enum NonLocalDefinitionsDiag {
depth: u32,
body_kind_descr: &'static str,
body_name: String,
help: Option<()>,
doctest_help: Option<()>,
doctest: bool,
cargo_update: Option<NonLocalDefinitionsCargoUpdateNote>,
},
}
Expand All @@ -1383,6 +1383,7 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
cargo_update,
const_anon,
move_to,
doctest,
may_remove,
has_trait,
self_ty_str,
Expand Down Expand Up @@ -1411,6 +1412,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);
Expand Down Expand Up @@ -1443,20 +1447,18 @@ impl<'a> LintDiagnostic<'a, ()> for NonLocalDefinitionsDiag {
depth,
body_kind_descr,
body_name,
help,
doctest_help,
doctest,
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 {
if doctest {
diag.help(fluent::lint_help_doctest);
} else {
diag.help(fluent::lint_help);
}

diag.note(fluent::lint_non_local);
Expand Down
83 changes: 45 additions & 38 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 @@ -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:
Expand Down Expand Up @@ -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<Span> = 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);

Expand Down Expand Up @@ -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<Span> = 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(
Expand All @@ -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(),
},
Expand All @@ -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,
Expand All @@ -296,8 +304,7 @@ impl<'tcx> LateLintPass<'tcx> for NonLocalDefinitions {
.map(|s| s.to_ident_string())
.unwrap_or_else(|| "<unnameable>".to_string()),
cargo_update: cargo_update(),
help: (!is_at_toplevel_doctest).then_some(()),
doctest_help: is_at_toplevel_doctest.then_some(()),
doctest: is_at_toplevel_doctest(),
},
)
}
Expand Down
1 change: 1 addition & 0 deletions tests/rustdoc-ui/doctest/auxiliary/pub_trait.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub trait Trait {}
31 changes: 31 additions & 0 deletions tests/rustdoc-ui/doctest/non-local-defs-impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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() {}
37 changes: 37 additions & 0 deletions tests/rustdoc-ui/doctest/non-local-defs-impl.stdout
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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() { ... }`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the real difference, before it generated something like this:

help: move the `impl` block outside of this function `_doctest_main_src_lib_rs_3_0` and up 2 bodies
 --> src/lib.rs:4:38
  |
4 | fn main() { #[allow(non_snake_case)] fn _doctest_main_src_lib_rs_3_0() {
  |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
6 | struct Local;
  | ------------ may need to be moved as well

= 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>
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

Loading