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.

Sign up for GitHub

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

Jump to bottom

Support fetching Attribute of items. #127022

Merged
merged 2 commits into from
Jun 28, 2024
Merged

Support fetching Attribute of items. #127022

merged 2 commits into from
Jun 28, 2024

Conversation

adwinwhite
Copy link
Contributor

Fixes rust-lang/project-stable-mir#83

rustc_ast::ast::Attribute doesn't impl Hash and Eq. Thus it cannot be directly used as key of IndexMap in rustc_smir::rustc_smir::Tables and we cannot define stable Attribute as index to rustc_ast::ast::Attribute like Span and many other stable definitions.

Since an string (or tokens) and its span contain all info about an attribute, I defined a simple Attribute struct on stable side.

I choose to fetch attributes via tcx::get_attrs_by_path() due to get_attrs() is marked as deprecated and get_attrs_by_name() cannot handle name of multiple segments like rustfmt::skip.

r? @celinval

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2024

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

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.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 27, 2024
celinval
celinval reviewed Jun 27, 2024
View reviewed changes
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

That's awesome! Thanks!

If you have the time, can you please add an option that returns all attributes attached to the definition? Not a blocker though, since we can add this later too.

compiler/stable_mir/src/ty.rs Outdated Show resolved Hide resolved
compiler/stable_mir/src/crate_def.rs Show resolved Hide resolved
@adwinwhite
Copy link
Contributor Author

adwinwhite commented Jun 28, 2024 •
edited
Loading

If you have the time, can you please add an option that returns all attributes attached to the definition?

I did have a method to return all attributes, but then I removed it when I found out the internal method is marked to be removed in the TyCtxt's source code and it's only used in several places in librustdoc and clippy, not in rustc itself.

Should I add the method back?

@adwinwhite
Copy link
Contributor Author

Well, even if the internal method is removed in the future. We can implement it ourselves using other methods on TyCtxt so it's not a problem.

I'll add a method to get all attributes on a definition.

@celinval
Copy link
Contributor

Nice!! @bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 28, 2024

📌 Commit 9387b0b has been approved by celinval

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 28, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2024
@bors
Auto merge of rust-lang#127076 - matthiaskrgr:rollup-l01gm36, r=matth…
99f77a2 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#124741 (patchable-function-entry: Add unstable compiler flag and attribute)
 - rust-lang#126470 (make cargo submodule optional)
 - rust-lang#126956 (core: avoid `extern type`s in formatting infrastructure)
 - rust-lang#126970 (Simplify `str::clone_into`)
 - rust-lang#127022 (Support fetching `Attribute` of items.)
 - rust-lang#127058 (Tighten `fn_decl_span` for async blocks)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d730f27 into rust-lang:master Jun 28, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2024
@rust-timer
Unrolled build for rust-lang#127022
f5ff54a 
Rollup merge of rust-lang#127022 - adwinwhite:attrs, r=celinval

Support fetching `Attribute` of items.

Fixes [https://github.com/rust-lang/project-stable-mir/issues/83](https://github.com/rust-lang/project-stable-mir/issues/83)

`rustc_ast::ast::Attribute` doesn't impl `Hash` and `Eq`. Thus it cannot be directly used as key of `IndexMap` in `rustc_smir::rustc_smir::Tables` and we cannot define stable `Attribute` as index to `rustc_ast::ast::Attribute` like `Span` and many other stable definitions.

Since an string (or tokens) and its span contain all info about an attribute, I defined a simple `Attribute` struct on stable side.

I choose to fetch attributes via `tcx::get_attrs_by_path()` due to `get_attrs()` is marked as deprecated and `get_attrs_by_name()` cannot handle name of multiple segments like `rustfmt::skip`.

r? `@celinval`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@celinval celinval celinval left review comments

Assignees

@celinval celinval

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.

Provide access to item attributes
4 participants
@adwinwhite @rustbot @celinval @bors