More deriving on packed structs by nnethercote · Pull Request #104429 · rust-lan...

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Note that the first three commits are from #104391, and have already been approved.

@RalfJung: even though you don't know the deriving code, there is enough stuff in this PR that you do know about (e.g. unsafe_derive_on_repr_packed) that I would like your review. If you want another co-reviewer as well I think that's fine.

@bors try

Trying commit 1e84976 with merge a31ace6...

Try build successful - checks-actions
Build commit: a31ace6 (a31ace633f6135f7232aa68f2493896256980c6a)

I looked at the testcases only; there are some more that should be added:

• non-generic with Copy+Clone (should get simple clone)
• non-generic with only Clone, no Copy (should work -- just to make sure this kicks in even when there are no generics)
• generic using an associated type (needs the extra bound on the where)

Looks like we don't even have a test for generic+associated without packed; that would be good to have, too.

The changes in check_packed_ref are fine, too.

I'll hand over review of the actual implementation to someone else, don't have the time to dig into the derive code currently, sorry. r? compiler

@bors try

Trying commit f23921e with merge b2686d7...

Try build successful - checks-actions
Build commit: b2686d7 (b2686d78d0034bb3435b348d5a63910b074a1990)

@craterbot check

Experiment pr-104429 created and queued.
Automatically detected try build b2686d7
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

Experiment pr-104429 is now running

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

Experiment pr-104429 is completed!
70 regressed and 15 fixed (248247 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

@RalfJung: I have added the lint. Could you check it, make sure it all looks reasonable? Grep for U8Slice/U8_SLICE/u8_slice. Thanks.

@RalfJung: I have added the lint. Could you check it, make sure it all looks reasonable? Grep for U8Slice/U8_SLICE/u8_slice. Thanks.

On the suggestion of @Nilstrieb, I renamed the lint. Search now for BYTE_SLICE_/ByteSlice/byte_slice.

Yeah, the lint looks good to me. :)

We discussed this in today's @rust-lang/lang meeting. Because this breaks multiple existing crates (even though some of them have been fixed), we'd like to see this done over an edition boundary. This doesn't seem like something so urgent that it can't wait for Rust 2024, to avoid a breaking change.

@nnethercote This comment (also in the commit description) are very helpful but difficult to find; could you please make it more easily discoverable from the PR description?

@joshtriplett so what is the plan then for #102513? That got approved by t-lang, but seems hard to land without landing this one here first (on all editions).

Also AFAIK the policy for edition changes requires warnings or even fixes from the previous edition. That does not seem possible with reasonable effort here.

@tmandry: Sorry this was hard to navigate.

The comment you mentioned is a good overview, but unfortunately it omitted one important detail. This other comment, addressed to @rust-lang/lang, mentions this important detail:

Besides allowing a lot more derive on packed structs, this PR unblocks #102513 by keeping old winapi-0.2 working even when references-to-packed-fields are a hard error.

(I have linked to the latter comment in the description.)

If you only saw the former comment, you'd think that this PR makes the language nicer, at the cost of breaking a handful of obscure crates. If you see the latter comment, it's more obvious that this PR does that, while also unbreaking the winapi-0.2 crate, which has been broken by the ongoing work to disallow unaligned references. I suspect that the amount of unbreaking caused by this PR in the crate ecosystem is significant, more so than the breaking caused by this PR, though @RalfJung would know for sure.

If the lang-team considered this, that's fine, I just want to make sure it wasn't overlooked because it's an important point and the structure of this PR's comments means it would have been easy to overlook.

@nnethercote That this unbreaks any crates was not something we were clear on, no. I'll leave this nominated for consideration.

Great, thanks. Sorry again for the lack of clarity.

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

@RalfJung I'd say it's a bit of both. One of the issues (at least for me) was that it took some digging to find the comment because of how GH hides comments in long threads. If I were to suggest a change here, it would be to edit the PR description to link directly to comments about nominations in PRs with long threads. Maybe we/I could also be more diligent in looking for that comment first, but not every nominated PR has one.

We did eventually see it, but it also wasn't immediately clear that this unbroke things. Your comment linked to an issue that I think implies this, and references the fact at the very end, but on the kind of skim that tends to happen during t-lang meetings it definitely doesn't jump out. (Again, we might've figured this out if we'd spent more time looking at your comment, but we spent more time looking around for other details about the PR before finding it.)

Hopefully this is helpful in some way. It's definitely just incremental changes that could improve things a bit, nothing groundbreaking :). And thanks to you and @nnethercote for clarifying!

It's still useful feedback, thanks. :) Writing these nomination comments is not easy, so at last I know one more potential pitfall now. The biggest problem however is as usual that I have all the context in my head, so it is hard to remember all the things I need to explain to someone who hasn't spent all that time thinking about the issue already... (like in this case, the relation to #102513 and that this helps unbreak things)

I have updated the nomination comment, and it is linked directly from the PR description now.

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

We discussed this in the lang-team meeting, taking into account the fact that this unbreaks a bunch of crates (notably winapi), as well as Ralf's comment about the general incoherence of the current rules.

Meeting consensus was that, even though this is a breaking change, we should...

@rfcbot fcp merge

...because:

• the current rules are incoherent and hard to use correctly or reliably
• this seems like it would fix more crates than it breaks
• there is limited breakage in practice, and not major breakage

Team member @nikomatsakis 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!

See this document for info about what commands tagged team members can give me.

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