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 da9198c332c..c352b511486 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/9.0.300.md +++ b/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)) diff --git a/src/Compiler/Checking/Expressions/CheckExpressions.fs b/src/Compiler/Checking/Expressions/CheckExpressions.fs index df64c4da62e..1b637ec2bab 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/src/Compiler/Interactive/ControlledExecution.fs b/src/Compiler/Interactive/ControlledExecution.fs index 2d696344b58..2d18a8819b2 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..3e3c79f82ee 100644 --- a/src/FSharp.Build/FSharpEmbedResourceText.fs +++ b/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 diff --git a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs b/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs index 50467c12e0a..4764554ce5f 100644 --- a/tests/FSharp.Compiler.ComponentTests/Language/Nullness/NullableReferenceTypesTests.fs +++ b/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."] +[] +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 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