From 2f638291a4795214a20dd38aebf825e08131b7d2 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Mon, 16 Mar 2026 11:50:03 +0100 Subject: [PATCH 1/4] Fix BuildMethodCall double address-taking for UoM value types (#19435) The MethInfoWithModifiedReturnType case in BuildMethodCall was recursively calling BuildMethodCall, which re-invoked TakeObjAddrForMethodCall on already-addressed object arguments. This created double-indirection (byref>) for value types, causing ToString() on UoM value types to return garbage when --checknulls+ was enabled. Fix: directly call BuildILMethInfoCall with the already-processed allArgs and valUseFlags, skipping the redundant TakeObjAddrForMethodCall. Added runtime tests verifying ToString on int and float UoM types produces correct output with --checknulls+. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Compiler/Checking/MethodCalls.fs | 16 ++++-- .../Nullness/NullableReferenceTypesTests.fs | 50 +++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/Compiler/Checking/MethodCalls.fs b/src/Compiler/Checking/MethodCalls.fs index 285dfa4fa8c..8de04727d1c 100644 --- a/src/Compiler/Checking/MethodCalls.fs +++ b/src/Compiler/Checking/MethodCalls.fs @@ -1231,9 +1231,19 @@ let rec BuildMethodCall tcVal g amap isMutable m isProp minfo valUseFlags minst let vExpr, vExprTy = tcVal vref valUseFlags (minfo.DeclaringTypeInst @ minst) m BuildFSharpMethodApp g m vref vExpr vExprTy allArgs - | MethInfoWithModifiedReturnType(mi,retTy) -> - let expr, exprTy = BuildMethodCall tcVal g amap isMutable m isProp mi valUseFlags minst objArgs args staticTyOpt - let expr = mkCoerceExpr(expr, retTy, m, exprTy) + | MethInfoWithModifiedReturnType(mi, retTy) -> + // Build the inner call directly, without re-invoking TakeObjAddrForMethodCall. + // objArgs are already address-taken and allArgs/valUseFlags are already processed. + let expr, exprTy = + match mi with + | ILMeth(_, ilMethInfo, _) -> + BuildILMethInfoCall g amap m isProp ilMethInfo valUseFlags minst direct allArgs + | FSMeth(_, _, vref, _) -> + let vExpr, vExprTy = tcVal vref valUseFlags (mi.DeclaringTypeInst @ minst) m + BuildFSharpMethodApp g m vref vExpr vExprTy allArgs + | _ -> failwith "MethInfoWithModifiedReturnType: unexpected inner method kind" + + let expr = mkCoerceExpr (expr, retTy, m, exprTy) expr, retTy // Build a 'call' to a struct default constructor diff --git a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs index 31dffcfb169..10eb8ab8813 100644 --- a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs @@ -734,6 +734,56 @@ let processAlias (x:mykgalias) = |> typeCheckWithStrictNullness |> shouldSucceed +// https://github.com/dotnet/fsharp/issues/19435 +[] +let ``ToString on value type with UoM does not produce garbage at runtime`` () = + FSharp """ +module MyProgram + +open FSharp.Data.UnitSystems.SI.UnitNames + +[] +let main _ = + let x = 42 + let result = x.ToString() + if result <> "42" then + eprintfn "BUG: Expected '42' but got '%s'" result + 1 + else + printf "42" + 0 + """ + |> withCheckNulls + |> compileExeAndRun + |> shouldSucceed + |> withStdOutContains "42" + +// https://github.com/dotnet/fsharp/issues/19435 +[] +let ``ToString on float with UoM does not produce garbage at runtime`` () = + FSharp """ +module MyProgram + +[] +type mykg + +[] +let main _ = + let x : float = LanguagePrimitives.FloatWithMeasure 3.14 + let result = x.ToString() + // Float ToString may vary by culture, just check it's not garbage + if result.Contains("3") && result.Contains("14") then + printf "OK" + 0 + else + eprintfn "BUG: got '%s'" result + 1 + """ + |> withCheckNulls + |> compileExeAndRun + |> shouldSucceed + |> withStdOutContains "OK" + [] let ``Printing a nullable string should pass`` () = FSharp """module MyLibrary From 60e2675b10cb9914782dc3ac8196dc530f8292c5 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Mon, 16 Mar 2026 13:27:59 +0100 Subject: [PATCH 2/4] Refactor UoM ToString runtime tests into parameterized Theory Combine two separate [] tests into a single [] with [] matching the adjacent test pattern. Add decimal coverage. Simplify embedded source to print x.ToString() directly and verify with withStdOutContains instead of embedded validation logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Nullness/NullableReferenceTypesTests.fs | 47 ++++--------------- 1 file changed, 10 insertions(+), 37 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs index 10eb8ab8813..c36c51b3a15 100644 --- a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs @@ -735,33 +735,12 @@ let processAlias (x:mykgalias) = |> shouldSucceed // https://github.com/dotnet/fsharp/issues/19435 -[] -let ``ToString on value type with UoM does not produce garbage at runtime`` () = - FSharp """ -module MyProgram - -open FSharp.Data.UnitSystems.SI.UnitNames - -[] -let main _ = - let x = 42 - let result = x.ToString() - if result <> "42" then - eprintfn "BUG: Expected '42' but got '%s'" result - 1 - else - printf "42" - 0 - """ - |> withCheckNulls - |> compileExeAndRun - |> shouldSucceed - |> withStdOutContains "42" - -// https://github.com/dotnet/fsharp/issues/19435 -[] -let ``ToString on float with UoM does not produce garbage at runtime`` () = - FSharp """ +[] +[", "42")>] +[", "42")>] +[", "42")>] +let ``ToString on value type with UoM does not produce garbage at runtime`` (valueExpr: string, expected: string) = + FSharp $""" module MyProgram [] @@ -769,20 +748,14 @@ type mykg [] let main _ = - let x : float = LanguagePrimitives.FloatWithMeasure 3.14 - let result = x.ToString() - // Float ToString may vary by culture, just check it's not garbage - if result.Contains("3") && result.Contains("14") then - printf "OK" - 0 - else - eprintfn "BUG: got '%s'" result - 1 + let x = {valueExpr} + System.Console.Write(x.ToString()) + 0 """ |> withCheckNulls |> compileExeAndRun |> shouldSucceed - |> withStdOutContains "OK" + |> withStdOutContains expected [] let ``Printing a nullable string should pass`` () = From 142d6dc6e0bf23c85718f4b7c5bf40bef1c23f95 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Mon, 16 Mar 2026 14:18:11 +0100 Subject: [PATCH 3/4] Remove dead FSMeth branch in MethInfoWithModifiedReturnType dispatch Use nested pattern MethInfoWithModifiedReturnType(ILMeth(...), retTy) to directly match the only inner method kind that is ever constructed, eliminating the duplicated FSMeth branch that could never be reached. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Compiler/Checking/MethodCalls.fs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/Compiler/Checking/MethodCalls.fs b/src/Compiler/Checking/MethodCalls.fs index 8de04727d1c..170767afb62 100644 --- a/src/Compiler/Checking/MethodCalls.fs +++ b/src/Compiler/Checking/MethodCalls.fs @@ -1231,21 +1231,17 @@ let rec BuildMethodCall tcVal g amap isMutable m isProp minfo valUseFlags minst let vExpr, vExprTy = tcVal vref valUseFlags (minfo.DeclaringTypeInst @ minst) m BuildFSharpMethodApp g m vref vExpr vExprTy allArgs - | MethInfoWithModifiedReturnType(mi, retTy) -> + | MethInfoWithModifiedReturnType(ILMeth(_, ilMethInfo, _), retTy) -> // Build the inner call directly, without re-invoking TakeObjAddrForMethodCall. - // objArgs are already address-taken and allArgs/valUseFlags are already processed. let expr, exprTy = - match mi with - | ILMeth(_, ilMethInfo, _) -> - BuildILMethInfoCall g amap m isProp ilMethInfo valUseFlags minst direct allArgs - | FSMeth(_, _, vref, _) -> - let vExpr, vExprTy = tcVal vref valUseFlags (mi.DeclaringTypeInst @ minst) m - BuildFSharpMethodApp g m vref vExpr vExprTy allArgs - | _ -> failwith "MethInfoWithModifiedReturnType: unexpected inner method kind" + BuildILMethInfoCall g amap m isProp ilMethInfo valUseFlags minst direct allArgs let expr = mkCoerceExpr (expr, retTy, m, exprTy) expr, retTy + | MethInfoWithModifiedReturnType _ -> + failwith "MethInfoWithModifiedReturnType: unexpected inner method kind" + // Build a 'call' to a struct default constructor | DefaultStructCtor (g, ty) -> if g.langFeatureNullness && g.checkNullness then From 5f988ba98dcb908fcade81eaf815726e6cf326f4 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Mon, 16 Mar 2026 16:21:16 +0100 Subject: [PATCH 4/4] Add release notes for UoM ToString fix Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/release-notes/.FSharp.Compiler.Service/11.0.100.md | 1 + 1 file changed, 1 insertion(+) 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 16495b7b1ca..10f1b7b2ec0 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -12,6 +12,7 @@ * Improve let-rec codegen: reorder bindings to allocate lambda closures before non-lambda values that reference them. ([PR #19339](https://github.com/dotnet/fsharp/pull/19339)) * Fix `YieldFromFinal`/`ReturnFromFinal` being incorrectly called in non-tail positions (`for`, `use`, `use!`, `try/with` handler). ([Issue #19402](https://github.com/dotnet/fsharp/issues/19402), [PR #19403](https://github.com/dotnet/fsharp/pull/19403)) * Fixed how the source ranges of warn directives are reported (as trivia) in the parser output (by not reporting leading spaces). ([Issue #19405](https://github.com/dotnet/fsharp/issues/19405), [PR #19408]((https://github.com/dotnet/fsharp/pull/19408))) +* Fix UoM value type `ToString()` returning garbage values when `--checknulls+` is enabled, caused by double address-taking in codegen. ([Issue #19435](https://github.com/dotnet/fsharp/issues/19435), [PR #19440](https://github.com/dotnet/fsharp/pull/19440)) ### Added