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 965957dbc0c..6b7c8a4099c 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 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 3539e235518..b543f0e19bd 100644 --- a/src/Compiler/AbstractIL/il.fsi +++ b/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. + +[] +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/Checking/PatternMatchCompilation.fs b/src/Compiler/Checking/PatternMatchCompilation.fs index 4128b8cf5f4..e5af41cb481 100644 --- a/src/Compiler/Checking/PatternMatchCompilation.fs +++ b/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) -> diff --git a/src/Compiler/Optimize/Optimizer.fs b/src/Compiler/Optimize/Optimizer.fs index a6fa90d1e3a..ffc7b92671b 100644 --- a/src/Compiler/Optimize/Optimizer.fs +++ b/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) + ([] 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) 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 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..e75b7d6dbff --- /dev/null +++ b/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)))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/ConsecutiveInts.bsl new file mode 100644 index 00000000000..49fd764e179 --- /dev/null +++ b/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"))))) 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..2adbe271983 --- /dev/null +++ b/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"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/EmptyString.bsl new file mode 100644 index 00000000000..848475282af --- /dev/null +++ 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))) 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..3ff3d872afa --- /dev/null +++ b/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"))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NonEmptyString.bsl new file mode 100644 index 00000000000..d948553e5f5 --- /dev/null +++ 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)))) diff --git a/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.bsl new file mode 100644 index 00000000000..408cccd7c53 --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ExpressionQuotations/QuotationRendering/NullOrEmpty.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..de4ce676aff --- /dev/null +++ b/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." + + [] + let EmptyString () = + quoteShouldRender "EmptyString" """<@ fun (x: string) -> match x with "" -> 1 | _ -> 0 @>""" + + [] + let NullOrEmpty () = + quoteShouldRender "NullOrEmpty" """<@ fun (x: string) -> match x with null | "" -> 1 | _ -> 0 @>""" + + [] + let NonEmptyString () = + quoteShouldRender "NonEmptyString" """<@ fun (x: string) -> match x with "a" -> 1 | "b" -> 2 | _ -> 0 @>""" + + [] + let ConsecutiveInts () = + quoteShouldRender "ConsecutiveInts" """<@ fun (x: int) -> match x with 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "z" @>""" + + [] + 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). + [] + let Int64 () = + quoteShouldRender "Int64" """<@ fun (x: int64) -> match x with 1L -> "a" | _ -> "b" @>""" + + [] + let Decimal () = + quoteShouldRender "Decimal" """<@ fun (x: decimal) -> match x with 1m -> "a" | _ -> "b" @>""" diff --git a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs b/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs index 8dbdf763f4d..6e859a4baf1 100644 --- a/tests/FSharp.Compiler.ComponentTests/EmittedIL/TypeTestsInPatternMatching.fs +++ 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. + [] + 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. + [] + 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`). + [] + 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 diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index 954c02b8aca..0ecc691ef57 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -113,6 +113,7 @@ +