From dba86069de0f7a7d7c97d40f4815f537f1e82d7d Mon Sep 17 00:00:00 2001 From: Frederik Krogsdal Jacobsen Date: Mon, 6 Oct 2025 15:45:55 +0200 Subject: [PATCH] Add analyzer for ignoring results in do!. This pattern is dangerous because it makes it easy to write very subtle bugs that ignore error handling. --- README.md | 72 ++++++++-- .../DoResultIgnoreAnalyzer.fs | 128 ++++++++++++++++++ src/FSharp.Analyzers/FSharp.Analyzers.fsproj | 1 + .../DoResultIgnoreAnalyzerTests.fs | 35 +++++ .../FSharp.Analyzers.Tests.fsproj | 1 + ...nalyzerTests.positive_ResultIgnore.fs.snap | 1 + ...yzerTests.positive_ResultMapIgnore.fs.snap | 1 + ...zerTests.positive_TaskResultIgnore.fs.snap | 1 + ...Tests.positive_TaskResultMapIgnore.fs.snap | 1 + .../negative/ResultIgnoreNormalDo.fs | 19 +++ .../negative/ResultMapIgnoreNormalDo.fs | 19 +++ .../doResultIgnore/positive/ResultIgnore.fs | 19 +++ .../positive/ResultMapIgnore.fs | 19 +++ .../positive/TaskResultIgnore.fs | 19 +++ .../positive/TaskResultMapIgnore.fs | 19 +++ 15 files changed, 344 insertions(+), 11 deletions(-) create mode 100644 src/FSharp.Analyzers/DoResultIgnoreAnalyzer.fs create mode 100644 tests/FSharp.Analyzers.Tests/DoResultIgnoreAnalyzerTests.fs create mode 100644 tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_ResultIgnore.fs.snap create mode 100644 tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_ResultMapIgnore.fs.snap create mode 100644 tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_TaskResultIgnore.fs.snap create mode 100644 tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_TaskResultMapIgnore.fs.snap create mode 100644 tests/FSharp.Analyzers.Tests/data/doResultIgnore/negative/ResultIgnoreNormalDo.fs create mode 100644 tests/FSharp.Analyzers.Tests/data/doResultIgnore/negative/ResultMapIgnoreNormalDo.fs create mode 100644 tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/ResultIgnore.fs create mode 100644 tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/ResultMapIgnore.fs create mode 100644 tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/TaskResultIgnore.fs create mode 100644 tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/TaskResultMapIgnore.fs diff --git a/README.md b/README.md index f767351..f478fa0 100644 --- a/README.md +++ b/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 | diff --git a/src/FSharp.Analyzers/DoResultIgnoreAnalyzer.fs b/src/FSharp.Analyzers/DoResultIgnoreAnalyzer.fs new file mode 100644 index 0000000..82f83c9 --- /dev/null +++ b/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}" + +[] +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 = async { + let allIgnoredResultsInDo = + let allDoBindingsWithReturnIgnorePipe = ResizeArray() + + // 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 +} + +[] +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/src/FSharp.Analyzers/FSharp.Analyzers.fsproj b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj index 3348f91..e67be41 100644 --- a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj +++ b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj @@ -37,6 +37,7 @@ + diff --git a/tests/FSharp.Analyzers.Tests/DoResultIgnoreAnalyzerTests.fs b/tests/FSharp.Analyzers.Tests/DoResultIgnoreAnalyzerTests.fs new file mode 100644 index 0000000..6f08a88 --- /dev/null +++ b/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 +} + +[] +[)>] +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/FSharp.Analyzers.Tests.fsproj b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj index 28e0241..2b9d990 100644 --- a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj +++ b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj @@ -14,6 +14,7 @@ + diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_ResultIgnore.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_ResultIgnore.fs.snap new file mode 100644 index 0000000..988e22f --- /dev/null +++ b/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"}] diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_ResultMapIgnore.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_ResultMapIgnore.fs.snap new file mode 100644 index 0000000..7222849 --- /dev/null +++ b/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"}] diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_TaskResultIgnore.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_TaskResultIgnore.fs.snap new file mode 100644 index 0000000..3ff0f12 --- /dev/null +++ b/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"}] diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_TaskResultMapIgnore.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/DoResultIgnoreAnalyzerTests.positive_TaskResultMapIgnore.fs.snap new file mode 100644 index 0000000..7374bfb --- /dev/null +++ b/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"}] diff --git a/tests/FSharp.Analyzers.Tests/data/doResultIgnore/negative/ResultIgnoreNormalDo.fs b/tests/FSharp.Analyzers.Tests/data/doResultIgnore/negative/ResultIgnoreNormalDo.fs new file mode 100644 index 0000000..b1a177e --- /dev/null +++ b/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 = result { + let a = "def" + do validate a |> Result.ignore + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doResultIgnore/negative/ResultMapIgnoreNormalDo.fs b/tests/FSharp.Analyzers.Tests/data/doResultIgnore/negative/ResultMapIgnoreNormalDo.fs new file mode 100644 index 0000000..e495ec8 --- /dev/null +++ b/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 = result { + let a = "def" + do validate a |> Result.map ignore + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/ResultIgnore.fs b/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/ResultIgnore.fs new file mode 100644 index 0000000..7b58ce9 --- /dev/null +++ b/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 = result { + let a = "def" + do! validate a |> Result.ignore + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/ResultMapIgnore.fs b/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/ResultMapIgnore.fs new file mode 100644 index 0000000..ea632f8 --- /dev/null +++ b/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 = result { + let a = "def" + do! validate a |> Result.map ignore + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/TaskResultIgnore.fs b/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/TaskResultIgnore.fs new file mode 100644 index 0000000..fb52e3e --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/TaskResultIgnore.fs @@ -0,0 +1,19 @@ +module M + +open FsToolkit.ErrorHandling + +let validateSubfunction (input : string) = + if (input <> "abc") then + TaskResult.error "Error" + else + TaskResult.ofResult (Result.Ok input) + +let validate (input : string) = taskResult { + return validateSubfunction input +} + +let main : TaskResult = taskResult { + let a = "def" + do! validate a |> TaskResult.ignore + return () +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/TaskResultMapIgnore.fs b/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/TaskResultMapIgnore.fs new file mode 100644 index 0000000..ee84786 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doResultIgnore/positive/TaskResultMapIgnore.fs @@ -0,0 +1,19 @@ +module M + +open FsToolkit.ErrorHandling + +let validateSubfunction (input : string) = + if (input <> "abc") then + TaskResult.error "Error" + else + TaskResult.ofResult (Result.Ok input) + +let validate (input : string) = taskResult { + return validateSubfunction input +} + +let main : TaskResult = taskResult { + let a = "def" + do! validate a |> TaskResult.map ignore + return () +} \ No newline at end of file