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.

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

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jun 13, 2024

This PR tweaks the lint output of the non_local_definitions lint to suggest using a standalone doctest instead of a moving the impl def to an impossible place as was already done with macro_rules! case in #124568.

Fixes #126339
r? @fmease

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 13, 2024
|
= 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

@rust-log-analyzer

This comment has been minimized.

@lqd
Copy link
Member

lqd commented Jun 13, 2024

I wonder, would it be beneficial/possible to have a machine-applicable suggestion here?

@Urgau Urgau force-pushed the doctest-impl-non-local-def branch from 3c57761 to c44d3cb Compare June 13, 2024 18:51
@Urgau
Copy link
Member Author

Urgau commented Jun 13, 2024

I wonder, would it be beneficial/possible to have a machine-applicable suggestion here?

Beneficial, yes. Possible, not currently, since when compiling doctests rustc only has access to the prepared test1 by rustdoc, rustc doesn't have access to the original source file, never-mind the real spans.

Footnotes

  1. this comment shows part of the test harness generated by rustdoc for the doctest

@rust-log-analyzer

This comment has been minimized.

@lqd
Copy link
Member

lqd commented Jun 13, 2024

(cc #126430 on that last failure)

@Urgau Urgau force-pushed the doctest-impl-non-local-def branch from c44d3cb to 94c2821 Compare June 15, 2024 11:09
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@fmease
Copy link
Member

fmease commented Jun 18, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 18, 2024

📌 Commit 94c2821 has been approved by fmease

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 18, 2024
…r=fmease

Suggest using a standalone doctest for non-local impl defs

This PR tweaks the lint output of the `non_local_definitions` lint to suggest using a standalone doctest instead of a moving the `impl` def to an impossible place as was already done with `macro_rules!` case in rust-lang#124568.

Fixes rust-lang#126339
r? `@fmease`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 18, 2024
…llaumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#123782 (Test that opaque types can't have themselves as a hidden type with incompatible lifetimes)
 - rust-lang#124580 (Suggest removing unused tuple fields if they are the last fields)
 - rust-lang#125852 (improve tip for inaccessible traits)
 - rust-lang#126422 (Suggest using a standalone doctest for non-local impl defs)
 - rust-lang#126427 (Rewrite `intrinsic-unreachable`, `sepcomp-cci-copies`, `sepcomp-inlining` and `sepcomp-separate` `run-make` tests to rmake.rs)
 - rust-lang#126493 (safe transmute: support non-ZST, variantful, uninhabited enums)
 - rust-lang#126572 (override user defined channel when using precompiled rustc)
 - rust-lang#126615 (Add `rustc-ice*` to `.gitignore`)
 - rust-lang#126632 (Replace `move||` with `move ||`)

Failed merges:

 - rust-lang#126558 (hir_typeck: be more conservative in making "note caller chooses ty param" note)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 19, 2024
…r=fmease

Suggest using a standalone doctest for non-local impl defs

This PR tweaks the lint output of the `non_local_definitions` lint to suggest using a standalone doctest instead of a moving the `impl` def to an impossible place as was already done with `macro_rules!` case in rust-lang#124568.

Fixes rust-lang#126339
r? ``@fmease``
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup of 10 pull requests

Successful merges:

 - rust-lang#124135 (delegation: Implement glob delegation)
 - rust-lang#125078 (fix: break inside async closure has incorrect span for enclosing closure)
 - rust-lang#125293 (Place tail expression behind terminating scope)
 - rust-lang#126422 (Suggest using a standalone doctest for non-local impl defs)
 - rust-lang#126493 (safe transmute: support non-ZST, variantful, uninhabited enums)
 - rust-lang#126504 (Sync fuchsia test runner with clang test runner)
 - rust-lang#126558 (hir_typeck: be more conservative in making "note caller chooses ty param" note)
 - rust-lang#126586 (Add `@badboy` and `@BlackHoleFox` as Mac Catalyst maintainers)
 - rust-lang#126615 (Add `rustc-ice*` to `.gitignore`)
 - rust-lang#126632 (Replace `move||` with `move ||`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 081cc5c into rust-lang:master Jun 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup merge of rust-lang#126422 - Urgau:doctest-impl-non-local-def, r=fmease

Suggest using a standalone doctest for non-local impl defs

This PR tweaks the lint output of the `non_local_definitions` lint to suggest using a standalone doctest instead of a moving the `impl` def to an impossible place as was already done with `macro_rules!` case in rust-lang#124568.

Fixes rust-lang#126339
r? ```@fmease```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

non_local_definitions lint triggers when implementing trait for reference in doc test under beta 1.80.0
6 participants