Skip to content

Correct StructLayout size emission for data-less struct unions#19759

Draft
Copilot wants to merge 2 commits into
mainfrom
copilot/fix-structlayoutattribute-size
Draft

Correct StructLayout size emission for data-less struct unions#19759
Copilot wants to merge 2 commits into
mainfrom
copilot/fix-structlayoutattribute-size

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 18, 2026

Struct unions with no declared payload fields were emitted with StructLayout(Size = 1), even though the compiler-generated tag field makes the actual runtime size larger. This updates union codegen so data-less struct unions no longer advertise an invalid explicit size.

  • Codegen

    • Adjust the struct-union layout path in IlxGen.fs to stop treating struct unions as empty structs.
    • For struct unions, emit sequential layout without forcing Size = 1, allowing the generated tag field to determine the effective size.
  • Regression coverage

    • Update the existing component test for data-less struct unions to assert that the emitted IL no longer contains:
      .pack 0
      .size 1
      
    • Keep the existing coverage for struct unions with instance fields unchanged.
  • Example

    [<Struct>]
    type ABC =
        | A
        | B
        | C

    Before, this could be emitted with StructLayout(Size = 1). With this change, the compiler no longer emits that incorrect explicit size for the struct union.

Copilot AI changed the title [WIP] Fix StructLayoutAttribute.Size for struct unions without data fields Correct StructLayout size emission for data-less struct unions May 18, 2026
Copilot finished work on behalf of T-Gro May 18, 2026 11:59
Copilot AI requested a review from T-Gro May 18, 2026 11:59
Copy link
Copy Markdown
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Correct StructLayout size emission for data-less struct unions

The fix is correct for the motivating scenario — multi-case all-nullary struct unions (e.g., [<Struct>] type ABC = A | B | C) were incorrectly emitted with .size 1 even though the compiler generates a _tag field that makes them non-empty. Removing the explicit size is the right call.

Issues to address

1. Dead else branch (bug)

The condition at line 12036:
sharp if tycon.IsUnionTycon then ILTypeDefLayout.Sequential { Size = None; Pack = None } else ILTypeDefLayout.Sequential { Size = Some 1; Pack = Some 0us }

This code lives inside the TFSharpUnion match arm (line 12000–12003), so tycon.IsUnionTycon is always true here. The else branch is unreachable dead code.

The intent should be simplified to:
sharp let layout = if isStructTy g thisTy then ILTypeDefLayout.Sequential { Size = None; Pack = None } else ILTypeDefLayout.Auto

2. Inaccurate comment

// Struct unions always carry a hidden tag field, so never emit size 1 here

This is not true for single-case struct unions (e.g., [<Struct>] type Foo = | Bar). Those use SingleCaseStruct layout which has NoTagField — no _tag field is emitted. The fix still works correctly for that case (CLR guarantees ≥1 byte for value types without explicit size), but the comment misleads future readers.

Suggested:
sharp // Multi-case struct unions carry a hidden tag field; single-case struct unions // are handled by the CLR's minimum-1-byte guarantee. No explicit size needed.

Minor

  • The test rename is good. Asserting that .pack 0 / .size 1 are absent properly validates the fix.
  • Consider adding a single-case struct union ([<Struct>] type X = | Y) to the test suite to document behavior in that edge case, since the original Size=1 annotation is being removed for all struct unions, not just multi-case ones.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label May 20, 2026
@T-Gro T-Gro self-requested a review May 20, 2026 12:49
@github-actions
Copy link
Copy Markdown
Contributor

❗ Release notes required

@copilot,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md No release notes found or release notes format is not correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewed PR reviewed by AI review council

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

Wrong StructLayoutAttribute.Size for struct unions with no data fields

2 participants