diff --git a/.github/instructions/TypedTreePickle.instructions.md b/.github/instructions/TypedTreePickle.instructions.md index 9314ccec74d..f7af53cf6c5 100644 --- a/.github/instructions/TypedTreePickle.instructions.md +++ b/.github/instructions/TypedTreePickle.instructions.md @@ -1,6 +1,7 @@ --- applyTo: - "src/Compiler/TypedTree/TypedTreePickle.{fs,fsi}" + - "src/Compiler/TypedTree/TypedTree.{fs,fsi}" - "src/Compiler/Driver/CompilerImports.{fs,fsi}" --- @@ -21,6 +22,12 @@ This means: 2. **Additions must be invisible to old readers.** New data goes in stream B, where readers that don't know about it get `0` (the default sentinel) past end-of-stream. New readers detect presence via a tag byte they write unconditionally. 3. **Tag values are forever.** Once a byte value means something in a reader's `match`, that meaning cannot change. Old DLLs encode that value with the old semantics. +## Flag Enums: Reinterpreting a Tag Breaks Old Binaries + +The `ValFlags`, `EntityFlags`, and `TyparFlags` types in `TypedTree.fs` pack enum cases into bit patterns exposed as `PickledBits` and serialized verbatim. Adding a case to such an enum must **not** reuse a bit pattern that already exists in shipped metadata with different semantics. Normalizing the new case on the *write* side protects only future binaries — an older compiler already emitted the old pattern into DLLs that exist permanently. If you reuse a pattern, add matching *read*-side normalization (see `ValFlags.OfPickledBits`) that maps the legacy pattern back to its original meaning; otherwise prefer an unused pattern. + +For a detailed example of what goes wrong when a serialized flag pattern is reinterpreted, see `docs/postmortems/regression-legacy-inline-metadata-dynamic-invocation.md`. + ## Reading and Writing Must Be Perfectly Aligned The format uses two parallel byte streams. Every `p_*` (write) function has a corresponding `u_*` (read) function. They must produce and consume the **exact same byte sequence** under **every possible code path** — including paths gated by feature flags, language versions, or target frameworks that your current build may not exercise. diff --git a/docs/postmortems/README.md b/docs/postmortems/README.md index f545a92baaf..5fa97424857 100644 --- a/docs/postmortems/README.md +++ b/docs/postmortems/README.md @@ -3,3 +3,8 @@ Detailed write-ups of bugs that were hard to diagnose, had non-obvious root causes, or taught us something worth preserving. Each document captures the symptoms, root cause, fix, and timeline so that future contributors can recognize similar patterns early. These are referenced from [agentic instructions](../../.github/instructions/) and serve as deeper reading — the instructions tell you *what* to do, the postmortems explain *why* the rules exist. + +## Index + +- [`regression-fs0229-bstream-misalignment.md`](regression-fs0229-bstream-misalignment.md) — a conditional write with an unconditional read shifted the pickle B-stream, producing `FS0229` when reading older metadata. +- [`regression-legacy-inline-metadata-dynamic-invocation.md`](regression-legacy-inline-metadata-dynamic-invocation.md) — a new inline-flag case reused a serialized bit pattern that already meant "required inline" in F# 5 binaries, breaking cross-assembly SRTP at runtime. diff --git a/docs/postmortems/regression-legacy-inline-metadata-dynamic-invocation.md b/docs/postmortems/regression-legacy-inline-metadata-dynamic-invocation.md new file mode 100644 index 00000000000..4c62d9ceb61 --- /dev/null +++ b/docs/postmortems/regression-legacy-inline-metadata-dynamic-invocation.md @@ -0,0 +1,65 @@ +# Regression: Legacy inline metadata decoded as non-inline, breaking cross-assembly SRTP + +## Summary + +Adding a new `ValInline.InlinedDefinition` case reused the serialized inline-flag bit pattern `0x00`, which already meant "required inline" in assemblies compiled by F# 5.0 and earlier. A newer compiler reading those older assemblies decoded the value as *not* inlined, dropped the inline body at the call site, and emitted a direct call to a dynamic-invocation stub. Cross-assembly SRTP APIs such as Aether 8.3.1 then threw `System.NotSupportedException` at runtime. Shipped in .NET SDK 10.0.400. + +## Error Manifestation + +A program that consumes an inline SRTP API from a pre-F#6 library compiles cleanly but throws at runtime, including in optimized Release builds: + +```text +Unhandled exception. System.NotSupportedException: +Dynamic invocation of op_HatEquals is not supported + at Aether.Optic.set[a,b,c](a optic, b value) +``` + +The same source built with SDK 10.0.303 prints the expected result. `--always-inline+` does not help; rebuilding the referenced library with a current compiler does. + +## Root Cause + +`ValFlags` packs a value's inline declaration into two bits of an `int64` that is serialized verbatim into assembly metadata. The bit patterns are a permanent on-disk contract. + +In F# 5.0 and earlier the field had a `PseudoVal` case — "must always be inlined, no IL body needed" — encoded as `0x00` with `ShouldInline = true`: + +```fsharp +match (flags &&& 0b110000L) with +| 0b000000L -> ValInline.PseudoVal // ShouldInline = true +| 0b010000L -> ValInline.Always +| ... +``` + +PR #6811 (July 2021, F# 6) removed `PseudoVal` and folded it into `Always`. Crucially, `0x00` kept decoding to a `ShouldInline = true` value, so libraries built before the removal continued to import correctly. + +PR #19548 introduced `ValInline.InlinedDefinition` and reused the now-"free-looking" `0x00` bit pattern for it — but with the *opposite* semantics, `ShouldInline = false`. The reader was changed so `0x00` decoded to `InlinedDefinition`. That silently reinterpreted every `0x00` inline value already sitting in shipped DLLs: a required-inline definition from an old library now imported as non-inline, so the consuming compiler emitted a direct call to the SRTP dynamic-invocation stub instead of inlining the resolved witness. + +The violated assumption is the "tag values are forever" rule: a bit pattern that already has a meaning in shipped metadata cannot be given a new, incompatible meaning. + +## Why It Escaped + +PR #19548 *did* add write-side normalization so a current compiler serializes `InlinedDefinition` as `Always` (`0x10`), keeping fresh round-trips correct. That protection is exactly what hid the bug: + +- Any in-repo test compiles the producer library **with the new compiler**, which never writes `0x00` for an inline value. So no test that builds its own fixtures could reproduce it — the poisoned byte only exists in binaries produced by an F# 5.0-or-earlier compiler. +- The `CompilerCompat` cross-version suite exercises recent SDKs (9 ↔ current), not pre-2021 F# 5 binaries, so the format generation that still emits `0x00` inline bits was outside its matrix. + +The gap was read-side: the new meaning was applied to old bytes, and nothing in CI reads bytes written by a 2021-era compiler. + +## Fix + +PR #20260 adds `ValFlags.OfPickledBits`, used by `u_ValData` when importing metadata. Because the write side always normalizes `InlinedDefinition` to `Always`, a serialized `0x00` inline field can only originate from a legacy compiler, where it meant required inline. `OfPickledBits` therefore maps legacy `0x00` back to `Always` on import. The serialized byte layout is unchanged; only interpretation of the legacy pattern is restored. + +## Timeline + +| Date | Event | +|---|---| +| ≤ 2021 | F# 5.0 and earlier encode required-inline values (`PseudoVal`) as inline bits `0x00`, `ShouldInline = true`. | +| 2021-07-19 | PR #6811 removes `PseudoVal`; `0x00` still decodes to a `ShouldInline = true` value. Old libraries keep working. | +| 2026-04-16 | Commit `761c8635b8` adds write-side normalization for the upcoming `InlinedDefinition` (`0x00` → `0x10` on pickle). | +| 2026-07-02 | PR #19548 merges: `InlinedDefinition` reuses `0x00` with `ShouldInline = false`; reader decodes `0x00` → `InlinedDefinition`. Latent regression for legacy binaries. | +| ~2026-08 | Ships in .NET SDK 10.0.400. | +| 2026-08-13 | Issue #20253 filed: Aether 8.3.1 SRTP call throws `NotSupportedException` under 10.0.400. | +| — | PR #20260 adds read-side normalization (`OfPickledBits`), restoring the invariant. | + +## Prevention + +The generalized rule — flag bit patterns baked into pickled metadata are permanent, and a pattern that already has a meaning in shipped DLLs must never be reinterpreted — is encoded in [`.github/instructions/TypedTreePickle.instructions.md`](../../.github/instructions/TypedTreePickle.instructions.md), whose `applyTo` covers the flag-encoding types in `src/Compiler/TypedTree/TypedTree.{fs,fsi}` and the pickle path. When adding a case to a serialized flag enum, either allocate an unused bit pattern or add read-side normalization that maps legacy patterns to their original semantics — write-side normalization alone only protects future binaries, never the ones already in the wild. diff --git a/src/Compiler/TypedTree/TypedTree.fs b/src/Compiler/TypedTree/TypedTree.fs index 1686f36aa28..983f7f84944 100644 --- a/src/Compiler/TypedTree/TypedTree.fs +++ b/src/Compiler/TypedTree/TypedTree.fs @@ -266,6 +266,17 @@ type ValFlags(flags: int64) = else bits + /// Reconstruct flags from the F# binary metadata. PickledBits always writes + /// ValInline.InlinedDefinition (0x00) out as ValInline.Always (0x01), so zero inline bits + /// are never produced by a compiler that has this normalization. Any zero bits seen here + /// are therefore legacy metadata from compilers older than PR #19548, which used the same + /// 0x00 bits to mean ValInline.Always (ShouldInline=true), and must be imported as such. + static member OfPickledBits(bits: int64) = + if bits &&& 0b00000000000000110000L = 0L then + ValFlags(bits ||| 0b00000000000000010000L) + else + ValFlags bits + /// Represents the kind of a type parameter [] type TyparKind = diff --git a/src/Compiler/TypedTree/TypedTree.fsi b/src/Compiler/TypedTree/TypedTree.fsi index 25149889328..5c3036fff90 100644 --- a/src/Compiler/TypedTree/TypedTree.fsi +++ b/src/Compiler/TypedTree/TypedTree.fsi @@ -104,7 +104,12 @@ type ValFlags = isGeneratedEventVal: bool -> ValFlags - new: flags: int64 -> ValFlags + /// Reconstruct flags from the F# binary metadata. PickledBits always writes + /// ValInline.InlinedDefinition (0x00) out as ValInline.Always (0x01), so zero inline bits + /// are never produced by a compiler that has this normalization. Any zero bits seen here + /// are therefore legacy metadata from compilers older than PR #19548, which used the same + /// 0x00 bits to mean ValInline.Always (ShouldInline=true), and must be imported as such. + static member OfPickledBits: bits: int64 -> ValFlags member WithIsCompilerGenerated: isCompGen: bool -> ValFlags diff --git a/src/Compiler/TypedTree/TypedTreePickle.fs b/src/Compiler/TypedTree/TypedTreePickle.fs index 4fe8eaf121f..afb5df91595 100644 --- a/src/Compiler/TypedTree/TypedTreePickle.fs +++ b/src/Compiler/TypedTree/TypedTreePickle.fs @@ -3301,7 +3301,7 @@ and u_ValData st = | Some(a, _) -> a) val_type = x2 val_stamp = newStamp () - val_flags = ValFlags x4 + val_flags = ValFlags.OfPickledBits x4 val_opt_data = match x1z, x1a, x10, x14, x13, x15, x8, x13b, x12, x9 with | None, None, None, None, TAccess [], None, None, ParentNone, "", [] -> None