Improve E0603 suggestions for private grouped imports - #158445
Improve E0603 suggestions for private grouped imports#158445raushan728 wants to merge 2 commits into
E0603 suggestions for private grouped imports#158445Conversation
|
r? @jackh726 rustbot has assigned @jackh726. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
I reviewed everything except the r? @fee1-dead please review, since you wanted this. |
|
|
|
Reminder, once the PR becomes ready for a review, use |
59ad765 to
4706b08
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
4706b08 to
12d8c86
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
12d8c86 to
567fb3a
Compare
This comment has been minimized.
This comment has been minimized.
E0603 suggestions for private grouped imports
I've cleaned this up to rely on the standard string search APIs ( |
567fb3a to
7599264
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
I removed the manual span/index computations and simplifying the data flow. Could you take a another look? thanks! |
Suggest direct imports for private items inside grouped imports. Split grouped imports when necessary and replace single-item groups with direct imports.
7599264 to
0c6885b
Compare
|
Sorry for the previous revisions. I've cleaned up the code and addressed the earlier feedback where I could. I think it's in a much better state now and hopefully easier to review. |
|
I invite you to join https://rust-lang.zulipchat.com/join/rlfvpemsaacs3pfi6kwqnqjb/ and start a thread asking for a mentor if you want to make progress here. |
| } else if single_nested | ||
| && !shown_candidates | ||
| && !outermost_res.is_some_and(|(_, outer)| outer.span != ident.span) | ||
| { |
There was a problem hiding this comment.
This whole block needs a summary comment. What is it doing? Why?
And explain the conditions, because they are not self-explanatory.
| sugg_paths.sort_by_key(|(p, reexport)| (p.len(), p[0].name == sym::core, *reexport)); | ||
| for (sugg, reexport) in sugg_paths { | ||
| if sugg.len() <= 1 { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This copies earlier code. Why is it useful here? Can it be deduplicated?
| let (Ok(before), Ok(after)) = ( | ||
| self.tcx.sess.source_map().span_to_snippet(before_span), | ||
| self.tcx.sess.source_map().span_to_snippet(after_span), | ||
| ) else { | ||
| continue; | ||
| }; | ||
|
|
||
| let mut replacement = format!("{before}{after}"); |
There was a problem hiding this comment.
Removal is done by suggesting to replace the contents of span_to_remove with an empty string. span_to_snippet is almost never the correct solution.
| if !inner.contains(',') && !inner.is_empty() { | ||
| replacement = format!("{}{}", replacement[..open].trim_end(), inner); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't try to be smart with braces. The user has rustfmt.
| // Replace the entire `use` instead of leaving `use foo::{}`. | ||
| if leaves_empty_group { | ||
| let line_span = self.tcx.sess.source_map().span_extend_to_line(root_span); | ||
| let suggestion_text = format!("{indentation}use {path};"); | ||
| err.multipart_suggestion( | ||
| msg, | ||
| vec![(line_span, suggestion_text)], | ||
| Applicability::MachineApplicable, | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Why not just replace the contents of root_span with path?
| // Insert before `root_span` to reuse the existing `use`. | ||
| err.multipart_suggestion( | ||
| msg, | ||
| vec![ | ||
| (root_span.shrink_to_lo(), format!("{path};\n{indentation}use ")), |
There was a problem hiding this comment.
Would it be simpler to insert a clean line at line_span.shrink_to_lo()?
| dedup_span: Span, | ||
| root_span: Span, |
There was a problem hiding this comment.
Could you take the opportunity to document what are those spans and where they point to?
View all comments
After PR #156244 removed broken suggestions for nested imports, E0603 was left without any suggestion. This restores helpful suggestions for grouped use statements.
For
use crate::two::{One, Two}with a privateOne:Single item groups get a direct replacement. Braces are removed when one item remains. Re-export chains are handled.
Fixes #157453.
r? @petrochenkov