diff --git a/Directory.Packages.props b/Directory.Packages.props index 134e1a6..50a3113 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -15,5 +15,6 @@ + \ No newline at end of file diff --git a/README.md b/README.md index db56dbe..9d4827e 100644 --- a/README.md +++ b/README.md @@ -87,4 +87,17 @@ 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 | + +### 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 | \ No newline at end of file diff --git a/src/FSharp.Analyzers/DoubleWrappedResultAnalyzer.fs b/src/FSharp.Analyzers/DoubleWrappedResultAnalyzer.fs new file mode 100644 index 0000000..048db42 --- /dev/null +++ b/src/FSharp.Analyzers/DoubleWrappedResultAnalyzer.fs @@ -0,0 +1,99 @@ +module Idura.FSharp.Analyzers.DoubleWrappedResultAnalyzer + +open FSharp.Analyzers.SDK +open FSharp.Analyzers.SDK.TASTCollecting +open FSharp.Compiler.Text +open FSharp.Compiler.Syntax +open FSharp.Compiler.CodeAnalysis +open FSharp.Compiler.Symbols + +[] +let code = "IDURA-RESULT-006" + +[] +let name = "Did you mean to wrap this value in Result twice?" + +[] +let msg = "Double-wrapping values in Result is often caused by accidentally ignoring an error" + +let private analyzer + (sourceText: ISourceText) + (_: ParsedInput) + (typedResults: FSharp.Compiler.Symbols.FSharpImplementationFileContents) : Async = async { + let allDoubleWrappedResultBindings = + let allBindingsWithDoubleResult = ResizeArray() + + let (|LongIdentAsString|) (lid: SynLongIdent) = + lid.LongIdent |> List.map (fun ident -> ident.idText) + + // If the type is a function, this recurses until it finds the "final" return type of the function + let rec getReturnType (t: FSharpType) = + if t.IsFunctionType then + let rangeType = t.GenericArguments[1] + getReturnType rangeType + else + t + + let RESULT_TYPE_NAME = typedefof>.FullName + + // FUTURE: In version 10.0.300 of FSharp.Compiler.Services, the BasicQualifiedName becomes an option + // which should allow us to remove the "try ... with" from this function + let isDoubleResult (t: FSharpType) = + let returnType = getReturnType t + try + if returnType.BasicQualifiedName = RESULT_TYPE_NAME then + let okType = returnType.GenericArguments[0] + okType.BasicQualifiedName = RESULT_TYPE_NAME + else + false + with + | :? System.InvalidOperationException -> + // This happens if the type does not have a name, e.g. if it is a tuple, type parameter, anonymous record, etc. + false + + let walker: TypedTreeCollectorBase = { + new TypedTreeCollectorBase() with + override _.WalkLet (var: FSharpMemberOrFunctionOrValue) expr body = + match var.FullTypeSafe with + | None -> () + | Some t -> + if isDoubleResult t then + allBindingsWithDoubleResult.Add(var.DisplayName, var.DeclarationLocation) + override _.WalkMemberOrFunctionOrValue (mfv: FSharpMemberOrFunctionOrValue) _ _ = + match mfv.FullTypeSafe with + | None -> () + | Some t -> + if isDoubleResult t then + allBindingsWithDoubleResult.Add(mfv.DisplayName, mfv.DeclarationLocation) + + } + + walkTast walker typedResults + allBindingsWithDoubleResult |> Seq.toList + + return + List.map (fun (ident: string, range) -> + { + Type = name + Message = $"""%s{msg}: %s{ident}""" + Code = code + Severity = Severity.Warning + Range = range + Fixes = [] + } + ) allDoubleWrappedResultBindings +} + +[] +let cliAnalyzer (ctx: CliContext) : Async = + match ctx.TypedTree with + | None -> async.Return [] + | Some tast -> + analyzer ctx.SourceText ctx.ParseFileResults.ParseTree tast + +[] +let editorAnalyzer (ctx: EditorContext) : Async = + match ctx.TypedTree with + | None -> async.Return [] + | Some tast -> + analyzer ctx.SourceText ctx.ParseFileResults.ParseTree tast \ No newline at end of file diff --git a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj index 448c8ca..3acc5d8 100644 --- a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj +++ b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj @@ -29,12 +29,13 @@ - + + diff --git a/tests/FSharp.Analyzers.Tests/DoubleWrappedResultAnalyzerTests.fs b/tests/FSharp.Analyzers.Tests/DoubleWrappedResultAnalyzerTests.fs new file mode 100644 index 0000000..4990b8a --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/DoubleWrappedResultAnalyzerTests.fs @@ -0,0 +1,44 @@ +module Idura.FSharp.Analyzers.Tests.DoubleWrappedResultAnalyzer + +open FSharp.Analyzers.SDK.Testing +open TestHelpers + +open Xunit +open Snapshooter +open Snapshooter.Xunit + +open Idura.FSharp.Analyzers.DoubleWrappedResultAnalyzer + +let setupContext () = async { + let! opts = + mkOptionsFromProject + "net9.0" + [ + { + Name = "FsToolkit.Errorhandling" + Version = "5.1.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 + +[] +let ``gives three warnings for TripleResult``() = async { + let! opts = setupContext() + let program = TestFiles.GetSource "doubleWrappedResult/positive/TripleResult.fs" + let ctx = getContext opts program + let! msgs = cliAnalyzer ctx + Assert.True(msgs.Length = 3) +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj index c757052..21d3d04 100644 --- a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj +++ b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj @@ -12,6 +12,7 @@ + diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_Basic.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_Basic.fs.snap new file mode 100644 index 0000000..ee0cd31 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_Basic.fs.snap @@ -0,0 +1 @@ +[{"Code":"IDURA-RESULT-006","Fixes":[],"Message":"Double-wrapping values in Result is often caused by accidentally ignoring an error: main","Range":{"End":{"Line":15,"Column":8},"EndColumn":8,"EndLine":15,"FileName":"A.fs","Start":{"Line":15,"Column":4},"StartColumn":4,"StartLine":15},"Severity":{"Case":"Warning"},"Type":"Did you mean to wrap this value in Result twice?"}] diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_LetBindingInFunction.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_LetBindingInFunction.fs.snap new file mode 100644 index 0000000..f4641a4 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_LetBindingInFunction.fs.snap @@ -0,0 +1 @@ +[{"Code":"IDURA-RESULT-006","Fixes":[],"Message":"Double-wrapping values in Result is often caused by accidentally ignoring an error: validate2","Range":{"End":{"Line":20,"Column":17},"EndColumn":17,"EndLine":20,"FileName":"A.fs","Start":{"Line":20,"Column":8},"StartColumn":8,"StartLine":20},"Severity":{"Case":"Warning"},"Type":"Did you mean to wrap this value in Result twice?"}] diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_TripleResult.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_TripleResult.fs.snap new file mode 100644 index 0000000..b2feaab --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_TripleResult.fs.snap @@ -0,0 +1 @@ +[{"Code":"IDURA-RESULT-006","Fixes":[],"Message":"Double-wrapping values in Result is often caused by accidentally ignoring an error: validate2","Range":{"End":{"Line":15,"Column":13},"EndColumn":13,"EndLine":15,"FileName":"A.fs","Start":{"Line":15,"Column":4},"StartColumn":4,"StartLine":15},"Severity":{"Case":"Warning"},"Type":"Did you mean to wrap this value in Result twice?"},{"Code":"IDURA-RESULT-006","Fixes":[],"Message":"Double-wrapping values in Result is often caused by accidentally ignoring an error: main","Range":{"End":{"Line":19,"Column":8},"EndColumn":8,"EndLine":19,"FileName":"A.fs","Start":{"Line":19,"Column":4},"StartColumn":4,"StartLine":19},"Severity":{"Case":"Warning"},"Type":"Did you mean to wrap this value in Result twice?"},{"Code":"IDURA-RESULT-006","Fixes":[],"Message":"Double-wrapping values in Result is often caused by accidentally ignoring an error: c","Range":{"End":{"Line":22,"Column":9},"EndColumn":9,"EndLine":22,"FileName":"A.fs","Start":{"Line":22,"Column":8},"StartColumn":8,"StartLine":22},"Severity":{"Case":"Warning"},"Type":"Did you mean to wrap this value in Result twice?"}] diff --git a/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_WithoutTypeAnnotation.fs.snap b/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_WithoutTypeAnnotation.fs.snap new file mode 100644 index 0000000..ee0cd31 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/__snapshots__/DoubleWrappedResultAnalyzer.positive_WithoutTypeAnnotation.fs.snap @@ -0,0 +1 @@ +[{"Code":"IDURA-RESULT-006","Fixes":[],"Message":"Double-wrapping values in Result is often caused by accidentally ignoring an error: main","Range":{"End":{"Line":15,"Column":8},"EndColumn":8,"EndLine":15,"FileName":"A.fs","Start":{"Line":15,"Column":4},"StartColumn":4,"StartLine":15},"Severity":{"Case":"Warning"},"Type":"Did you mean to wrap this value in Result twice?"}] diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnArrayTypes.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnArrayTypes.fs new file mode 100644 index 0000000..47c0c84 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnArrayTypes.fs @@ -0,0 +1,7 @@ +module M + +let main (input : string array) : string = + if input.Length > 0 then + input[0] + else + "none" \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnTuples.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnTuples.fs new file mode 100644 index 0000000..544f129 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnTuples.fs @@ -0,0 +1,4 @@ +module M + +let main : (string * string) = + "abc", "def" \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnTypeParameters.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnTypeParameters.fs new file mode 100644 index 0000000..cf495a4 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/DoesNotCrashOnTypeParameters.fs @@ -0,0 +1,4 @@ +module M + +let main (input : 'a) : 'a = + input \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/IgnoreSingleResult.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/IgnoreSingleResult.fs new file mode 100644 index 0000000..225f3a9 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/IgnoreSingleResult.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 b +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/IgnoreSingleResultWithoutTypeAnnotation.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/IgnoreSingleResultWithoutTypeAnnotation.fs new file mode 100644 index 0000000..d033edf --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/negative/IgnoreSingleResultWithoutTypeAnnotation.fs @@ -0,0 +1,22 @@ +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 { + let a = "def" + let! b = validate a + if true then + return b + else + return! Result.Error "check it out" +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/Basic.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/Basic.fs new file mode 100644 index 0000000..5fbead7 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/Basic.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, string> = result { + let a = "def" + let b = validate a + return b +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/LetBindingInFunction.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/LetBindingInFunction.fs new file mode 100644 index 0000000..91fa352 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/LetBindingInFunction.fs @@ -0,0 +1,25 @@ +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 + + // This should give a warning + let validate2 input = result { + return Result.bind validate input + } + + return! b +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/TripleResult.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/TripleResult.fs new file mode 100644 index 0000000..c05047b --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/TripleResult.fs @@ -0,0 +1,24 @@ +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 validate2 input = result { + return Result.bind validate input +} + +let main : Result, string>, string> = result { + let a = "def" + let b = validate a + let c = validate2 b + return c +} \ No newline at end of file diff --git a/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/WithoutTypeAnnotation.fs b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/WithoutTypeAnnotation.fs new file mode 100644 index 0000000..c1625d6 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/data/doubleWrappedResult/positive/WithoutTypeAnnotation.fs @@ -0,0 +1,22 @@ +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 { + let a = "def" + let b = validate a + if true then + return b + else + return! Result.Error "check it out" +} \ No newline at end of file