Skip to content

Check let bindings through active patterns like a match (#16856) - #20383

Open
edgarfgp wants to merge 4 commits into
dotnet:mainfrom
edgarfgp:fix/16856-active-pattern-let-binding
Open

Check let bindings through active patterns like a match (#16856)#20383
edgarfgp wants to merge 4 commits into
dotnet:mainfrom
edgarfgp:fix/16856-active-pattern-let-binding

Conversation

@edgarfgp

Copy link
Copy Markdown
Contributor

Problem

Using an active pattern in a let binding whose right-hand side is a generic value crashes the compiler with an internal error, while the equivalent match is fine.

Fixes #16856.

Before

let (|T|) (f: _ -> _) = ()
match id with T -> ()   // ok
let (T) = id            // error FS0073: internal error: Unexpected generalized type variables when compiling an active pattern

After

let (T) = id            // ok, same as the match

let (|Id|) f = f
let (Id g) = id         // error FS0030: Value restriction ... same as `let g = match id with Id g -> g`
let (Id h) = id
h 1                     // ok, h: int -> int

Cause

TcLetBinding generalizes the right-hand side before it looks at the pattern. id is a generalizable value, so the binding becomes a generic patternInputTmp<'a> and the match compiler is asked to compile an active pattern against a generic input, which it can't do (the active pattern result is computed once at a dummy instantiation), hence the guard.

A pattern binding through an active pattern evaluates that active pattern, so it is now checked exactly like match rhs with pat -> ...: as for a non-generalizable right-hand side, no inferred type parameters are candidates for generalization. Phase-1 pattern checking records whether an active pattern occurs (TcPatLinearEnv.usesActivePattern, set in TcPatLongIdentActivePatternCase), CheckedBindingInfo carries it, and TcLetBinding uses it. Nothing that compiled before is affected, since any such binding with generalized type parameters hit the internal error.

Pattern bindings without active patterns are unchanged and still generalize, e.g. let (a, b) = id, id.

@github-actions

github-actions Bot commented Aug 27, 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

@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Aug 27, 2026

@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.

This is a lovely fix — the framing that a pattern binding through an active pattern is just match rhs with pat -> ..., so it should generalize exactly like the match does, is the right mental model and makes the whole change fall out naturally.

A few things I checked that make this safe to take:

  • The no-regression argument holds in both directions. When the RHS is generalizable the old code always hit the internal error, and when it isn't there were no inferred typars to drop, so [] changes nothing. And because the flag is only set in TcPatLongIdentActivePatternCase, union-case patterns like let (Some x) = Some id still generalize — the narrowing is exactly the pattern class the match compiler can't compile at a generic instantiation.

  • The threading is airtight: usesAP1 || usesAP2 is the correct join for or-patterns, and since CheckedBindingInfo is abstract in CheckExpressions.fsi the extra field needs no signature change while the DU arity still makes it impossible to miss a construction site.

  • The tests earn their keep by asserting the active pattern runs exactly once and that the value-restriction case matches let g = match id with Id g -> g, rather than just checking the crash is gone.

One optional follow-up: an active pattern anywhere in the pattern de-generalizes the whole binding (let (Id g), h = id, id no longer generalizes h). That's match-consistent and correct, but a one-line test would pin the intent so a future reader doesn't mistake it for an oversight.

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Aug 28, 2026
@T-Gro
T-Gro self-requested a review August 28, 2026 05:56
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Aug 28, 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-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Internal error of single case active pattern within let binding

2 participants