diff --git a/README.md b/README.md index 9d4827e..f767351 100644 --- a/README.md +++ b/README.md @@ -100,4 +100,15 @@ Intentionally wrapping a value twice is very rare, but can sometimes occur, so t | Code | `IDURA-RESULT-006` | | Message | Double-wrapping values in Result is often caused by accidentally ignoring an error. | | Severity | Warning | -| Works in | CLI, Ionide | \ No newline at end of file +| Works in | CLI, Ionide | + +### Using wildcards with let! bindings +It is dangerous to use the pattern `let! _ = ...` because this may inadvertently end up ignoring an `Error` if the function being piped accidentally returns a nested `Result`. +Experience has shown that it is easy to accidentally introduce subtle and high severity bugs with this pattern. + +| About this analyzer | | +|---------------------|--------------------------------------------------------------------------------------------------------------------------| +| Code | `IDURA-RESULT-005` | +| Message | The pattern let! _ = ... is dangerous because it makes it easy to accidentally ignore errors. | +| Severity | Error | +| Works in | CLI, Ionide | diff --git a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj index 3acc5d8..3348f91 100644 --- a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj +++ b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj @@ -36,6 +36,7 @@ + diff --git a/src/FSharp.Analyzers/LetWildcardResultAnalyzer.fs b/src/FSharp.Analyzers/LetWildcardResultAnalyzer.fs new file mode 100644 index 0000000..87e4dc7 --- /dev/null +++ b/src/FSharp.Analyzers/LetWildcardResultAnalyzer.fs @@ -0,0 +1,62 @@ +module Idura.FSharp.Analyzers.LetWildcardResultAnalyzer + +open FSharp.Analyzers.SDK +open FSharp.Compiler.Text +open FSharp.Compiler.Syntax +open FSharp.Compiler.CodeAnalysis +open FSharp.Analyzers.SDK.ASTCollecting + +[] +let code = "IDURA-RESULT-005" + +[] +let name = "Ignored variable in let! binding." + +[] +let msg = "The pattern let! _ = ... is dangerous because it makes it easy to accidentally ignore errors." + +let private analyzer + (_: ISourceText) + (untypedTree: ParsedInput) + (_: FSharpCheckFileResults) : Async = async { + let allWildLetBangBindings = + let allWildLetBangBindings = ResizeArray() + + let walker = { + new SyntaxCollectorBase() with + override _.WalkExpr(_, expr) = + match expr with + | SynExpr.LetOrUse(_, false, _, true, [binding], _, _, _) -> + match binding with + | SynBinding(_, _, _, _, _, _, _, SynPat.Wild _, _, _, range, _, _) -> + allWildLetBangBindings.Add range + | _ -> () + | _ -> () + } + + walkAst walker untypedTree + allWildLetBangBindings |> Seq.toList + + return + List.map (fun range -> + { + Type = name + Message = $"""%s{msg}""" + Code = code + Severity = Severity.Error + Range = range + Fixes = [] + } + ) allWildLetBangBindings +} + +[] +let cliAnalyzer (ctx: CliContext) : Async = + analyzer ctx.SourceText ctx.ParseFileResults.ParseTree ctx.CheckFileResults + +[] +let editorAnalyzer (ctx: EditorContext) : Async = + match ctx.CheckFileResults with + | None -> async.Return [] + | Some (checkResults: FSharpCheckFileResults) -> + analyzer ctx.SourceText ctx.ParseFileResults.ParseTree checkResults diff --git a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj index 21d3d04..28e0241 100644 --- a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj +++ b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj @@ -13,6 +13,7 @@ + @@ -23,6 +24,7 @@ + diff --git a/tests/FSharp.Analyzers.Tests/LetWildcardResultAnalyzerTests.fs b/tests/FSharp.Analyzers.Tests/LetWildcardResultAnalyzerTests.fs new file mode 100644 index 0000000..1467d94 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/LetWildcardResultAnalyzerTests.fs @@ -0,0 +1,35 @@ +module Idura.FSharp.Analyzers.Tests.LetWildcardResultAnalyzerTests + +open FSharp.Analyzers.SDK.Testing +open TestHelpers + +open Xunit +open Snapshooter +open Snapshooter.Xunit + +open Idura.FSharp.Analyzers.LetWildcardResultAnalyzer + +let setupContext () = async { + let! opts = + mkOptionsFromProject + "netstandard2.0" // ensures compatibility with both .NET Framework and newer .NET versions + [ + { + Name = "FsToolkit.Errorhandling" + Version = "5.0.0" + } + ] + |> Async.AwaitTask + return opts +} + +[] +[)>] +let ``positive``(program : string, filename: string) = + let snapshotName = Snapshot.FullName(SnapshotNameExtension.Create filename) + runPositiveTest snapshotName setupContext cliAnalyzer program + +[] +[)>] +let ``negative``(program : string, _: string) = + runNegativeTest setupContext cliAnalyzer program diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/LetWildcardResultAnalyzerTests.positive_Basic.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/LetWildcardResultAnalyzerTests.positive_Basic.fs.snap new file mode 100644 index 0000000..a46813f --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/__snapshots__/LetWildcardResultAnalyzerTests.positive_Basic.fs.snap @@ -0,0 +1 @@ +[{"Code":"IDURA-RESULT-005","Fixes":[],"Message":"The pattern let! _ = ... is dangerous because it makes it easy to accidentally ignore errors.","Range":{"End":{"Line":21,"Column":23},"EndColumn":23,"EndLine":21,"FileName":"A.fs","Start":{"Line":21,"Column":4},"StartColumn":4,"StartLine":21},"Severity":{"Case":"Error"},"Type":"Ignored variable in let! binding."}] diff --git a/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNonWildcardLetBang.fs b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNonWildcardLetBang.fs new file mode 100644 index 0000000..835b3a0 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNonWildcardLetBang.fs @@ -0,0 +1,19 @@ +module M + +open FsToolkit.ErrorHandling + +let validateSubfunction (input : string) = + if (input <> "abc") then + Result.Error "Error" + else + Result.Ok input + +let validate (input : string) = result { + return! validateSubfunction input +} + +let main : Result = result { + let a = "def" + let! b = validate a + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNormalLet.fs b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNormalLet.fs new file mode 100644 index 0000000..751348d --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNormalLet.fs @@ -0,0 +1,19 @@ +module M + +open FsToolkit.ErrorHandling + +let validateSubfunction (input : string) = + if (input <> "abc") then + Result.Error "Error" + else + Result.Ok input + +let validate (input : string) = result { + return! validateSubfunction input +} + +let main : Result = result { + let a = "def" + let _ = validate a + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNormalUse.fs b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNormalUse.fs new file mode 100644 index 0000000..820fa01 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreNormalUse.fs @@ -0,0 +1,13 @@ +module M + +open FsToolkit.ErrorHandling + +let makeResource name = + { new System.IDisposable + with member this.Dispose() = printfn "%s disposed" name } + +let main : Result = result { + let a = "def" + use _ = makeResource "a" + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreUseBang.fs b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreUseBang.fs new file mode 100644 index 0000000..85bc61d --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/negative/IgnoreUseBang.fs @@ -0,0 +1,14 @@ +module M + +open FsToolkit.ErrorHandling + +let makeResource name = + { new System.IDisposable + with member this.Dispose() = printfn "%s disposed" name } + |> Result.Ok + +let main : Result = result { + let a = "def" + use! _ = makeResource "a" + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/positive/Basic.fs b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/positive/Basic.fs new file mode 100644 index 0000000..1b9b2a7 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/letWildcardResultData/positive/Basic.fs @@ -0,0 +1,23 @@ +module M + +open FsToolkit.ErrorHandling + +// This function has type: string -> Result +let validateSubfunction (input : string) = + if (input <> "abc") then + Result.Error "Error" + else + Result.Ok input + +// This function has type: string -> Result, 'a> +let validate (input : string) = result { + return validateSubfunction input +} + +let main : Result = result { + let a = "def" + // The next line has value `Result.Error "Error"`, but this is not caught due to the double-wrapping in `validate`. + // This means that validation result is completely ignored. + let! _ = validate a + return () +} \ No newline at end of file