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

disallow repr() on invalid items #133925

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Dec 5, 2024

fixes #129606
fixes #132391

Disallows repr() (so a repr with no arguments) on items where that won't ever make sense.

Also this generates an error when repr is used on a trait method and the fn_align feature is not enabled. Looks like that was missed here:

https://github.com/rust-lang/rust/pull/110313/files

Which first accepts the align attribute on trait methods.

r? @compiler-errors

cc @jdonszelmann who claimed #132391 and generally has been working on attributes

Also this generates an error when `repr` is used on a trait method and
the `fn_align` feature is not enabled. Looks like that was missed here:

https://github.com/rust-lang/rust/pull/110313/files

Which first enables the align attribute on trait methods.
@rustbot rustbot added 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 Dec 5, 2024
@compiler-errors
Copy link
Member

@bors try

@compiler-errors
Copy link
Member

Nominating as a courtesy for T-lang, should be a trivial decision since this was likely never intended

@compiler-errors compiler-errors added the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination label Dec 5, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
…<try>

disallow `repr()` on invalid items

fixes rust-lang#129606

Disallows `repr()` (so a repr with no arguments) on items where that won't ever make sense.

Also this generates an error when `repr` is used on a trait method and the `fn_align` feature is not enabled. Looks like that was missed here:

https://github.com/rust-lang/rust/pull/110313/files

Which first accepts the align attribute on trait methods.

r? `@compiler-errors`
@bors
Copy link
Contributor

bors commented Dec 5, 2024

⌛ Trying commit dfd76c1 with merge 617b810...

@rust-log-analyzer

This comment has been minimized.

@jdonszelmann
Copy link
Contributor

Oh nice! If this fixes an issue you have right now, great! Especially the tests are nice so I can't mess it up again, but the code in check_attrs.rs will likely be deleted within a month. (sorry)

@folkertdev
Copy link
Contributor Author

well this change unintentionally turns out to fix that ICE. And yeah the test coverage is the most important part, I'm not at all attached to this code. Good luck with your rebase!

@jdonszelmann
Copy link
Contributor

jdonszelmann commented Dec 5, 2024

lol thanks, can't be much worse than the current one (I'm 4 days in)

@compiler-errors
Copy link
Member

r=me when T-lang comes back w/ a decision

@rustbot team

@compiler-errors compiler-errors added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed 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 Dec 10, 2024
@traviscross
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 8, 2025

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 8, 2025
@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 8, 2025
@tmandry
Copy link
Member

tmandry commented Jan 8, 2025

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 8, 2025
@rfcbot
Copy link

rfcbot commented Jan 8, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@compiler-errors
Copy link
Member

I started a try build but never ran crater. Let's do that really quick since crater is empty.

@craterbot check

@craterbot

This comment was marked as outdated.

@compiler-errors
Copy link
Member

Oh, it's because the try build was canceled because you pushed another commit.

@bors try

@bors
Copy link
Contributor

bors commented Jan 18, 2025

⌛ Trying commit a4bb0d4 with merge a75eafc...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2025
…<try>

disallow `repr()` on invalid items

fixes rust-lang#129606
fixes rust-lang#132391

Disallows `repr()` (so a repr with no arguments) on items where that won't ever make sense.

Also this generates an error when `repr` is used on a trait method and the `fn_align` feature is not enabled. Looks like that was missed here:

https://github.com/rust-lang/rust/pull/110313/files

Which first accepts the align attribute on trait methods.

r? `@compiler-errors`

cc `@jdonszelmann` who claimed rust-lang#132391 and generally has been working on attributes
@bors
Copy link
Contributor

bors commented Jan 18, 2025

☀️ Try build successful - checks-actions
Build commit: a75eafc (a75eafceea413a1b40c0e0769db85e08ec17160b)

@traviscross traviscross removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 19, 2025
@folkertdev
Copy link
Contributor Author

@compiler-errors did you still want to run crater here?

@compiler-errors
Copy link
Member

yes let's do that

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-133925 created and queued.
🤖 Automatically detected try build a75eafc
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jan 21, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-133925 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-133925 is completed!
📊 6 regressed and 3 fixed (571795 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 24, 2025
@folkertdev
Copy link
Contributor Author

I don't think the crater run showed any serious issues? Or in any case nothing related to this change as far as I can see.

@compiler-errors
Copy link
Member

Crater looks fine.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 6, 2025

📌 Commit a4bb0d4 has been approved by compiler-errors

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 Feb 6, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2025
…r=compiler-errors

disallow `repr()` on invalid items

fixes rust-lang#129606
fixes rust-lang#132391

Disallows `repr()` (so a repr with no arguments) on items where that won't ever make sense.

Also this generates an error when `repr` is used on a trait method and the `fn_align` feature is not enabled. Looks like that was missed here:

https://github.com/rust-lang/rust/pull/110313/files

Which first accepts the align attribute on trait methods.

r? `@compiler-errors`

cc `@jdonszelmann` who claimed rust-lang#132391 and generally has been working on attributes
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#133925 (disallow `repr()` on invalid items)
 - rust-lang#135549 (Document some safety constraints and use more safe wrappers)
 - rust-lang#136069 (Simplify slice indexing in next trait solver)
 - rust-lang#136152 (Stabilize `map_many_mut` feature)
 - rust-lang#136219 (Misc. `rustc_hir` cleanups 🧹)
 - rust-lang#136580 (Couple of changes to run rustc in miri)
 - rust-lang#136636 (Couple of minor cleanups to the diagnostic infrastructure)
 - rust-lang#136645 (Clippy subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133925 (disallow `repr()` on invalid items)
 - rust-lang#136069 (Simplify slice indexing in next trait solver)
 - rust-lang#136152 (Stabilize `map_many_mut` feature)
 - rust-lang#136219 (Misc. `rustc_hir` cleanups 🧹)
 - rust-lang#136580 (Couple of changes to run rustc in miri)
 - rust-lang#136636 (Couple of minor cleanups to the diagnostic infrastructure)
 - rust-lang#136645 (Clippy subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133925 (disallow `repr()` on invalid items)
 - rust-lang#136069 (Simplify slice indexing in next trait solver)
 - rust-lang#136152 (Stabilize `map_many_mut` feature)
 - rust-lang#136219 (Misc. `rustc_hir` cleanups 🧹)
 - rust-lang#136580 (Couple of changes to run rustc in miri)
 - rust-lang#136636 (Couple of minor cleanups to the diagnostic infrastructure)
 - rust-lang#136645 (Clippy subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4b7e55a into rust-lang:master Feb 7, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 7, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
Rollup merge of rust-lang#133925 - folkertdev:improve-repr-warnings, r=compiler-errors

disallow `repr()` on invalid items

fixes rust-lang#129606
fixes rust-lang#132391

Disallows `repr()` (so a repr with no arguments) on items where that won't ever make sense.

Also this generates an error when `repr` is used on a trait method and the `fn_align` feature is not enabled. Looks like that was missed here:

https://github.com/rust-lang/rust/pull/110313/files

Which first accepts the align attribute on trait methods.

r? `@compiler-errors`

cc `@jdonszelmann` who claimed rust-lang#132391 and generally has been working on attributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: malformed malformed repr(align(N)) #[repr()] is allowed where it shouldn't