Add analyzer for ignoring results in do!.

README.md

@@ -87,23 +87,59 @@ This analyzer detects use of the constructors and `Create` methods of the `Rando
 | Code                | `IDURA-CRYPTO-002`                                                                                                          |
 | Message             | Do not use your own instance of RandomNumberGenerator. Depend on a global RNG pool to ensure stability of the generator.    |
 | Severity            | Warning                                                                                                                     |
-| Works in            | CLI, Ionide                                                                                                                 |
+| Works in            | CLI, Ionide                                                                                                                 |                                                                                                            |
 
-### Did you mean to wrap this value in Result twice?
-Wrapping a value in the `Result` monad twice is often caused by accidentally ignoring a `Result.Error`.
-This can be catastrophic when doing validation of security-related properties.
 
-Intentionally wrapping a value twice is very rare, but can sometimes occur, so this is only a warning.
+### Piping into `Result.ignore` in `do!` statements
+It is dangerous to use the pattern `do! ... |> Result.ignore` because this may inadvertently end up ignoring an `Error` if the function being piped accidentally returns a nested `Result`.
+
+| About this analyzer |                                                                                                                          |
+|---------------------|--------------------------------------------------------------------------------------------------------------------------|
+| Code                | `IDURA-RESULT-001`                                                                                                       |
+| Message             | The pattern do! ... |> Result.ignore is dangerous because it makes it easy to accidentally ignore errors.                |
+| Severity            | Error                                                                                                                    |
+| Works in            | CLI, Ionide                                                                                                              |
+
+
+### Piping into `TaskResult.ignore` in `do!` statements
+It is dangerous to use the pattern `do! ... |> TaskResult.ignore` because this may inadvertently end up ignoring an `Error` if the function being piped accidentally returns a nested `TaskResult`.
+Experience has shown that it is easy to accidentally introduce subtle and high severity bugs with this pattern.
+
+| About this analyzer |                                                                                                                          |
+|---------------------|--------------------------------------------------------------------------------------------------------------------------|
+| Code                | `IDURA-RESULT-002`                                                                                                       |
+| Message             | The pattern do! ... |> TaskResult.ignore is dangerous because it makes it easy to accidentally ignore errors.            |
+| Severity            | Error                                                                                                                    |
+| Works in            | CLI, Ionide                                                                                                              |
+
+
+### Piping into `Result.map ignore` in `do!` statements
+It is dangerous to use the pattern `do! ... |> Result.ignore` 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-003`                                                                                                       |
+| Message             | The pattern do! ... |> Result.map ignore is dangerous because it makes it easy to accidentally ignore errors.            |
+| Severity            | Error                                                                                                                    |
+| Works in            | CLI, Ionide                                                                                                              |
+
+
+### Piping into `TaskResult.map ignore` in `do!` statements
+It is dangerous to use the pattern `do! ... |> TaskResult.map ignore` because this may inadvertently end up ignoring an `Error` if the function being piped accidentally returns a nested `TaskResult`.
+Experience has shown that it is easy to accidentally introduce subtle and high severity bugs with this pattern.
+
+| About this analyzer |                                                                                                                          |
+|---------------------|--------------------------------------------------------------------------------------------------------------------------|
+| Code                | `IDURA-RESULT-004`                                                                                                       |
+| Message             | The pattern do! ... |> TaskResult.map ignore is dangerous because it makes it easy to accidentally ignore errors.        |
+| Severity            | Error                                                                                                                    |
+| Works in            | CLI, Ionide                                                                                                              |
 
-| About this analyzer |                                                                                                                             |
-|---------------------|-----------------------------------------------------------------------------------------------------------------------------|
-| Code                | `IDURA-RESULT-006`                                                                                                          |
-| Message             | Double-wrapping values in Result is often caused by accidentally ignoring an error.                                         |
-| Severity            | Warning                                                                                                                     |
-| 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 |                                                                                                                          |
@@ -112,3 +148,17 @@ Experience has shown that it is easy to accidentally introduce subtle and high s
 | Message             | The pattern let! _ = ... is dangerous because it makes it easy to accidentally ignore errors.                            |
 | Severity            | Error                                                                                                                    |
 | Works in            | CLI, Ionide                                                                                                              |
+
+
+### Did you mean to wrap this value in Result twice?
+Wrapping a value in the `Result` monad twice is often caused by accidentally ignoring a `Result.Error`.
+This can be catastrophic when doing validation of security-related properties.
+
+Intentionally wrapping a value twice is very rare, but can sometimes occur, so this is only a warning.
+
+| About this analyzer |                                                                                                                             |
+|---------------------|-----------------------------------------------------------------------------------------------------------------------------|
+| Code                | `IDURA-RESULT-006`                                                                                                          |
+| Message             | Double-wrapping values in Result is often caused by accidentally ignoring an error.                                         |
+| Severity            | Warning                                                                                                                     |
+| Works in            | CLI, Ionide                                                                                                                 |

src/FSharp.Analyzers/DoResultIgnoreAnalyzer.fs

@@ -0,0 +1,128 @@
+module Idura.FSharp.Analyzers.DoResultIgnoreAnalyzer
+
+open FSharp.Analyzers.SDK
+open FSharp.Compiler.Text
+open FSharp.Compiler.Syntax
+open FSharp.Compiler.CodeAnalysis
+open FSharp.Analyzers.SDK.ASTCollecting
+
+let codePattern code = $"IDURA-DO-IGNORE-{code}"
+
+[<Literal>]
+let name = "Do not ignore results in do! statements"
+
+let msgPattern func = $"The pattern do! ... |> {func} is dangerous because it makes it easy to accidentally ignore errors."
+
+let (|SynLongIdentAsString|) (lid: SynLongIdent) =
+    lid.LongIdent |> List.map (fun ident -> ident.idText)
+
+let (|LongIdentAsString|Other|) (lid: SynExpr) =
+    match lid with
+    | SynExpr.LongIdent(_, id, _, _) ->
+        let (SynLongIdentAsString ident) = id
+        LongIdentAsString ident
+    | _ -> Other
+
+let private analyzer
+    (_: ISourceText)
+    (untypedTree: ParsedInput)
+    (_: FSharpCheckFileResults) : Async<Message list> = async {
+    let allIgnoredResultsInDo =
+        let allDoBindingsWithReturnIgnorePipe = ResizeArray<range * string * string>()
+
+        // Patterns to find:
+        // _ |> Result.ignore
+        // _ |> TaskResult.ignore
+        // _ |> Result.map ignore
+        // _ |> TaskResult.map ignore
+        //
+        // There  is no need to find the Option versions, since they do require the inner
+        // success type to be an option and are thus not susceptible to accidental
+        // double-wrapping of results.
+
+        let rec addResultIgnore range  = function
+        | SynExpr.App(_, _, funcExpr: SynExpr, argExpr, _) ->
+            match funcExpr with
+            // op_PipeRight is the internal name for the pipe right |> operator.
+            // Internal operation names are always mapped to long identifiers.
+            | SynExpr.App(_, _, LongIdentAsString ["op_PipeRight"], _, _) ->
+                // The FsToolkit modules are tagged with RequireqQualifiedAccess, so
+                // we only need to look for long identifiers.
+                match argExpr with
+                | LongIdentAsString ["Result"; "ignore"] ->
+                    allDoBindingsWithReturnIgnorePipe.Add (range, "Result.ignore", "001")
+                | LongIdentAsString ["TaskResult"; "ignore"] ->
+                    allDoBindingsWithReturnIgnorePipe.Add (range, "TaskResult.ignore", "002")
+                | SynExpr.App(_, _, func, SynExpr.Ident id, _) when id.idText = "ignore" ->
+                    match func with
+                    | LongIdentAsString ["Result"; "map"] ->
+                        allDoBindingsWithReturnIgnorePipe.Add (range, "Result.map ignore", "003")
+                    | LongIdentAsString ["TaskResult"; "map"] ->
+                        allDoBindingsWithReturnIgnorePipe.Add (range, "TaskResult.map ignore", "004")
+                    | _ -> ()
+                | _ -> ()
+            | _ -> ()
+        | SynExpr.DebugPoint(_, _, expr) -> addResultIgnore range expr
+        | SynExpr.DiscardAfterMissingQualificationAfterDot(expr, _, _) -> addResultIgnore range expr
+        | SynExpr.Downcast(expr, _, _) -> addResultIgnore range expr
+        | SynExpr.For(_, _, _, _, _, _, _, expr, _) -> addResultIgnore range expr
+        | SynExpr.ForEach(_, _, _, _, _, _, expr, _) -> addResultIgnore range expr
+        | SynExpr.FromParseError(expr, _) -> addResultIgnore range expr
+        | SynExpr.IfThenElse(_, expr1, expr2, _, _, _, _) ->
+            addResultIgnore range expr1; Option.iter (addResultIgnore range) expr2
+        | SynExpr.InferredDowncast(expr, _) -> addResultIgnore range expr
+        | SynExpr.InferredUpcast(expr, _) -> addResultIgnore range expr
+        | SynExpr.JoinIn(expr1, _, expr2, _) ->
+            addResultIgnore range expr1
+            addResultIgnore range expr2
+        | SynExpr.Lazy(expr, _) -> addResultIgnore range expr
+        | SynExpr.Match(_, _, clauses, _, _) ->
+            List.iter (fun clause ->
+                match clause with
+                | SynMatchClause(_, _, expr, _, _, _) ->
+                    addResultIgnore range expr
+            ) clauses
+        | SynExpr.Paren(expr, _, _, _) -> addResultIgnore range expr
+        | SynExpr.Sequential(_, _, expr1, expr2, _, _) ->
+            addResultIgnore range expr1; addResultIgnore range expr2
+        | SynExpr.TryFinally(expr1, expr2, _, _, _, _) ->
+            addResultIgnore range expr1; addResultIgnore range expr2
+        | SynExpr.TryWith(expr1, _, _, _, _, _) ->
+            addResultIgnore range expr1
+        |  _ -> ()
+
+        let walker = {
+            new SyntaxCollectorBase() with
+                override _.WalkExpr(_, expr) =
+                    match expr with
+                    | SynExpr.DoBang(expr, range, _) ->
+                        addResultIgnore range expr
+                    | _ -> ()
+        }
+
+        walkAst walker untypedTree
+        allDoBindingsWithReturnIgnorePipe |> Seq.toList
+
+    return
+        List.map (fun (range, func, code) ->
+            {
+                Type = name
+                Message = $"""%s{msgPattern func}"""
+                Code = codePattern code
+                Severity = Severity.Error
+                Range = range
+                Fixes = []
+            }
+        ) allIgnoredResultsInDo
+}
+
+[<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

src/FSharp.Analyzers/FSharp.Analyzers.fsproj

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

tests/FSharp.Analyzers.Tests/DoResultIgnoreAnalyzerTests.fs

@@ -0,0 +1,35 @@
+module Idura.FSharp.Analyzers.Tests.DoResultIgnoreAnalyzerTests
+
+open FSharp.Analyzers.SDK.Testing
+open TestHelpers
+
+open Xunit
+open Snapshooter
+open Snapshooter.Xunit
+
+open Idura.FSharp.Analyzers.DoResultIgnoreAnalyzer
+
+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=[|"doResultIgnore/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=[|"doResultIgnore/negative"|], MemberType=typeof<TestFiles>)>]
+let ``negative``(program : string, _: string) =
+  runNegativeTest setupContext cliAnalyzer program

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

@@ -14,6 +14,7 @@
     <Compile Include="DoNotUseYourOwnRandomAnalyzerTests.fs" />
     <Compile Include="DoubleWrappedResultAnalyzerTests.fs" />
     <Compile Include="LetWildcardResultAnalyzerTests.fs" />
+    <Compile Include="DoResultIgnoreAnalyzerTests.fs" />
   </ItemGroup>
 
   <ItemGroup>

tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_ResultIgnore.fs.snap

@@ -0,0 +1 @@
+[{"Code":"IDURA-DO-IGNORE-001","Fixes":[],"Message":"The pattern do! ... |\u003E Result.ignore is dangerous because it makes it easy to accidentally ignore errors.","Range":{"End":{"Line":17,"Column":35},"EndColumn":35,"EndLine":17,"FileName":"A.fs","Start":{"Line":17,"Column":4},"StartColumn":4,"StartLine":17},"Severity":{"Case":"Error"},"Type":"Do not ignore results in do! statements"}]

tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_ResultMapIgnore.fs.snap

@@ -0,0 +1 @@
+[{"Code":"IDURA-DO-IGNORE-003","Fixes":[],"Message":"The pattern do! ... |\u003E Result.map ignore is dangerous because it makes it easy to accidentally ignore errors.","Range":{"End":{"Line":17,"Column":39},"EndColumn":39,"EndLine":17,"FileName":"A.fs","Start":{"Line":17,"Column":4},"StartColumn":4,"StartLine":17},"Severity":{"Case":"Error"},"Type":"Do not ignore results in do! statements"}]

tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_TaskResultIgnore.fs.snap

@@ -0,0 +1 @@
+[{"Code":"IDURA-DO-IGNORE-002","Fixes":[],"Message":"The pattern do! ... |\u003E TaskResult.ignore is dangerous because it makes it easy to accidentally ignore errors.","Range":{"End":{"Line":17,"Column":39},"EndColumn":39,"EndLine":17,"FileName":"A.fs","Start":{"Line":17,"Column":4},"StartColumn":4,"StartLine":17},"Severity":{"Case":"Error"},"Type":"Do not ignore results in do! statements"}]

tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_TaskResultMapIgnore.fs.snap

@@ -0,0 +1 @@
+[{"Code":"IDURA-DO-IGNORE-004","Fixes":[],"Message":"The pattern do! ... |\u003E TaskResult.map ignore is dangerous because it makes it easy to accidentally ignore errors.","Range":{"End":{"Line":17,"Column":43},"EndColumn":43,"EndLine":17,"FileName":"A.fs","Start":{"Line":17,"Column":4},"StartColumn":4,"StartLine":17},"Severity":{"Case":"Error"},"Type":"Do not ignore results in do! statements"}]

tests/FSharp.Analyzers.Tests/data/doResultIgnore/negative/ResultIgnoreNormalDo.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"
+    do validate a |> Result.ignore
+    return ()
+}

tests/FSharp.Analyzers.Tests/data/doResultIgnore/negative/ResultMapIgnoreNormalDo.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"
+    do validate a |> Result.map ignore
+    return ()
+}

tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/ResultIgnore.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"
+    do! validate a |> Result.ignore
+    return ()
+}

tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/ResultMapIgnore.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"
+    do! validate a |> Result.map ignore
+    return ()
+}