Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hir attributes #131808

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

jdonszelmann
Copy link
Contributor

@jdonszelmann jdonszelmann commented Oct 17, 2024

This PR needs some explanation, it's somewhat large.

  • This is step one as described in Attribute handling reworks compiler-team#796. I've added a new hir::Attribute which is a lowered version of ast::Attribute. Right now, this has few concrete effects, however every place that after this PR parses a hir::Attribute should later get a pre-parsed attribute as described in Attribute handling reworks compiler-team#796 and transitively Refactoring attributes in the compiler #131229.
  • I've done my best to split up this change into smaller commits to make it reviewable:
    • b5fd0f6 modified ast::Attribute because some parts are not necessary anymore. They used to model hir-specfic parts of attributes even though they were part of the ast
    • 04307d4 adds an extension trait AttributeExt which is implemented for both ast::Attribute and hir::Atribute. This makes hir::Attributes mostly compatible with code that used to parse ast::Attribute. Some day I hope I can remove this trait, once parsing happens in a central location but for now it's a useful abstraction
    • 46e2505 adds a new hir::Attribute.
    • 1bfc0b5 is the largest commit, but honestly the most boring. This changes over all the places in the compiler that used to refer to ast::Attribute but should really refer to hir::Attribute. This is mostly just fixing up imports.
    • d2dcaaa changes over code that used to hash ast::Attribute. This also removes some assertions that were necessary because the type ast::Attribute did not model the fact that certain values should not exist anymore while hashing.
    • ac9d9fc pretty printing had to slightly change because we're dealing with hir::Attribute now
    • Then, three commits edit rustdoc, clippy and rustfmt to use hir::Attribute. The one for clippy is largest, and was made in a call together with @xFrednet who signed off on it.
    • Finally, some tests needed fixing. First, the hir stats changed slightly. Mostly because Attribute didn't used to be in there. Another test also changed, but it seems for the better. I'm unsure what I did to change it but I looked at it with Fred and we agreed that the new result is actually preferable.

Finally, I may have broken intra-doc links with something. As far as I know, this is nothing fundamental, just a dumb mistake somewhere. I'm still debugging that, hence this is a draft PR but in fairness all the complicated logic is done.

r? @oli-obk

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

Could not assign reviewer from: oli-obk.
User(s) oli-obk are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustdoc-json Area: Rustdoc JSON backend PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 17, 2024
@rust-log-analyzer

This comment has been minimized.

compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved
compiler/rustc_ast/src/ast.rs Show resolved Hide resolved
compiler/rustc_ast/src/ast.rs Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-release Relevant to the release subteam, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Oct 17, 2024
@jdonszelmann jdonszelmann force-pushed the hir-attributes branch 4 times, most recently from cf3179b to e3133bb Compare October 17, 2024 06:52
@petrochenkov petrochenkov self-assigned this Oct 17, 2024
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Clippy changes look good to me :D

=^.^=

@jdonszelmann jdonszelmann marked this pull request as ready for review October 17, 2024 18:44
@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

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 to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

Some changes occurred in compiler/rustc_sanitizers

cc @rust-lang/project-exploit-mitigations, @rcvalle

@jdonszelmann jdonszelmann force-pushed the hir-attributes branch 2 times, most recently from 1920abd to cefa0ea Compare October 17, 2024 22:02
@cjgillot cjgillot self-assigned this Oct 17, 2024
@nnethercote
Copy link
Contributor

First, the hir stats changed slightly. Mostly because Attribute didn't used to be in there.

I see this line:

hir-stats Attribute 128 ( 1.4%) 4 32

replaced with this line lower down:

hir-stats Attribute 384 ( 4.2%) 4 96

So the size has tripled?

@jdonszelmann
Copy link
Contributor Author

First, the hir stats changed slightly. Mostly because Attribute didn't used to be in there.

I see this line:

hir-stats Attribute 128 ( 1.4%) 4 32

replaced with this line lower down:

hir-stats Attribute 384 ( 4.2%) 4 96

So the size has tripled?

I think you're right. This slipped through while it really shouldn't have, investigating... It seems that I forgot a layer of indirection which ast::Attribute had which is probably quite valuable. I'm choosing to make these boxes even though it's in HIR because that's what ast::Attribute also used to do in hir, and because at some point I think we can mostly remove these layers of indirection when preparsing.

@jdonszelmann
Copy link
Contributor Author

@nnethercote well that did it, the file even disappeared from the diff cause they're equal now 😊

@jdonszelmann jdonszelmann force-pushed the hir-attributes branch 2 times, most recently from db44f71 to 823a920 Compare October 18, 2024 07:07
@bors
Copy link
Contributor

bors commented Oct 18, 2024

☔ The latest upstream changes (presumably #131892) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustdoc-json Area: Rustdoc JSON backend PG-exploit-mitigations Project group: Exploit mitigations 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.