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

Specialize infinite-type "insert some indirection" suggestion for Option #91416

Merged
merged 2 commits into from
Mar 31, 2022
Merged

Specialize infinite-type "insert some indirection" suggestion for Option #91416

merged 2 commits into from
Mar 31, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Dec 1, 2021 •
edited
Loading

Suggest Option<Box<_>> instead of Box<Option<_>> for infinitely-recursive members of a struct.

Not sure if I can get the span of the generic subty of the Option so I can make this a +++-style suggestion. The current output is a tiny bit less fancy looking than the original suggestion.

Should I limit the specialization to just Option<Box<TheOuterStruct>>? Because right now it applies to all Option members in the struct that are returned by Representability::SelfRecursive.

Fixes #91402

r? @estebank
(since you wrote the original suggestion and are definitely most familiar with it!)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 1, 2021
estebank
estebank approved these changes Dec 2, 2021
View reviewed changes
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.

Regarding getting the right span, the correct thing to do would be to somehow get the appropriate hir::Ty node so that you can get the appropriate span. The hacky second option is to use the snippet and string manipulation to try and find the span edges manually. This final option is to do what you've done here. The last two will have trouble when it comes to macros modifying the user written token stream.

Can you add a FIXME comment above the special casing of the option to mention something along the lines of "figure out a way to get the correct span and be resilient to type renaming of both Option and Box"? r=me after that.

src/test/ui/issues/issue-17431-1.stderr Outdated
Comment on lines 11 to 12
LL | struct Foo { foo: Option<Box<Option<Foo>>> }
| ~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Contributor

Choose a reason for hiding this comment

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

This test case shows a place where the approach breaks down slightly :)

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs Outdated
ty::Adt(def, args) if Some(def.did) == tcx.get_diagnostic_item(sym::Option) => {
vec![(
span,
format!("Option<Box<{}>>", args.iter().next().unwrap().expect_ty()),
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem I see with this is that it will cause trouble with any situation where Option has been either shadowed or been locally renamed. Of course, we could make the same argument about Box in the original...

@compiler-errors

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member Author

Added FIXME per request.

@compiler-errors
Copy link
Member Author

So I reformulated this to work with the HirId of the struct field, instead of it's spanless TyS. This gives us the right span information to turn an Option<T> into a T so we can suggest wrapping that in Box. The diagnostics should be much improved.

Added a few other test cases as well.

@estebank, do you mind taking a look at this sometime? Thanks!

@bors
Copy link
Contributor

bors commented Jan 11, 2022

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

@bors
Copy link
Contributor

bors commented Jan 16, 2022

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

estebank
estebank approved these changes Feb 17, 2022
View reviewed changes
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.

Apologies for the delay. r=me after rebasing.

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved
@estebank estebank 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 Feb 17, 2022
@compiler-errors
Copy link
Member Author

ready for an r+ i think!

@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 Feb 18, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@compiler-errors compiler-errors force-pushed the infinite-ty-option-box branch from 5f9b774 to c74f7a3 Compare March 31, 2022 15:23
@compiler-errors
Copy link
Member Author

rebased and re-tested

@bors r=estebank rollup

@bors
Copy link
Contributor

bors commented Mar 31, 2022

📌 Commit c74f7a3 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 Mar 31, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 31, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2022
@bors
Auto merge of rust-lang#95526 - Dylan-DPC:rollup-0ikl5l5, r=Dylan-DPC
0677edc 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#91416 (Specialize infinite-type "insert some indirection" suggestion for Option)
 - rust-lang#95384 (Update target_has_atomic documentation for stabilization)
 - rust-lang#95517 (small rustc_borrowck cleanup)
 - rust-lang#95520 (Fix typos in core::ptr docs)
 - rust-lang#95523 (remove unused field from `infcx`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 521c590 into rust-lang:master Mar 31, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 31, 2022
@compiler-errors compiler-errors deleted the infinite-ty-option-box branch November 2, 2022 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank approved these changes

Assignees

@estebank estebank

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.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

rustc suggests Box<Option<T>>, but could specialize to suggest Option<Box<T>> instead
7 participants
@compiler-errors @bors @estebank @JohnCSimon @apiraino @rust-highfive @rustbot