Type called union wreaks havoc since 1.54

[dtolnay]

When C-compatible untagged unions were introduced by RFC 1444, it was careful not to break existing valid uses of union identifier, such as https://doc.rust-lang.org/std/collections/struct.BTreeSet.html#method.union.

However, the unions from RFC 2102 (#49804) appear to be more disruptive, at least as currently implemented. The following stable code works with every stable rustc from 1.0.0 through 1.53.0, but can no longer be parsed by rustc 1.54.0 or current nightly.

struct union;

impl union {
    pub fn new() -> Self {
        unimplemented!()
    }
}

fn main() {}

error: functions are not allowed in union definitions
 --> src/main.rs:4:5
  |
4 | /     pub fn new() -> Self {
5 | |         unimplemented!()
6 | |     }
  | |_____^
  |
  = help: unlike in C++, Java, and C#, functions are declared in `impl` blocks
  = help: see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information

error: expected `(`, found `main`
 --> src/main.rs:9:4
  |
9 | fn main() {}
  |    ^^^^ expected `(`

@jedel1043 @joshtriplett

Labeling T-compiler for what to do about the regression and T-lang because it's not clear to me what the grammar of anonymous union is intended to be from the parser's perspective.

[apiraino]

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-critical

[joshtriplett]

Anonymous unions were not intended to change the "contextual keyword" nature of union. Anonymous unions do allow the keyword union to appear in more places, but impl union { should parse as the implementation of a type named union.

I'd suggest temporarily reverting the parsing of anonymous unions until the parsing can be fixed to handle this case correctly. We should also add a test case covering this specific usage, as well as other possibilities:

struct union { field: u32 };

impl union {
    pub fn new() -> Self {
        unimplemented!()
    }
}

type union = u32;
struct S { field: union };

struct union { field: u32 };
struct S { field: union };
fn main() {
    let s = S { field: union { field: 42 } };
}

[dtolnay]

Is the following code intended to compile as envisioned by the anonymous unions RFC (sans any future expansion of the feature)?

#[cfg(any())]
type A = union { field: u32 };

Relatedly—?

macro_rules! ty {
    ($ty:ty) => {};
}

ty!(union { field: u32 });

[joshtriplett]

@dtolnay The first wouldn't be allowed by the base RFC as written, no. This would be allowed by the "General anonymous types" possibility discussed at https://github.com/rust-lang/rfcs/blob/master/text/2102-unnamed-fields.md#general-anonymous-types , but that wasn't the approach that was approved in the RFC.

For the second, I'm not sure. An anonymous union (or anonymous struct) can't appear everywhere a type can appear, only as a field type for a field named _ in a struct or union. In theory you should be able to generate that field definition via a macro. But I think parsing union { ... } generically as a type would require "General anonymous types" again.

[dtolnay]

Okay. In that case the implementation approach from #84571 probably significantly has to change. According to the test:

rust/src/test/pretty/anonymous-types.rs

Lines 19 to 22 in 1cf8fdd

	type A =
	struct {
	field: u8,
	};

they seem to have deferred validation until after parsing anonymous struct/union in arbitrary type position, rather than parsing it only in the narrow context you've described.

@petrochenkov

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting. @pnkfelix and I both agree that this should be fixed by reverting the current preliminary parsing support for anonymous unions; we can re-add it when it no longer causes this breakage.

We also think this fix needs to get into 1.56 (due to the edition).

Finally, we really need to add more test cases to catch more uses of union as a non-keyword, before we try to re-add parsing support.

[pnkfelix]

nominating to ensure this gets eyeballs at (or before) compiler triage meeting, mostly to ensure the aforementioned revert gets done.

[pnkfelix]

Reversion PR posted in PR #88775

[estebank]

Looking at the parser changes, I believe I can fix all potentially problematic issues by gating the bare types to only be allowed in ADT field types and using the "parser snapshot" approach in all other cases (clone the parser, try to parse normally, if that fails try the alternate –bare union/struct– parse and if that succeeds, complain about it). Looking at the RFC, these were only ever supposed to be supported in named field types, right? Namely, should they be accepted in tuple structs and variants?

[estebank]

Opened #88815 with more restrictive parsing, without reverting the changes already in stable.