Skip to content

Rewrite inline attribute parser to use new infrastructure and improve diagnostics for all parsed attributes #138165

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
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Rewrite inline attribute parser to use new infrastructure and improve diagnostics for all parsed attributes #138165

wants to merge 8 commits into from
+1,189 −647

Conversation

jdonszelmann
Copy link
Contributor

@jdonszelmann jdonszelmann commented Mar 7, 2025 •
edited
Loading

r? @oli-obk

This PR:

Builds on top of #138164

Closes #137950

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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. labels Mar 7, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
jdonszelmann
jdonszelmann commented Mar 8, 2025
View reviewed changes
compiler/rustc_codegen_ssa/src/codegen_attrs.rs
@@ -584,7 +546,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
return OptimizeAttr::Default;
};

inline_span = Some(attr.span());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes #137950

@jdonszelmann jdonszelmann force-pushed the inline branch 2 times, most recently from e6dad01 to 2a707bf Compare March 9, 2025 15:34
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jdonszelmann jdonszelmann mentioned this pull request Mar 9, 2025
Open
@bors
Copy link
Collaborator

bors commented Mar 11, 2025

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

@oli-obk oli-obk 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 28, 2025
@jdonszelmann jdonszelmann force-pushed the inline branch 2 times, most recently from b7dda57 to 88a713a Compare June 9, 2025 22:40
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jdonszelmann jdonszelmann mentioned this pull request Jun 9, 2025
Merged
@jdonszelmann
Copy link
Contributor Author

@rustbot review

@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 9, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jdonszelmann jdonszelmann mentioned this pull request Jun 9, 2025
Merged
@jdonszelmann
Copy link
Contributor Author

ah shit, broke clippy 😭

oli-obk
oli-obk reviewed Jun 16, 2025
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jdonszelmann jdonszelmann force-pushed the inline branch 2 times, most recently from 220615f to 43dd618 Compare June 16, 2025 17:16
@rustbot
Copy link
Collaborator

rustbot commented Jun 16, 2025

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

@jdonszelmann
Copy link
Contributor Author

I think that addresses all the review comments by others @oli-obk :)

@bors
Copy link
Collaborator

bors commented Jun 16, 2025

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

aDotInTheVoid
aDotInTheVoid reviewed Jun 16, 2025
View reviewed changes
src/rustdoc-json-types/lib.rs Outdated
@@ -30,7 +30,7 @@ pub type FxHashMap<K, V> = HashMap<K, V>; // re-export for use in src/librustdoc
/// This integer is incremented with every breaking change to the API,
/// and is returned along with the JSON blob as [`Crate::format_version`].
/// Consuming code should assert that this value matches the format version(s) that it supports.
pub const FORMAT_VERSION: u32 = 46;
pub const FORMAT_VERSION: u32 = 47;
Copy link
Member

Choose a reason for hiding this comment

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

If this PR isn't bitrotty or urgent, it'd be really nice if could land after #142601 (and a rebase), so it's guaranteed not to race with #142502 to do the same bump. (#142601 fixes this by making this a conflict, so we'll never need to manually worry about this again 🤞)

Copy link
Contributor Author

@jdonszelmann jdonszelmann Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this one seems to have few conflicts, will preemptively bump to 48

@rust-log-analyzer

This comment has been minimized.

Sign in to view
oli-obk
oli-obk approved these changes Jun 17, 2025
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

r=me whenever the rustdoc json version bump conflict is not an issue

@jdonszelmann
Copy link
Contributor Author

@bors r=oli_obk rollup=iffy

@bors
Copy link
Collaborator

bors commented Jun 17, 2025

📌 Commit 81f8b57 has been approved by oli_obk

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 17, 2025
@fmease
Copy link
Member

fmease commented Jun 17, 2025

It's a dash, not an underscore. Otherwise oli won't get proper attribution on thanks.r-l.org ;)

@bors r=oli-obk

@bors
Copy link
Collaborator

bors commented Jun 17, 2025

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Collaborator

bors commented Jun 17, 2025

📌 Commit 81f8b57 has been approved by oli-obk

It is now in the queue for this repository.

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

@obi1kenobi obi1kenobi obi1kenobi left review comments

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

@aDotInTheVoid aDotInTheVoid aDotInTheVoid left review comments

@oli-obk oli-obk oli-obk approved these changes

@mejrs mejrs mejrs approved these changes

Assignees

@oli-obk oli-obk

Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-rustdoc-json Area: Rustdoc JSON backend 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
No milestone
Development

Successfully merging this pull request may close these issues.

Diagnostics error in interaction between #[optimize(..)] and #[no_sanitize(..)]
10 participants
@jdonszelmann @rust-log-analyzer @bors @rustbot @fmease @oli-obk @obi1kenobi @GuillaumeGomez @aDotInTheVoid @mejrs