Add a visibility suggestion in private-in-public errors#113983
Add a visibility suggestion in private-in-public errors#113983nyurik wants to merge 5 commits intorust-lang:masterfrom
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
r? compiler-errors |
|
I don't think this change is still relevant after #113126. |
|
@petrochenkov thx for the heads up! I looked through that PR (even left a comment), but I'm not certain I understand if it will cover all cases? I see this change adds if self.in_assoc_ty && !vis.is_at_least(self.required_visibility, self.tcx) { |
|
r? petrochenkov |
This comment was marked as resolved.
This comment was marked as resolved.
|
@nyurik any updates? |
e9c1d6d to
bb3e619
Compare
|
@Dylan-DPC thx for the ping, i have updated the PR, but it still has some outstanding questions that need some feedback. Thx! |
|
@petrochenkov hi, would you be the right person to review this? I would love to move forward, but it is a bit unclear. Plus @JohnCSimon was pinging the original author of this PR (before my refactoring). Hopefully the outstanding questions can be easily addressed. Thx!! |
Yes, but maybe a bit later, I'm busy this week. |
| }; | ||
|
|
||
| // FIXME: this code was adapted from the above `vis_descr` computation, | ||
| // but it's not clear if it's correct. |
There was a problem hiding this comment.
I think fn vis_to_string should be usable here.
There was a problem hiding this comment.
I am not sure it will work. I tried to
- add
rustc_ast_pretty = { path = "../rustc_ast_pretty" }to Cargo.toml - use it with
let vis_sugg = rustc_ast_pretty::pprust::vis_to_string(&self.required_visibility);
But the ty::Visibility that I have here -- enum { Default, Hidden, Protected } is not the same as the one vis_to_string expects -- a struct ast::Visibility with kind/span/tokens. Is there a way i can get the struct?
There was a problem hiding this comment.
I meant ty::Visibility::to_string, not vis_to_string from rustc_ast_pretty, sorry.
ty::Visibility::to_string was also called vis_to_string when this comment was written, but it was renamed in #115993.
This comment was marked as resolved.
This comment was marked as resolved.
|
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged. You can start a rebase with the following commands: The following commits are merge commits: |
7ebf1a8 to
95db42f
Compare
|
@nyurik any updates on replying to the changes requested in the review and implement them accordingly? |
nyurik
left a comment
There was a problem hiding this comment.
Thx for the ping! I am still interested in this PR, but got a bit stuck on implementing it
| }; | ||
|
|
||
| // FIXME: this code was adapted from the above `vis_descr` computation, | ||
| // but it's not clear if it's correct. |
There was a problem hiding this comment.
I am not sure it will work. I tried to
- add
rustc_ast_pretty = { path = "../rustc_ast_pretty" }to Cargo.toml - use it with
let vis_sugg = rustc_ast_pretty::pprust::vis_to_string(&self.required_visibility);
But the ty::Visibility that I have here -- enum { Default, Hidden, Protected } is not the same as the one vis_to_string expects -- a struct ast::Visibility with kind/span/tokens. Is there a way i can get the struct?
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
#113983 (comment) wasn't addressed, the suggestion spans are incorrect and CI is failing. |
|
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. @rustbot label: +S-inactive |
This modifies the original #112540 by @aryan-debug to fix #112284
This PR introduces a new field
vis_suggto suggest the proper visibility for the error.TODO: What should the
TODOvalues be here? Note that not a single test anywhere has used these???TODO???values, so it is unclear how they can be generated. Maybe this whole match should be simplified to mapRestricted(_) => "pub(crate)"?