Add new rule FavourReRaise

Fixes #509

Author: su8898

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

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

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

@@ -0,0 +1,29 @@
+---
+title: FL0073
+category: how-to
+hide_menu: true
+---
+
+# FailwithBadUsage (FL0073)
+
+*Introduced in `0.20.3`*
+
+## Cause
+
+Rule to detect usage of `raise ex` instead of `reraise()` inside `try...with|ex` blocks.
+
+## Rationale
+
+Using `raise ex` to re-raise an exception is bad as it loses valuable stacktrace information from the original exception.
+
+## How To Fix
+
+Use `reraise()` instead of `raise ex` inside `try...with|ex` blocks.
+
+## Rule Settings
+
+    {
+        "favourReRaise": {
+           "enabled": true
+        }
+    }

src/FSharpLint.Core/FSharpLint.Core.fsproj

@@ -47,6 +47,7 @@
     <Compile Include="Rules\Conventions\NestedStatements.fs" />
     <Compile Include="Rules\Conventions\NoPartialFunctions.fs" />
     <Compile Include="Rules\Conventions\CyclomaticComplexity.fs" />
+    <Compile Include="Rules\Conventions\FavourReRaise.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\RaiseWithTooManyArgumentsHelper.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\FailwithWithSingleArgument.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\RaiseWithSingleArgument.fs" />

src/FSharpLint.Core/Rules/Conventions/FavourReRaise.fs

@@ -0,0 +1,51 @@
+module FSharpLint.Rules.FavourReRaise
+
+open System
+open FSharpLint.Framework
+open FSharpLint.Framework.Suggestion
+open FSharp.Compiler.Syntax
+open FSharpLint.Framework.Ast
+open FSharpLint.Framework.Rules
+
+let private runner (args: AstNodeRuleParams) =
+    let generateError range =
+        { Range = range
+          Message = Resources.GetString "RulesFavourReRaise"
+          SuggestedFix = None
+          TypeChecks = List.empty }
+        |> Array.singleton
+
+    let rec checkExpr (expr) maybeIdent =
+        match expr with
+        | SynExpr.App (_, _, SynExpr.Ident raiseId, expression, range) when raiseId.idText = "raise" ->
+            match expression with
+            | SynExpr.Ident ident ->
+                match maybeIdent with
+                | Some id when id = ident.idText ->
+                    generateError range
+                | _ -> Array.empty
+            | SynExpr.LongIdent (_, LongIdentWithDots (id, _), _, range) -> generateError range
+            | _ -> Array.empty
+        | SynExpr.TryWith (expressions, _, clauseList, _expression, _range, _, _) as expr ->
+            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)
+        | SynExpr.IfThenElse (_, expr, _, _, _, _, _) -> checkExpr expr maybeIdent
+        | _ -> Array.empty
+
+    match args.AstNode with
+    | AstNode.Expression expr -> checkExpr expr None
+    | _ -> Array.empty
+
+let rule =
+    { Name = "FavourReRaise"
+      Identifier = Identifiers.FavourReRaise
+      RuleConfig =
+        { AstNodeRuleConfig.Runner = runner
+          Cleanup = ignore } }
+    |> AstNodeRule

src/FSharpLint.Core/Rules/Identifiers.fs

@@ -77,3 +77,4 @@ let GenericTypesNames = identifier 69
 let FavourTypedIgnore = identifier 70
 let CyclomaticComplexity = identifier 71
 let FailwithBadUsage = identifier 72
+let FavourReRaise = identifier 73

src/FSharpLint.Core/Text.resx

@@ -327,4 +327,7 @@
   <data name="RulesFailwithBadUsage" xml:space="preserve">
     <value>Bad usage of failwith: {0}.</value>
   </data>
+  <data name="RulesFavourReRaise" xml:space="preserve">
+    <value>Rather use reraise() instead of using raise with the same captured exception.</value>
+  </data>
 </root>

src/FSharpLint.Core/fsharplint.json

@@ -265,6 +265,7 @@
     "uselessBinding": { "enabled": true },
     "tupleOfWildcards": { "enabled": true },
     "favourTypedIgnore": { "enabled": false },
+    "favourReRaise": { "enabled": true },
     "indentation": {
         "enabled": false
     },

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

@@ -35,6 +35,7 @@
     <Compile Include="Rules\Conventions\FunctionReimplementation.fs" />
     <Compile Include="Rules\Conventions\SourceLength.fs" />
     <Compile Include="Rules\Conventions\NoPartialFunctions.fs" />
+    <Compile Include="Rules\Conventions\FavourReRaise.fs" />
     <Compile Include="Rules\Conventions\Naming\NamingHelpers.fs" />
     <Compile Include="Rules\Conventions\Naming\InterfaceNames.fs" />
     <Compile Include="Rules\Conventions\Naming\ExceptionNames.fs" />

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

@@ -0,0 +1,91 @@
+module FSharpLint.Core.Tests.Rules.Conventions.FavourReRaise
+
+open NUnit.Framework
+open FSharpLint.Rules
+
+[<TestFixture>]
+type TestBindingFavourReRaise() =
+    inherit TestAstNodeRuleBase.TestAstNodeRuleBase(FavourReRaise.rule)
+
+    [<Test>]
+    member this.``favour reraise() instead of raise``() =
+        this.Parse
+            """
+try
+    foo ()
+with
+| ex ->
+    if someCondition then
+        reraise() """
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``using raise ex must generate error``() =
+        this.Parse
+            """
+ try
+     foo ()
+ with
+ | ex ->
+     if someCondition then
+         raise ex """
+
+        Assert.IsTrue(this.ErrorsExist)
+        Assert.IsTrue(this.ErrorExistsAt(7, 9))
+
+    [<Test>]
+    member this.``using raise outside with block must not generate error``() =
+        this.Parse
+            """
+let function1 x y =
+   try
+     try
+        if x = y then raise (InnerError("inner"))
+        else raise (OuterError("outer"))
+     with
+      | InnerError(str) -> printfn "Error1 %s" str
+   finally
+      printfn "Always print this." """
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``using raise with an exception that's not in the with block must not generate error``() =
+        this.Parse
+            """
+try
+    foo bar
+with
+| ex ->
+    let e = ex.InnerException
+    if null <> e then
+        raise e """
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``using raise with an exception that's not in the with block must not generate error (2)``() =
+        this.Parse
+            """
+try
+    foo bar
+with
+| ex ->
+    let e = Exception("baz", ex)
+    raise e """
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``using raise with an exception that's not in the with block must not generate error (3)``() =
+        this.Parse
+            """
+try
+    foo bar
+with
+| e ->
+    let ex = Exception("baz", e)
+    raise ex """
+
+        this.AssertNoWarnings()