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

Remove textual span from diagnostic string #89555

Merged
merged 7 commits into from
Oct 13, 2021

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Oct 5, 2021

This is an unnecessary repetition, as the diagnostic prints the span anyway in the source path right below the message.

I further removed the identification of the node, as that does not give any new information in any of the cases that are changed in tests.

EDIT: also inserted a suggestion that other diagnostics were already emitting

@oli-obk oli-obk added the A-diagnostics Area: Messages for errors, warnings, and lints label Oct 5, 2021
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2021
Comment on lines 4 to +7
LL | fn hide_ref<'a, 'b, T: 'static>(x: &'a mut &'b T) -> impl Swap + 'a {
| ^^^^^^^^^^^^^^
| -- ^^^^^^^^^^^^^^
| |
| hidden type `&'a mut &'b T` captures the lifetime `'b` as defined here
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention that "hidden type &'a mut &'b T" is impl Swap + 'a in some way? Either with a label on the primary span or mentioning it explicitly in a note? (Doesn't need to be addressed in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll also wait with this until the primary span stops pointing to the return type with lazy TAIT

Comment on lines 1 to +7
error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
--> $DIR/error-handling-2.rs:13:60
--> $DIR/error-handling-2.rs:10:60
|
LL | fn foo<'a: 'b, 'b, 'c>(x: &'static i32, mut y: &'a i32) -> E<'b, 'c> {
| ^^^^^^^^^
|
note: hidden type `*mut &'a i32` captures the lifetime `'a` as defined on the function body at 13:8
--> $DIR/error-handling-2.rs:13:8
|
LL | fn foo<'a: 'b, 'b, 'c>(x: &'static i32, mut y: &'a i32) -> E<'b, 'c> {
| ^^
| -- ^^^^^^^^^
| |
| hidden type `*mut &'a i32` captures the lifetime `'a` as defined here
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, then we should maybe have a label pointing at the type E definition saying "this is the hidden type", if we do what I mentioned earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. Is is helpful to know where the opaque type is declared? If it were a struct or just a regular type alias, we would not be pointing at the definition either.

I could see it making sense to point at the definition for trait bound mismatches, as these are only written at the definition, but all the lifetimes are local to this function and could have confusingly different names on the declaration

Copy link
Contributor

Choose a reason for hiding this comment

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

What about having a primary span saying "hiden type returned here" or something along those lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤣 yes, that is exactly what we need, but without lazy TAIT that is going to be a pain to figure out... or... hmm maybe not, I have an idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea would kind of work, but it's horrible and not something to do quickly

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll love to hear about it ☺️

@estebank
Copy link
Contributor

estebank commented Oct 7, 2021

r? @estebank

r=me with or without the nitpicks addressed (I see that this is blocking another PR, so if you don't want to deal with it we can create an E-easy ticket for someone else to tackle it)

@rust-highfive rust-highfive assigned estebank and unassigned jackh726 Oct 7, 2021
@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 8, 2021

@bors r=estebank

@bors
Copy link
Contributor

bors commented Oct 8, 2021

📌 Commit d3871f5 has been approved by estebank

@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 Oct 8, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 8, 2021
… r=estebank

Remove textual span from diagnostic string

This is an unnecessary repetition, as the diagnostic prints the span anyway in the source path right below the message.

I further removed the identification of the node, as that does not give any new information in any of the cases that are changed in tests.

EDIT: also inserted a suggestion that other diagnostics were already emitting
@camelid
Copy link
Member

camelid commented Oct 8, 2021

@bors r-

Failed in rollup: #89671 (comment)

Looks like some more tests may need to be blessed.

@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 Oct 8, 2021
@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 8, 2021

@bors r=estebank

@bors
Copy link
Contributor

bors commented Oct 8, 2021

📌 Commit 09d1774 has been approved by estebank

@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 Oct 8, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 9, 2021
… r=estebank

Remove textual span from diagnostic string

This is an unnecessary repetition, as the diagnostic prints the span anyway in the source path right below the message.

I further removed the identification of the node, as that does not give any new information in any of the cases that are changed in tests.

EDIT: also inserted a suggestion that other diagnostics were already emitting
@GuillaumeGomez
Copy link
Member

Failed in #89702.

@bors: r-

@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 Oct 9, 2021
@camelid
Copy link
Member

camelid commented Oct 9, 2021

@bors rollup=iffy

@oli-obk oli-obk force-pushed the nll_member_constraint_diag branch from 09d1774 to 724bc1c Compare October 12, 2021 15:47
@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 12, 2021

@bors r=estebank

@bors
Copy link
Contributor

bors commented Oct 12, 2021

📌 Commit 724bc1cbd3161ab1581ab54789807731f3628056 has been approved by estebank

@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 Oct 12, 2021
@bors
Copy link
Contributor

bors commented Oct 13, 2021

⌛ Testing commit 724bc1cbd3161ab1581ab54789807731f3628056 with merge 8e9f48623c91b6baadd21004928795845c9baa98...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 13, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 13, 2021
@oli-obk oli-obk force-pushed the nll_member_constraint_diag branch from 724bc1c to 20d6aad Compare October 13, 2021 11:06
@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 13, 2021

@bors r=estebank

@bors
Copy link
Contributor

bors commented Oct 13, 2021

📌 Commit 20d6aad has been approved by estebank

@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 Oct 13, 2021
@bors
Copy link
Contributor

bors commented Oct 13, 2021

⌛ Testing commit 20d6aad with merge dfc5add...

@bors
Copy link
Contributor

bors commented Oct 13, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing dfc5add to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2021
@bors bors merged commit dfc5add into rust-lang:master Oct 13, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 13, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (dfc5add): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -0.9% on full builds of diesel)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants