From 698e7b2070dd9f92bae441c1ed1c4dd2565476a4 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 5 Jun 2026 14:18:30 +0200 Subject: [PATCH 01/11] Move empty-string match optimization to Optimizer so quotations stay clean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The empty-string lowering added in #19189 produced `if x <> null then x.Length = 0 else false` inside `BuildSwitch`, which leaked through pattern-match compilation into captured quotations (#19873). Move the rewrite to `OptimizeExprOp`, which already skips `Expr.Quote` bodies. `match s with "" -> _` keeps the same IL and quotations now see `op_Equality(s, "")`. As a side effect, `if s = "" then _` gets the same null-safe length-check IL — improvement, not regression. Also adds a generic `verifyOutputAgainstBaseline` helper in `Compiler.fs` and a `.bsl`-driven quotation rendering test suite under `Conformance/Expressions/ ExpressionQuotations/QuotationRendering/` to catch future leaks of pattern-match lowerings into quotation ASTs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../.FSharp.Compiler.Service/11.0.100.md | 1 + .../Checking/PatternMatchCompilation.fs | 10 +- src/Compiler/Optimize/Optimizer.fs | 31 +++++ .../QuotationRendering/CharMatch.bsl | 7 ++ .../ConsecutiveIntMatch.bsl | 41 ++++++ .../QuotationRendering/DecimalMatch.bsl | 12 ++ .../QuotationRendering/EmptyStringMatch.bsl | 3 + .../QuotationRendering/FloatMatch.bsl | 4 + .../IfThenElseLiteralEqEmpty.bsl | 3 + .../QuotationRendering/Int64Match.bsl | 7 ++ .../NonEmptyStringMatch.bsl | 4 + .../QuotationRendering/NullOrEmptyMatch.bsl | 4 + .../QuotationRenderingTests.fs | 117 ++++++++++++++++++ .../QuotationRendering/SparseIntMatch.bsl | 7 ++ .../EmittedIL/TypeTestsInPatternMatching.fs | 34 +++++ .../FSharp.Compiler.ComponentTests.fsproj | 1 + tests/FSharp.Test.Utilities/Compiler.fs | 15 +++ 17 files changed, 294 insertions(+), 7 deletions(-) create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/CharMatch.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveIntMatch.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/DecimalMatch.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyStringMatch.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/FloatMatch.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfThenElseLiteralEqEmpty.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64Match.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyStringMatch.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmptyMatch.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/SparseIntMatch.bsl 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 9509ac72a3a..73b682f0b04 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -75,6 +75,7 @@ ([PR #19724](https://github.com/dotnet/fsharp/pull/19724)) * Emit debug points at a stack-empty position ([PR #19877](https://github.com/dotnet/fsharp/pull/19877)) * Fix spurious XmlDoc warnings (unknown parameter / no documentation for parameter) under `--warnon:3390` when a get/set property documents the full parameter set across both accessors. ([Issue #13684](https://github.com/dotnet/fsharp/issues/13684), [PR #19884](https://github.com/dotnet/fsharp/pull/19884)) +* Quotations: `<@ match s with "" -> ... @>` and `<@ null | "" -> ... @>` no longer leak the `if s <> null then s.Length = 0 else false` lowering introduced by the empty-string optimization. The optimization was moved from pattern-match compilation into the optimizer, so quoted expressions retain their original `op_Equality(s, "")` shape. As a side effect, `if s = "" then ...` now produces the same null-safe length-check IL as `match s with "" -> _`. ([Issue #19873](https://github.com/dotnet/fsharp/issues/19873)) ### Added diff --git a/src/Compiler/Checking/PatternMatchCompilation.fs b/src/Compiler/Checking/PatternMatchCompilation.fs index 4128b8cf5f4..e9996efcb1c 100644 --- a/src/Compiler/Checking/PatternMatchCompilation.fs +++ b/src/Compiler/Checking/PatternMatchCompilation.fs @@ -810,14 +810,10 @@ let rec BuildSwitch inpExprOpt g isNullFiltered expr edges dflt m = let test = mkILAsmCeq g m (mkLdlen g m vExpr) (mkInt g m n) let finalTest = if isNullFiltered then test else mkLazyAnd g m (mkNonNullTest g m vExpr) test mkLetBind m bind finalTest - | DecisionTreeTest.Const (Const.String "") -> - // Optimize empty string check to use null-safe length check - let _v, vExpr, bind = mkCompGenLocalAndInvisibleBind g "testExpr" m testexpr - let test = mkILAsmCeq g m (mkGetStringLength g m vExpr) (mkInt g m 0) - // Skip null check if we're in a null-filtered context - let finalTest = if isNullFiltered then test else mkLazyAnd g m (mkNonNullTest g m vExpr) test - mkLetBind m bind finalTest | DecisionTreeTest.Const (Const.String _ as c) -> + // Empty-string is detected by the Optimizer (`OptimizeStringEqualsEmpty`) + // and rewritten to a null-safe length check at that point. Pattern-match + // compilation keeps the F# `=` form so quotations see the original shape. mkCallEqualsOperator g m g.string_ty testexpr (Expr.Const (c, m, g.string_ty)) | DecisionTreeTest.Const (Const.Decimal _ as c) -> mkCallEqualsOperator g m g.decimal_ty testexpr (Expr.Const (c, m, g.decimal_ty)) diff --git a/src/Compiler/Optimize/Optimizer.fs b/src/Compiler/Optimize/Optimizer.fs index 3cbb574598c..07dfe7296b4 100644 --- a/src/Compiler/Optimize/Optimizer.fs +++ b/src/Compiler/Optimize/Optimizer.fs @@ -2291,6 +2291,15 @@ let TryDetectQueryQuoteAndRun cenv (expr: Expr) = //printfn "Not eliminating because no Run found" None +let IsILMethodRefSystemStringEquals (mref: ILMethodRef) = + mref.Name = "Equals" && + mref.DeclaringTypeRef.Name = "System.String" && + (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = "System.Boolean") && + (mref.ArgCount = 2 && + mref.ArgTypes + |> List.forall (fun ilTy -> + ilTy.IsNominal && ilTy.TypeRef.Name = "System.String")) + let IsILMethodRefSystemStringConcat (mref: ILMethodRef) = mref.Name = "Concat" && mref.DeclaringTypeRef.Name = "System.String" && @@ -2540,6 +2549,18 @@ and MakeOptimizedSystemStringConcatCall cenv env m args = | _ -> OptimizeExpr cenv env expr +/// Rewrite `System.String.Equals(x, "")` (or `Equals("", x)`) into the null-safe length check +/// `if x = null then false else x.Length = 0`. Equivalent semantics — `String.Equals` returns false +/// on null. This optimization originally lived in `BuildSwitch` for `match s with "" -> _` but was +/// moved here so that quotations capture the original `op_Equality(_, "")` shape (issue #19873). +and MakeOptimizedStringEqualsEmptyCall cenv env m nonEmptyArg = + let g = cenv.g + let _, vExpr, bind = mkCompGenLocalAndInvisibleBind g "testExpr" m nonEmptyArg + let lengthIsZero = mkILAsmCeq g m (mkGetStringLength g m vExpr) (mkInt g m 0) + let nullSafe = mkLazyAnd g m (mkNonNullTest g m vExpr) lengthIsZero + let optimized = mkLetBind m bind nullSafe + OptimizeExpr cenv env optimized + /// Optimize/analyze an application of an intrinsic operator to arguments and OptimizeExprOp cenv env (op, tyargs, args, m) = @@ -2611,6 +2632,16 @@ and OptimizeExprOp cenv env (op, tyargs, args, m) = when IsILMethodRefSystemStringConcat ilMethRef -> MakeOptimizedSystemStringConcatCall cenv env m args + // Optimize `String.Equals(x, "")` (and `String.Equals("", x)`) into the null-safe length check + // `if x <> null then x.Length = 0 else false`. Mirrors the original BuildSwitch lowering for + // `match s with "" -> _`. Done here (post-inlining) so quotations capture the un-optimized shape. + | TOp.ILCall(_, _, _, _, _, _, _, ilMethRef, _, _, _), _, [arg1; Expr.Const(Const.String "", _, _)] + when IsILMethodRefSystemStringEquals ilMethRef -> + MakeOptimizedStringEqualsEmptyCall cenv env m arg1 + | TOp.ILCall(_, _, _, _, _, _, _, ilMethRef, _, _, _), _, [Expr.Const(Const.String "", _, _); arg2] + when IsILMethodRefSystemStringEquals ilMethRef -> + MakeOptimizedStringEqualsEmptyCall cenv env m arg2 + | _ -> // Reductions OptimizeExprOpReductions cenv env (op, tyargs, args, m) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/CharMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/CharMatch.bsl new file mode 100644 index 00000000000..73fea3a9425 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/CharMatch.bsl @@ -0,0 +1,7 @@ +Lambda (c, + IfThenElse (Call (None, op_Equality, [c, Value ('a')]), Value (1), + IfThenElse (Call (None, op_Equality, [c, Value ('b')]), + Value (2), + IfThenElse (Call (None, op_Equality, + [c, Value ('c')]), Value (3), + Value (0))))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveIntMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveIntMatch.bsl new file mode 100644 index 00000000000..e0c74904068 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveIntMatch.bsl @@ -0,0 +1,41 @@ +Lambda (i, + IfThenElse (Call (None, op_Equality, [i, Value (1)]), Value ("one"), + IfThenElse (Call (None, op_Equality, [i, Value (2)]), + Value ("two"), + IfThenElse (Call (None, op_Equality, + [i, Value (3)]), + Value ("three"), + IfThenElse (Call (None, op_Equality, + [i, Value (4)]), + Value ("four"), + IfThenElse (Call (None, + op_Equality, + [i, + Value (5)]), + Value ("five"), + IfThenElse (Call (None, + op_Equality, + [i, + Value (6)]), + Value ("six"), + IfThenElse (Call (None, + op_Equality, + [i, + Value (7)]), + Value ("seven"), + IfThenElse (Call (None, + op_Equality, + [i, + Value (8)]), + Value ("eight"), + IfThenElse (Call (None, + op_Equality, + [i, + Value (9)]), + Value ("nine"), + IfThenElse (Call (None, + op_Equality, + [i, + Value (10)]), + Value ("ten"), + Value ("other")))))))))))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/DecimalMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/DecimalMatch.bsl new file mode 100644 index 00000000000..03543d561ed --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/DecimalMatch.bsl @@ -0,0 +1,12 @@ +Lambda (d, + IfThenElse (Call (None, op_Equality, + [d, + Call (None, MakeDecimal, + [Value (1), Value (0), Value (0), Value (false), + Value (0uy)])]), Value ("a"), + IfThenElse (Call (None, op_Equality, + [d, + Call (None, MakeDecimal, + [Value (2), Value (0), Value (0), + Value (false), Value (0uy)])]), + Value ("b"), Value ("other")))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyStringMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyStringMatch.bsl new file mode 100644 index 00000000000..848475282af --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyStringMatch.bsl @@ -0,0 +1,3 @@ +Lambda (x, + IfThenElse (Call (None, op_Equality, [x, Value ("")]), Value (1), + Value (0))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/FloatMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/FloatMatch.bsl new file mode 100644 index 00000000000..10f1dd6ae27 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/FloatMatch.bsl @@ -0,0 +1,4 @@ +Lambda (f, + IfThenElse (Call (None, op_Equality, [f, Value (1.0)]), Value ("a"), + IfThenElse (Call (None, op_Equality, [f, Value (2.0)]), + Value ("b"), Value ("other")))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfThenElseLiteralEqEmpty.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfThenElseLiteralEqEmpty.bsl new file mode 100644 index 00000000000..6de55e76496 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfThenElseLiteralEqEmpty.bsl @@ -0,0 +1,3 @@ +Lambda (s, + IfThenElse (Call (None, op_Equality, [s, Value ("")]), Value (1), + Value (0))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64Match.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64Match.bsl new file mode 100644 index 00000000000..fef4ec2a85c --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64Match.bsl @@ -0,0 +1,7 @@ +Lambda (i, + IfThenElse (Call (None, op_Equality, [i, Value (1L)]), Value ("a"), + IfThenElse (Call (None, op_Equality, [i, Value (2L)]), + Value ("b"), + IfThenElse (Call (None, op_Equality, + [i, Value (3L)]), Value ("c"), + Value ("other"))))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyStringMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyStringMatch.bsl new file mode 100644 index 00000000000..3c450cdcde4 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyStringMatch.bsl @@ -0,0 +1,4 @@ +Lambda (x, + IfThenElse (Call (None, op_Equality, [x, Value ("foo")]), Value (1), + IfThenElse (Call (None, op_Equality, [x, Value ("bar")]), + Value (2), Value (0)))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmptyMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmptyMatch.bsl new file mode 100644 index 00000000000..408cccd7c53 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmptyMatch.bsl @@ -0,0 +1,4 @@ +Lambda (x, + IfThenElse (Call (None, op_Equality, [x, Value ()]), Value (1), + IfThenElse (Call (None, op_Equality, [x, Value ("")]), + Value (1), Value (0)))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs new file mode 100644 index 00000000000..b1871a63080 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs @@ -0,0 +1,117 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +// Snapshot tests for the F# rendering of quoted expressions (sprintf "%A" <@ ... @>). +// +// Each test compiles a tiny program that prints the rendered quotation, executes it, and diffs +// the captured stdout against a .bsl file in this folder. Update baselines with TEST_UPDATE_BSL=1. +// +// Purpose: detect accidental leakage of pattern-match-compilation lowerings (range checks, jump +// tables, null+Length rewrites, etc.) into the public quotation AST. The quotation translator +// runs after PatternMatchCompilation, so any rewrite that lives in PatternMatchCompilation will +// also show up here as a (potentially surprising) shape change. +// +// Related: https://github.com/dotnet/fsharp/issues/19873, PR https://github.com/dotnet/fsharp/pull/19532 + +namespace Conformance.Expressions.ExpressionQuotations + +open System.IO +open Xunit +open FSharp.Test.Compiler + +module QuotationRendering = + + let private baselineDir = __SOURCE_DIRECTORY__ + + /// Compile a snippet that builds a quotation `q` and prints it with `printfn "%A" q`, + /// then diff its stdout against `.bsl`. + let private renderingShouldMatch (bslName: string) (quoteBody: string) = + let program = "module Test\n[]\nlet main _ =\n let q = " + quoteBody + "\n printfn \"%A\" q\n 0\n" + let bslPath = Path.Combine(baselineDir, bslName + ".bsl") + + FSharp program + |> asExe + |> ignoreWarnings + |> compileExeAndRun + |> shouldSucceed + |> verifyOutputAgainstBaseline bslPath + |> ignore + + // ---------------------------------------------------------------------- + // The "leaky" cases: PatternMatchCompilation rewrites that show up in quotations. + // ---------------------------------------------------------------------- + + // https://github.com/dotnet/fsharp/issues/19873 + // Empty-string match leaks `IfThenElse (op_Inequality(x,null), x.Length = 0, false)`. + [] + let ``Match empty string`` () = + renderingShouldMatch "EmptyStringMatch" "<@ fun (x: string) -> match x with \"\" -> 1 | _ -> 0 @>" + + // pezipink's second example in #19873 — the same lowering also appears for plain `s = ""`. + [] + let ``If-then-else with literal empty string equality`` () = + renderingShouldMatch "IfThenElseLiteralEqEmpty" "<@ fun (s: string) -> if s = \"\" then 1 else 0 @>" + + // null | "" -> ... should consolidate to a single null check, then a length check. + [] + let ``Match null or empty string`` () = + renderingShouldMatch "NullOrEmptyMatch" "<@ fun (x: string) -> match x with null | \"\" -> 1 | _ -> 0 @>" + + // Array-length match also lowers to null + Length check inline. + // This is a pre-existing leakage: BuildSwitch lowers `match a with [| _ |] -> _` into raw + // `I_ldlen` inline IL via `mkLdlen` before the quotation translator runs. The translator + // then rejects the raw IL with FS0452. When the optimization is moved to a later phase, + // this test should be flipped to assert successful compilation and a clean rendering. + [] + let ``Match array length fails in quotation (FS0452)`` () = + FSharp """module Test +let q = <@ fun (a: int[]) -> match a with [| _ |] -> 1 | _ -> 0 @> +""" + |> asLibrary + |> typecheck + |> shouldFail + |> withErrorCode 452 + |> withDiagnosticMessageMatches "Quotations cannot contain inline assembly code or pattern matching on arrays" + |> ignore + + // ---------------------------------------------------------------------- + // Reference cases: confirm other PatternMatchCompilation optimizations do + // NOT leak. These baselines should be a clean if/then/else chain of + // op_Equality calls regardless of compactification or jump-table lowering. + // ---------------------------------------------------------------------- + + [] + let ``Match non-empty strings`` () = + renderingShouldMatch "NonEmptyStringMatch" "<@ fun (x: string) -> match x with \"foo\" -> 1 | \"bar\" -> 2 | _ -> 0 @>" + + // Consecutive ints 1..10 — PatternMatchCompilation compactifies them into a TDSwitch + // which (in IL) eventually becomes a switch table. The quotation should *not* see that; + // it should be a chain of op_Equality tests. + [] + let ``Match consecutive ints 1 to 10`` () = + renderingShouldMatch "ConsecutiveIntMatch" + "<@ fun (i: int) -> match i with 1 -> \"one\" | 2 -> \"two\" | 3 -> \"three\" | 4 -> \"four\" | 5 -> \"five\" | 6 -> \"six\" | 7 -> \"seven\" | 8 -> \"eight\" | 9 -> \"nine\" | 10 -> \"ten\" | _ -> \"other\" @>" + + [] + let ``Match sparse ints`` () = + renderingShouldMatch "SparseIntMatch" + "<@ fun (i: int) -> match i with 1 -> \"a\" | 5 -> \"b\" | 10 -> \"c\" | _ -> \"other\" @>" + + [] + let ``Match chars`` () = + renderingShouldMatch "CharMatch" + "<@ fun (c: char) -> match c with 'a' -> 1 | 'b' -> 2 | 'c' -> 3 | _ -> 0 @>" + + [] + let ``Match int64`` () = + renderingShouldMatch "Int64Match" + "<@ fun (i: int64) -> match i with 1L -> \"a\" | 2L -> \"b\" | 3L -> \"c\" | _ -> \"other\" @>" + + [] + let ``Match float`` () = + renderingShouldMatch "FloatMatch" + "<@ fun (f: float) -> match f with 1.0 -> \"a\" | 2.0 -> \"b\" | _ -> \"other\" @>" + + [] + let ``Match decimal`` () = + renderingShouldMatch "DecimalMatch" + "<@ fun (d: decimal) -> match d with 1m -> \"a\" | 2m -> \"b\" | _ -> \"other\" @>" diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/SparseIntMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/SparseIntMatch.bsl new file mode 100644 index 00000000000..615790172f6 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/SparseIntMatch.bsl @@ -0,0 +1,7 @@ +Lambda (i, + IfThenElse (Call (None, op_Equality, [i, Value (1)]), Value ("a"), + IfThenElse (Call (None, op_Equality, [i, Value (5)]), + Value ("b"), + IfThenElse (Call (None, op_Equality, + [i, Value (10)]), Value ("c"), + Value ("other"))))) diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs index 8dbdf763f4d..0e6732736b7 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs @@ -343,6 +343,40 @@ let TestEmptyStringPattern(x: string) = IL_000b: ldstr "empty" IL_0010: ret + IL_0011: ldstr "other" + IL_0016: ret + } +""" + ] + + // https://github.com/dotnet/fsharp/issues/19873 + // Side effect of moving the empty-string optimization to the Optimizer (was previously only + // applied for `match`): `if s = ""` now produces the same null-safe length-check IL. + [] + let ``Test codegen for if-then-else with literal empty-string equality``() = + FSharp """ +module Test +let TestEmptyStringEq(x: string) = + if x = "" then "empty" else "other" + """ + |> compile + |> shouldSucceed + |> verifyIL [ + """ + .method public static string TestEmptyStringEq(string x) cil managed + { + + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: brfalse.s IL_0011 + + IL_0003: ldarg.0 + IL_0004: callvirt instance int32 [runtime]System.String::get_Length() + IL_0009: brtrue.s IL_0011 + + IL_000b: ldstr "empty" + IL_0010: ret + IL_0011: ldstr "other" IL_0016: ret } diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index 3550b60c13e..4307e3f8230 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -113,6 +113,7 @@ + diff --git a/tests/FSharp.Test.Utilities/Compiler.fs b/tests/FSharp.Test.Utilities/Compiler.fs index d5ae4a34a27..cdec86ec163 100644 --- a/tests/FSharp.Test.Utilities/Compiler.fs +++ b/tests/FSharp.Test.Utilities/Compiler.fs @@ -2210,6 +2210,21 @@ $ code --diff {outFile} {expectedFile} let withEvalTypeEquals t (result: CompilationResult) : CompilationResult = assertEvalOutput (fun (x: FsiValue) -> x.ReflectionType) t result + /// Snapshot the captured stdout of an executed test program and diff it against a `.bsl` baseline file. + /// Combine with `compileExeAndRun`. Useful for locking in textual representations such as `sprintf "%A" <@ ... @>` + /// to detect accidental changes in the F# representation of quotations, error messages, printf output, etc. + /// Update with `TEST_UPDATE_BSL=1`. + let verifyOutputAgainstBaseline (baselineFilePath: string) (result: CompilationResult) : CompilationResult = + match result.RunOutput with + | None -> + failwith "Execution output is missing; verifyOutputAgainstBaseline requires a run result (use compileExeAndRun)." + | Some (ExecutionOutput e) -> + let actual = e.StdOut |> normalizeNewlines + checkBaseline actual baselineFilePath + | Some _ -> + failwith "verifyOutputAgainstBaseline only supports ExecutionOutput; received EvalOutput." + result + let signatureText (pageWidth: int option) (checkResults: FSharp.Compiler.CodeAnalysis.FSharpCheckFileResults) = checkResults.GenerateSignature(?pageWidth = pageWidth) |> Option.defaultWith (fun _ -> failwith "Unable to generate signature text.") From a75707d5b9a599764bf5e522a86e9e4f6b6495a4 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 5 Jun 2026 15:37:58 +0200 Subject: [PATCH 02/11] Lock in IL trade-offs from rubber-duck review; tighten optimizer guard Adversarial reviews flagged two IL-quality concerns. Verified both by inspecting actual ildasm output, accepted both, and added IL tests locking the current behavior in so future changes are conscious: 1. --optimize- (debug) builds no longer get the null+Length fast path for `match s with "" -> _` because F#'s `(=)` is only inlined when LocalOptimizationsEnabled is true. The fallback `String.Equals(s,"")` call is still correct; JIT tiered compilation reaches the fast path. 2. `match s with null | "" -> _` emits one redundant `brfalse` on the empty-string branch because the optimizer cannot see the enclosing null-filtered context that BuildSwitch's `isNullFiltered` flag tracked. JIT trivially eliminates the redundant branch. Also tightens `IsILMethodRefSystemStringEquals` to require the call to be static (rejects hypothetical instance/user-defined `System.String` type with a 2-arg static `Equals`) and fixes a stale function-name comment in PatternMatchCompilation.fs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../.FSharp.Compiler.Service/11.0.100.md | 2 +- .../Checking/PatternMatchCompilation.fs | 2 +- src/Compiler/Optimize/Optimizer.fs | 1 + .../EmittedIL/TypeTestsInPatternMatching.fs | 90 +++++++++++++++++++ 4 files changed, 93 insertions(+), 2 deletions(-) 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 73b682f0b04..8596763ca9c 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -75,7 +75,7 @@ ([PR #19724](https://github.com/dotnet/fsharp/pull/19724)) * Emit debug points at a stack-empty position ([PR #19877](https://github.com/dotnet/fsharp/pull/19877)) * Fix spurious XmlDoc warnings (unknown parameter / no documentation for parameter) under `--warnon:3390` when a get/set property documents the full parameter set across both accessors. ([Issue #13684](https://github.com/dotnet/fsharp/issues/13684), [PR #19884](https://github.com/dotnet/fsharp/pull/19884)) -* Quotations: `<@ match s with "" -> ... @>` and `<@ null | "" -> ... @>` no longer leak the `if s <> null then s.Length = 0 else false` lowering introduced by the empty-string optimization. The optimization was moved from pattern-match compilation into the optimizer, so quoted expressions retain their original `op_Equality(s, "")` shape. As a side effect, `if s = "" then ...` now produces the same null-safe length-check IL as `match s with "" -> _`. ([Issue #19873](https://github.com/dotnet/fsharp/issues/19873)) +* Quotations: `<@ match s with "" -> ... @>` and `<@ null | "" -> ... @>` no longer leak the `if s <> null then s.Length = 0 else false` lowering introduced by the empty-string optimization. The optimization was moved from pattern-match compilation into the optimizer, so quoted expressions retain their original `op_Equality(s, "")` shape. As a side effect, `if s = "" then ...` now produces the same null-safe length-check IL as `match s with "" -> _`. Trade-offs (documented as locked-in IL baselines): `--optimize-` debug builds emit a plain `String.Equals(s, "")` call (still correct; JIT tiered compilation reaches the fast path); `match s with null | "" -> _` emits a redundant `brfalse` on the empty-string branch that the JIT trivially eliminates. ([Issue #19873](https://github.com/dotnet/fsharp/issues/19873)) ### Added diff --git a/src/Compiler/Checking/PatternMatchCompilation.fs b/src/Compiler/Checking/PatternMatchCompilation.fs index e9996efcb1c..ee57cf40610 100644 --- a/src/Compiler/Checking/PatternMatchCompilation.fs +++ b/src/Compiler/Checking/PatternMatchCompilation.fs @@ -811,7 +811,7 @@ let rec BuildSwitch inpExprOpt g isNullFiltered expr edges dflt m = let finalTest = if isNullFiltered then test else mkLazyAnd g m (mkNonNullTest g m vExpr) test mkLetBind m bind finalTest | DecisionTreeTest.Const (Const.String _ as c) -> - // Empty-string is detected by the Optimizer (`OptimizeStringEqualsEmpty`) + // Empty-string is detected by the Optimizer (`MakeOptimizedStringEqualsEmptyCall`) // and rewritten to a null-safe length check at that point. Pattern-match // compilation keeps the F# `=` form so quotations see the original shape. mkCallEqualsOperator g m g.string_ty testexpr (Expr.Const (c, m, g.string_ty)) diff --git a/src/Compiler/Optimize/Optimizer.fs b/src/Compiler/Optimize/Optimizer.fs index 07dfe7296b4..2f00bc51c1a 100644 --- a/src/Compiler/Optimize/Optimizer.fs +++ b/src/Compiler/Optimize/Optimizer.fs @@ -2293,6 +2293,7 @@ let TryDetectQueryQuoteAndRun cenv (expr: Expr) = let IsILMethodRefSystemStringEquals (mref: ILMethodRef) = mref.Name = "Equals" && + mref.CallingConv.IsStatic && mref.DeclaringTypeRef.Name = "System.String" && (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = "System.Boolean") && (mref.ArgCount = 2 && diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs index 0e6732736b7..34cc30bef77 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs @@ -380,5 +380,95 @@ let TestEmptyStringEq(x: string) = IL_0011: ldstr "other" IL_0016: ret } +""" + ] + + // https://github.com/dotnet/fsharp/issues/19873 + // Trade-off recorded: when the empty-string optimization moved from BuildSwitch to the Optimizer, + // `--optimize-` (debug) builds no longer get the null+Length form because F#'s `(=)` operator is + // only inlined when `LocalOptimizationsEnabled = true`. Debug builds now emit a plain + // `String.Equals(s, "")` call. JIT tiered compilation reaches the same fast path; debug-mode IL + // size is not a perf concern. Locked in so a future change can revisit consciously. + [] + let ``Test codegen for empty string pattern under --optimize-``() = + FSharp """ +module Test +let TestEmptyStringPattern(x: string) = + match x with + | "" -> "empty" + | _ -> "other" + """ + |> withNoOptimize + |> compile + |> shouldSucceed + |> verifyIL [ + """ + .method public static string TestEmptyStringPattern(string x) cil managed + { + + .maxstack 4 + .locals init (string V_0) + IL_0000: ldarg.0 + IL_0001: stloc.0 + IL_0002: ldloc.0 + IL_0003: ldstr "" + IL_0008: call bool [netstandard]System.String::Equals(string, + string) + IL_000d: brfalse.s IL_0015 + + IL_000f: ldstr "empty" + IL_0014: ret + + IL_0015: ldstr "other" + IL_001a: ret + } +""" + ] + + // https://github.com/dotnet/fsharp/issues/19873 + // Trade-off recorded: `null | "" -> _` patterns previously elided the inner null check via + // BuildSwitch's `isNullFiltered` flag (so the IL emitted only the `null` pattern's `brfalse` + // followed by a bare `s.Length = 0`). After moving the optimization to the Optimizer, the + // optimizer has no view of the enclosing null-filtered context and so re-emits the null check. + // The result is two back-to-back `brfalse.s` on the same argument. JIT trivially eliminates the + // redundant branch; we accept the few extra IL bytes in exchange for clean quotations. + [] + let ``Test codegen for null-or-empty-string pattern``() = + FSharp """ +module Test +let TestNullOrEmpty(x: string) = + match x with + | null | "" -> "empty" + | _ -> "other" + """ + |> compile + |> shouldSucceed + |> verifyIL [ + """ + .method public static string TestNullOrEmpty(string x) cil managed + { + + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: brfalse.s IL_0014 + + IL_0003: ldarg.0 + IL_0004: brfalse.s IL_0011 + + IL_0006: ldarg.0 + IL_0007: callvirt instance int32 [runtime]System.String::get_Length() + IL_000c: ldc.i4.0 + IL_000d: ceq + IL_000f: br.s IL_0012 + + IL_0011: ldc.i4.0 + IL_0012: brfalse.s IL_001a + + IL_0014: ldstr "empty" + IL_0019: ret + + IL_001a: ldstr "other" + IL_001f: ret + } """ ] \ No newline at end of file From b947b6c04e20bfe2b77a64d804e1133663b875bc Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 5 Jun 2026 21:24:50 +0200 Subject: [PATCH 03/11] Polish quotation rendering test suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Driver: extract `printerProgram` helper using a triple-quoted F# string (no more `\"\"` escape hell); group tests into Regression / NoLeakReference / SideEffect / PreExistingError submodules. Baselines: drop the `Match` suffix (folder is already `QuotationRendering`), delete redundant `SparseIntMatch` (same code path as Consecutive), shrink all multi-case tests to the minimum that still demonstrates the point: - ConsecutiveInts: 1..10 → 1..3 (was 41 lines, now 7) - Chars, NonEmptyString: 3 → 2 cases - Int64, Float, Decimal: 3 → 1 case Result: 9 baselines totalling 37 lines, each fits on screen. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../QuotationRendering/CharMatch.bsl | 7 - .../QuotationRendering/Chars.bsl | 4 + .../ConsecutiveIntMatch.bsl | 41 ---- ...SparseIntMatch.bsl => ConsecutiveInts.bsl} | 6 +- .../QuotationRendering/Decimal.bsl | 6 + .../QuotationRendering/DecimalMatch.bsl | 12 -- .../{EmptyStringMatch.bsl => EmptyString.bsl} | 0 .../QuotationRendering/Float.bsl | 3 + .../QuotationRendering/FloatMatch.bsl | 4 - ...lseLiteralEqEmpty.bsl => IfEqualEmpty.bsl} | 0 .../QuotationRendering/Int64.bsl | 3 + .../QuotationRendering/Int64Match.bsl | 7 - ...mptyStringMatch.bsl => NonEmptyString.bsl} | 4 +- .../{NullOrEmptyMatch.bsl => NullOrEmpty.bsl} | 0 .../QuotationRenderingTests.fs | 196 ++++++++++-------- 15 files changed, 127 insertions(+), 166 deletions(-) delete mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/CharMatch.bsl create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl delete mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveIntMatch.bsl rename tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/{SparseIntMatch.bsl => ConsecutiveInts.bsl} (63%) create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl delete mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/DecimalMatch.bsl rename tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/{EmptyStringMatch.bsl => EmptyString.bsl} (100%) create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Float.bsl delete mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/FloatMatch.bsl rename tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/{IfThenElseLiteralEqEmpty.bsl => IfEqualEmpty.bsl} (100%) create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl delete mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64Match.bsl rename tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/{NonEmptyStringMatch.bsl => NonEmptyString.bsl} (59%) rename tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/{NullOrEmptyMatch.bsl => NullOrEmpty.bsl} (100%) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/CharMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/CharMatch.bsl deleted file mode 100644 index 73fea3a9425..00000000000 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/CharMatch.bsl +++ /dev/null @@ -1,7 +0,0 @@ -Lambda (c, - IfThenElse (Call (None, op_Equality, [c, Value ('a')]), Value (1), - IfThenElse (Call (None, op_Equality, [c, Value ('b')]), - Value (2), - IfThenElse (Call (None, op_Equality, - [c, Value ('c')]), Value (3), - Value (0))))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl new file mode 100644 index 00000000000..7de6ea4d390 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl @@ -0,0 +1,4 @@ +Lambda (c, + IfThenElse (Call (None, op_Equality, [c, Value ('a')]), Value (1), + IfThenElse (Call (None, op_Equality, [c, Value ('b')]), + Value (2), Value (0)))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveIntMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveIntMatch.bsl deleted file mode 100644 index e0c74904068..00000000000 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveIntMatch.bsl +++ /dev/null @@ -1,41 +0,0 @@ -Lambda (i, - IfThenElse (Call (None, op_Equality, [i, Value (1)]), Value ("one"), - IfThenElse (Call (None, op_Equality, [i, Value (2)]), - Value ("two"), - IfThenElse (Call (None, op_Equality, - [i, Value (3)]), - Value ("three"), - IfThenElse (Call (None, op_Equality, - [i, Value (4)]), - Value ("four"), - IfThenElse (Call (None, - op_Equality, - [i, - Value (5)]), - Value ("five"), - IfThenElse (Call (None, - op_Equality, - [i, - Value (6)]), - Value ("six"), - IfThenElse (Call (None, - op_Equality, - [i, - Value (7)]), - Value ("seven"), - IfThenElse (Call (None, - op_Equality, - [i, - Value (8)]), - Value ("eight"), - IfThenElse (Call (None, - op_Equality, - [i, - Value (9)]), - Value ("nine"), - IfThenElse (Call (None, - op_Equality, - [i, - Value (10)]), - Value ("ten"), - Value ("other")))))))))))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/SparseIntMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl similarity index 63% rename from tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/SparseIntMatch.bsl rename to tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl index 615790172f6..45187cc931d 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/SparseIntMatch.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl @@ -1,7 +1,7 @@ Lambda (i, IfThenElse (Call (None, op_Equality, [i, Value (1)]), Value ("a"), - IfThenElse (Call (None, op_Equality, [i, Value (5)]), + IfThenElse (Call (None, op_Equality, [i, Value (2)]), Value ("b"), IfThenElse (Call (None, op_Equality, - [i, Value (10)]), Value ("c"), - Value ("other"))))) + [i, Value (3)]), Value ("c"), + Value ("z"))))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl new file mode 100644 index 00000000000..8e7a7a28f94 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl @@ -0,0 +1,6 @@ +Lambda (d, + IfThenElse (Call (None, op_Equality, + [d, + Call (None, MakeDecimal, + [Value (1), Value (0), Value (0), Value (false), + Value (0uy)])]), Value ("a"), Value ("b"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/DecimalMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/DecimalMatch.bsl deleted file mode 100644 index 03543d561ed..00000000000 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/DecimalMatch.bsl +++ /dev/null @@ -1,12 +0,0 @@ -Lambda (d, - IfThenElse (Call (None, op_Equality, - [d, - Call (None, MakeDecimal, - [Value (1), Value (0), Value (0), Value (false), - Value (0uy)])]), Value ("a"), - IfThenElse (Call (None, op_Equality, - [d, - Call (None, MakeDecimal, - [Value (2), Value (0), Value (0), - Value (false), Value (0uy)])]), - Value ("b"), Value ("other")))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyStringMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl similarity index 100% rename from tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyStringMatch.bsl rename to tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Float.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Float.bsl new file mode 100644 index 00000000000..ec5dffac878 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Float.bsl @@ -0,0 +1,3 @@ +Lambda (f, + IfThenElse (Call (None, op_Equality, [f, Value (1.0)]), Value ("a"), + Value ("b"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/FloatMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/FloatMatch.bsl deleted file mode 100644 index 10f1dd6ae27..00000000000 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/FloatMatch.bsl +++ /dev/null @@ -1,4 +0,0 @@ -Lambda (f, - IfThenElse (Call (None, op_Equality, [f, Value (1.0)]), Value ("a"), - IfThenElse (Call (None, op_Equality, [f, Value (2.0)]), - Value ("b"), Value ("other")))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfThenElseLiteralEqEmpty.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl similarity index 100% rename from tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfThenElseLiteralEqEmpty.bsl rename to tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl new file mode 100644 index 00000000000..c30b9443b71 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl @@ -0,0 +1,3 @@ +Lambda (i, + IfThenElse (Call (None, op_Equality, [i, Value (1L)]), Value ("a"), + Value ("b"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64Match.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64Match.bsl deleted file mode 100644 index fef4ec2a85c..00000000000 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64Match.bsl +++ /dev/null @@ -1,7 +0,0 @@ -Lambda (i, - IfThenElse (Call (None, op_Equality, [i, Value (1L)]), Value ("a"), - IfThenElse (Call (None, op_Equality, [i, Value (2L)]), - Value ("b"), - IfThenElse (Call (None, op_Equality, - [i, Value (3L)]), Value ("c"), - Value ("other"))))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyStringMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl similarity index 59% rename from tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyStringMatch.bsl rename to tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl index 3c450cdcde4..d948553e5f5 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyStringMatch.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl @@ -1,4 +1,4 @@ Lambda (x, - IfThenElse (Call (None, op_Equality, [x, Value ("foo")]), Value (1), - IfThenElse (Call (None, op_Equality, [x, Value ("bar")]), + IfThenElse (Call (None, op_Equality, [x, Value ("a")]), Value (1), + IfThenElse (Call (None, op_Equality, [x, Value ("b")]), Value (2), Value (0)))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmptyMatch.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl similarity index 100% rename from tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmptyMatch.bsl rename to tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs index b1871a63080..d9688cc585c 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs @@ -3,14 +3,14 @@ // Snapshot tests for the F# rendering of quoted expressions (sprintf "%A" <@ ... @>). // // Each test compiles a tiny program that prints the rendered quotation, executes it, and diffs -// the captured stdout against a .bsl file in this folder. Update baselines with TEST_UPDATE_BSL=1. +// stdout against the matching .bsl file in this folder. Regenerate baselines with TEST_UPDATE_BSL=1. // -// Purpose: detect accidental leakage of pattern-match-compilation lowerings (range checks, jump -// tables, null+Length rewrites, etc.) into the public quotation AST. The quotation translator -// runs after PatternMatchCompilation, so any rewrite that lives in PatternMatchCompilation will -// also show up here as a (potentially surprising) shape change. +// Purpose: detect accidental leakage of pattern-match-compilation lowerings (jump tables, null +// + Length rewrites, raw IL asm, ...) into the public quotation AST. The quotation translator +// runs *after* PatternMatchCompilation, so any rewrite done there shows up here as a (potentially +// surprising) shape change. // -// Related: https://github.com/dotnet/fsharp/issues/19873, PR https://github.com/dotnet/fsharp/pull/19532 +// Related: https://github.com/dotnet/fsharp/issues/19873 namespace Conformance.Expressions.ExpressionQuotations @@ -22,96 +22,112 @@ module QuotationRendering = let private baselineDir = __SOURCE_DIRECTORY__ - /// Compile a snippet that builds a quotation `q` and prints it with `printfn "%A" q`, - /// then diff its stdout against `.bsl`. - let private renderingShouldMatch (bslName: string) (quoteBody: string) = - let program = "module Test\n[]\nlet main _ =\n let q = " + quoteBody + "\n printfn \"%A\" q\n 0\n" - let bslPath = Path.Combine(baselineDir, bslName + ".bsl") + /// Wrap a quotation expression in the smallest compilable program that prints its rendering. + let private printerProgram quoteExpr = + $"module Test\nprintfn \"%%A\" {quoteExpr}\n" - FSharp program + /// Compile, run, diff stdout against .bsl. Use TEST_UPDATE_BSL=1 to regenerate. + let private quoteShouldRender name quoteExpr = + FSharp (printerProgram quoteExpr) |> asExe |> ignoreWarnings |> compileExeAndRun |> shouldSucceed - |> verifyOutputAgainstBaseline bslPath + |> verifyOutputAgainstBaseline (Path.Combine(baselineDir, name + ".bsl")) |> ignore - // ---------------------------------------------------------------------- - // The "leaky" cases: PatternMatchCompilation rewrites that show up in quotations. - // ---------------------------------------------------------------------- - - // https://github.com/dotnet/fsharp/issues/19873 - // Empty-string match leaks `IfThenElse (op_Inequality(x,null), x.Length = 0, false)`. - [] - let ``Match empty string`` () = - renderingShouldMatch "EmptyStringMatch" "<@ fun (x: string) -> match x with \"\" -> 1 | _ -> 0 @>" - - // pezipink's second example in #19873 — the same lowering also appears for plain `s = ""`. - [] - let ``If-then-else with literal empty string equality`` () = - renderingShouldMatch "IfThenElseLiteralEqEmpty" "<@ fun (s: string) -> if s = \"\" then 1 else 0 @>" - - // null | "" -> ... should consolidate to a single null check, then a length check. - [] - let ``Match null or empty string`` () = - renderingShouldMatch "NullOrEmptyMatch" "<@ fun (x: string) -> match x with null | \"\" -> 1 | _ -> 0 @>" - - // Array-length match also lowers to null + Length check inline. - // This is a pre-existing leakage: BuildSwitch lowers `match a with [| _ |] -> _` into raw - // `I_ldlen` inline IL via `mkLdlen` before the quotation translator runs. The translator - // then rejects the raw IL with FS0452. When the optimization is moved to a later phase, - // this test should be flipped to assert successful compilation and a clean rendering. - [] - let ``Match array length fails in quotation (FS0452)`` () = - FSharp """module Test + // --------------------------------------------------------------------------- + // Regression: shapes the empty-string optimization used to leak (#19873). + // After moving the rewrite out of BuildSwitch the baselines render `op_Equality` + // rather than the null + Length lowering. A leak resurfacing here will fail + // the test with a diff. + // --------------------------------------------------------------------------- + module Regression = + + [] + let ``match x with empty string`` () = + quoteShouldRender "EmptyString" + """<@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @>""" + + [] + let ``match x with null or empty string`` () = + quoteShouldRender "NullOrEmpty" + """<@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @>""" + + // --------------------------------------------------------------------------- + // Reference: PatternMatchCompilation lowerings that have *never* leaked. + // These baselines prove that compactification (consecutive int → TDSwitch → jump + // table), char tables, and per-type `mkILAsmCeq` paths all survive as clean + // `op_Equality` chains in quotations. If a future BuildSwitch optimization + // changes this, the diff is the smoking gun. + // --------------------------------------------------------------------------- + module NoLeakReference = + + [] + let ``match x with non-empty string`` () = + quoteShouldRender "NonEmptyString" + """<@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @>""" + + /// Two-or-more consecutive ints trigger BuildSwitch's compactify branch. + /// We want to verify the resulting TDSwitch still renders as op_Equality calls + /// (not as an opaque jump-table marker) inside quotations. + [] + let ``match i with consecutive ints`` () = + quoteShouldRender "ConsecutiveInts" + """<@ fun (i: int) -> match i with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @>""" + + [] + let ``match c with chars`` () = + quoteShouldRender "Chars" + """<@ fun (c: char) -> match c with 'a' -> 1 | 'b' -> 2 | _ -> 0 @>""" + + /// Int64/Float/Decimal each take a different BuildSwitch arm but all rely on + /// QuotationTranslator's `[AI_ceq]` → `op_Equality` recovery to render cleanly. + [] + let ``match i with int64`` () = + quoteShouldRender "Int64" + """<@ fun (i: int64) -> match i with 1L -> "a" | _ -> "b" @>""" + + [] + let ``match f with float`` () = + quoteShouldRender "Float" + """<@ fun (f: float) -> match f with 1.0 -> "a" | _ -> "b" @>""" + + [] + let ``match d with decimal`` () = + quoteShouldRender "Decimal" + """<@ fun (d: decimal) -> match d with 1m -> "a" | _ -> "b" @>""" + + // --------------------------------------------------------------------------- + // Side effect of the fix: pezipink's second example in #19873 — `if s = ""` + // was already rendering cleanly in quotations (no BuildSwitch path involved) + // and continues to do so even after the Optimizer-level rewrite, because the + // Optimizer skips Expr.Quote bodies. + // --------------------------------------------------------------------------- + module SideEffect = + + [] + let ``if s = empty string`` () = + quoteShouldRender "IfEqualEmpty" + """<@ fun (s: string) -> if s = "" then 1 else 0 @>""" + + // --------------------------------------------------------------------------- + // Pre-existing leakage: array-length patterns lower to raw `I_ldlen` IL via + // `mkLdlen` *before* the quotation translator runs, which then rejects them + // with FS0452. This was broken before #19189 and is left untouched. If a + // future change moves this lowering out of BuildSwitch, flip to `shouldSucceed` + // and add an `ArrayLength.bsl` baseline. + // --------------------------------------------------------------------------- + module PreExistingError = + + [] + let ``match a with array length`` () = + FSharp """module Test let q = <@ fun (a: int[]) -> match a with [| _ |] -> 1 | _ -> 0 @> """ - |> asLibrary - |> typecheck - |> shouldFail - |> withErrorCode 452 - |> withDiagnosticMessageMatches "Quotations cannot contain inline assembly code or pattern matching on arrays" - |> ignore - - // ---------------------------------------------------------------------- - // Reference cases: confirm other PatternMatchCompilation optimizations do - // NOT leak. These baselines should be a clean if/then/else chain of - // op_Equality calls regardless of compactification or jump-table lowering. - // ---------------------------------------------------------------------- - - [] - let ``Match non-empty strings`` () = - renderingShouldMatch "NonEmptyStringMatch" "<@ fun (x: string) -> match x with \"foo\" -> 1 | \"bar\" -> 2 | _ -> 0 @>" - - // Consecutive ints 1..10 — PatternMatchCompilation compactifies them into a TDSwitch - // which (in IL) eventually becomes a switch table. The quotation should *not* see that; - // it should be a chain of op_Equality tests. - [] - let ``Match consecutive ints 1 to 10`` () = - renderingShouldMatch "ConsecutiveIntMatch" - "<@ fun (i: int) -> match i with 1 -> \"one\" | 2 -> \"two\" | 3 -> \"three\" | 4 -> \"four\" | 5 -> \"five\" | 6 -> \"six\" | 7 -> \"seven\" | 8 -> \"eight\" | 9 -> \"nine\" | 10 -> \"ten\" | _ -> \"other\" @>" - - [] - let ``Match sparse ints`` () = - renderingShouldMatch "SparseIntMatch" - "<@ fun (i: int) -> match i with 1 -> \"a\" | 5 -> \"b\" | 10 -> \"c\" | _ -> \"other\" @>" - - [] - let ``Match chars`` () = - renderingShouldMatch "CharMatch" - "<@ fun (c: char) -> match c with 'a' -> 1 | 'b' -> 2 | 'c' -> 3 | _ -> 0 @>" - - [] - let ``Match int64`` () = - renderingShouldMatch "Int64Match" - "<@ fun (i: int64) -> match i with 1L -> \"a\" | 2L -> \"b\" | 3L -> \"c\" | _ -> \"other\" @>" - - [] - let ``Match float`` () = - renderingShouldMatch "FloatMatch" - "<@ fun (f: float) -> match f with 1.0 -> \"a\" | 2.0 -> \"b\" | _ -> \"other\" @>" - - [] - let ``Match decimal`` () = - renderingShouldMatch "DecimalMatch" - "<@ fun (d: decimal) -> match d with 1m -> \"a\" | 2m -> \"b\" | _ -> \"other\" @>" + |> asLibrary + |> typecheck + |> shouldFail + |> withErrorCode 452 + |> withDiagnosticMessageMatches "Quotations cannot contain inline assembly code or pattern matching on arrays" + |> ignore From 44faef87df84ddff506585dc9ef0c0609a183ce7 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Mon, 8 Jun 2026 16:09:24 +0200 Subject: [PATCH 04/11] Tighten test comments; collapse no-leak quotation tests to Theory The empty-string regression tests and surrounding trade-off comments grew beyond what they need to convey. This trims prose to the causal mechanism only, flattens four banner-only sub-modules into one, collapses the five no-leak reference tests into a parametrised Theory, and corrects a stale docstring claim that Int64/Float/Decimal each take a distinct BuildSwitch arm (Int64 and Float share the mkILAsmCeq path, so Float.bsl was redundant with Int64.bsl and is removed). Release note shrinks from a paragraph to the one user-observable change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../.FSharp.Compiler.Service/11.0.100.md | 2 +- src/Compiler/Optimize/Optimizer.fs | 6 +- .../QuotationRendering/Float.bsl | 3 - .../QuotationRenderingTests.fs | 150 +++++------------- .../EmittedIL/TypeTestsInPatternMatching.fs | 19 +-- 5 files changed, 54 insertions(+), 126 deletions(-) delete mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Float.bsl 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 8596763ca9c..49ec2d86c18 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -75,7 +75,7 @@ ([PR #19724](https://github.com/dotnet/fsharp/pull/19724)) * Emit debug points at a stack-empty position ([PR #19877](https://github.com/dotnet/fsharp/pull/19877)) * Fix spurious XmlDoc warnings (unknown parameter / no documentation for parameter) under `--warnon:3390` when a get/set property documents the full parameter set across both accessors. ([Issue #13684](https://github.com/dotnet/fsharp/issues/13684), [PR #19884](https://github.com/dotnet/fsharp/pull/19884)) -* Quotations: `<@ match s with "" -> ... @>` and `<@ null | "" -> ... @>` no longer leak the `if s <> null then s.Length = 0 else false` lowering introduced by the empty-string optimization. The optimization was moved from pattern-match compilation into the optimizer, so quoted expressions retain their original `op_Equality(s, "")` shape. As a side effect, `if s = "" then ...` now produces the same null-safe length-check IL as `match s with "" -> _`. Trade-offs (documented as locked-in IL baselines): `--optimize-` debug builds emit a plain `String.Equals(s, "")` call (still correct; JIT tiered compilation reaches the fast path); `match s with null | "" -> _` emits a redundant `brfalse` on the empty-string branch that the JIT trivially eliminates. ([Issue #19873](https://github.com/dotnet/fsharp/issues/19873)) +* Quotations of `match s with "" -> _` and `match s with null | "" -> _` no longer leak the `if s <> null then s.Length = 0 else false` lowering; the empty-string optimization moved from pattern-match compilation to the optimizer, so quoted expressions retain their original `op_Equality(s, "")` shape. As a side effect, `if s = "" then _` now produces the same null-safe length-check IL as `match s with "" -> _`. ([Issue #19873](https://github.com/dotnet/fsharp/issues/19873)) ### Added diff --git a/src/Compiler/Optimize/Optimizer.fs b/src/Compiler/Optimize/Optimizer.fs index 2f00bc51c1a..a2a790d56a7 100644 --- a/src/Compiler/Optimize/Optimizer.fs +++ b/src/Compiler/Optimize/Optimizer.fs @@ -2633,9 +2633,9 @@ and OptimizeExprOp cenv env (op, tyargs, args, m) = when IsILMethodRefSystemStringConcat ilMethRef -> MakeOptimizedSystemStringConcatCall cenv env m args - // Optimize `String.Equals(x, "")` (and `String.Equals("", x)`) into the null-safe length check - // `if x <> null then x.Length = 0 else false`. Mirrors the original BuildSwitch lowering for - // `match s with "" -> _`. Done here (post-inlining) so quotations capture the un-optimized shape. + // See MakeOptimizedStringEqualsEmptyCall. Applied post-inlining (on the `String.Equals` shape + // that `(=)` lowers to) so `Expr.Quote` bodies, which the optimizer skips, keep the original + // `op_Equality(s, "")` form. | TOp.ILCall(_, _, _, _, _, _, _, ilMethRef, _, _, _), _, [arg1; Expr.Const(Const.String "", _, _)] when IsILMethodRefSystemStringEquals ilMethRef -> MakeOptimizedStringEqualsEmptyCall cenv env m arg1 diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Float.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Float.bsl deleted file mode 100644 index ec5dffac878..00000000000 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Float.bsl +++ /dev/null @@ -1,3 +0,0 @@ -Lambda (f, - IfThenElse (Call (None, op_Equality, [f, Value (1.0)]), Value ("a"), - Value ("b"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs index d9688cc585c..f9af02f566d 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs @@ -1,16 +1,7 @@ // Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. -// Snapshot tests for the F# rendering of quoted expressions (sprintf "%A" <@ ... @>). -// -// Each test compiles a tiny program that prints the rendered quotation, executes it, and diffs -// stdout against the matching .bsl file in this folder. Regenerate baselines with TEST_UPDATE_BSL=1. -// -// Purpose: detect accidental leakage of pattern-match-compilation lowerings (jump tables, null -// + Length rewrites, raw IL asm, ...) into the public quotation AST. The quotation translator -// runs *after* PatternMatchCompilation, so any rewrite done there shows up here as a (potentially -// surprising) shape change. -// -// Related: https://github.com/dotnet/fsharp/issues/19873 +// Snapshot tests pinning the shape of quoted expressions that pattern-match compilation can rewrite. +// Regression coverage for https://github.com/dotnet/fsharp/issues/19873. namespace Conformance.Expressions.ExpressionQuotations @@ -22,11 +13,9 @@ module QuotationRendering = let private baselineDir = __SOURCE_DIRECTORY__ - /// Wrap a quotation expression in the smallest compilable program that prints its rendering. let private printerProgram quoteExpr = $"module Test\nprintfn \"%%A\" {quoteExpr}\n" - /// Compile, run, diff stdout against .bsl. Use TEST_UPDATE_BSL=1 to regenerate. let private quoteShouldRender name quoteExpr = FSharp (printerProgram quoteExpr) |> asExe @@ -36,98 +25,47 @@ module QuotationRendering = |> verifyOutputAgainstBaseline (Path.Combine(baselineDir, name + ".bsl")) |> ignore - // --------------------------------------------------------------------------- - // Regression: shapes the empty-string optimization used to leak (#19873). - // After moving the rewrite out of BuildSwitch the baselines render `op_Equality` - // rather than the null + Length lowering. A leak resurfacing here will fail - // the test with a diff. - // --------------------------------------------------------------------------- - module Regression = - - [] - let ``match x with empty string`` () = - quoteShouldRender "EmptyString" - """<@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @>""" - - [] - let ``match x with null or empty string`` () = - quoteShouldRender "NullOrEmpty" - """<@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @>""" - - // --------------------------------------------------------------------------- - // Reference: PatternMatchCompilation lowerings that have *never* leaked. - // These baselines prove that compactification (consecutive int → TDSwitch → jump - // table), char tables, and per-type `mkILAsmCeq` paths all survive as clean - // `op_Equality` chains in quotations. If a future BuildSwitch optimization - // changes this, the diff is the smoking gun. - // --------------------------------------------------------------------------- - module NoLeakReference = - - [] - let ``match x with non-empty string`` () = - quoteShouldRender "NonEmptyString" - """<@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @>""" - - /// Two-or-more consecutive ints trigger BuildSwitch's compactify branch. - /// We want to verify the resulting TDSwitch still renders as op_Equality calls - /// (not as an opaque jump-table marker) inside quotations. - [] - let ``match i with consecutive ints`` () = - quoteShouldRender "ConsecutiveInts" - """<@ fun (i: int) -> match i with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @>""" - - [] - let ``match c with chars`` () = - quoteShouldRender "Chars" - """<@ fun (c: char) -> match c with 'a' -> 1 | 'b' -> 2 | _ -> 0 @>""" - - /// Int64/Float/Decimal each take a different BuildSwitch arm but all rely on - /// QuotationTranslator's `[AI_ceq]` → `op_Equality` recovery to render cleanly. - [] - let ``match i with int64`` () = - quoteShouldRender "Int64" - """<@ fun (i: int64) -> match i with 1L -> "a" | _ -> "b" @>""" - - [] - let ``match f with float`` () = - quoteShouldRender "Float" - """<@ fun (f: float) -> match f with 1.0 -> "a" | _ -> "b" @>""" - - [] - let ``match d with decimal`` () = - quoteShouldRender "Decimal" - """<@ fun (d: decimal) -> match d with 1m -> "a" | _ -> "b" @>""" - - // --------------------------------------------------------------------------- - // Side effect of the fix: pezipink's second example in #19873 — `if s = ""` - // was already rendering cleanly in quotations (no BuildSwitch path involved) - // and continues to do so even after the Optimizer-level rewrite, because the - // Optimizer skips Expr.Quote bodies. - // --------------------------------------------------------------------------- - module SideEffect = - - [] - let ``if s = empty string`` () = - quoteShouldRender "IfEqualEmpty" - """<@ fun (s: string) -> if s = "" then 1 else 0 @>""" - - // --------------------------------------------------------------------------- - // Pre-existing leakage: array-length patterns lower to raw `I_ldlen` IL via - // `mkLdlen` *before* the quotation translator runs, which then rejects them - // with FS0452. This was broken before #19189 and is left untouched. If a - // future change moves this lowering out of BuildSwitch, flip to `shouldSucceed` - // and add an `ArrayLength.bsl` baseline. - // --------------------------------------------------------------------------- - module PreExistingError = - - [] - let ``match a with array length`` () = - FSharp """module Test + [] + let ``Regression #19873: match x with empty string renders as op_Equality`` () = + quoteShouldRender "EmptyString" + """<@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @>""" + + [] + let ``Regression #19873: match x with null or empty string renders as op_Equality`` () = + quoteShouldRender "NullOrEmpty" + """<@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @>""" + + // Reference baselines: pattern-match-compilation lowerings that have never leaked into + // quotations. Pins the BuildSwitch arms so a future optimization that starts rewriting + // them in-place (the bug class behind #19873) shows up as a diff. + // - NonEmptyString, ConsecutiveInts, Chars : op_Equality and the compactify/jump-table path + // - Int64 : the `mkILAsmCeq` arm + `[AI_ceq]` -> op_Equality recovery + // - Decimal : op_Equality with a MakeDecimal literal + [] + [ match x with "a" -> 1 | "b" -> 2 | _ -> 0 @>""")>] + [ match i with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @>""")>] + [ match c with 'a' -> 1 | 'b' -> 2 | _ -> 0 @>""")>] + [ match i with 1L -> "a" | _ -> "b" @>""")>] + [ match d with 1m -> "a" | _ -> "b" @>""")>] + let ``no-leak reference: BuildSwitch arms render cleanly in quotations`` (name: string) (quoteExpr: string) = + quoteShouldRender name quoteExpr + + [] + let ``Second repro from #19873: if s = empty string also renders cleanly`` () = + quoteShouldRender "IfEqualEmpty" + """<@ fun (s: string) -> if s = "" then 1 else 0 @>""" + + // Pre-existing leakage: array-length patterns lower to `I_ldlen` IL via `mkLdlen` *before* the + // quotation translator runs, which rejects them with FS0452. Out of scope for #19873; flip to + // shouldSucceed + add an ArrayLength.bsl when that lowering moves out of BuildSwitch. + [] + let ``Pre-existing: match a with array length still errors with FS0452`` () = + FSharp """module Test let q = <@ fun (a: int[]) -> match a with [| _ |] -> 1 | _ -> 0 @> """ - |> asLibrary - |> typecheck - |> shouldFail - |> withErrorCode 452 - |> withDiagnosticMessageMatches "Quotations cannot contain inline assembly code or pattern matching on arrays" - |> ignore + |> asLibrary + |> typecheck + |> shouldFail + |> withErrorCode 452 + |> withDiagnosticMessageMatches "Quotations cannot contain inline assembly code or pattern matching on arrays" + |> ignore diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs index 34cc30bef77..d372ba14426 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs @@ -383,12 +383,9 @@ let TestEmptyStringEq(x: string) = """ ] - // https://github.com/dotnet/fsharp/issues/19873 - // Trade-off recorded: when the empty-string optimization moved from BuildSwitch to the Optimizer, - // `--optimize-` (debug) builds no longer get the null+Length form because F#'s `(=)` operator is - // only inlined when `LocalOptimizationsEnabled = true`. Debug builds now emit a plain - // `String.Equals(s, "")` call. JIT tiered compilation reaches the same fast path; debug-mode IL - // size is not a perf concern. Locked in so a future change can revisit consciously. + // Baseline for #19873: under `--optimize-` the null+Length form is not emitted, because F#'s + // `(=)` is only inlined when `LocalOptimizationsEnabled = true`; the call falls through to + // `String.Equals(s, "")`. JIT tiered compilation still reaches the fast path. [] let ``Test codegen for empty string pattern under --optimize-``() = FSharp """ @@ -425,13 +422,9 @@ let TestEmptyStringPattern(x: string) = """ ] - // https://github.com/dotnet/fsharp/issues/19873 - // Trade-off recorded: `null | "" -> _` patterns previously elided the inner null check via - // BuildSwitch's `isNullFiltered` flag (so the IL emitted only the `null` pattern's `brfalse` - // followed by a bare `s.Length = 0`). After moving the optimization to the Optimizer, the - // optimizer has no view of the enclosing null-filtered context and so re-emits the null check. - // The result is two back-to-back `brfalse.s` on the same argument. JIT trivially eliminates the - // redundant branch; we accept the few extra IL bytes in exchange for clean quotations. + // Baseline for #19873: the optimizer can't see BuildSwitch's `isNullFiltered` flag, so the + // empty-string branch under `null | "" -> _` re-emits its own null check. Result: two + // back-to-back `brfalse.s` on the same argument (the JIT folds the duplicate). [] let ``Test codegen for null-or-empty-string pattern``() = FSharp """ From 01d4e0b43ea191c830f15557fa51aa0dbd78b98b Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 9 Jun 2026 10:27:39 +0200 Subject: [PATCH 05/11] Refactor quotation rendering tests to in-process literals after multi-architect review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three rounds of adversarial multi-model (gpt-5.5, opus-4.7-xhigh, opus-4.8) architect review converged on a cleaner design for the empty-string regression coverage: - Quotation literals now live directly in test method bodies (`<@ … @>`) instead of inside a generated FSI script. They are desugared at test-project compile time by the proto fsc, which converts a runtime baseline diff into a compile-time gate: if #19873 ever regresses, the test source itself fails to compile with FS0452. - Removes the shared-FSI session, the no-op `RunTestCasesInSequence` cargo cult, the string-templated printer with its triple-quote escape guard, and the unused `verifyOutputAgainstBaseline` helper this PR briefly introduced. - Baselines carry a `// ` provenance header so a `.bsl` opened in isolation identifies itself in PR diffs. - Adds an orphan-guard `[]` that fails when `*.bsl` on disk drifts from the test method set (skipped during `TEST_UPDATE_BSL=1` to avoid racing with writes). - Adds a structural Expr walker as belt-and-suspenders alongside the baseline for the two known leaked-lowering shapes (`String.Length`, `String.Equals`). - Adds a convergence assertion that the `match x with ""` and `if x = ""` quotations desugar to byte-identical Exprs, using shared module-level let bindings so the proof and the baselines cannot drift apart. - Splits the FS0452 array-pattern diagnostic test into its own file since it uses a different harness (`FSharp |> typecheck |> shouldFail`, not a `.bsl` snapshot). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../QuotationRendering/Chars.bsl | 7 +- .../QuotationRendering/ConsecutiveInts.bsl | 9 +- .../QuotationRendering/Decimal.bsl | 5 +- .../QuotationRendering/EmptyString.bsl | 1 + .../QuotationRendering/IfEqualEmpty.bsl | 5 +- .../QuotationRendering/Int64.bsl | 5 +- .../QuotationRendering/NonEmptyString.bsl | 1 + .../QuotationRendering/NullOrEmpty.bsl | 1 + .../QuotationRenderingTests.fs | 168 +++++++++++++----- .../QuotationUnsupportedConstructsTests.fs | 28 +++ .../FSharp.Compiler.ComponentTests.fsproj | 1 + tests/FSharp.Test.Utilities/Compiler.fs | 15 -- 12 files changed, 176 insertions(+), 70 deletions(-) create mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationUnsupportedConstructsTests.fs diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl index 7de6ea4d390..021d0c928e7 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl @@ -1,4 +1,5 @@ -Lambda (c, - IfThenElse (Call (None, op_Equality, [c, Value ('a')]), Value (1), - IfThenElse (Call (None, op_Equality, [c, Value ('b')]), +// Chars +Lambda (x, + IfThenElse (Call (None, op_Equality, [x, Value ('a')]), Value (1), + IfThenElse (Call (None, op_Equality, [x, Value ('b')]), Value (2), Value (0)))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl index 45187cc931d..3eb5fb16b25 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl @@ -1,7 +1,8 @@ -Lambda (i, - IfThenElse (Call (None, op_Equality, [i, Value (1)]), Value ("a"), - IfThenElse (Call (None, op_Equality, [i, Value (2)]), +// ConsecutiveInts +Lambda (x, + IfThenElse (Call (None, op_Equality, [x, Value (1)]), Value ("a"), + IfThenElse (Call (None, op_Equality, [x, Value (2)]), Value ("b"), IfThenElse (Call (None, op_Equality, - [i, Value (3)]), Value ("c"), + [x, Value (3)]), Value ("c"), Value ("z"))))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl index 8e7a7a28f94..5a3c28bd680 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl @@ -1,6 +1,7 @@ -Lambda (d, +// Decimal +Lambda (x, IfThenElse (Call (None, op_Equality, - [d, + [x, Call (None, MakeDecimal, [Value (1), Value (0), Value (0), Value (false), Value (0uy)])]), Value ("a"), Value ("b"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl index 848475282af..da2e876a86b 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl @@ -1,3 +1,4 @@ +// EmptyString Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value ("")]), Value (1), Value (0))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl index 6de55e76496..d354abc4620 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl @@ -1,3 +1,4 @@ -Lambda (s, - IfThenElse (Call (None, op_Equality, [s, Value ("")]), Value (1), +// IfEqualEmpty +Lambda (x, + IfThenElse (Call (None, op_Equality, [x, Value ("")]), Value (1), Value (0))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl index c30b9443b71..361159dd782 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl @@ -1,3 +1,4 @@ -Lambda (i, - IfThenElse (Call (None, op_Equality, [i, Value (1L)]), Value ("a"), +// Int64 +Lambda (x, + IfThenElse (Call (None, op_Equality, [x, Value (1L)]), Value ("a"), Value ("b"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl index d948553e5f5..e266fe0e825 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl @@ -1,3 +1,4 @@ +// NonEmptyString Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value ("a")]), Value (1), IfThenElse (Call (None, op_Equality, [x, Value ("b")]), diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl index 408cccd7c53..8bbf2168727 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl @@ -1,3 +1,4 @@ +// NullOrEmpty Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value ()]), Value (1), IfThenElse (Call (None, op_Equality, [x, Value ("")]), diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs index f9af02f566d..aa76fd17b2e 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs @@ -1,11 +1,28 @@ // Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. -// Snapshot tests pinning the shape of quoted expressions that pattern-match compilation can rewrite. +// Snapshot tests pinning `sprintf "%A"` of quoted expressions that pattern-match compilation can rewrite. // Regression coverage for https://github.com/dotnet/fsharp/issues/19873. +// +// Design notes: +// - Quotation literals are written directly as F# in the test method body. They are desugared by +// the F# compiler that builds the test project — the bootstrapped (proto) fsc, NOT the freshly +// built FSharp.Compiler.Service.dll that the test ProjectReferences. CI typically rebuilds the +// proto per run; inner-loop dev after touching `src/Compiler/Checking/PatternMatchCompilation.fs` +// (or other desugar paths) requires `./build.sh --bootstrap -test`, otherwise these tests +// exercise the cached proto compiler. +// - Conversely, this is a stronger regression gate than a runtime assertion: if the #19873 fix is +// ever reverted such that `match x with ""` lowers to inline IL, the literals in this file fail +// to compile with FS0452 (see QuotationUnsupportedConstructsTests.fs). +// - All lambda parameters are named `x` so the binder name carries no signal and parameter renames +// never cause baseline diffs. +// - Baselines start with `// ` so a `.bsl` opened in isolation is self-describing. namespace Conformance.Expressions.ExpressionQuotations open System.IO +open Microsoft.FSharp.Quotations +open Microsoft.FSharp.Quotations.Patterns +open Microsoft.FSharp.Quotations.ExprShape open Xunit open FSharp.Test.Compiler @@ -13,59 +30,126 @@ module QuotationRendering = let private baselineDir = __SOURCE_DIRECTORY__ - let private printerProgram quoteExpr = - $"module Test\nprintfn \"%%A\" {quoteExpr}\n" + let private renderToBaseline (name: string) (q: Expr) = + let body = sprintf "%A" q |> normalizeNewlines + let actual = sprintf "// %s\n%s\n" name body + checkBaseline actual (Path.Combine(baselineDir, name + ".bsl")) - let private quoteShouldRender name quoteExpr = - FSharp (printerProgram quoteExpr) - |> asExe - |> ignoreWarnings - |> compileExeAndRun - |> shouldSucceed - |> verifyOutputAgainstBaseline (Path.Combine(baselineDir, name + ".bsl")) - |> ignore + /// Belt-and-suspenders sniff for the two specific lowering shapes #19873 was about: + /// `String.Length` PropertyGet and `String.Equals` Call. The `.bsl` baseline below is the + /// authoritative guard (it catches any %A shape drift); this walker only adds a clearer + /// failure message for the two known shapes, so a future leaked lowering through a different + /// API (String.IsNullOrEmpty, indexer, etc.) is still caught by the baseline diff, not here. + let rec private containsEmptyStringLowering (e: Expr) = + let isStringMember (t: System.Type) = + not (isNull t) && t.FullName = "System.String" + match e with + | PropertyGet (_, pi, _) when isStringMember pi.DeclaringType && pi.Name = "Length" -> true + | Call (_, mi, _) when isStringMember mi.DeclaringType && mi.Name = "Equals" -> true + | ShapeCombination (_, args) -> args |> List.exists containsEmptyStringLowering + | ShapeLambda (_, body) -> containsEmptyStringLowering body + | ShapeVar _ -> false + + let private assertNoEmptyStringLowering (q: Expr) = + Assert.False(containsEmptyStringLowering q, + sprintf "Quotation leaked the empty-string lowering (String.Length or String.Equals):\n%A" q) + + // Shared quotation literals: the convergence assertion below proves the SAME exprs that feed + // the EmptyString and IfEqualEmpty baselines desugar identically. Without this hoist, an editor + // could change one literal and leave the convergence test green against an out-of-date pair. + let private qMatchEmpty = <@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @> + let private qIfEqualEmpty = <@ fun (x: string) -> if x = "" then 1 else 0 @> + + /// Authoritative list of case names. The orphan-guard test below enforces that + /// `*.bsl` on disk equals this set exactly. Skipped during baseline refresh + /// (`TEST_UPDATE_BSL=1`) to avoid racing with in-progress baseline writes. + let private expectedBaselines = + Set.ofList [ + "EmptyString" + "NullOrEmpty" + "NonEmptyString" + "ConsecutiveInts" + "Chars" + "Int64" + "Decimal" + "IfEqualEmpty" + ] + + [] + let ``Baseline directory: no orphans, no missing`` () = + if System.Environment.GetEnvironmentVariable("TEST_UPDATE_BSL") <> null then () else + let onDisk = + Directory.GetFiles(baselineDir, "*.bsl") + |> Array.map Path.GetFileNameWithoutExtension + |> Set.ofArray + let orphans = Set.difference onDisk expectedBaselines + let missing = Set.difference expectedBaselines onDisk + if not orphans.IsEmpty || not missing.IsEmpty then + let parts = [ + if not orphans.IsEmpty then + yield sprintf "Orphan .bsl files (delete the file, or add a matching test + entry in expectedBaselines): %A" orphans + if not missing.IsEmpty then + yield sprintf "Missing .bsl files (add the test that calls renderToBaseline and run with TEST_UPDATE_BSL=1): %A" missing + ] + Assert.Fail(String.concat "\n" parts) + + // Regression for #19873: empty-string optimization must not leak `if s <> null then s.Length = 0 else false` + // into quotation ASTs. Each Fact pairs the baseline pin with a structural assertion that doesn't + // depend on `%A` formatting. [] - let ``Regression #19873: match x with empty string renders as op_Equality`` () = - quoteShouldRender "EmptyString" - """<@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @>""" + let ``match x with empty string renders as op_Equality (#19873)`` () = + assertNoEmptyStringLowering qMatchEmpty + renderToBaseline "EmptyString" qMatchEmpty [] - let ``Regression #19873: match x with null or empty string renders as op_Equality`` () = - quoteShouldRender "NullOrEmpty" - """<@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @>""" + let ``match x with null or empty string renders as op_Equality (#19873)`` () = + let q = <@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @> + assertNoEmptyStringLowering q + renderToBaseline "NullOrEmpty" q // Reference baselines: pattern-match-compilation lowerings that have never leaked into // quotations. Pins the BuildSwitch arms so a future optimization that starts rewriting // them in-place (the bug class behind #19873) shows up as a diff. - // - NonEmptyString, ConsecutiveInts, Chars : op_Equality and the compactify/jump-table path + // - NonEmptyString, ConsecutiveInts, Chars : op_Equality + compactify/jump-table path // - Int64 : the `mkILAsmCeq` arm + `[AI_ceq]` -> op_Equality recovery // - Decimal : op_Equality with a MakeDecimal literal - [] - [ match x with "a" -> 1 | "b" -> 2 | _ -> 0 @>""")>] - [ match i with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @>""")>] - [ match c with 'a' -> 1 | 'b' -> 2 | _ -> 0 @>""")>] - [ match i with 1L -> "a" | _ -> "b" @>""")>] - [ match d with 1m -> "a" | _ -> "b" @>""")>] - let ``no-leak reference: BuildSwitch arms render cleanly in quotations`` (name: string) (quoteExpr: string) = - quoteShouldRender name quoteExpr [] - let ``Second repro from #19873: if s = empty string also renders cleanly`` () = - quoteShouldRender "IfEqualEmpty" - """<@ fun (s: string) -> if s = "" then 1 else 0 @>""" + let NonEmptyString () = + renderToBaseline "NonEmptyString" + <@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @> + + [] + let ConsecutiveInts () = + renderToBaseline "ConsecutiveInts" + <@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @> + + [] + let Chars () = + renderToBaseline "Chars" + <@ fun (x: char) -> match x with 'a' -> 1 | 'b' -> 2 | _ -> 0 @> + + [] + let Int64 () = + renderToBaseline "Int64" + <@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @> + + [] + let Decimal () = + renderToBaseline "Decimal" + <@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @> + + // Side effect of the move: `if x = ""` was already clean in quotations (no BuildSwitch path) + // and stays clean after the rewrite moved to the Optimizer (which skips Expr.Quote bodies). + [] + let ``if x = empty string renders cleanly (second repro from #19873)`` () = + assertNoEmptyStringLowering qIfEqualEmpty + renderToBaseline "IfEqualEmpty" qIfEqualEmpty - // Pre-existing leakage: array-length patterns lower to `I_ldlen` IL via `mkLdlen` *before* the - // quotation translator runs, which rejects them with FS0452. Out of scope for #19873; flip to - // shouldSucceed + add an ArrayLength.bsl when that lowering moves out of BuildSwitch. + // Convergence: surface forms `match x with "" -> _` and `if x = "" then _` desugar to the + // SAME quoted Expr after binder standardization. Asserts on the SHARED literals so the + // convergence proof cannot drift relative to the baselined exprs above. [] - let ``Pre-existing: match a with array length still errors with FS0452`` () = - FSharp """module Test -let q = <@ fun (a: int[]) -> match a with [| _ |] -> 1 | _ -> 0 @> -""" - |> asLibrary - |> typecheck - |> shouldFail - |> withErrorCode 452 - |> withDiagnosticMessageMatches "Quotations cannot contain inline assembly code or pattern matching on arrays" - |> ignore + let ``match-empty and if-equal-empty quotations converge to the same Expr`` () = + Assert.Equal(sprintf "%A" qMatchEmpty, sprintf "%A" qIfEqualEmpty) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationUnsupportedConstructsTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationUnsupportedConstructsTests.fs new file mode 100644 index 00000000000..031400d9eb9 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationUnsupportedConstructsTests.fs @@ -0,0 +1,28 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. + +// Diagnostic test (FS0452): array-length patterns lower to `I_ldlen` IL via `mkLdlen` BEFORE the +// quotation translator runs, which rejects them. Lives in its own file because it uses the +// typecheck/diagnostic harness (FSharp |> typecheck |> shouldFail), not the in-process baseline +// harness in the sibling QuotationRenderingTests.fs. +// +// Out of scope for #19873. Flip to shouldSucceed and add an `ArrayLength.bsl` baseline when that +// lowering moves out of BuildSwitch. + +namespace Conformance.Expressions.ExpressionQuotations + +open Xunit +open FSharp.Test.Compiler + +module QuotationUnsupportedConstructs = + + [] + let ``match x with array length still errors with FS0452`` () = + FSharp """module Test +let q = <@ fun (x: int[]) -> match x with [| _ |] -> 1 | _ -> 0 @> +""" + |> asLibrary + |> typecheck + |> shouldFail + |> withErrorCode 452 + |> withDiagnosticMessageMatches "Quotations cannot contain inline assembly code or pattern matching on arrays" + |> ignore diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index 4307e3f8230..f6f9fa84ecc 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -114,6 +114,7 @@ + diff --git a/tests/FSharp.Test.Utilities/Compiler.fs b/tests/FSharp.Test.Utilities/Compiler.fs index cdec86ec163..d5ae4a34a27 100644 --- a/tests/FSharp.Test.Utilities/Compiler.fs +++ b/tests/FSharp.Test.Utilities/Compiler.fs @@ -2210,21 +2210,6 @@ $ code --diff {outFile} {expectedFile} let withEvalTypeEquals t (result: CompilationResult) : CompilationResult = assertEvalOutput (fun (x: FsiValue) -> x.ReflectionType) t result - /// Snapshot the captured stdout of an executed test program and diff it against a `.bsl` baseline file. - /// Combine with `compileExeAndRun`. Useful for locking in textual representations such as `sprintf "%A" <@ ... @>` - /// to detect accidental changes in the F# representation of quotations, error messages, printf output, etc. - /// Update with `TEST_UPDATE_BSL=1`. - let verifyOutputAgainstBaseline (baselineFilePath: string) (result: CompilationResult) : CompilationResult = - match result.RunOutput with - | None -> - failwith "Execution output is missing; verifyOutputAgainstBaseline requires a run result (use compileExeAndRun)." - | Some (ExecutionOutput e) -> - let actual = e.StdOut |> normalizeNewlines - checkBaseline actual baselineFilePath - | Some _ -> - failwith "verifyOutputAgainstBaseline only supports ExecutionOutput; received EvalOutput." - result - let signatureText (pageWidth: int option) (checkResults: FSharp.Compiler.CodeAnalysis.FSharpCheckFileResults) = checkResults.GenerateSignature(?pageWidth = pageWidth) |> Option.defaultWith (fun _ -> failwith "Unable to generate signature text.") From 1ff156c16aa419d80bee1a769994559a62e77392 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 9 Jun 2026 11:44:08 +0200 Subject: [PATCH 06/11] Use tname_* constants instead of magic strings in IL method-ref matchers `tname_String`, `tname_Bool`, `tname_Type` already exist in `src/Compiler/AbstractIL/il.fs` as `[]` constants alongside ~20 other type-name literals, but they were not exposed via `il.fsi` so callers in `Optimize/Optimizer.fs` and `Symbols/Exprs.fs` had been duplicating the magic strings (`"System.String"`, `"System.Boolean"`, `"System.Type"`). Exposes the three names actually referenced outside `il.fs` via `il.fsi` and switches the four affected `IL{TypeRef,MethodRef}.Name` comparisons to use them. No behavior change. The fix applies to the empty-string-equals matcher this PR added plus the pre-existing `String.Concat` / `String.GetHashCode` / `Type.GetTypeFromHandle` matchers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Compiler/AbstractIL/il.fsi | 14 ++++++++++++++ src/Compiler/Optimize/Optimizer.fs | 18 +++++++++--------- src/Compiler/Symbols/Exprs.fs | 4 ++-- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/Compiler/AbstractIL/il.fsi b/src/Compiler/AbstractIL/il.fsi index 3539e235518..3d94b80d472 100644 --- a/src/Compiler/AbstractIL/il.fsi +++ b/src/Compiler/AbstractIL/il.fsi @@ -2471,6 +2471,20 @@ val internal ecmaPublicKey: PublicKey /// Strips ILType.Modified from the ILType. val internal stripILModifiedFromTy: ILType -> ILType +// Names of important built-in types, exported so callers can use them in +// `mref.DeclaringTypeRef.Name = tname_X` comparisons instead of repeating magic strings. +// Only the names referenced outside `il.fs` are exposed; add more here as needed. +// (TcGlobals.fsi separately exports `tname_Enum` / `tname_Array`.) + +[] +val internal tname_String: string = "System.String" + +[] +val internal tname_Type: string = "System.Type" + +[] +val internal tname_Bool: string = "System.Boolean" + /// Discriminating different important built-in types. val internal isILObjectTy: ILGlobals -> ILType -> bool val internal isILStringTy: ILGlobals -> ILType -> bool diff --git a/src/Compiler/Optimize/Optimizer.fs b/src/Compiler/Optimize/Optimizer.fs index a2a790d56a7..90516f7bfbd 100644 --- a/src/Compiler/Optimize/Optimizer.fs +++ b/src/Compiler/Optimize/Optimizer.fs @@ -2294,33 +2294,33 @@ let TryDetectQueryQuoteAndRun cenv (expr: Expr) = let IsILMethodRefSystemStringEquals (mref: ILMethodRef) = mref.Name = "Equals" && mref.CallingConv.IsStatic && - mref.DeclaringTypeRef.Name = "System.String" && - (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = "System.Boolean") && + mref.DeclaringTypeRef.Name = tname_String && + (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = tname_Bool) && (mref.ArgCount = 2 && mref.ArgTypes |> List.forall (fun ilTy -> - ilTy.IsNominal && ilTy.TypeRef.Name = "System.String")) + ilTy.IsNominal && ilTy.TypeRef.Name = tname_String)) let IsILMethodRefSystemStringConcat (mref: ILMethodRef) = mref.Name = "Concat" && - mref.DeclaringTypeRef.Name = "System.String" && - (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = "System.String") && + mref.DeclaringTypeRef.Name = tname_String && + (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = tname_String) && (mref.ArgCount >= 2 && mref.ArgCount <= 4 && mref.ArgTypes |> List.forall (fun ilTy -> - ilTy.IsNominal && ilTy.TypeRef.Name = "System.String")) + ilTy.IsNominal && ilTy.TypeRef.Name = tname_String)) let IsILMethodRefSystemStringConcatArray (mref: ILMethodRef) = mref.Name = "Concat" && - mref.DeclaringTypeRef.Name = "System.String" && - (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = "System.String") && + mref.DeclaringTypeRef.Name = tname_String && + (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = tname_String) && (mref.ArgCount = 1 && mref.ArgTypes |> List.forall (fun ilTy -> match ilTy with | ILType.Array (shape, ilTy) when shape = ILArrayShape.SingleDimensional && ilTy.IsNominal && - ilTy.TypeRef.Name = "System.String" -> true + ilTy.TypeRef.Name = tname_String -> true | _ -> false)) let rec IsDebugPipeRightExpr cenv expr = diff --git a/src/Compiler/Symbols/Exprs.fs b/src/Compiler/Symbols/Exprs.fs index 4281393e5e8..0f494f402b9 100644 --- a/src/Compiler/Symbols/Exprs.fs +++ b/src/Compiler/Symbols/Exprs.fs @@ -759,14 +759,14 @@ module FSharpExprConvert = ConvExprPrim cenv env op | TOp.ILAsm ([ I_call (Normalcall, mspec, None) ], _), _, [arg] - when mspec.MethodRef.DeclaringTypeRef.Name = "System.String" && mspec.Name = "GetHashCode" -> + when mspec.MethodRef.DeclaringTypeRef.Name = tname_String && mspec.Name = "GetHashCode" -> let ty = tyOfExpr g arg let op = mkCallHash g m ty arg ConvExprPrim cenv env op | TOp.ILCall (_, _, _, _, _, _, _, ilMethRef, _, _, _), [], [Expr.Op (TOp.ILAsm ([ I_ldtoken (ILToken.ILType _) ], _), [ty], _, _)] - when ilMethRef.DeclaringTypeRef.Name = "System.Type" && ilMethRef.Name = "GetTypeFromHandle" -> + when ilMethRef.DeclaringTypeRef.Name = tname_Type && ilMethRef.Name = "GetTypeFromHandle" -> let op = mkCallTypeOf g m ty ConvExprPrim cenv env op From 8523cd026d9ad0dea6d2b4b2308d390f810781fa Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 9 Jun 2026 11:46:00 +0200 Subject: [PATCH 07/11] Trim bloated comments in QuotationRenderingTests.fs 5-line walker docstring, 3-line section-divider blocks, 6-line bullet list above the no-leak reference Facts, and a 12-line "Design notes" header were rationalising self-evident code or restating test names. Trimmed to the essentials: top-of-file purpose + bootstrap caveat, one-line note next to the shared `let` bindings, and the one genuinely-non-obvious comment (Int64 takes the mkILAsmCeq arm). Net effect: file shrinks from 156 to 100 lines, signal density up, tests unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../QuotationRenderingTests.fs | 103 ++++-------------- 1 file changed, 22 insertions(+), 81 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs index aa76fd17b2e..c029517e75f 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs @@ -1,21 +1,8 @@ // Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. -// Snapshot tests pinning `sprintf "%A"` of quoted expressions that pattern-match compilation can rewrite. -// Regression coverage for https://github.com/dotnet/fsharp/issues/19873. -// -// Design notes: -// - Quotation literals are written directly as F# in the test method body. They are desugared by -// the F# compiler that builds the test project — the bootstrapped (proto) fsc, NOT the freshly -// built FSharp.Compiler.Service.dll that the test ProjectReferences. CI typically rebuilds the -// proto per run; inner-loop dev after touching `src/Compiler/Checking/PatternMatchCompilation.fs` -// (or other desugar paths) requires `./build.sh --bootstrap -test`, otherwise these tests -// exercise the cached proto compiler. -// - Conversely, this is a stronger regression gate than a runtime assertion: if the #19873 fix is -// ever reverted such that `match x with ""` lowers to inline IL, the literals in this file fail -// to compile with FS0452 (see QuotationUnsupportedConstructsTests.fs). -// - All lambda parameters are named `x` so the binder name carries no signal and parameter renames -// never cause baseline diffs. -// - Baselines start with `// ` so a `.bsl` opened in isolation is self-describing. +// Quotation rendering snapshots — regression for https://github.com/dotnet/fsharp/issues/19873. +// Literals are desugared by the bootstrap fsc that built the test project; rerun with +// `./build.sh --bootstrap -test` after touching `src/Compiler/Checking/PatternMatchCompilation.fs`. namespace Conformance.Expressions.ExpressionQuotations @@ -32,20 +19,13 @@ module QuotationRendering = let private renderToBaseline (name: string) (q: Expr) = let body = sprintf "%A" q |> normalizeNewlines - let actual = sprintf "// %s\n%s\n" name body - checkBaseline actual (Path.Combine(baselineDir, name + ".bsl")) - - /// Belt-and-suspenders sniff for the two specific lowering shapes #19873 was about: - /// `String.Length` PropertyGet and `String.Equals` Call. The `.bsl` baseline below is the - /// authoritative guard (it catches any %A shape drift); this walker only adds a clearer - /// failure message for the two known shapes, so a future leaked lowering through a different - /// API (String.IsNullOrEmpty, indexer, etc.) is still caught by the baseline diff, not here. + checkBaseline (sprintf "// %s\n%s\n" name body) (Path.Combine(baselineDir, name + ".bsl")) + let rec private containsEmptyStringLowering (e: Expr) = - let isStringMember (t: System.Type) = - not (isNull t) && t.FullName = "System.String" + let isString (t: System.Type) = not (isNull t) && t.FullName = "System.String" match e with - | PropertyGet (_, pi, _) when isStringMember pi.DeclaringType && pi.Name = "Length" -> true - | Call (_, mi, _) when isStringMember mi.DeclaringType && mi.Name = "Equals" -> true + | PropertyGet (_, pi, _) when isString pi.DeclaringType && pi.Name = "Length" -> true + | Call (_, mi, _) when isString mi.DeclaringType && mi.Name = "Equals" -> true | ShapeCombination (_, args) -> args |> List.exists containsEmptyStringLowering | ShapeLambda (_, body) -> containsEmptyStringLowering body | ShapeVar _ -> false @@ -54,25 +34,14 @@ module QuotationRendering = Assert.False(containsEmptyStringLowering q, sprintf "Quotation leaked the empty-string lowering (String.Length or String.Equals):\n%A" q) - // Shared quotation literals: the convergence assertion below proves the SAME exprs that feed - // the EmptyString and IfEqualEmpty baselines desugar identically. Without this hoist, an editor - // could change one literal and leave the convergence test green against an out-of-date pair. - let private qMatchEmpty = <@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @> + // Shared so the convergence test below cannot drift from the baselined exprs. + let private qMatchEmpty = <@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @> let private qIfEqualEmpty = <@ fun (x: string) -> if x = "" then 1 else 0 @> - /// Authoritative list of case names. The orphan-guard test below enforces that - /// `*.bsl` on disk equals this set exactly. Skipped during baseline refresh - /// (`TEST_UPDATE_BSL=1`) to avoid racing with in-progress baseline writes. let private expectedBaselines = Set.ofList [ - "EmptyString" - "NullOrEmpty" - "NonEmptyString" - "ConsecutiveInts" - "Chars" - "Int64" - "Decimal" - "IfEqualEmpty" + "EmptyString"; "NullOrEmpty"; "NonEmptyString"; "ConsecutiveInts" + "Chars"; "Int64"; "Decimal"; "IfEqualEmpty" ] [] @@ -85,17 +54,10 @@ module QuotationRendering = let orphans = Set.difference onDisk expectedBaselines let missing = Set.difference expectedBaselines onDisk if not orphans.IsEmpty || not missing.IsEmpty then - let parts = [ - if not orphans.IsEmpty then - yield sprintf "Orphan .bsl files (delete the file, or add a matching test + entry in expectedBaselines): %A" orphans - if not missing.IsEmpty then - yield sprintf "Missing .bsl files (add the test that calls renderToBaseline and run with TEST_UPDATE_BSL=1): %A" missing - ] - Assert.Fail(String.concat "\n" parts) - - // Regression for #19873: empty-string optimization must not leak `if s <> null then s.Length = 0 else false` - // into quotation ASTs. Each Fact pairs the baseline pin with a structural assertion that doesn't - // depend on `%A` formatting. + Assert.Fail( + [ if not orphans.IsEmpty then yield sprintf "Orphan .bsl: %A (delete or add a matching test)" orphans + if not missing.IsEmpty then yield sprintf "Missing .bsl: %A (add the test and run with TEST_UPDATE_BSL=1)" missing ] + |> String.concat "\n") [] let ``match x with empty string renders as op_Equality (#19873)`` () = @@ -108,48 +70,27 @@ module QuotationRendering = assertNoEmptyStringLowering q renderToBaseline "NullOrEmpty" q - // Reference baselines: pattern-match-compilation lowerings that have never leaked into - // quotations. Pins the BuildSwitch arms so a future optimization that starts rewriting - // them in-place (the bug class behind #19873) shows up as a diff. - // - NonEmptyString, ConsecutiveInts, Chars : op_Equality + compactify/jump-table path - // - Int64 : the `mkILAsmCeq` arm + `[AI_ceq]` -> op_Equality recovery - // - Decimal : op_Equality with a MakeDecimal literal - [] - let NonEmptyString () = - renderToBaseline "NonEmptyString" - <@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @> + let NonEmptyString () = renderToBaseline "NonEmptyString" <@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @> [] - let ConsecutiveInts () = - renderToBaseline "ConsecutiveInts" - <@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @> + let ConsecutiveInts () = renderToBaseline "ConsecutiveInts" <@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @> [] - let Chars () = - renderToBaseline "Chars" - <@ fun (x: char) -> match x with 'a' -> 1 | 'b' -> 2 | _ -> 0 @> + let Chars () = renderToBaseline "Chars" <@ fun (x: char) -> match x with 'a' -> 1 | 'b' -> 2 | _ -> 0 @> + // Int64 takes the mkILAsmCeq arm + [AI_ceq] -> op_Equality recovery; the other primitives go through op_Equality directly. [] - let Int64 () = - renderToBaseline "Int64" - <@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @> + let Int64 () = renderToBaseline "Int64" <@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @> [] - let Decimal () = - renderToBaseline "Decimal" - <@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @> + let Decimal () = renderToBaseline "Decimal" <@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @> - // Side effect of the move: `if x = ""` was already clean in quotations (no BuildSwitch path) - // and stays clean after the rewrite moved to the Optimizer (which skips Expr.Quote bodies). [] let ``if x = empty string renders cleanly (second repro from #19873)`` () = assertNoEmptyStringLowering qIfEqualEmpty renderToBaseline "IfEqualEmpty" qIfEqualEmpty - // Convergence: surface forms `match x with "" -> _` and `if x = "" then _` desugar to the - // SAME quoted Expr after binder standardization. Asserts on the SHARED literals so the - // convergence proof cannot drift relative to the baselined exprs above. [] let ``match-empty and if-equal-empty quotations converge to the same Expr`` () = Assert.Equal(sprintf "%A" qMatchEmpty, sprintf "%A" qIfEqualEmpty) From e0fc2b056de153267676c1490ffe891115cc9193 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 9 Jun 2026 11:47:25 +0200 Subject: [PATCH 08/11] Remove orphan-baseline guard Fact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It tested filesystem bookkeeping (does *.bsl on disk equal a hand-maintained expectedBaselines set), not the compiler. The drift it catches — a deleted test leaving its .bsl behind — is already visible in `git diff` and adds zero signal anyone would actually use to find a regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../QuotationRenderingTests.fs | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs index c029517e75f..572f9117607 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs @@ -38,27 +38,6 @@ module QuotationRendering = let private qMatchEmpty = <@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @> let private qIfEqualEmpty = <@ fun (x: string) -> if x = "" then 1 else 0 @> - let private expectedBaselines = - Set.ofList [ - "EmptyString"; "NullOrEmpty"; "NonEmptyString"; "ConsecutiveInts" - "Chars"; "Int64"; "Decimal"; "IfEqualEmpty" - ] - - [] - let ``Baseline directory: no orphans, no missing`` () = - if System.Environment.GetEnvironmentVariable("TEST_UPDATE_BSL") <> null then () else - let onDisk = - Directory.GetFiles(baselineDir, "*.bsl") - |> Array.map Path.GetFileNameWithoutExtension - |> Set.ofArray - let orphans = Set.difference onDisk expectedBaselines - let missing = Set.difference expectedBaselines onDisk - if not orphans.IsEmpty || not missing.IsEmpty then - Assert.Fail( - [ if not orphans.IsEmpty then yield sprintf "Orphan .bsl: %A (delete or add a matching test)" orphans - if not missing.IsEmpty then yield sprintf "Missing .bsl: %A (add the test and run with TEST_UPDATE_BSL=1)" missing ] - |> String.concat "\n") - [] let ``match x with empty string renders as op_Equality (#19873)`` () = assertNoEmptyStringLowering qMatchEmpty From b7d3f155bf1309a5da0ec958433efe6944e1ec33 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 9 Jun 2026 13:04:44 +0200 Subject: [PATCH 09/11] Cut surviving garbage after multi-agent adversarial scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three garbage hunters (production / tests / prose) found redundancies that slipped through the earlier iterations: Production: - Merge two String.Equals(_, "")/("", _) optimizer arms into one or-pattern. - Drop the redundant CallingConv.IsStatic guard in IsILMethodRefSystemStringEquals (ArgCount=2 + both args String + return Bool already pin the static overload). - Inline single-use let bindings in MakeOptimizedStringEqualsEmptyCall. - Shrink 4-line docstring to 2 lines; shrink match-arm comment to one line. - Shrink il.fsi block comment and PatternMatchCompilation.fs trade-off comment. Tests: - Delete QuotationUnsupportedConstructsTests.fs (FS0452 array-pattern coverage already exists in Regressions/E_DecomposeArray01.fs — pure duplication). - Delete IfEqualEmpty.bsl + its test + the convergence Fact + the shared `let` bindings + assertNoEmptyStringLowering walker; all duplicated EmptyString.bsl coverage either as identical AST or as belt-and-suspenders restating the baseline diff. - Strip the `// ` provenance header from the printer and from all 7 remaining baselines — the test method name already identifies each baseline. - Trim trade-off comment tails in TypeTestsInPatternMatching.fs (drop the JIT-folds-the-duplicate / tiered-compilation trivia). Release notes: drop the implementation-detail bullet about `if s = ""` getting the same null-safe IL — implied by moving the rewrite to the optimizer. Net: -115/+30 across 16 files. 14 tests still pass; build clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../.FSharp.Compiler.Service/11.0.100.md | 2 +- src/Compiler/AbstractIL/il.fsi | 5 +- .../Checking/PatternMatchCompilation.fs | 4 +- src/Compiler/Optimize/Optimizer.fs | 23 +++---- .../QuotationRendering/Chars.bsl | 1 - .../QuotationRendering/ConsecutiveInts.bsl | 1 - .../QuotationRendering/Decimal.bsl | 1 - .../QuotationRendering/EmptyString.bsl | 1 - .../QuotationRendering/IfEqualEmpty.bsl | 4 -- .../QuotationRendering/Int64.bsl | 1 - .../QuotationRendering/NonEmptyString.bsl | 1 - .../QuotationRendering/NullOrEmpty.bsl | 1 - .../QuotationRenderingTests.fs | 61 +++++-------------- .../QuotationUnsupportedConstructsTests.fs | 28 --------- .../EmittedIL/TypeTestsInPatternMatching.fs | 10 ++- .../FSharp.Compiler.ComponentTests.fsproj | 1 - 16 files changed, 30 insertions(+), 115 deletions(-) delete mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl delete mode 100644 tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationUnsupportedConstructsTests.fs 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 49ec2d86c18..6860d2728d6 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -75,7 +75,7 @@ ([PR #19724](https://github.com/dotnet/fsharp/pull/19724)) * Emit debug points at a stack-empty position ([PR #19877](https://github.com/dotnet/fsharp/pull/19877)) * Fix spurious XmlDoc warnings (unknown parameter / no documentation for parameter) under `--warnon:3390` when a get/set property documents the full parameter set across both accessors. ([Issue #13684](https://github.com/dotnet/fsharp/issues/13684), [PR #19884](https://github.com/dotnet/fsharp/pull/19884)) -* Quotations of `match s with "" -> _` and `match s with null | "" -> _` no longer leak the `if s <> null then s.Length = 0 else false` lowering; the empty-string optimization moved from pattern-match compilation to the optimizer, so quoted expressions retain their original `op_Equality(s, "")` shape. As a side effect, `if s = "" then _` now produces the same null-safe length-check IL as `match s with "" -> _`. ([Issue #19873](https://github.com/dotnet/fsharp/issues/19873)) +* Quotations of `match s with "" -> _` no longer leak the `s <> null && s.Length = 0` lowering; the empty-string optimization moved from pattern-match compilation to the optimizer so quoted expressions keep `op_Equality(s, "")`. ([Issue #19873](https://github.com/dotnet/fsharp/issues/19873)) ### Added diff --git a/src/Compiler/AbstractIL/il.fsi b/src/Compiler/AbstractIL/il.fsi index 3d94b80d472..b543f0e19bd 100644 --- a/src/Compiler/AbstractIL/il.fsi +++ b/src/Compiler/AbstractIL/il.fsi @@ -2471,10 +2471,7 @@ val internal ecmaPublicKey: PublicKey /// Strips ILType.Modified from the ILType. val internal stripILModifiedFromTy: ILType -> ILType -// Names of important built-in types, exported so callers can use them in -// `mref.DeclaringTypeRef.Name = tname_X` comparisons instead of repeating magic strings. -// Only the names referenced outside `il.fs` are exposed; add more here as needed. -// (TcGlobals.fsi separately exports `tname_Enum` / `tname_Array`.) +// Built-in type names exposed for `mref.DeclaringTypeRef.Name = tname_X` matchers outside il.fs. [] val internal tname_String: string = "System.String" diff --git a/src/Compiler/Checking/PatternMatchCompilation.fs b/src/Compiler/Checking/PatternMatchCompilation.fs index ee57cf40610..45e0ba9d6a3 100644 --- a/src/Compiler/Checking/PatternMatchCompilation.fs +++ b/src/Compiler/Checking/PatternMatchCompilation.fs @@ -811,9 +811,7 @@ let rec BuildSwitch inpExprOpt g isNullFiltered expr edges dflt m = let finalTest = if isNullFiltered then test else mkLazyAnd g m (mkNonNullTest g m vExpr) test mkLetBind m bind finalTest | DecisionTreeTest.Const (Const.String _ as c) -> - // Empty-string is detected by the Optimizer (`MakeOptimizedStringEqualsEmptyCall`) - // and rewritten to a null-safe length check at that point. Pattern-match - // compilation keeps the F# `=` form so quotations see the original shape. + // Empty-string is rewritten to a null-safe length check by the Optimizer (#19873). mkCallEqualsOperator g m g.string_ty testexpr (Expr.Const (c, m, g.string_ty)) | DecisionTreeTest.Const (Const.Decimal _ as c) -> mkCallEqualsOperator g m g.decimal_ty testexpr (Expr.Const (c, m, g.decimal_ty)) diff --git a/src/Compiler/Optimize/Optimizer.fs b/src/Compiler/Optimize/Optimizer.fs index 90516f7bfbd..036addd4653 100644 --- a/src/Compiler/Optimize/Optimizer.fs +++ b/src/Compiler/Optimize/Optimizer.fs @@ -2293,7 +2293,6 @@ let TryDetectQueryQuoteAndRun cenv (expr: Expr) = let IsILMethodRefSystemStringEquals (mref: ILMethodRef) = mref.Name = "Equals" && - mref.CallingConv.IsStatic && mref.DeclaringTypeRef.Name = tname_String && (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = tname_Bool) && (mref.ArgCount = 2 && @@ -2550,17 +2549,13 @@ and MakeOptimizedSystemStringConcatCall cenv env m args = | _ -> OptimizeExpr cenv env expr -/// Rewrite `System.String.Equals(x, "")` (or `Equals("", x)`) into the null-safe length check -/// `if x = null then false else x.Length = 0`. Equivalent semantics — `String.Equals` returns false -/// on null. This optimization originally lived in `BuildSwitch` for `match s with "" -> _` but was -/// moved here so that quotations capture the original `op_Equality(_, "")` shape (issue #19873). +/// Rewrite `String.Equals(x, "")` to `x <> null && x.Length = 0` (issue #19873 — +/// done here so quotation bodies, which the optimizer skips, keep `op_Equality(_, "")`). and MakeOptimizedStringEqualsEmptyCall cenv env m nonEmptyArg = let g = cenv.g let _, vExpr, bind = mkCompGenLocalAndInvisibleBind g "testExpr" m nonEmptyArg let lengthIsZero = mkILAsmCeq g m (mkGetStringLength g m vExpr) (mkInt g m 0) - let nullSafe = mkLazyAnd g m (mkNonNullTest g m vExpr) lengthIsZero - let optimized = mkLetBind m bind nullSafe - OptimizeExpr cenv env optimized + OptimizeExpr cenv env (mkLetBind m bind (mkLazyAnd g m (mkNonNullTest g m vExpr) lengthIsZero)) /// Optimize/analyze an application of an intrinsic operator to arguments and OptimizeExprOp cenv env (op, tyargs, args, m) = @@ -2633,15 +2628,11 @@ and OptimizeExprOp cenv env (op, tyargs, args, m) = when IsILMethodRefSystemStringConcat ilMethRef -> MakeOptimizedSystemStringConcatCall cenv env m args - // See MakeOptimizedStringEqualsEmptyCall. Applied post-inlining (on the `String.Equals` shape - // that `(=)` lowers to) so `Expr.Quote` bodies, which the optimizer skips, keep the original - // `op_Equality(s, "")` form. - | TOp.ILCall(_, _, _, _, _, _, _, ilMethRef, _, _, _), _, [arg1; Expr.Const(Const.String "", _, _)] + // See MakeOptimizedStringEqualsEmptyCall (issue #19873). `(=)` lowers to `String.Equals` post-inlining. + | TOp.ILCall(_, _, _, _, _, _, _, ilMethRef, _, _, _), _, + ([nonEmpty; Expr.Const(Const.String "", _, _)] | [Expr.Const(Const.String "", _, _); nonEmpty]) when IsILMethodRefSystemStringEquals ilMethRef -> - MakeOptimizedStringEqualsEmptyCall cenv env m arg1 - | TOp.ILCall(_, _, _, _, _, _, _, ilMethRef, _, _, _), _, [Expr.Const(Const.String "", _, _); arg2] - when IsILMethodRefSystemStringEquals ilMethRef -> - MakeOptimizedStringEqualsEmptyCall cenv env m arg2 + MakeOptimizedStringEqualsEmptyCall cenv env m nonEmpty | _ -> // Reductions diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl index 021d0c928e7..e75b7d6dbff 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl @@ -1,4 +1,3 @@ -// Chars Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value ('a')]), Value (1), IfThenElse (Call (None, op_Equality, [x, Value ('b')]), diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl index 3eb5fb16b25..49fd764e179 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl @@ -1,4 +1,3 @@ -// ConsecutiveInts Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value (1)]), Value ("a"), IfThenElse (Call (None, op_Equality, [x, Value (2)]), diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl index 5a3c28bd680..2adbe271983 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl @@ -1,4 +1,3 @@ -// Decimal Lambda (x, IfThenElse (Call (None, op_Equality, [x, diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl index da2e876a86b..848475282af 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl @@ -1,4 +1,3 @@ -// EmptyString Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value ("")]), Value (1), Value (0))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl deleted file mode 100644 index d354abc4620..00000000000 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/IfEqualEmpty.bsl +++ /dev/null @@ -1,4 +0,0 @@ -// IfEqualEmpty -Lambda (x, - IfThenElse (Call (None, op_Equality, [x, Value ("")]), Value (1), - Value (0))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl index 361159dd782..3ff3d872afa 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl @@ -1,4 +1,3 @@ -// Int64 Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value (1L)]), Value ("a"), Value ("b"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl index e266fe0e825..d948553e5f5 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl @@ -1,4 +1,3 @@ -// NonEmptyString Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value ("a")]), Value (1), IfThenElse (Call (None, op_Equality, [x, Value ("b")]), diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl index 8bbf2168727..408cccd7c53 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl @@ -1,4 +1,3 @@ -// NullOrEmpty Lambda (x, IfThenElse (Call (None, op_Equality, [x, Value ()]), Value (1), IfThenElse (Call (None, op_Equality, [x, Value ("")]), diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs index 572f9117607..6565da810c2 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs @@ -1,15 +1,11 @@ // Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. // Quotation rendering snapshots — regression for https://github.com/dotnet/fsharp/issues/19873. -// Literals are desugared by the bootstrap fsc that built the test project; rerun with -// `./build.sh --bootstrap -test` after touching `src/Compiler/Checking/PatternMatchCompilation.fs`. namespace Conformance.Expressions.ExpressionQuotations open System.IO open Microsoft.FSharp.Quotations -open Microsoft.FSharp.Quotations.Patterns -open Microsoft.FSharp.Quotations.ExprShape open Xunit open FSharp.Test.Compiler @@ -18,58 +14,33 @@ module QuotationRendering = let private baselineDir = __SOURCE_DIRECTORY__ let private renderToBaseline (name: string) (q: Expr) = - let body = sprintf "%A" q |> normalizeNewlines - checkBaseline (sprintf "// %s\n%s\n" name body) (Path.Combine(baselineDir, name + ".bsl")) - - let rec private containsEmptyStringLowering (e: Expr) = - let isString (t: System.Type) = not (isNull t) && t.FullName = "System.String" - match e with - | PropertyGet (_, pi, _) when isString pi.DeclaringType && pi.Name = "Length" -> true - | Call (_, mi, _) when isString mi.DeclaringType && mi.Name = "Equals" -> true - | ShapeCombination (_, args) -> args |> List.exists containsEmptyStringLowering - | ShapeLambda (_, body) -> containsEmptyStringLowering body - | ShapeVar _ -> false - - let private assertNoEmptyStringLowering (q: Expr) = - Assert.False(containsEmptyStringLowering q, - sprintf "Quotation leaked the empty-string lowering (String.Length or String.Equals):\n%A" q) - - // Shared so the convergence test below cannot drift from the baselined exprs. - let private qMatchEmpty = <@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @> - let private qIfEqualEmpty = <@ fun (x: string) -> if x = "" then 1 else 0 @> - - [] - let ``match x with empty string renders as op_Equality (#19873)`` () = - assertNoEmptyStringLowering qMatchEmpty - renderToBaseline "EmptyString" qMatchEmpty - - [] - let ``match x with null or empty string renders as op_Equality (#19873)`` () = - let q = <@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @> - assertNoEmptyStringLowering q - renderToBaseline "NullOrEmpty" q + checkBaseline (sprintf "%A\n" q) (Path.Combine(baselineDir, name + ".bsl")) [] - let NonEmptyString () = renderToBaseline "NonEmptyString" <@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @> + let EmptyString () = + renderToBaseline "EmptyString" <@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @> [] - let ConsecutiveInts () = renderToBaseline "ConsecutiveInts" <@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @> + let NullOrEmpty () = + renderToBaseline "NullOrEmpty" <@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @> [] - let Chars () = renderToBaseline "Chars" <@ fun (x: char) -> match x with 'a' -> 1 | 'b' -> 2 | _ -> 0 @> + let NonEmptyString () = + renderToBaseline "NonEmptyString" <@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @> - // Int64 takes the mkILAsmCeq arm + [AI_ceq] -> op_Equality recovery; the other primitives go through op_Equality directly. [] - let Int64 () = renderToBaseline "Int64" <@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @> + let ConsecutiveInts () = + renderToBaseline "ConsecutiveInts" <@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @> [] - let Decimal () = renderToBaseline "Decimal" <@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @> + let Chars () = + renderToBaseline "Chars" <@ fun (x: char) -> match x with 'a' -> 1 | 'b' -> 2 | _ -> 0 @> + // Int64 takes the mkILAsmCeq arm + [AI_ceq] -> op_Equality recovery (distinct from the op_Equality-direct primitives). [] - let ``if x = empty string renders cleanly (second repro from #19873)`` () = - assertNoEmptyStringLowering qIfEqualEmpty - renderToBaseline "IfEqualEmpty" qIfEqualEmpty + let Int64 () = + renderToBaseline "Int64" <@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @> [] - let ``match-empty and if-equal-empty quotations converge to the same Expr`` () = - Assert.Equal(sprintf "%A" qMatchEmpty, sprintf "%A" qIfEqualEmpty) + let Decimal () = + renderToBaseline "Decimal" <@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @> diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationUnsupportedConstructsTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationUnsupportedConstructsTests.fs deleted file mode 100644 index 031400d9eb9..00000000000 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationUnsupportedConstructsTests.fs +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. - -// Diagnostic test (FS0452): array-length patterns lower to `I_ldlen` IL via `mkLdlen` BEFORE the -// quotation translator runs, which rejects them. Lives in its own file because it uses the -// typecheck/diagnostic harness (FSharp |> typecheck |> shouldFail), not the in-process baseline -// harness in the sibling QuotationRenderingTests.fs. -// -// Out of scope for #19873. Flip to shouldSucceed and add an `ArrayLength.bsl` baseline when that -// lowering moves out of BuildSwitch. - -namespace Conformance.Expressions.ExpressionQuotations - -open Xunit -open FSharp.Test.Compiler - -module QuotationUnsupportedConstructs = - - [] - let ``match x with array length still errors with FS0452`` () = - FSharp """module Test -let q = <@ fun (x: int[]) -> match x with [| _ |] -> 1 | _ -> 0 @> -""" - |> asLibrary - |> typecheck - |> shouldFail - |> withErrorCode 452 - |> withDiagnosticMessageMatches "Quotations cannot contain inline assembly code or pattern matching on arrays" - |> ignore diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs index d372ba14426..6e859a4baf1 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs +++ b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs @@ -383,9 +383,8 @@ let TestEmptyStringEq(x: string) = """ ] - // Baseline for #19873: under `--optimize-` the null+Length form is not emitted, because F#'s - // `(=)` is only inlined when `LocalOptimizationsEnabled = true`; the call falls through to - // `String.Equals(s, "")`. JIT tiered compilation still reaches the fast path. + // #19873: under `--optimize-` `(=)` isn't inlined (only when LocalOptimizationsEnabled), + // so the call falls through to `String.Equals(s, "")` instead of the null+Length form. [] let ``Test codegen for empty string pattern under --optimize-``() = FSharp """ @@ -422,9 +421,8 @@ let TestEmptyStringPattern(x: string) = """ ] - // Baseline for #19873: the optimizer can't see BuildSwitch's `isNullFiltered` flag, so the - // empty-string branch under `null | "" -> _` re-emits its own null check. Result: two - // back-to-back `brfalse.s` on the same argument (the JIT folds the duplicate). + // #19873: the optimizer doesn't see BuildSwitch's `isNullFiltered` flag, so the empty-string + // branch under `null | "" -> _` re-emits its own null check (two back-to-back `brfalse.s`). [] let ``Test codegen for null-or-empty-string pattern``() = FSharp """ diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index f6f9fa84ecc..4307e3f8230 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -114,7 +114,6 @@ - From bf7e44265c92859d093e4f1dadea1a1e5a03f6d0 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Tue, 9 Jun 2026 16:45:52 +0200 Subject: [PATCH 10/11] Refactor String IL-method matchers via active pattern + InlineIfLambda Extracts a `(|StringTy|_|)` partial active pattern over `ILType` and a small `isILMethodRefOnSystemString` helper that bakes in `tname_String` as the declaring type and takes the per-arg-shape check as an `[]`. Each `IsILMethodRefSystem*` now reads as the literal shape it matches: `[StringTy; StringTy]` for Equals(string, string); the three overload arities of String.Concat as alternative list patterns; `[ILType.Array (shape, StringTy)]` for Concat(string[]). No `ArgCount` book-keeping or `List.forall` walks left. Behaviour unchanged: build clean, 14 quotation + IL tests still green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Compiler/Optimize/Optimizer.fs | 49 +++++++++++++++--------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/src/Compiler/Optimize/Optimizer.fs b/src/Compiler/Optimize/Optimizer.fs index 036addd4653..481092a3ab7 100644 --- a/src/Compiler/Optimize/Optimizer.fs +++ b/src/Compiler/Optimize/Optimizer.fs @@ -2291,36 +2291,35 @@ let TryDetectQueryQuoteAndRun cenv (expr: Expr) = //printfn "Not eliminating because no Run found" None -let IsILMethodRefSystemStringEquals (mref: ILMethodRef) = - mref.Name = "Equals" && +let inline (|StringTy|_|) (ilTy: ILType) : bool = + ilTy.IsNominal && ilTy.TypeRef.Name = tname_String + +let inline private isILMethodRefOnSystemString + (methodName: string) + (returnTypeName: string) + ([] argsCheck: ILType list -> bool) + (mref: ILMethodRef) = + mref.Name = methodName && mref.DeclaringTypeRef.Name = tname_String && - (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = tname_Bool) && - (mref.ArgCount = 2 && - mref.ArgTypes - |> List.forall (fun ilTy -> - ilTy.IsNominal && ilTy.TypeRef.Name = tname_String)) + mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = returnTypeName && + argsCheck mref.ArgTypes + +let IsILMethodRefSystemStringEquals (mref: ILMethodRef) = + mref |> isILMethodRefOnSystemString "Equals" tname_Bool (function + | [StringTy; StringTy] -> true + | _ -> false) let IsILMethodRefSystemStringConcat (mref: ILMethodRef) = - mref.Name = "Concat" && - mref.DeclaringTypeRef.Name = tname_String && - (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = tname_String) && - (mref.ArgCount >= 2 && mref.ArgCount <= 4 && - mref.ArgTypes - |> List.forall (fun ilTy -> - ilTy.IsNominal && ilTy.TypeRef.Name = tname_String)) + mref |> isILMethodRefOnSystemString "Concat" tname_String (function + | [StringTy; StringTy] + | [StringTy; StringTy; StringTy] + | [StringTy; StringTy; StringTy; StringTy] -> true + | _ -> false) let IsILMethodRefSystemStringConcatArray (mref: ILMethodRef) = - mref.Name = "Concat" && - mref.DeclaringTypeRef.Name = tname_String && - (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = tname_String) && - (mref.ArgCount = 1 && - mref.ArgTypes - |> List.forall (fun ilTy -> - match ilTy with - | ILType.Array (shape, ilTy) when shape = ILArrayShape.SingleDimensional && - ilTy.IsNominal && - ilTy.TypeRef.Name = tname_String -> true - | _ -> false)) + mref |> isILMethodRefOnSystemString "Concat" tname_String (function + | [ILType.Array (shape, StringTy)] when shape = ILArrayShape.SingleDimensional -> true + | _ -> false) let rec IsDebugPipeRightExpr cenv expr = let g = cenv.g From 076c4ebdd9532763aae3788fd74f204b95bfe827 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Wed, 10 Jun 2026 19:50:48 +0200 Subject: [PATCH 11/11] Address PR review + fix CI: drop misplaced comment, run quotations via FSI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - PatternMatchCompilation.fs: remove the misplaced `// Empty-string is rewritten…` comment on the (now-pristine) string arm (review feedback from @abonie). - QuotationRenderingTests.fs: replace literal `<@…@>` quotations with source-string evaluation through a shared FSI session. The bootstrap fsc that builds the test project still has the pre-fix desugar and rejects literal `match s with ""` quotations with FS0452 — the literals only become valid AFTER this PR is in the bootstrap. FSI uses the just-built FCS (with the fix), so source-as-string evaluation is bootstrap-immune. The .bsl baselines are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Checking/PatternMatchCompilation.fs | 1 - .../QuotationRenderingTests.fs | 34 +++++++++++++------ 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/Compiler/Checking/PatternMatchCompilation.fs b/src/Compiler/Checking/PatternMatchCompilation.fs index 45e0ba9d6a3..e5af41cb481 100644 --- a/src/Compiler/Checking/PatternMatchCompilation.fs +++ b/src/Compiler/Checking/PatternMatchCompilation.fs @@ -811,7 +811,6 @@ let rec BuildSwitch inpExprOpt g isNullFiltered expr edges dflt m = let finalTest = if isNullFiltered then test else mkLazyAnd g m (mkNonNullTest g m vExpr) test mkLetBind m bind finalTest | DecisionTreeTest.Const (Const.String _ as c) -> - // Empty-string is rewritten to a null-safe length check by the Optimizer (#19873). mkCallEqualsOperator g m g.string_ty testexpr (Expr.Const (c, m, g.string_ty)) | DecisionTreeTest.Const (Const.Decimal _ as c) -> mkCallEqualsOperator g m g.decimal_ty testexpr (Expr.Const (c, m, g.decimal_ty)) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs index 6565da810c2..de4ce676aff 100644 --- a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs @@ -1,46 +1,60 @@ // Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. // Quotation rendering snapshots — regression for https://github.com/dotnet/fsharp/issues/19873. +// Quotation literals are evaluated at test runtime via a shared FSI session that uses the +// just-built FSharp.Compiler.Service, so the desugar under test is the one this PR ships +// (the bootstrap fsc that builds this test project still has the pre-fix desugar and +// rejects literal `match s with ""` quotations with FS0452). namespace Conformance.Expressions.ExpressionQuotations open System.IO -open Microsoft.FSharp.Quotations open Xunit open FSharp.Test.Compiler +open FSharp.Test.ScriptHelpers module QuotationRendering = let private baselineDir = __SOURCE_DIRECTORY__ - let private renderToBaseline (name: string) (q: Expr) = - checkBaseline (sprintf "%A\n" q) (Path.Combine(baselineDir, name + ".bsl")) + let private fsiSession = getSessionForEval [||] LangVersion.Preview + + let private quoteShouldRender (name: string) (quoteExpr: string) = + let result = + Fsx (sprintf "printfn \"%%A\" %s" quoteExpr) + |> evalInSharedSession fsiSession + |> shouldSucceed + match result.RunOutput with + | Some (EvalOutput e) -> + checkBaseline (e.StdOut |> normalizeNewlines) (Path.Combine(baselineDir, name + ".bsl")) + | _ -> + failwith "Expected eval output from shared FSI session." [] let EmptyString () = - renderToBaseline "EmptyString" <@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @> + quoteShouldRender "EmptyString" """<@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @>""" [] let NullOrEmpty () = - renderToBaseline "NullOrEmpty" <@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @> + quoteShouldRender "NullOrEmpty" """<@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @>""" [] let NonEmptyString () = - renderToBaseline "NonEmptyString" <@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @> + quoteShouldRender "NonEmptyString" """<@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @>""" [] let ConsecutiveInts () = - renderToBaseline "ConsecutiveInts" <@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @> + quoteShouldRender "ConsecutiveInts" """<@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @>""" [] let Chars () = - renderToBaseline "Chars" <@ fun (x: char) -> match x with 'a' -> 1 | 'b' -> 2 | _ -> 0 @> + quoteShouldRender "Chars" """<@ fun (x: char) -> match x with 'a' -> 1 | 'b' -> 2 | _ -> 0 @>""" // Int64 takes the mkILAsmCeq arm + [AI_ceq] -> op_Equality recovery (distinct from the op_Equality-direct primitives). [] let Int64 () = - renderToBaseline "Int64" <@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @> + quoteShouldRender "Int64" """<@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @>""" [] let Decimal () = - renderToBaseline "Decimal" <@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @> + quoteShouldRender "Decimal" """<@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @>"""