Skip to content

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

Open
Copilot wants to merge 5 commits into
mainfrom
copilot/fix-structlayoutattribute-size
Open

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

Conversation

Copilot AI commented May 18, 2026

Copy link
Copy Markdown
Contributor

Struct unions with no declared payload fields were emitted with StructLayout(Size = 1), even though they are not actually empty value types. 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. Multi-case struct unions carry a hidden tag field; single-case struct unions rely on the CLR's minimum-1-byte guarantee for value types, so no explicit size is needed in either case.
  • Regression coverage

    • Update the existing component test for multi-case data-less struct unions to assert that the emitted IL no longer contains:
      .pack 0
      .size 1
    • Add a regression test for a single-case struct DU ([<Struct>] type X = | Y) to document behavior for that edge case.
    • Keep the existing coverage for struct unions with instance fields unchanged.

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

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

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

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

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


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

T-Gro and others added 2 commits May 25, 2026 14:25
…ngle-case test

- Simplify IlxGen layout logic by removing the always-true tycon.IsUnionTycon
  check inside the TFSharpUnion match arm (dead else branch)
- Update comment to accurately describe behavior for both multi-case and
  single-case struct unions
- Add regression test for single-case struct DU ([<Struct>] type X = | Y)
- Add release notes entry

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro

T-Gro commented May 29, 2026

Copy link
Copy Markdown
Member

@copilot : Address the feedback above.

Copilot AI commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

@copilot : Address the feedback above.

All three points were already addressed in c4c3941: the dead else branch was removed (layout is now if isStructTy g thisTy then Sequential { Size = None; Pack = None } else Auto), the comment was corrected to describe both multi-case and single-case struct unions, and a single-case struct DU regression test was added. Verified building and the StructLayout tests pass.

Copilot finished work on behalf of T-Gro May 29, 2026 11:34
@T-Gro T-Gro marked this pull request as ready for review June 4, 2026 12:37
@T-Gro T-Gro requested a review from a team as a code owner June 4, 2026 12:37
@T-Gro T-Gro requested a review from abonie June 4, 2026 12:37
@T-Gro T-Gro enabled auto-merge (squash) June 4, 2026 12:37
@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Jun 4, 2026
@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jun 8, 2026
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 AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Wrong StructLayoutAttribute.Size for struct unions with no data fields

3 participants