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

Gate repr(Rust) correctly on non-ADT items #129422

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Aug 22, 2024

#114201 added repr(Rust) but didn't add any attribute validation to it like repr(C) has, to only allow it on ADT items.

I consider this code to be nonsense, for example:

#[repr(Rust)]
fn foo() {}

Reminder that it's different from extern "Rust", which is valid on function items. But also this now disallows repr(Rust) on modules, impls, traits, etc.

I'll crater it, if it looks bad then I'll add an FCW.


relnotes Marks issues that should be documented in the release notes of the next release. : Compatibility (minor breaking change).

@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 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 Aug 22, 2024
@compiler-errors
Copy link
Member Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2024
Gate `repr(Rust)` correctly on non-ADT items

rust-lang#114201 added `repr(Rust)` but didn't add any attribute validation to it like `repr(C)` has, to only allow it on ADT items.

I consider this code to be nonsense, for example:
```
#[repr(Rust)]
fn foo() {}
```

Reminder that it's different from `extern "Rust"`, which *is* valid on function items. But also this now disallows `repr(Rust)` on modules, impls, traits, etc.

I'll crater it, if it looks bad then I'll add an FCW.
@bors
Copy link
Contributor

bors commented Aug 22, 2024

⌛ Trying commit 363addc with merge 0edd13c...

@fmease
Copy link
Member

fmease commented Aug 22, 2024

r? fmease
r=me after crater etc.

@rustbot rustbot assigned fmease and unassigned pnkfelix Aug 22, 2024
@bors
Copy link
Contributor

bors commented Aug 22, 2024

☀️ Try build successful - checks-actions
Build commit: 0edd13c (0edd13c68435d50367c79cef43ac2768d2ec4f3d)

@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-129422 created and queued.
🤖 Automatically detected try build 0edd13c
🔍 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 S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
[CRATER] Crater Rollup

This is a " crater rollup" of:
* rust-lang#126452
* rust-lang#128784
* rust-lang#129392
* rust-lang#129422
* rust-lang#129543

**What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build.

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2024
[CRATER] Crater Rollup

This is a " crater rollup" of:
* rust-lang#126452
* rust-lang#128784
* rust-lang#129392
* rust-lang#129422
* rust-lang#129543
* rust-lang#129604

**What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build.

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2024
[CRATER] Crater Rollup

This is a " crater rollup" of:
* rust-lang#126452
* rust-lang#128784
* rust-lang#129392
* rust-lang#129422
* rust-lang#129543
* rust-lang#129604

**What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build.

r? `@ghost`
@craterbot
Copy link
Collaborator

📝 Configuration of the pr-129422 experiment changed.

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

@traviscross
Copy link
Contributor

@craterbot p=1

(Bump priority is this will run quickly.)

@craterbot
Copy link
Collaborator

📝 Configuration of the pr-129422 experiment changed.

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

@craterbot
Copy link
Collaborator

🚧 Experiment pr-129422 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-129422 is completed!
📊 4 regressed and 2 fixed (98236 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ 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 Sep 1, 2024
@compiler-errors
Copy link
Member Author

compiler-errors commented Sep 1, 2024

Given that this is a single crate regression (https://crates.io/crates/list_tools/0.1.9-features) with no dependents, I'd say we should just break it. This is very obviously a bugfix. I would offer to put up a pull request to fix, but the crate seems to be hosted on some other code hosting site (gitee) that I don't have an account for.

I have absolutely no idea what compelled the crate author to put #[repr(Rust)] on a macro_rules definition, though. 🤔 The git history seems to only have 1 commit, so this crate must've either been moved recently or the author squashes their history across versions (which is a shame for the purposes of understanding this situation lol). It must've been somewhat recent though, since repr rust has only been stable for about 6 months or so...

@fmease fmease added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 4, 2024
@fmease fmease added the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 4, 2024
@fmease
Copy link
Member

fmease commented Sep 4, 2024

I-lang-nominated Nominated for discussion during a lang team meeting. : T-lang, do you accept this minor breaking change? Namely, rejecting #[repr(Rust)] (stable since 1.74; Nov. 16, 2023) on non-ADT items. We have a single breakage. See errs's comment above.

@nirenyang This PR will break your crate list_tools (https://crates.io/crates/list_tools/0.1.9-features) because it contains #[repr(Rust)] on items that aren't structs/enums/unions which don't actually have any effect and which shouldn't have been allowed in the first place. You can safely remove them.

E.g., here: https://docs.rs/crate/list_tools/0.1.9-features/source/src/fasts.rs#10 (on macro_rules!), https://docs.rs/crate/list_tools/0.1.9-features/source/src/tools.rs#3 (on mod), https://docs.rs/crate/list_tools/0.1.9-features/source/src/tools.rs#7 (on fn).

@fmease fmease added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2024
@compiler-errors
Copy link
Member Author

T-lang said this specific issue was obvious to fix.

@bors r=fmease rollup

@bors
Copy link
Contributor

bors commented Sep 18, 2024

📌 Commit 363addc has been approved by fmease

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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 18, 2024
@compiler-errors compiler-errors removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 18, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 18, 2024
Gate `repr(Rust)` correctly on non-ADT items

rust-lang#114201 added `repr(Rust)` but didn't add any attribute validation to it like `repr(C)` has, to only allow it on ADT items.

I consider this code to be nonsense, for example:
```
#[repr(Rust)]
fn foo() {}
```

Reminder that it's different from `extern "Rust"`, which *is* valid on function items. But also this now disallows `repr(Rust)` on modules, impls, traits, etc.

I'll crater it, if it looks bad then I'll add an FCW.

---

https://github.com/rust-lang/rust/labels/relnotes: Compatibility (minor breaking change).
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2024
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#97524 (Add `Thread::{into_raw, from_raw}`)
 - rust-lang#127988 (Do not ICE with incorrect empty suggestion)
 - rust-lang#129422 (Gate `repr(Rust)` correctly on non-ADT items)
 - rust-lang#129934 (Win: Open dir for sync access in remove_dir_all)
 - rust-lang#130450 (Reduce confusion about `make_indirect_byval` by renaming it)
 - rust-lang#130476 (Implement ACP 429: add `Lazy{Cell,Lock}::get[_mut]` and `force_mut`)
 - rust-lang#130487 (Update the minimum external LLVM to 18)
 - rust-lang#130513 (Clarify docs for std::fs::File::write)
 - rust-lang#130522 ([Clippy] Swap `manual_retain` to use diagnostic items instead of paths)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2eb65a6 into rust-lang:master Sep 19, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2024
Rollup merge of rust-lang#129422 - compiler-errors:repr-rust, r=fmease

Gate `repr(Rust)` correctly on non-ADT items

rust-lang#114201 added `repr(Rust)` but didn't add any attribute validation to it like `repr(C)` has, to only allow it on ADT items.

I consider this code to be nonsense, for example:
```
#[repr(Rust)]
fn foo() {}
```

Reminder that it's different from `extern "Rust"`, which *is* valid on function items. But also this now disallows `repr(Rust)` on modules, impls, traits, etc.

I'll crater it, if it looks bad then I'll add an FCW.

---

https://github.com/rust-lang/rust/labels/relnotes: Compatibility (minor breaking change).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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
Development

Successfully merging this pull request may close these issues.

7 participants