Skip to content

Simplify ast::UseTreeKind - #162794

Open
nnethercote wants to merge 3 commits into
rust-lang:mainfrom
nnethercote:simplify-UseTreeKind
Open

Simplify ast::UseTreeKind#162794
nnethercote wants to merge 3 commits into
rust-lang:mainfrom
nnethercote:simplify-UseTreeKind

Conversation

@nnethercote

@nnethercote nnethercote commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

We can make things simpler by adjusting how the ids are stored. Details in individual commits.

r? @petrochenkov

@nnethercote
Improve build_reduced_graph_for_use_tree arguments
521c4eb 
It makes sense to put the top-level item first, before the use tree
within the item. And it makes sense to have a single function-level
comment explaining this rather than inline comments at the definition
and every call site.
@nnethercote
Remove an out-of-date comment
605149c 
`is_public` is not relevant here.
@rustbot

rustbot commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

rustfmt is developed in its own repository. If possible, consider making this change to rust-lang/rustfmt instead.

cc @rust-lang/rustfmt

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2026
@rustbot rustbot added T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue. labels Sep 15, 2026
@nnethercote

nnethercote commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

@oli-obk: this will conflict a little with your #161349, but I think they're fundamentally doing different things and are compatible.

@nnethercote

nnethercote commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

LLM disclosure: some of the ideas for these changes came from an LLM. I made all the changes myself.

@petrochenkov

petrochenkov commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

I can't express why exactly, but I'm not sure I like the idea (the last commit, I mean).
It makes writing some code more convenient, I can't deny that, but it feels like the AST representation is now "farther from the truth".

I wonder what issues this non-uniqueness of NodeIds can potentially bring.
Do we ever need to map from NodeId to an AST piece? Now a single ID could refer to two different AST pieces (and item and a use tree), this wasn't possible before.

r? @oli-obk what do you think?

@rustbot rustbot assigned oli-obk and unassigned petrochenkov Sep 15, 2026
@oli-obk

oli-obk commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Yea it doesn't really feel nice. We'd then want to do this for the HIR, too, I suspect, and there it's def wrong. I think it is sth I'd probably end up reverting or obsoleting anyway depending on how far we can refactor the use trees away from being a normal item (they aren't really, they don't share much of the side tables with the rest of the items)

@nnethercote
Introduce ast::UseTreeAndId
4bfe145 
Currently a `NodeId` is stored in a pair with each nested use tree. This
commit changes the pair to a named type `UseTreeAndId`. In most places
this doesn't make much difference but in the AST visitor it gets rid of
several weird special cases.
@nnethercote

nnethercote commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

I have changed the final commit to introduce a new type UseTreeAndId. This avoids the id duplication but still gets rid of the weird special cases in the AST visitor.

@petrochenkov petrochenkov self-assigned this Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@oli-obk oli-obk

@petrochenkov petrochenkov

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nnethercote @rustbot @petrochenkov @oli-obk