Repository navigation
Restore E0603 suggestions for private imports in grouped use - #165642
raushan728 wants to merge 6 commits into
Conversation
rustbot
added
S-waiting-on-review
|
r? @davidtwco rustbot has assigned @davidtwco. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Hi, can you please disclose LLM usage? Because from my perspective this code is slightly over commented, there is so much comments and they are all so detailed which is not a usual thing |
Yes, i used LLM. In the browser i used claude (sonnet) for my discussion. In my IDE i used free models for coding assistance. i don't have a fixed model there because the free model changed several times during the work
The PR is a bit large and has many test cases, so i added comments to explain the spans, the conditions and why some cases get no suggestion. If any of them are too detailed, i am happy to trim them In my earlier attempt (#158445), reviewers asked for explanations of the spans and conditions Also, the PR is split into small commits, so it may be easier to review commit by commit |
|
r? @mu001999 |
This comment has been minimized.
This comment has been minimized.
|
Have you found a dedicated LLM reviewer for this? |
nested is stored on ImportData rather than ImportKind::Single so that glob imports inside groups are also treated as nested.
When a private item is imported inside a group where it is the only
item of the root use-tree (e.g. `use a::{B};`, `use a::{b::{C}};`),
emit an in-place suggestion replacing the root use-tree with the
accessible path.
Single-item groups carrying renames (`use a::{B as C};`) or globs
(`use a::{m::*};`), as well as imports expanded from macros where the
root span cannot be used for suggestions, are skipped to prevent
dropping aliases, trailing glob wildcards, or proposing edits inside
macro definitions.
…in E0603
When a private item is imported inside a multi-item grouped import
statement (e.g. `use a::{B, C};` where `B` is private), emit a
multipart suggestion extracting `B` into its own standalone `use`
statement prepended before the root import and removing `B` from the
group.
Extraction is restricted to `use` items with inherited visibility and
empty attributes to avoid losing visibility or attributes, and
skipped for renames (`as Renamed`), globs (`::*`), and spans
originating from macro expansions where spans cannot be used for
suggestions.
28f35de to
102131e
Compare
|
No, I don't have a dedicated LLM reviewer. I do use LLMs as an assistant in all my PRs, including this one. I'm a student and I use them to learn the codebase and move faster. |
|
Hi @raushan728, moderator here. Please allow some time for a review. Understand that we don't need to move faster, we just need to find time for a review (see reminder from our bot). We're a community of contributors, not a soulless code-churning machine. We appreciate your interest in the Rust project but your influx of pull requests is work for us; many mentions create background noise. You've been already warned, please be respectful and allow us some time. |
|
How does this differ from #158445? |
|
@apiraino Understood, I'm sorry for the noise. I use LLMs as an assistant, but this PR is not blindly generated. |
The goal is the same, but the implementation is different. I closed that PR because it was not correct The old PR found the item to remove by reading the source text and counting byte positions. That was fragile it could break with multibyte characters. This PR does not read the source text at all. It takes the removal span directly from the AST, using the same formula that The old PR also copied a lot of the suggestion code and tried to remove the braces by hand. This PR reuses the existing suggestion code and leaves the braces as they are ( You can compare the test cases of both PRs. If you run the test cases of this PR on the old implementation, it generates wrong output for some of them. That is why I remade it instead of patching the old PR. |
Fixes #157453.
For a single-item group like
use a::{B};, the wholeusetree is replaced with the path so no braces are left. for a multi item group likeuse a::{B, C};, the failing item is removed from the group and a newuse <path>is added above it. nested groups work the same way.Spans come from the AST only, with no source-text scanning. The span formula is factored out of
calc_unused_spansintonested_item_removal_span, with no behavior change.PrivacyError::single_nestedis replaced bynested Option<NestedImportRemoval>, stored onImportDataso globs inside groups are covered too.Braces are left as they are in the multi-item case (
use a::{Two};after extraction), to avoid span arithmetic. Removing them would need byte position arithmetic on the source. because it can cause ICEs with multibyte charactersrustfmtwill handle this.No suggestion is emitted for renames, globs, macro-expanded spans, or multi-item groups with
pubor attributes, since the replacement would drop them.AI assistance: Claude (browser) for discussion, free models in the IDE for coding help. I reviewed and tested all changes myself