Nullness :: Improve 'useless null handling' warning to work with piping

docs/release-notes/.FSharp.Compiler.Service/9.0.300.md

@@ -26,6 +26,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))

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

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

src/FSharp.Build/FSharpEmbedResourceText.fs

@@ -268,12 +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
-
     static let getCurrentAssembly () = System.Reflection.Assembly.GetExecutingAssembly()
 
     static let getTypeInfo (t: System.Type) = t

tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs

@@ -1266,6 +1266,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."]
 
+[<Fact>]
+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."]
+
+[<Fact>]
+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."]
+
+[<Fact>]
+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."]
+
 [<Fact>]
 let ``Regression: Useless usage in nested calls`` () = 
     FSharp """module MyLibrary

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    

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