Skip to content

Emit a nicer error on impl Self { #103609

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Oct 28, 2022
Merged

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Oct 27, 2022

currently it emits a "cycle detected error" but this PR makes it emit a more user friendly error specifically saying that Self is disallowed in that position. this is a pretty hacky fix so i dont expect this to be merged (I basically only made this PR because i wanted to see if CI passes)

r? @compiler-errors

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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. labels Oct 27, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2022

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

@bors
Copy link
Collaborator

bors commented Oct 27, 2022

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

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.

r=me, with a rebase, a squash, and either the nitpick for the more specific span addressed or a ticket to follow up on that.

@@ -137,3 +137,5 @@ hir_analysis_expected_used_symbol = expected `used`, `used(compiler)` or `used(l
hir_analysis_missing_parentheses_in_range = can't call method `{$method_name}` on type `{$ty_str}`

hir_analysis_add_missing_parentheses_in_range = you must surround the range in parentheses to call its `{$func_name}` function

hir_analysis_self_in_impl_self = `Self` is not valid at this location
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hir_analysis_self_in_impl_self = `Self` is not valid at this location
hir_analysis_self_in_impl_self = `Self` is not valid in the `impl` type

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a span label pointing at the Self on its own with the text "not valid at this location"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that we cant since #[note] spans: MultiSpan is an error, I think the errors are also relatively clear as to what the problematic location is even without that span being there so its not a huge loss. 🤔

@compiler-errors
Copy link
Member

pls squash as well

@BoxyUwU BoxyUwU force-pushed the fix_impl_self_cycle branch from 23ef6f9 to c00ff9c Compare October 27, 2022 20:49
@compiler-errors
Copy link
Member

r=me after amending the last commit with a newline on the ftl file

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Oct 28, 2022

Im not sure im supposed to r=you this since i also resolved your review about splitting the visitor instead a separate fn and you didnt mention r+ing this after doing that 😆

@compiler-errors compiler-errors changed the title Emit a nicer erorr on impl Self { Emit a nicer error on impl Self { Oct 28, 2022
@compiler-errors
Copy link
Member

compiler-errors commented Oct 28, 2022

Thanks. If @estebank still has a concern about the rendering of this diagnostic, then I will follow-up on it! But I think it looks pretty good as is.

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 28, 2022

📌 Commit b342558 has been approved by compiler-errors

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 Oct 28, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 28, 2022
…piler-errors

Emit a nicer error on `impl Self {`

currently it emits a "cycle detected error" but this PR makes it emit a more user friendly error specifically saying that `Self` is disallowed in that position. this is a pretty hacky fix so i dont expect this to be merged (I basically only made this PR because i wanted to see if CI passes)

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 28, 2022
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#103585 (Migrate source line numbers CSS to CSS variables)
 - rust-lang#103608 (Remap early bound lifetimes in return-position `impl Trait` in traits too)
 - rust-lang#103609 (Emit a nicer error on `impl Self {`)
 - rust-lang#103631 (Add test for issue 36007)
 - rust-lang#103643 (rustdoc: stop hiding focus outlines on non-rustdoc-toggle details tags)
 - rust-lang#103645 (Update cargo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ab0d9df into rust-lang:master Oct 28, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 28, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants