Fix quotations leaking empty-string match lowering into AST (#19873)

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

src/Compiler/AbstractIL/il.fsi

@@ -2471,6 +2471,17 @@ val internal ecmaPublicKey: PublicKey
 /// Strips ILType.Modified from the ILType.
 val internal stripILModifiedFromTy: ILType -> ILType
 
+// Built-in type names exposed for `mref.DeclaringTypeRef.Name = tname_X` matchers outside il.fs.
+
+[<Literal>]
+val internal tname_String: string = "System.String"
+
+[<Literal>]
+val internal tname_Type: string = "System.Type"
+
+[<Literal>]
+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

src/Compiler/Checking/PatternMatchCompilation.fs

@@ -810,13 +810,6 @@ 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)  ->
                         mkCallEqualsOperator g m g.string_ty testexpr (Expr.Const (c, m, g.string_ty))
                     | DecisionTreeTest.Const (Const.Decimal _ as c)  ->

src/Compiler/Optimize/Optimizer.fs

@@ -2291,27 +2291,35 @@ let TryDetectQueryQuoteAndRun cenv (expr: Expr) =
         //printfn "Not eliminating because no Run found"
         None
 
+let inline (|StringTy|_|) (ilTy: ILType) : bool =
+    ilTy.IsNominal && ilTy.TypeRef.Name = tname_String
+
+let inline private isILMethodRefOnSystemString
+        (methodName: string)
+        (returnTypeName: string)
+        ([<InlineIfLambda>] argsCheck: ILType list -> bool)
+        (mref: ILMethodRef) =
+    mref.Name = methodName &&
+    mref.DeclaringTypeRef.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 = "System.String" &&
-    (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = "System.String") &&
-    (mref.ArgCount >= 2 && mref.ArgCount <= 4 &&
-        mref.ArgTypes 
-        |> List.forall (fun ilTy ->
-            ilTy.IsNominal && ilTy.TypeRef.Name = "System.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 = "System.String" &&
-    (mref.ReturnType.IsNominal && mref.ReturnType.TypeRef.Name = "System.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
-            | _ -> 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
@@ -2540,6 +2548,14 @@ and MakeOptimizedSystemStringConcatCall cenv env m args =
     | _ ->
         OptimizeExpr cenv env expr
 
+/// 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)
+    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) =
 
@@ -2611,6 +2627,12 @@ and OptimizeExprOp cenv env (op, tyargs, args, m) =
       when IsILMethodRefSystemStringConcat ilMethRef ->
         MakeOptimizedSystemStringConcatCall cenv env m args
 
+    // 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 nonEmpty
+
     | _ -> 
         // Reductions
         OptimizeExprOpReductions cenv env (op, tyargs, args, m)

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
 

tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Chars.bsl

@@ -0,0 +1,4 @@
+Lambda (x,
+        IfThenElse (Call (None, op_Equality, [x, Value ('a')]), Value (1),
+                    IfThenElse (Call (None, op_Equality, [x, Value ('b')]),
+                                Value (2), Value (0))))

tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl

@@ -0,0 +1,7 @@
+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,
+                                                  [x, Value (3)]), Value ("c"),
+                                            Value ("z")))))

tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Decimal.bsl

@@ -0,0 +1,6 @@
+Lambda (x,
+        IfThenElse (Call (None, op_Equality,
+                          [x,
+                           Call (None, MakeDecimal,
+                                 [Value (1), Value (0), Value (0), Value (false),
+                                  Value (0uy)])]), Value ("a"), Value ("b")))

tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl

@@ -0,0 +1,3 @@
+Lambda (x,
+        IfThenElse (Call (None, op_Equality, [x, Value ("")]), Value (1),
+                    Value (0)))

tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/Int64.bsl

@@ -0,0 +1,3 @@
+Lambda (x,
+        IfThenElse (Call (None, op_Equality, [x, Value (1L)]), Value ("a"),
+                    Value ("b")))

tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl

@@ -0,0 +1,4 @@
+Lambda (x,
+        IfThenElse (Call (None, op_Equality, [x, Value ("a")]), Value (1),
+                    IfThenElse (Call (None, op_Equality, [x, Value ("b")]),
+                                Value (2), Value (0))))

tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl

@@ -0,0 +1,4 @@
+Lambda (x,
+        IfThenElse (Call (None, op_Equality, [x, Value (<null>)]), Value (1),
+                    IfThenElse (Call (None, op_Equality, [x, Value ("")]),
+                                Value (1), Value (0))))

tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/QuotationRenderingTests.fs

@@ -0,0 +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 Xunit
+open FSharp.Test.Compiler
+open FSharp.Test.ScriptHelpers
+
+module QuotationRendering =
+
+    let private baselineDir = __SOURCE_DIRECTORY__
+
+    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."
+
+    [<Fact>]
+    let EmptyString () =
+        quoteShouldRender "EmptyString" """<@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @>"""
+
+    [<Fact>]
+    let NullOrEmpty () =
+        quoteShouldRender "NullOrEmpty" """<@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @>"""
+
+    [<Fact>]
+    let NonEmptyString () =
+        quoteShouldRender "NonEmptyString" """<@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @>"""
+
+    [<Fact>]
+    let ConsecutiveInts () =
+        quoteShouldRender "ConsecutiveInts" """<@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @>"""
+
+    [<Fact>]
+    let Chars () =
+        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).
+    [<Fact>]
+    let Int64 () =
+        quoteShouldRender "Int64" """<@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @>"""
+
+    [<Fact>]
+    let Decimal () =
+        quoteShouldRender "Decimal" """<@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @>"""

tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs

@@ -346,5 +346,120 @@ let TestEmptyStringPattern(x: string) =
     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.
+    [<Fact>]
+    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
+  } 
+"""
+       ]
+
+    // #19873: under `--optimize-` `(=)` isn't inlined (only when LocalOptimizationsEnabled),
+    // so the call falls through to `String.Equals(s, "")` instead of the null+Length form.
+    [<Fact>]
+    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
+  } 
+"""
+       ]
+
+    // #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`).
+    [<Fact>]
+    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
+  } 
 """
        ]

tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj

@@ -113,6 +113,7 @@
     <Compile Include="Conformance\Expressions\EvaluationOfElaboratedForms\EvaluationOfElaboratedForms.fs" />
     <Compile Include="Conformance\Expressions\ExpressionQuotations\Baselines.fs" />
     <Compile Include="Conformance\Expressions\ExpressionQuotations\Regressions.fs" />
+    <Compile Include="Conformance\Expressions\ExpressionQuotations\QuotationRendering\QuotationRenderingTests.fs" />
     <Compile Include="Conformance\Expressions\SyntacticSugar\SyntacticSugar.fs" />
     <Compile Include="Conformance\Expressions\SyntacticSugarAndAmbiguities\SyntacticSugarAndAmbiguities.fs" />
     <Compile Include="Conformance\InferenceProcedures\ByrefSafetyAnalysis\ByrefSafetyAnalysis.fs" />