[traviscross]

Let's link the relevant UI tests to the Reference's rules for unions.

[traviscross]

(As work within the scope of the lang-docs team, this is outside of the scope of the rust-forge#1040 policy; nonetheless, I'll note this work was created with the assistance of AI tools and that this has been prearranged with all reviewers.)

r? @traviscross (placeholder for @PLeVasseur to review while we fix permissions)

[DanielEScherzer]

mostly looks good to me, one rule addition I'm confused by

View changes since this review

[DanielEScherzer]

This doesn't seem to be testing the common storage so much as something like expr.mut.valid-places if I'm reading this correctly (for the second error) and items.union.init.intro for the initialization

[DanielEScherzer]

Test was added in #53258 to "add tests for assigning fields without initializing var", though the "This should never be allowed" was in the original form of the test, src/test/ui/nll/reassignment_immutable_fields_overlapping.rs

[traviscross]

Thanks. The Reference identifiers are meant to match the intent of the test, and the intent of this test, as described in the comment, is:

// This should never be allowed -- `foo.a` and `foo.b` are
// overlapping, so since `x` is not `mut` we should not permit
// reassignment.

Similarly, the test function is unsafe fn overlapping_fields(). So I think this Reference rule is correct, in the sense of matching the intent of the test.

But, similar to you, it's not immediately clear to me how the test itself matches its stated intent.

[traviscross]

I looked into this. What it's testing here is subtle, and it's made more subtle by the fact that the test doesn't annotate the error messages. Note that if we use mut on the binding, it's still an error:

union U { a: u8, b: u8 }
let mut x: U;
x.a = 1; //~ ERROR partially assigned binding `x` isn't fully initialized
x.b = 2;

(I.e., we have to fully initialize x either in the let or via an assignment to x.)

Without the mut, we get:

union U { a: u8, b: u8 }
let x: U;
x.a = 1; //~ ERROR partially assigned binding `x` isn't fully initialized
x.b = 2; //~ ERROR cannot assign to `x.b`, as `x` is not declared as mutable

What matters here is that the two error messages are different. Due to common storage, any write to the union marks the entire union as initialized. So when we get to the assignment to x.b, we fall through and notice that the binding isn't mut. If this were a struct, that wouldn't happen.

That's pretty subtle and compiler-specific, though, so I'll remove this annotation. Thanks for flagging this.

[PLeVasseur]

LGTM

r=me

View changes since this review

[traviscross]

Thanks @PLeVasseur, @DanielEScherzer.

@bors r=PLeVasseur,traviscross rollup

[rust-bors]

📌 Commit 6d7d075 has been approved by PLeVasseur,traviscross

It is now in the queue for this repository.

[traviscross]

@bors r-

I'm thinking about moving the Reference annotations to the bottom to minimize the churn on .stderr files as these annotations change. I'll evaluate that and then may update this PR accordingly.

[rust-bors]

This pull request was unapproved.

View changes since this unapproval

[traviscross]

Though it's less usual for directives to be at the bottom, the Reference identifiers are special. These are going to change more often than the other directives. Since we don't anonymize line numbers on --> lines pointing to tests, changes to these annotations would otherwise create a lot of churn, and it seems worth avoiding that. In the series of commits I'm working on for adding these annotations, putting them at the bottom dramatically reduces this churn. So let's do that.

I've moved the annotations to the bottom. They're otherwise the same.

@bors r=PLeVasseur,traviscross rollup

[rust-bors]

📌 Commit 205d6ac has been approved by PLeVasseur,traviscross

It is now in the queue for this repository.