Add new rule FailwithBadUsage (#515)

Fixes #508

Author: su8898

docs/content/how-tos/rule-configuration.md

@@ -111,4 +111,5 @@ The following rules can be specified for linting.
 - [InternalValuesNames (FL0068)](rules/FL0068.html)
 - [GenericTypesNames (FL0069)](rules/FL0069.html)
 - [FavourTypedIgnore (FL0070)](rules/FL0070.html)
-- [CyclomaticComplexity (FL0071)](rules/FL0071.html)
+- [CyclomaticComplexity (FL0071)](rules/FL0071.html)
+- [FailwithBadUsage (FL0072)](rules/FL0072.html)

docs/content/how-tos/rules/FL0072.md

@@ -0,0 +1,31 @@
+---
+title: FL0072
+category: how-to
+hide_menu: true
+---
+
+# FailwithBadUsage (FL0072)
+
+*Introduced in `0.20.3`*
+
+## Cause
+
+Rule to detect bad usage of failwith.
+
+## Rationale
+
+Passing empty strings or duplicate messages to failwith statements makes it much harder to understand/locate & subsequently fix bugs. 
+It's also true when exceptions are swallowed in try...with blocks.
+
+## How To Fix
+
+Do not pass an empty string or a duplicate message to failwith. 
+Instead of swallowing exception messages in try...with blocks, pass the exception as an innerException parameter of a new Exception.
+
+## Rule Settings
+
+    {
+        "failwithBadUsage": {
+           "enabled": true
+        }
+    }

src/FSharpLint.Core/FSharpLint.Core.fsproj

@@ -54,6 +54,7 @@
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\InvalidOpWithSingleArgument.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\InvalidArgWithTwoArguments.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\FailwithfWithArgumentsMatchingFormatString.fs" />
+    <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\FailwithBadUsage.fs" />
     <Compile Include="Rules\Conventions\SourceLength\SourceLengthHelper.fs" />
     <Compile Include="Rules\Conventions\SourceLength\MaxLinesInLambdaFunction.fs" />
     <Compile Include="Rules\Conventions\SourceLength\MaxLinesInMatchLambdaFunction.fs" />

src/FSharpLint.Core/Rules/Conventions/RaiseWithTooManyArguments/FailwithBadUsage.fs

@@ -0,0 +1,107 @@
+module FSharpLint.Rules.FailwithBadUsage
+
+open FSharpLint.Framework
+open FSharpLint.Framework.Suggestion
+open FSharp.Compiler.Syntax
+open FSharpLint.Framework.Ast
+open FSharpLint.Framework.Rules
+open System
+open System.Collections.Generic
+
+let mutable failwithErrorMessageList = Set.empty
+
+type private BadUsageType =
+    | EmptyMessage
+    | DuplicateMessage
+    | SwallowedException
+
+let private runner (args: AstNodeRuleParams) =
+    let generateError
+        failwithKeyword
+        failwithErrorMessage
+        range
+        (badUsageType: BadUsageType)
+        (exceptionParam: Option<string>)
+        =
+        let suggestedFix =
+            match exceptionParam with
+            | Some param ->
+                Some(
+                    lazy
+                        (Some
+                            { FromText = sprintf "%s %s" failwithKeyword failwithErrorMessage
+                              FromRange = range
+                              ToText = sprintf "raise <| Exception(\"%s\", %s)" failwithErrorMessage param })
+                )
+            | _ -> None
+
+        let message =
+            match badUsageType with
+            | EmptyMessage -> "Consider using non-empty error messages with failwith"
+            | DuplicateMessage -> "Consider using unique error messages with failwith"
+            | SwallowedException ->
+                "failwith must not swallow exception details with it, rather use raise passing the current exception as innerException (2nd parameter of Exception constructor)"
+
+        let error =
+            { Range = range
+              Message = String.Format(Resources.GetString "RulesFailwithBadUsage", message)
+              SuggestedFix = suggestedFix
+              TypeChecks = List.Empty }
+            |> Array.singleton
+
+        error
+
+    let rec checkExpr node maybeIdentifier =
+        match node with
+        | SynExpr.App (_, _, SynExpr.Ident failwithId, expression, range) when
+            failwithId.idText = "failwith"
+            || failwithId.idText = "failwithf"
+            ->
+            match expression with
+            | SynExpr.Const (SynConst.String (id, _, _), _) when id = "" ->
+                generateError failwithId.idText id range BadUsageType.EmptyMessage maybeIdentifier
+            | SynExpr.Const (SynConst.String (id, _, _), _) ->
+                if Set.contains id failwithErrorMessageList then
+                    generateError failwithId.idText id range BadUsageType.DuplicateMessage maybeIdentifier
+                else
+                    match maybeIdentifier with
+                    | Some maybeId ->
+                        generateError failwithId.idText id range BadUsageType.SwallowedException (Some maybeId)
+                    | _ ->
+                        failwithErrorMessageList <- failwithErrorMessageList.Add(id)
+                        Array.empty
+            | SynExpr.LongIdent (_, LongIdentWithDots (id, _), _, _) when
+                (ExpressionUtilities.longIdentToString id) = "String.Empty"
+                ->
+                generateError
+                    failwithId.idText
+                    (ExpressionUtilities.longIdentToString id)
+                    range
+                    (BadUsageType.EmptyMessage)
+                    (None)
+            | _ -> Array.empty
+        | SynExpr.TryWith (_, _, clauseList, _expression, _range, _, _) ->
+            clauseList
+            |> List.toArray
+            |> Array.collect (fun clause ->
+                match clause with
+                | SynMatchClause (pat, _, app, _, _) ->
+                    match pat with
+                    | SynPat.Named (_, id, _, _, _) -> checkExpr app (Some id.idText)
+                    | _ -> checkExpr app None)
+        | _ -> Array.empty
+        | _ -> Array.empty
+
+    match args.AstNode with
+    | AstNode.Expression expr -> checkExpr expr None
+    | _ -> Array.empty
+
+let cleanup () = failwithErrorMessageList <- Set.empty
+
+let rule =
+    { Name = "FailwithBadUsage"
+      Identifier = Identifiers.FailwithBadUsage
+      RuleConfig =
+        { AstNodeRuleConfig.Runner = runner
+          Cleanup = cleanup } }
+    |> AstNodeRule

src/FSharpLint.Core/Rules/Identifiers.fs

@@ -76,3 +76,4 @@ let InternalValuesNames = identifier 68
 let GenericTypesNames = identifier 69
 let FavourTypedIgnore = identifier 70
 let CyclomaticComplexity = identifier 71
+let FailwithBadUsage = identifier 72

src/FSharpLint.Core/Text.resx

@@ -324,4 +324,7 @@
   <data name="RulesFavourTypedIgnore" xml:space="preserve">
     <value>Use a generic type parameter when calling the 'ignore' function.</value>
   </data>
+  <data name="RulesFailwithBadUsage" xml:space="preserve">
+    <value>Bad usage of failwith: {0}.</value>
+  </data>
 </root>

src/FSharpLint.Core/fsharplint.json

@@ -44,6 +44,7 @@
     "invalidOpWithSingleArgument": { "enabled": true },
     "invalidArgWithTwoArguments": { "enabled": true },
     "failwithfWithArgumentsMatchingFormatString": { "enabled": true },
+    "failwithBadUsage": { "enabled": true },
     "maxLinesInLambdaFunction": {
         "enabled": false,
         "config": {

tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj

@@ -29,6 +29,7 @@
     <Compile Include="Rules\Conventions\RecursiveAsyncFunction.fs" />
     <Compile Include="Rules\Conventions\RedundantNewKeyword.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments.fs" />
+    <Compile Include="Rules\Conventions\FailwithBadUsage.fs" />
     <Compile Include="Rules\Conventions\NestedStatements.fs" />
     <Compile Include="Rules\Conventions\CyclomaticComplexity.fs" />
     <Compile Include="Rules\Conventions\FunctionReimplementation.fs" />

tests/FSharpLint.Core.Tests/Rules/Conventions/FailwithBadUsage.fs

@@ -0,0 +1,118 @@
+module FSharpLint.Core.Tests.Rules.Conventions.FailwithBadUsage
+
+open NUnit.Framework
+open FSharpLint.Rules
+open System
+
+[<TestFixture>]
+type TestConventionsFailwithBadUsage() =
+    inherit TestAstNodeRuleBase.TestAstNodeRuleBase(FailwithBadUsage.rule)
+
+    [<Test>]
+    member this.FailwithWithBadArgumentsEmptyMessage() =
+        this.Parse """
+let foo () =
+    failwith "" """
+        Assert.IsTrue this.ErrorsExist
+        Assert.IsTrue(this.ErrorExistsAt(3, 4))
+
+    [<Test>]
+    member this.FailwithWithBadArgumentsEmptyMessage2() =
+        this.Parse """
+let bar () =
+    failwith String.Empty"""
+
+        Assert.IsTrue this.ErrorsExist
+        Assert.IsTrue(this.ErrorExistsAt(3, 4))
+
+    [<Test>]
+    member this.FailwithWithBadArgumentsEmptyMessage3() =
+        this.Parse """
+let foo () =
+    failwith "foo"
+let bar () =
+    failwith String.Empty"""
+
+        Assert.IsTrue this.ErrorsExist
+        Assert.IsTrue(this.ErrorExistsAt(5, 4))
+
+    [<Test>]
+    member this.FailwithWithGoodArguments() =
+        this.Parse """
+let foo () =
+    failwith "foo"
+let bar () =
+    failwith "bar" """
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.FailwithWithGoodArguments2() =
+        this.Parse """
+let foo () =
+    failwith "foo" """
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.FailwithWithBadArgumentsDuplicateExceptionMessage() =
+        this.Parse """
+let foo () =
+    failwith "foobar"
+let bar () =
+    failwith "foobar" """
+
+        Assert.IsTrue this.ErrorsExist
+        Assert.IsTrue(this.ErrorExistsAt(5, 4))
+
+    [<Test>]
+    member this.FailwithShouldNotSwallowExceptions() =
+        this.Parse """
+try
+    foo()
+with
+| e ->
+    failwith "bar" """
+
+        Assert.IsTrue this.ErrorsExist
+        Assert.IsTrue(this.ErrorExistsAt(6, 4))
+
+    [<Test>]
+    member this.FailwithWithProperExceptions() =
+        this.Parse """
+try
+    foo()
+with
+| e ->
+    raise new Exception("bar",e) """
+
+        Assert.IsTrue this.NoErrorsExist
+
+    [<Test>]
+    member this.FailwithfShouldNotSwallowExceptions() =
+        this.Parse """
+try
+    foo()
+with
+| e ->
+    failwithf "bar" """
+
+        Assert.IsTrue this.ErrorsExist
+        Assert.IsTrue(this.ErrorExistsAt(6, 4))
+
+    [<Test>]
+    member this.FailwithWithfGoodArguments2() =
+        this.Parse """
+let foo () =
+    failwithf "foo" """
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.FailwithfWithBadArgumentsEmptyMessage3() =
+        this.Parse """
+let foo () =
+    failwithf String.Empty"""
+
+        Assert.IsTrue this.ErrorsExist
+        Assert.IsTrue(this.ErrorExistsAt(3, 4))