Skip to content

Disentangle AST crates and error crates#155237

Open
nnethercote wants to merge 4 commits intorust-lang:mainfrom
nnethercote:ast-errors-rejig
Open

Disentangle AST crates and error crates#155237
nnethercote wants to merge 4 commits intorust-lang:mainfrom
nnethercote:ast-errors-rejig

Conversation

@nnethercote
Copy link
Copy Markdown
Contributor

@nnethercote nnethercote commented Apr 13, 2026 •
edited
Loading

Currently the rustc_ast* crates and the rustc_error* crates (and rustc_lint_defs) are quite intertwined. This PR disentangles them. Details in individual commits.

r? @davidtwco

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 13, 2026

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

cc @fmease

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

rustc_error_messages was changed

cc @TaKO8Ki

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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. labels Apr 13, 2026
@nnethercote
Copy link
Copy Markdown
Contributor Author

nnethercote commented Apr 13, 2026 •
edited
Loading

The path from rustc_hir to rustc_span, before:
image

And after:
image

I think the latter is a lot nicer -- much less linear, with rustc_hir_id gone and rustc_error* fully separated from rustc_ast*.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nnethercote nnethercote marked this pull request as draft April 13, 2026 12:43
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2026
@nik-rev
Copy link
Copy Markdown
Contributor

nik-rev commented Apr 13, 2026

@nnethercote that crate graph visualization looks nice, what tool did you use to generate it?

@nnethercote
Copy link
Copy Markdown
Contributor Author

nnethercote commented Apr 13, 2026

@nik-rev:

cargo +nightly depgraph --all-deps --dedup-transitive-deps --workspace-only --root=rustc-main,rustc_codegen_llvm ~/graph.dot
dot -Tsvg ~/graph.dot > ~/graph.svg

Requires installing cargo-depgraph.

@nnethercote nnethercote changed the title Adjust rustc_ast* dependencies Distentangle AST crates and error crates Apr 14, 2026
@nnethercote nnethercote changed the title Distentangle AST crates and error crates Disentangle AST crates and error crates Apr 14, 2026
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Apr 14, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nnethercote nnethercote marked this pull request as ready for review April 14, 2026 10:25
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2026
@nnethercote
Copy link
Copy Markdown
Contributor Author

nnethercote commented Apr 14, 2026

@davidtwco: this is now ready for review.

@rust-bors

This comment has been minimized.

Sign in to view
@nnethercote
Invert dependency between rustc_error_messages and rustc_ast*.
20cee0f 
`rustc_error_messages` currently depends on
`rustc_ast`/`rustc_ast_pretty`. This is odd, because
`rustc_error_messages` feels like a very low-level module but
`rustc_ast`/`rustc_ast_pretty` do not.

The reason is that a few AST types impl `IntoDiagArg` via
pretty-printing. `rustc_error_messages` can define `IntoDiagArg` and
then impl it for the AST types. But if we invert the dependency we hit
a problem with the orphan rule: `rustc_ast` must impl `IntoDiagArg`
for the AST types, but that requires calling pretty-printing code which
is in `rustc_ast_pretty`, a downstream crate.

This commit avoids this problem by just removing the `IntoDiagArg` impls
for these AST types. There aren't that many of them, and we can just use
`String` in the relevant error structs and use the pretty printer in the
downstream crates that construct the error structs. There are plenty of
existing examples where `String` is used in error structs.

There is now no dependency between `rustc_ast*` and
`rustc_error_messages`.
@nnethercote
Eliminate rustc_lint_defs' dependency on rustc_ast.
46db830 
It currently only depends on two things:
- `rustc_ast::AttrId`: this is just a re-export of `rustc_span::AttrId`,
  so we can import the original instead.
- `rustc_ast::AttributeExt`: needed only for the `name` and `id`
  methods. We can instead pass in the `name` and `id` directly.
@nnethercote
Remove rustc_hir_id crate.
3d3c966 
`rustc_hir_id` is a very small crate containing some basic HIR types
(e.g. `HirId`) so that other crates can use them without depending on
`rustc_hir` (which is a much bigger crate).

However, `rustc_span` already has a module called `def_id` which also
contains some basic HIR types (e.g. `DefId`). So there's not much point
also have `rustc_hir_id`. This commit moves the contents of
`rustc_hir_id` into that module.
@nnethercote
Move node_id module from rustc_ast to rustc_span.
14bd0c8 
This puts it alongside `rustc_span::def_id`, which serves a similar
purpose.

This lets us remove `rustc_errors`'s dependency on `rustc_ast`,
completing the disentangling of the AST crates and the error crates.
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 19, 2026

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.

@nnethercote
Copy link
Copy Markdown
Contributor Author

nnethercote commented Apr 19, 2026

I rebased.

@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented Apr 25, 2026 •
edited
Loading

I think, ideally, generic linting APIs living in crates like rustc_error_messages should not depend on concrete "lint reporting position" types like HirId and NodeId at all, especially not HirId.
They should ideally use some abstract "LintNodeId" APIs instead, which would be filled with concrete HIR/AST ids when HIR and AST respectively are available.

That would untangle some users of the generic linting infra, like rustc_attr_parsing from HIR ids as well.
(Because the Attribute IR is not HIR, should not contain HirIds or any other HIR parts, should not live in rustc_hir, and rusc_attr_parsing should not depend on rustc_hir as well cc @JonathanBrouwer)
(Untangling it from NodeIds is less important, because attr parsing definitely needs token streams, and they currently live in rustc_ast despite not being AST, and rustc_ast defines NodeId which is AST. Although it could be nice to try doing some day in the future.)

That's why I don't like that HirId and NodeId are moved to rustc_span so they are available in rustc_error_messages more easily.
Although I understand that reworking the linting APIs would be a much more involved change, so I don't want to block this PR which should be a clear improvement to the compiler build times.

However, rustc_span already has a module called def_id which also contains some basic HIR types (e.g. DefId)

I wouldn't classify DefId as HIR or any other IR, it's more basic infra, and it's actively used starting from early macro expansion, when even AST is not fully built.

@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented Apr 25, 2026 •
edited
Loading

That's why I don't like that HirId and NodeId are moved to rustc_span so they are available in rustc_error_messages more easily.

Hmmm, perhaps their definitions can be moved to rustc_error_messages itself, where they are first needed currently?
Maybe accompanied with a FIXME for reworking the linting APIs.
That would make it clear that something is not right at the moment.
Then they'd be reexported from their "canonical" locations in rustc_ast and rustc_hir.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@davidtwco davidtwco

Labels

A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler 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.

6 participants

@nnethercote @rustbot @rust-log-analyzer @nik-rev @petrochenkov @davidtwco