Add a visibility suggestion in private-in-public errors #113983
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
nyurik
changed the title
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 |
rustbot
added
S-blocked
This comment was marked as resolved.
This comment was marked as resolved.
rustbot
added
S-waiting-on-author
|
@nyurik any updates? |
nyurik
force-pushed
the
pub-in-priv-err
branch
from
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
added
S-waiting-on-review
|
@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. |
| @@ -1475,12 +1475,31 @@ impl SearchInterfaceForPrivateItemsVisitor<'_> { | |||
| } | |||
| }; | |||
|
|
|||
| // 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.
petrochenkov
added
S-waiting-on-author
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: |
nyurik
force-pushed
the
pub-in-priv-err
branch
from
7ebf1a8 to
95db42f
Compare
|
@nyurik any updates on replying to the changes requested in the review and implement them accordingly? |
nyurik
commented
Feb 12, 2024
| @@ -1475,12 +1475,31 @@ impl SearchInterfaceForPrivateItemsVisitor<'_> { | |||
| } | |||
| }; | |||
|
|
|||
| // 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) |
Dylan-DPC
added
S-waiting-on-review
|
#113983 (comment) wasn't addressed, the suggestion spans are incorrect and CI is failing. |
rustbot
added
S-waiting-on-author
|
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)"?