From a1cb9fb41b2895baa973ef093eb0bbb8a0a128c3 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 20 Feb 2025 16:22:34 +0100 Subject: [PATCH 1/6] Improve 'useless null handling' warning to work with piping --- .../Checking/Expressions/CheckExpressions.fs | 34 ++++++++------- .../Nullness/NullableReferenceTypesTests.fs | 41 +++++++++++++++++++ 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index d80470b9e53..f2af75d41d6 100644 --- a/src/Compiler/Checking/Expressions/CheckExpressions.fs +++ b/src/Compiler/Checking/Expressions/CheckExpressions.fs @@ -8568,29 +8568,21 @@ and TcApplicationThen (cenv: cenv) (overallTy: OverallTy) env tpenv mExprAndArg | _ -> synArg - let (arg, tpenv), cenv = + let (arg, tpenv) = // treat left and right of '||' and '&&' as control flow, so for example // f expr1 && g expr2 // will have debug points on "f expr1" and "g expr2" - let env,cenv = + let env = match leftExpr with | ApplicableExpr(expr=Expr.Val (vref, _, _)) | ApplicableExpr(expr=Expr.App (Expr.Val (vref, _, _), _, _, [_], _)) when valRefEq g vref g.and_vref || valRefEq g vref g.and2_vref || valRefEq g vref g.or_vref - || valRefEq g vref g.or2_vref -> - { env with eIsControlFlow = true },cenv - | ApplicableExpr(expr=Expr.Val (valRef=vref)) - | ApplicableExpr(expr=Expr.App (funcExpr=Expr.Val (valRef=vref))) -> - match TryFindLocalizedFSharpStringAttribute g g.attrib_WarnOnWithoutNullArgumentAttribute vref.Attribs with - | Some _ as msg -> env,{ cenv with css.WarnWhenUsingWithoutNullOnAWithNullTarget = msg} - | None when cenv.css.WarnWhenUsingWithoutNullOnAWithNullTarget <> None -> - env, { cenv with css.WarnWhenUsingWithoutNullOnAWithNullTarget = None} - | None -> env,cenv - | _ -> env,cenv - - TcExprFlex2 cenv domainTy env false tpenv synArg, cenv + || valRefEq g vref g.or2_vref -> { env with eIsControlFlow = true } + | _ -> env + + TcExprFlex2 cenv domainTy env false tpenv synArg let exprAndArg, resultTy = buildApp cenv leftExpr resultTy arg mExprAndArg TcDelayed cenv overallTy env tpenv mExprAndArg exprAndArg resultTy atomicFlag delayed @@ -9289,6 +9281,7 @@ and TcValueItemThen cenv overallTy env vref tpenv mItem afterResolution delayed PropagateThenTcDelayed cenv overallTy env tpenv mExprAndTypeArgs vexpFlex vexpFlex.Type ExprAtomicFlag.Atomic otherDelayed // Value get + | _ -> let _, vExpr, isSpecial, _, _, tpenv = TcVal cenv env tpenv vref None (Some afterResolution) mItem @@ -9297,6 +9290,19 @@ and TcValueItemThen cenv overallTy env vref tpenv mItem afterResolution delayed | Expr.Const (Const.String value, _, _) -> TcConstStringExpr cenv overallTy env mItem tpenv value LiteralArgumentType.StaticField | _ -> vExpr, tpenv + let getCenvForVref cenv (vref:ValRef) = + match TryFindLocalizedFSharpStringAttribute g g.attrib_WarnOnWithoutNullArgumentAttribute vref.Attribs with + | Some _ as msg -> { cenv with css.WarnWhenUsingWithoutNullOnAWithNullTarget = msg} + | None when cenv.css.WarnWhenUsingWithoutNullOnAWithNullTarget <> None -> + // We need to reset the warning back to default once in a nested call, to prevent false warnings e.g. in `Option.ofObj (Path.GetDirectoryName "")` + { cenv with css.WarnWhenUsingWithoutNullOnAWithNullTarget = None} + | None -> cenv + + let cenv = + match vExpr with + | Expr.App (funcExpr=Expr.Val (valRef=vref)) -> getCenvForVref cenv vref + | _ -> cenv + let vexpFlex = if isSpecial then MakeApplicableExprNoFlex cenv vExpr else MakeApplicableExprWithFlex cenv env vExpr PropagateThenTcDelayed cenv overallTy env tpenv mItem vexpFlex vexpFlex.Type ExprAtomicFlag.Atomic delayed diff --git a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs index 018b81c9384..3d08afd95cc 100644 --- a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs @@ -1178,6 +1178,47 @@ let mappedMaybe = nonNull maybeNull |> shouldFail |> withDiagnostics [Error 3262, Line 4, Col 25, Line 4, Col 39, "Value known to be without null passed to a function meant for nullables: You can remove this `nonNull` assertion."] +[] +let ``Useless null check when used in piping`` () = + FSharp """module MyLibrary + +let foo = "test" +let bar = foo |> Option.ofObj // Should produce FS3262, but did not +""" + |> asLibrary + |> typeCheckWithStrictNullness + |> shouldFail + |> withDiagnostics [Error 3262, Line 4, Col 18, Line 4, Col 30, "Value known to be without null passed to a function meant for nullables: You can create 'Some value' directly instead of 'ofObj', or consider not using an option for this value."] + +[] +let ``Useless null check when used in multi piping`` () = + FSharp """module MyLibrary + +let myFunc whateverArg = + whateverArg |> string |> Option.ofObj +""" + |> asLibrary + |> typeCheckWithStrictNullness + |> shouldFail + |> withDiagnostics [Error 3262, Line 4, Col 30, Line 4, Col 42, "Value known to be without null passed to a function meant for nullables: You can create 'Some value' directly instead of 'ofObj', or consider not using an option for this value."] + +[] +let ``Useless null check when used in more exotic pipes`` () = + FSharp """module MyLibrary + +let functionComposition x = x |> (string >> Option.ofObj) +let doublePipe x = ("x","y") ||> (fun x _y -> Option.ofObj x) +let intToint x = x + 1 +let pointfree = intToint >> string >> Option.ofObj +""" + |> asLibrary + |> typeCheckWithStrictNullness + |> shouldFail + |> withDiagnostics + [ Error 3262, Line 3, Col 45, Line 3, Col 57, "Value known to be without null passed to a function meant for nullables: You can create 'Some value' directly instead of 'ofObj', or consider not using an option for this value." + Error 3262, Line 4, Col 60, Line 4, Col 61, "Value known to be without null passed to a function meant for nullables: You can create 'Some value' directly instead of 'ofObj', or consider not using an option for this value." + Error 3262, Line 6, Col 39, Line 6, Col 51, "Value known to be without null passed to a function meant for nullables: You can create 'Some value' directly instead of 'ofObj', or consider not using an option for this value."] + [] let ``Regression: Useless usage in nested calls`` () = FSharp """module MyLibrary From 0fa6e648b7c927ffbec9ffab1175deb6a2cf9b50 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Thu, 20 Feb 2025 19:51:02 +0100 Subject: [PATCH 2/6] Please build. --- src/Compiler/Interactive/ControlledExecution.fs | 2 ++ src/FSharp.Build/FSharpEmbedResourceText.fs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/Compiler/Interactive/ControlledExecution.fs b/src/Compiler/Interactive/ControlledExecution.fs index 2d696344b58..9f700b12fbf 100644 --- a/src/Compiler/Interactive/ControlledExecution.fs +++ b/src/Compiler/Interactive/ControlledExecution.fs @@ -6,6 +6,8 @@ // because we continue to support older coreclrs and the windows desktop framework through netstandard2.0 // we access the features using reflection +#nowarn 3262 // The `Option.ofObj (Type.GetType..) construct warns in ns20, because Type.GetType is not annotated as nullable + namespace FSharp.Compiler.Interactive open System diff --git a/src/FSharp.Build/FSharpEmbedResourceText.fs b/src/FSharp.Build/FSharpEmbedResourceText.fs index 9af5eaf4d72..7bbf9a04854 100644 --- a/src/FSharp.Build/FSharpEmbedResourceText.fs +++ b/src/FSharp.Build/FSharpEmbedResourceText.fs @@ -273,6 +273,7 @@ open Printf let StringBoilerPlate fileName = @" // BEGIN BOILERPLATE +#nowarn 3262 // The call to Option.ofObj below is applied in multiple compilation modes for GetString, sometimes the value is typed as a non-nullable string static let getCurrentAssembly () = System.Reflection.Assembly.GetExecutingAssembly() From bd085383d449d1d7e569de8bd32b3f6b0bdf2489 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 21 Feb 2025 10:57:21 +0100 Subject: [PATCH 3/6] notes + fix resource build --- docs/release-notes/.FSharp.Compiler.Service/9.0.300.md | 1 + src/FSharp.Build/FSharpEmbedResourceText.fs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md index f64f090744b..b49f29cc19b 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md @@ -24,6 +24,7 @@ * Update `Obsolete` attribute checking to account for `DiagnosticId` and `UrlFormat` properties. ([PR #18224](https://github.com/dotnet/fsharp/pull/18224)) * Remove `Cancellable.UsingToken` from tests ([PR #18276](https://github.com/dotnet/fsharp/pull/18276)) * Added nullability annotations to `.Using` builder method for `async`, `task` and compiler-internal builders ([PR #18292](https://github.com/dotnet/fsharp/pull/18292)) +* Warning for "useless null handling" works with piped syntax constructs now ([PR #18331](https://github.com/dotnet/fsharp/pull/18331)) ### Breaking Changes * Struct unions with overlapping fields now generate mappings needed for reading via reflection ([Issue #18121](https://github.com/dotnet/fsharp/issues/17797), [PR #18274](https://github.com/dotnet/fsharp/pull/17877)) diff --git a/src/FSharp.Build/FSharpEmbedResourceText.fs b/src/FSharp.Build/FSharpEmbedResourceText.fs index 7bbf9a04854..feae192e513 100644 --- a/src/FSharp.Build/FSharpEmbedResourceText.fs +++ b/src/FSharp.Build/FSharpEmbedResourceText.fs @@ -273,7 +273,7 @@ open Printf let StringBoilerPlate fileName = @" // BEGIN BOILERPLATE -#nowarn 3262 // The call to Option.ofObj below is applied in multiple compilation modes for GetString, sometimes the value is typed as a non-nullable string +#nowarn ""3262"" // The call to Option.ofObj below is applied in multiple compilation modes for GetString, sometimes the value is typed as a non-nullable string static let getCurrentAssembly () = System.Reflection.Assembly.GetExecutingAssembly() From 3f3de9c2a9541fc73a4d75f11a8921028194c556 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 21 Feb 2025 11:17:54 +0100 Subject: [PATCH 4/6] place #nowarn correctly in generated file --- src/FSharp.Build/FSharpEmbedResourceText.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/FSharp.Build/FSharpEmbedResourceText.fs b/src/FSharp.Build/FSharpEmbedResourceText.fs index feae192e513..3e3c79f82ee 100644 --- a/src/FSharp.Build/FSharpEmbedResourceText.fs +++ b/src/FSharp.Build/FSharpEmbedResourceText.fs @@ -268,13 +268,13 @@ open Microsoft.FSharp.Core.Operators open Microsoft.FSharp.Text open Microsoft.FSharp.Collections open Printf + +#nowarn ""3262"" // The call to Option.ofObj below is applied in multiple compilation modes for GetString, sometimes the value is typed as a non-nullable string " let StringBoilerPlate fileName = @" // BEGIN BOILERPLATE -#nowarn ""3262"" // The call to Option.ofObj below is applied in multiple compilation modes for GetString, sometimes the value is typed as a non-nullable string - static let getCurrentAssembly () = System.Reflection.Assembly.GetExecutingAssembly() static let getTypeInfo (t: System.Type) = t From 7043c87736fb56c657218de682b1c732652840c9 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 21 Feb 2025 11:25:46 +0100 Subject: [PATCH 5/6] Make Fantomas happy --- src/Compiler/Interactive/ControlledExecution.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Interactive/ControlledExecution.fs b/src/Compiler/Interactive/ControlledExecution.fs index 9f700b12fbf..2d18a8819b2 100644 --- a/src/Compiler/Interactive/ControlledExecution.fs +++ b/src/Compiler/Interactive/ControlledExecution.fs @@ -6,7 +6,7 @@ // because we continue to support older coreclrs and the windows desktop framework through netstandard2.0 // we access the features using reflection -#nowarn 3262 // The `Option.ofObj (Type.GetType..) construct warns in ns20, because Type.GetType is not annotated as nullable +#nowarn "3262" // The `Option.ofObj (Type.GetType..) construct warns in ns20, because Type.GetType is not annotated as nullable namespace FSharp.Compiler.Interactive From f9087151740e604964faca1b68354eb5c4196117 Mon Sep 17 00:00:00 2001 From: Tomas Grosup Date: Fri, 21 Feb 2025 15:07:35 +0100 Subject: [PATCH 6/6] adjust tests --- .../Language/Nullness/NullableRegressionTests.fs | 3 ++- .../Nullness/library-functions.fs.checknulls_on.err.bsl | 6 +----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableRegressionTests.fs b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableRegressionTests.fs index 9bf4730e404..c549cc09aa2 100644 --- a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableRegressionTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableRegressionTests.fs @@ -42,7 +42,8 @@ let ``Signature conformance`` langVersion checknulls = |> compile |> shouldFail |> withDiagnostics - [(Warning 3261, Line 4, Col 5, Line 4, Col 10, "Nullness warning: Module 'M' contains + [Warning 3262, Line 18, Col 48, Line 18, Col 60, "Value known to be without null passed to a function meant for nullables: You can create 'Some value' directly instead of 'ofObj', or consider not using an option for this value." + (Warning 3261, Line 4, Col 5, Line 4, Col 10, "Nullness warning: Module 'M' contains val test2: x: string | null -> unit but its signature specifies val test2: string -> unit diff --git a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/library-functions.fs.checknulls_on.err.bsl b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/library-functions.fs.checknulls_on.err.bsl index 026cfabacff..64237a113c2 100644 --- a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/library-functions.fs.checknulls_on.err.bsl +++ b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/library-functions.fs.checknulls_on.err.bsl @@ -1,13 +1,9 @@ -library-functions.fs (6,27)-(6,29) typecheck error Value known to be without null passed to a function meant for nullables: You can remove this `nonNull` assertion. library-functions.fs (7,10)-(7,17) typecheck error Nullness warning: The type 'String | null' supports 'null' but a non-null type is expected. -library-functions.fs (7,33)-(7,35) typecheck error Value known to be without null passed to a function meant for nullables: You can remove this `nonNull` assertion. library-functions.fs (8,18)-(8,20) typecheck error Value known to be without null passed to a function meant for nullables: You can remove this `nonNull` assertion. -library-functions.fs (10,28)-(10,39) typecheck error Value known to be without null passed to a function meant for nullables: You can remove this `nonNull` assertion. library-functions.fs (11,18)-(11,26) typecheck error Value known to be without null passed to a function meant for nullables: You can remove this `nonNull` assertion. library-functions.fs (17,27)-(17,38) typecheck error Nullness warning: A non-nullable 'String' was expected but this expression is nullable. Consider either changing the target to also be nullable, or use pattern matching to safely handle the null case of this expression. library-functions.fs (18,28)-(18,39) typecheck error Nullness warning: A non-nullable 'String' was expected but this expression is nullable. Consider either changing the target to also be nullable, or use pattern matching to safely handle the null case of this expression. library-functions.fs (20,28)-(20,36) typecheck error Nullness warning: The type 'String | null' supports 'null' but a non-null type is expected. library-functions.fs (26,17)-(26,19) typecheck error Nullness warning: The type 'string' does not support 'null'. library-functions.fs (27,10)-(27,16) typecheck error Nullness warning: The type 'String' does not support 'null'. -library-functions.fs (37,17)-(37,21) typecheck error Nullness warning: A non-nullable 'String' was expected but this expression is nullable. Consider either changing the target to also be nullable, or use pattern matching to safely handle the null case of this expression. -library-functions.fs (41,26)-(41,28) typecheck error Value known to be without null passed to a function meant for nullables: You can remove this `nonNull` assertion. \ No newline at end of file +library-functions.fs (37,17)-(37,21) typecheck error Nullness warning: A non-nullable 'String' was expected but this expression is nullable. Consider either changing the target to also be nullable, or use pattern matching to safely handle the null case of this expression. \ No newline at end of file