Add analyzer detecting let! _ = ... .

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

src/FSharp.Analyzers/FSharp.Analyzers.fsproj

@@ -36,6 +36,7 @@
     <Compile Include="MissingTypeAnnotationInlineDataAnalyzer.fs" />
     <Compile Include="DoNotUseYourOwnRandomAnalyzer.fs" />
     <Compile Include="DoubleWrappedResultAnalyzer.fs" />
+    <Compile Include="LetWildcardResultAnalyzer.fs" />
   </ItemGroup>
   <Target Name="_AddAnalyzersToOutput">
     <ItemGroup>

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
+
+[<Literal>]
+let code = "IDURA-RESULT-005"
+
+[<Literal>]
+let name = "Ignored variable in let! binding."
+
+[<Literal>]
+let msg = "The pattern let! _ = ... is dangerous because it makes it easy to accidentally ignore errors."
+
+let private analyzer
+    (_: ISourceText)
+    (untypedTree: ParsedInput)
+    (_: FSharpCheckFileResults) : Async<Message list> = async {
+    let allWildLetBangBindings =
+        let allWildLetBangBindings = ResizeArray<range>()
+
+        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
+}
+
+[<CliAnalyzer(name)>]
+let cliAnalyzer (ctx: CliContext) : Async<Message list> =
+    analyzer ctx.SourceText ctx.ParseFileResults.ParseTree ctx.CheckFileResults
+
+[<EditorAnalyzer(name)>]
+let editorAnalyzer (ctx: EditorContext) : Async<Message list> =
+    match ctx.CheckFileResults with
+    | None -> async.Return []
+    | Some (checkResults: FSharpCheckFileResults) ->
+        analyzer ctx.SourceText ctx.ParseFileResults.ParseTree checkResults

tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj

@@ -13,6 +13,7 @@
     <Compile Include="MissingTypeAnnotationInlineDataAnalyzerTests.fs" />
     <Compile Include="DoNotUseYourOwnRandomAnalyzerTests.fs" />
     <Compile Include="DoubleWrappedResultAnalyzerTests.fs" />
+    <Compile Include="LetWildcardResultAnalyzerTests.fs" />
   </ItemGroup>
 
   <ItemGroup>
@@ -23,6 +24,7 @@
     <PackageReference Include="xunit" />
     <PackageReference Include="xunit.runner.visualstudio" />
     <PackageReference Include="FSharp.Analyzers.SDK.Testing" />
+    <PackageReference Include="FsToolkit.Errorhandling" />
   </ItemGroup>
   <ItemGroup>
         <None Include="data\**\*.fs*" />

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
+}
+
+[<Theory>]
+[<MemberData(nameof(TestFiles.GetSources), parameters=[|"letWildcardResultData/positive"|], MemberType=typeof<TestFiles>)>]
+let ``positive``(program : string, filename: string) =
+  let snapshotName = Snapshot.FullName(SnapshotNameExtension.Create filename)
+  runPositiveTest snapshotName setupContext cliAnalyzer program
+
+[<Theory>]
+[<MemberData(nameof(TestFiles.GetSources), parameters=[|"letWildcardResultData/negative"|], MemberType=typeof<TestFiles>)>]
+let ``negative``(program : string, _: string) =
+  runNegativeTest setupContext cliAnalyzer program

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."}]

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<unit,string> = result {
+    let a = "def"
+    let! b = validate a
+    return ()
+}

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<unit,string> = result {
+    let a = "def"
+    let _ = validate a
+    return ()
+}

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<unit,string> = result {
+    let a = "def"
+    use _ = makeResource "a"
+    return ()
+}

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<unit,string> = result {
+    let a = "def"
+    use! _ = makeResource "a"
+    return ()
+}

tests/FSharp.Analyzers.Tests/data/letWildcardResultData/positive/Basic.fs

@@ -0,0 +1,23 @@
+module M
+
+open FsToolkit.ErrorHandling
+
+// This function has type: string -> Result<string, string>
+let validateSubfunction (input : string) =
+    if (input <> "abc") then
+        Result.Error "Error"
+    else
+        Result.Ok input
+
+// This function has type: string -> Result<Result<string, string>, 'a>
+let validate (input : string) = result {
+    return validateSubfunction input
+}
+
+let main : Result<unit,string> = 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 ()
+}