Skip to content

Port #[target_feature] to new attribute parsing infrastructure #142876

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JonathanBrouwer
Copy link
Contributor

Ports target_feature to the new attribute parsing infrastructure for #131229 (comment)

r? @jdonszelmann

@@ -231,7 +231,10 @@ pub(crate) enum AttributeOrder {
KeepLast,
}

type ConvertFn<E> = fn(ThinVec<E>) -> AttributeKind;
pub(crate) enum ConvertFn<E> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed ConvertFn to an enum so I can also keep track of the span of the first attribute. I use this span to lint on the attribute itselfs (rather than one of the elements) for a few of the diagnostics. Does this seem reasonsable?

@obi1kenobi
Copy link
Member

This should probably bump the rustdoc JSON FORMAT_VERSION too. This attribute is used by cargo-semver-checks today, and should probably also be in the rustdoc JSON test suite as well.

cc @aDotInTheVoid purely for awareness

@JonathanBrouwer
Copy link
Contributor Author

Ah, I missed it because it's not part of the tests for rustdoc-json. I'll make sure to give this attribute the nice formatting too, and to bump the FORMAT_VERSION

@obi1kenobi
Copy link
Member

Ah, I missed it because it's not part of the tests for rustdoc-json. I'll make sure to give this attribute the nice formatting too, and to bump the FORMAT_VERSION

You are my favorite person!

Just noting that this attribute has three valid forms:

  • it can be repeated on the same item, like #[target_feature(enable = "one")] #[target_feature(enable = "another")]
  • it can have multiple enable clauses, like #[target_feature(enable = "one", enable = "another")], and
  • it can have multiple features in the same enable clause, like #[target_feature(enable = "one,another")]

I believe any mix of these is also legal. For nice formatting, I don't have strong feelings how we normalize so long as we do normalize if at all possible. The last form is the most compact and IIRC how the attribute is explained in the docs, so all else equal that's what I'd pick if I were choosing. But feel free to do whatever is easiest.

And yes, entirely reasonable to miss it since we never tested it. We never tested it because it's only the new cargo-semver-checks releasing ~this week that uses it :)

@JonathanBrouwer
Copy link
Contributor Author

JonathanBrouwer commented Jun 22, 2025

I encountered these the hard way when writing the attribute :p I discover so much about Rust when doing these refactorings
Actually, since I do the pretty printing, does the FORMAT_VERSION even need to be bumped? Or does the normalizing count as a change?

@JonathanBrouwer JonathanBrouwer force-pushed the target_feature_parser branch from 8dd448b to 34dd949 Compare June 22, 2025 15:37
@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-rustdoc-json Area: Rustdoc JSON backend 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 Jun 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 22, 2025

These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.

Please ensure that if you've changed the output:

  • It's intentional.
  • The FORMAT_VERSION in src/librustdoc-json-types is bumped if necessary.

cc @aDotInTheVoid, @obi1kenobi

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 compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

@JonathanBrouwer
Copy link
Contributor Author

This push should fix the pretty printing for rustdoc. Thanks for the feedback <3
I didn't end up bumping the FORMAT_VERSION but if I need to lmk

@obi1kenobi
Copy link
Member

I think normalizing to an already-valid form doesn't need to bump the format version, since any downstream consumers would have had to be able to handle it already.

@JonathanBrouwer
Copy link
Contributor Author

@rustbot ready
r? @jdonszelmann

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

rustbot commented Jun 22, 2025

jdonszelmann is currently at their maximum review capacity.
They may take a while to respond.

@jdonszelmann
Copy link
Contributor

@rustbot author

@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 Jun 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 22, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@JonathanBrouwer JonathanBrouwer force-pushed the target_feature_parser branch from 34dd949 to c14ebc4 Compare June 22, 2025 19:37
@bors
Copy link
Collaborator

bors commented Jun 22, 2025

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

@JonathanBrouwer JonathanBrouwer force-pushed the target_feature_parser branch from c14ebc4 to 33e67cb Compare June 22, 2025 21:13
@JonathanBrouwer
Copy link
Contributor Author

Fixed the issues above & rebased on master
@rustbot ready

@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 Jun 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-rustdoc-json Area: Rustdoc JSON backend 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants