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

Tweak output of resolve errors #126810

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Tweak output of resolve errors #126810

wants to merge 9 commits into from
+2,063 −1,814

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jun 21, 2024 •
edited
Loading 

error[E0576]: cannot find method or associated constant `BAR`
  --> $DIR/issue-87638.rs:20:27
   |
LL |     let _ = <S as Trait>::BAR;
   |                           ^^^ not found in trait `Trait`

instead of

error[E0576]: cannot find method or associated constant `BAR` in trait `Trait`
  --> $DIR/issue-87638.rs:20:27
   |
LL |     let _ = <S as Trait>::BAR;
   |                           ^^^ not found in `Trait`

The general rule is that the primary span label should work well on its own (suitable to be shown in editors), while the main message provides additional context of the general case. When using --error-format=short, we will concatenate the main message and the primary labels, so they should be complementary.

@estebank estebank marked this pull request as ready for review June 21, 2024 22:31
@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Jun 21, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank estebank force-pushed the resolve-error-wording branch from 5f349c3 to b4c34b6 Compare June 21, 2024 22:58
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank estebank force-pushed the resolve-error-wording branch from b4c34b6 to d740595 Compare June 22, 2024 01:32
@BoxyUwU
Copy link
Member

BoxyUwU commented Jun 23, 2024 •
edited
Loading

bors-r-plus

@bors
Copy link
Contributor

bors commented Jun 23, 2024

📌 Commit d740595 has been approved by BoxyUwU

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 23, 2024
@bors

This comment was marked as resolved.

Sign in to view
@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 23, 2024
@estebank estebank force-pushed the resolve-error-wording branch from d740595 to 57423a7 Compare June 23, 2024 19:46
@estebank
Copy link
Contributor Author

@bors r=BoxyUwU rollup=never

@bors
Copy link
Contributor

bors commented Jun 23, 2024

📌 Commit 57423a7 has been approved by BoxyUwU

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 23, 2024
@estebank estebank force-pushed the resolve-error-wording branch from 57423a7 to 1e450c7 Compare June 23, 2024 19:54
@estebank
Copy link
Contributor Author

Failed to --amend 🤦

@bors r=BoxyUwU rollup=never

@bors
Copy link
Contributor

bors commented Jun 23, 2024

📌 Commit 1e450c7 has been approved by BoxyUwU

It is now in the queue for this repository.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
@bors
Auto merge of rust-lang#126810 - estebank:resolve-error-wording, r=Bo…
84e6a0c 
…xyUwU

Remove redundancy in resolve error between main message and primary label

```
error[E0576]: cannot find method or associated constant `BAR`
  --> $DIR/issue-87638.rs:20:27
   |
LL |     let _ = <S as Trait>::BAR;
   |                           ^^^ not found in trait `Trait`
```

instead of

```
error[E0576]: cannot find method or associated constant `BAR` in trait `Trait`
  --> $DIR/issue-87638.rs:20:27
   |
LL |     let _ = <S as Trait>::BAR;
   |                           ^^^ not found in `Trait`
```

The general rule is that the primary span label should work well on its own (suitable to be shown in editors), while the main message provides additional context of the general case. When using `--error-format=short`, we will concatenate the main message and the primary labels, so they should be complementary.
@bors
Copy link
Contributor

bors commented Jun 23, 2024

⌛ Testing commit 1e450c7 with merge 84e6a0c...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
estebank added 8 commits July 11, 2024 22:46
@estebank
Unify output of the different "cannot find X" errors
3abad14 
Do not mention the scope where an ident couldn't be resolved (particularly
for macros), and add a primary span label mentioning "this scope" (ideally
we would replace the text with the actual scope name in the future):

```
error: cannot find derive macro `rustfmt`
  --> $DIR/tool-attributes-misplaced-1.rs:4:10
   |
LL | #[derive(rustfmt)]
   |          ^^^^^^^ not found in this scope
```
@estebank
revert wording changes to main message
5f47d1f
@estebank
More accurate description of unresolved imported item
ec83028 
State when the element that couldn't be found was expected to be
a module, a generic item, a value or a macro.

State what scope the E0433 the item couldn't be found in, when
available.
@estebank
Account for _ in import paths
de92cd4
@estebank
Fix miri test
045e8dd
@estebank
fix windows test
a9fe7c8
@estebank
Add scope context to unresolved macros
f0d9e3f
@estebank
Fix ui-fulldeps test
e1fed5f
@estebank estebank force-pushed the resolve-error-wording branch from e120581 to e1fed5f Compare July 11, 2024 23:03
@estebank estebank 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 Jul 12, 2024
@bors
Copy link
Contributor

bors commented Jul 13, 2024

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

jneem pushed a commit to tweag/cargo that referenced this pull request Jul 15, 2024
@estebank @jneem
Change tests to support rustc wording changes
3c74e99 
Between rust-lang/rust#126810 and rust-lang/rust#126810
the output of `rustc` for resolution errors is going to change in such a
way that some existing cargo tests will fail. Change them to support
both the current and future output, so that those PRs can land in
`rustc`.
petrochenkov
petrochenkov reviewed Jul 16, 2024
View reviewed changes
compiler/rustc_resolve/src/ident.rs
"can't use `super` on the crate root, there are no further modules to go \"up\" to"
} else {
"there are too many leading `super` keywords"
};
Copy link
Contributor

Choose a reason for hiding this comment

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

All this logic should go into the closure, so it's not executed on a good path (finalize.is_none()).

@petrochenkov
Copy link
Contributor

This PR is too large

  • It hangs my browser when I try to review the diff on github
  • A number of smaller changes is lost among the most massive change, which is probably the addition of the "not found in this scope" label.

Could you move the single change responsible for the majority of the diff in tests to a separate PR?

petrochenkov
petrochenkov reviewed Jul 16, 2024
View reviewed changes
tests/ui/where-clauses/ignore-err-clauses.rs
@@ -4,7 +4,7 @@ fn dbl<T>(x: T) -> <T as Add>::Output
where
T: Copy + Add,
UUU: Copy,
//~^ ERROR cannot find type `UUU` in this scope
//~^ ERROR cannot find type `UUU`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, there's actually a lot of unnecessary error annotation changes like this that blow up the PR size.
Could you avoid all of them?

@petrochenkov petrochenkov 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 Jul 16, 2024
This was referenced Jul 23, 2024
Open
Open
estebank added a commit to estebank/cargo that referenced this pull request Aug 1, 2024
@estebank
Change tests to support rustc wording changes
830db6f 
Between rust-lang/rust#126810 and rust-lang/rust#126810 the output of rustc for resolution errors is going to change in such a way that some existing cargo tests will fail. Change them to support both the current and future output, so that those PRs can land in `rustc`.
stupendoussuperpowers pushed a commit to stupendoussuperpowers/cargo that referenced this pull request Aug 7, 2024
@estebank @stupendoussuperpowers
Change tests to support rustc wording changes
e8f5644 
Between rust-lang/rust#126810 and rust-lang/rust#126810 the output of rustc for resolution errors is going to change in such a way that some existing cargo tests will fail. Change them to support both the current and future output, so that those PRs can land in `rustc`.
weihanglo pushed a commit to weihanglo/cargo that referenced this pull request Aug 7, 2024
@estebank @weihanglo
Change tests to support rustc wording changes
eb4920f 
Between rust-lang/rust#126810 and rust-lang/rust#126810 the output of rustc for resolution errors is going to change in such a way that some existing cargo tests will fail. Change them to support both the current and future output, so that those PRs can land in `rustc`.
weihanglo pushed a commit to weihanglo/cargo that referenced this pull request Aug 7, 2024
@estebank @weihanglo
Change tests to support rustc wording changes
d886e7b 
Between rust-lang/rust#126810 and rust-lang/rust#126810 the output of rustc for resolution errors is going to change in such a way that some existing cargo tests will fail. Change them to support both the current and future output, so that those PRs can land in `rustc`.
weihanglo pushed a commit to weihanglo/cargo that referenced this pull request Aug 7, 2024
@estebank @weihanglo
Change tests to support rustc wording changes
db5be49 
Between rust-lang/rust#126810 and rust-lang/rust#126810 the output of rustc for resolution errors is going to change in such a way that some existing cargo tests will fail. Change them to support both the current and future output, so that those PRs can land in `rustc`.
epage pushed a commit to epage/cargo that referenced this pull request Aug 7, 2024
@estebank @epage
Change tests to support rustc wording changes
a15ec6a 
Between rust-lang/rust#126810 and rust-lang/rust#126810 the output of rustc for resolution errors is going to change in such a way that some existing cargo tests will fail. Change them to support both the current and future output, so that those PRs can land in `rustc`.
antoniospg pushed a commit to antoniospg/cargo that referenced this pull request Sep 8, 2024
@estebank @antoniospg
Change tests to support rustc wording changes
c8b15c4 
Between rust-lang/rust#126810 and rust-lang/rust#126810 the output of rustc for resolution errors is going to change in such a way that some existing cargo tests will fail. Change them to support both the current and future output, so that those PRs can land in `rustc`.
@alex-semenyuk alex-semenyuk 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 1, 2024
@JohnCSimon
Copy link
Member

@estebank
ping from triage - can you post your status on this PR? This PR has not received an update in a few months, and has merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@estebank @rustbot @rust-log-analyzer @BoxyUwU @bors @petrochenkov @JohnCSimon @alex-semenyuk