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

Improve diagnostic output of non_local_definitions lint #125089

Merged
merged 11 commits into from
May 28, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented May 13, 2024

This PR improves (or at least tries to improve) the diagnostic output of non_local_definitions lint, by simplifying the wording, by adding a "sort of" explanation of bounds interaction that leak the impl...

This PR is best reviewed commit by commit and is voluntarily made a bit vague as to have a starting point to improve on.

Related to https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/non_local_defs.20wording.20improvements

Fixes #125068
Fixes #124396
cc @workingjubilee
r? @estebank

@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 May 13, 2024
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the non_local_def-suggestions branch from 0c69265 to 2819950 Compare May 13, 2024 20:20
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

feel free to "nah actually" I am just trying to throw a couple wording ideas at the wall.

compiler/rustc_lint/messages.ftl Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/src/non_local_def.rs Show resolved Hide resolved
@Urgau
Copy link
Member Author

Urgau commented May 14, 2024

I pushed several more commits that tries to improve the clarity the message were are trying to pass, mainly by moving the "move help message" to a separate span with some labels.

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
  --> $DIR/consts.rs:43:5
   |
LL |     impl Test {
   |     ^^^^^^^^^
   |
   = note: methods and assoc const are still usable outside the current expression, only `impl Local` and `impl dyn Local` are local and only if the `Local` type is at the same nesting as the `impl` block
help: move this `impl` block and outside the of the current function `main`
  --> $DIR/consts.rs:43:5
   |
LL |       impl Test {
   |       ^    ---- may need to be moved as well
   |  _____|
   | |
LL | |
LL | |         fn foo() {}
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>

compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Show resolved Hide resolved
@Urgau Urgau force-pushed the non_local_def-suggestions branch from 9a5bb1f to a39cf37 Compare May 14, 2024 21:41
@rustbot
Copy link
Collaborator

rustbot commented May 15, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@Urgau
Copy link
Member Author

Urgau commented May 15, 2024

I continued to improved the diagnostic output by adding labels that points to Self type and Trait to better put emphasis on the fact that they are non-local for the lint.

With the example in #125068 (comment), it now gives:

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
 --> a.rs:6:5
  |
6 |     impl Trait for Option<Local> {}
  |     ^^^^^-----^^^^^------^^^^^^^
  |          |         |
  |          |         `Option` is not 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: move this `impl` block outside of the current function `main`
 --> a.rs:6:5
  |
6 |     impl Trait for Option<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: `#[warn(non_local_definitions)]` on by default
with colors

image

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

There's quite a bit of code that I need to continue reviewing, but I have some nitpicks based on the output.

tests/ui/lint/non-local-defs/weird-exprs.stderr Outdated Show resolved Hide resolved
tests/ui/lint/non-local-defs/weird-exprs.stderr Outdated Show resolved Hide resolved
tests/ui/lint/non-local-defs/suggest-moving-inner.stderr Outdated Show resolved Hide resolved
compiler/rustc_lint/src/non_local_def.rs Show resolved Hide resolved
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2024
@Urgau Urgau force-pushed the non_local_def-suggestions branch from abefe35 to 5044fb6 Compare May 18, 2024 13:10
@Urgau
Copy link
Member Author

Urgau commented May 20, 2024

All the review comments have been addressed or have been replied to.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 20, 2024
@bors
Copy link
Contributor

bors commented May 22, 2024

☔ The latest upstream changes (presumably #124417) made this pull request unmergeable. Please resolve the merge conflicts.

@Urgau Urgau force-pushed the non_local_def-suggestions branch from 5044fb6 to 7236f18 Compare May 22, 2024 05:59
@Urgau
Copy link
Member Author

Urgau commented May 27, 2024

@estebank could you give this PR another review.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Minor nitpick in wording, beyond that this is ready to land.

@Urgau Urgau force-pushed the non_local_def-suggestions branch from 7236f18 to c7d3004 Compare May 27, 2024 22:01
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2024

📌 Commit c7d3004 has been approved by estebank

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 May 28, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 28, 2024
…=estebank

Improve diagnostic output the `non_local_definitions` lint

This PR improves (or at least tries to improve) the diagnostic output the `non_local_definitions` lint, by simplifying the wording, by adding a "sort of" explanation of bounds interaction that leak the impl...

This PR is best reviewed commit by commit and is voluntarily made a bit vague as to have a starting point to improve on.

Related to https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/non_local_defs.20wording.20improvements

Fixes rust-lang#125068
Fixes rust-lang#124396
cc `@workingjubilee`
r? `@estebank`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request May 28, 2024
…=estebank

Improve diagnostic output the `non_local_definitions` lint

This PR improves (or at least tries to improve) the diagnostic output the `non_local_definitions` lint, by simplifying the wording, by adding a "sort of" explanation of bounds interaction that leak the impl...

This PR is best reviewed commit by commit and is voluntarily made a bit vague as to have a starting point to improve on.

Related to https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/non_local_defs.20wording.20improvements

Fixes rust-lang#125068
Fixes rust-lang#124396
cc ``@workingjubilee``
r? ``@estebank``
bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2024
…kingjubilee

Rollup of 4 pull requests

Successful merges:

 - rust-lang#125089 (Improve diagnostic output the `non_local_definitions` lint)
 - rust-lang#125343 (`-Znext-solver`: eagerly normalize when adding goals)
 - rust-lang#125411 (check host's libstdc++ version when using ci llvm)
 - rust-lang#125551 (Stabilise `IpvNAddr::{BITS, to_bits, from_bits}` (`ip_bits`))

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee workingjubilee changed the title Improve diagnostic output the non_local_definitions lint Improve diagnostic output of non_local_definitions lint May 28, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2024
…kingjubilee

Rollup of 5 pull requests

Successful merges:

 - rust-lang#125089 (Improve diagnostic output the `non_local_definitions` lint)
 - rust-lang#125343 (`-Znext-solver`: eagerly normalize when adding goals)
 - rust-lang#125551 (Stabilise `IpvNAddr::{BITS, to_bits, from_bits}` (`ip_bits`))
 - rust-lang#125640 (Don't suggest turning non-char-literal exprs of ty `char` into string literals)
 - rust-lang#125647 (update tracking issue for lazy_cell_consume)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8e89f83 into rust-lang:master May 28, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2024
Rollup merge of rust-lang#125089 - Urgau:non_local_def-suggestions, r=estebank

Improve diagnostic output the `non_local_definitions` lint

This PR improves (or at least tries to improve) the diagnostic output the `non_local_definitions` lint, by simplifying the wording, by adding a "sort of" explanation of bounds interaction that leak the impl...

This PR is best reviewed commit by commit and is voluntarily made a bit vague as to have a starting point to improve on.

Related to https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/non_local_defs.20wording.20improvements

Fixes rust-lang#125068
Fixes rust-lang#124396
cc ```@workingjubilee```
r? ```@estebank```
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 13, 2024
…ge, r=estebank

Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang#125089 (comment)
Fixes rust-lang#125681
r? `@estebank`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 14, 2024
…ge, r=estebank

Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang#125089 (comment)
Fixes rust-lang#125681
r? ``@estebank``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 14, 2024
…ge, r=estebank

Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang#125089 (comment)
Fixes rust-lang#125681
r? ```@estebank```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 14, 2024
…ge, r=estebank

Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang#125089 (comment)
Fixes rust-lang#125681
r? ````@estebank````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 14, 2024
…ge, r=estebank

Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang#125089 (comment)
Fixes rust-lang#125681
r? `````@estebank`````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 15, 2024
…ge, r=estebank

Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang#125089 (comment)
Fixes rust-lang#125681
r? ``````@estebank``````
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2024
…, r=estebank

Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang#125089 (comment)
Fixes rust-lang#125681
r? `@estebank`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 17, 2024
Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang/rust#125089 (comment)
Fixes #125681
r? `@estebank`
bors added a commit to rust-lang/rust-analyzer that referenced this pull request Jun 20, 2024
Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang/rust#125089 (comment)
Fixes #125681
r? `@estebank`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
Indicate in `non_local_defs` lint that the macro needs to change

This PR adds a note to indicate that the macro needs to change in the `non_local_definitions` lint output.

Address rust-lang/rust#125089 (comment)
Fixes #125681
r? `@estebank`
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
6 participants