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/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 7dd68964cb3..9b5ad43cfe1 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -44,6 +44,7 @@ * Extend the `=` adjacent to an interpolated string fix to the verbatim (`=$@"…"`, `=@$"…"`) and extended multi-dollar (`=$$"""…"""`) interpolated-string forms. ([Issue #16696](https://github.com/dotnet/fsharp/issues/16696), [PR #19984](https://github.com/dotnet/fsharp/pull/19984)) * Preserve type abbreviations (`string`, user-defined aliases) in the refined type of bindings introduced after a `| null` pattern in a `match` expression. ([Issue #19646](https://github.com/dotnet/fsharp/issues/19646), [PR #19745](https://github.com/dotnet/fsharp/pull/19745)) * Fix attributes on return type of unparenthesized tuple methods being silently dropped from IL. ([Issue #462](https://github.com/dotnet/fsharp/issues/462), [PR #19714](https://github.com/dotnet/fsharp/pull/19714)) +* Fix cross-assembly calls to inline SRTP functions from libraries compiled with F# 4.7 and earlier. ([Issue #20253](https://github.com/dotnet/fsharp/issues/20253), [PR #20260](https://github.com/dotnet/fsharp/pull/20260)) * Fix enum values losing their type when used in a custom attribute argument of type `obj` (they were stored as the underlying integer instead of the enum). ([Issue #995](https://github.com/dotnet/fsharp/issues/995), [PR #19975](https://github.com/dotnet/fsharp/pull/19975)) * Fix false-positive nullness warning (FS3261) when pattern matching narrows nullness inside seq/list/array comprehensions. ([Issue #19644](https://github.com/dotnet/fsharp/issues/19644), [PR #19743](https://github.com/dotnet/fsharp/pull/19743)) * Fix internal error FS0073 "Undefined or unsolved type variable" in IlxGen when nested inline SRTP functions with multiple overloads leave unsolved typars in the non-witness codegen path. ([Issue #19709](https://github.com/dotnet/fsharp/issues/19709), [PR #19710](https://github.com/dotnet/fsharp/pull/19710)) diff --git a/src/Compiler/TypedTree/TypedTree.fs b/src/Compiler/TypedTree/TypedTree.fs index 236a4a9b798..44e17e3b667 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 98c4ab0e840..dede50853d2 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 5b64b10f600..81891a408f2 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