Merge PR #646 from webwarrior-ws/rule-suggestions

Add quickfix suggestions for several rules.

Author: knocte

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

@@ -9,23 +9,23 @@ open FSharpLint.Framework.Ast
 open FSharpLint.Framework.Rules
 
 let runner (args: AstNodeRuleParams) =
-    let errors range =
+    let errors range suggestedFix =
         {
             Range = range
             Message = String.Format(Resources.GetString ("RulesAvoidSinglePipeOperator"))
-            SuggestedFix = None
+            SuggestedFix = suggestedFix
             TypeChecks = List.Empty
         } |> Array.singleton
 
-    let rec checkExpr (expr: SynExpr) (parentList: AstNode list): WarningDetails array =
+    let rec checkExpr (expr: SynExpr) (outerArgExpr: SynExpr) (range: FSharp.Compiler.Text.range) (parentList: AstNode list): WarningDetails array =
         let checkParentPiped (expr: AstNode) =
             match expr with
             | AstNode.Expression(SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, _argExpr, _range)) ->
-                checkExpr funcExpr [] |> Seq.isEmpty
+                checkExpr funcExpr outerArgExpr range [] |> Seq.isEmpty
             | _ -> false
 
         match expr with
-        | SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, argExpr, _range) ->
+        | SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, argExpr, appRange) ->
             match funcExpr with
             | ExpressionUtilities.Identifier([ ident ], _) ->
                 if ident.idText = "op_PipeRight" then
@@ -42,7 +42,15 @@ let runner (args: AstNodeRuleParams) =
                         if isParentPiped then
                             Array.empty
                         else
-                            errors ident.idRange
+                            let suggestedFix = lazy(
+                                let maybeFuncText = ExpressionUtilities.tryFindTextOfRange outerArgExpr.Range args.FileContent
+                                let maybeArgText = ExpressionUtilities.tryFindTextOfRange argExpr.Range args.FileContent
+                                match maybeFuncText, maybeArgText with
+                                | Some(funcText), Some(argText) ->
+                                    let replacementText = sprintf "%s %s" funcText argText
+                                    Some { FromText=args.FileContent; FromRange=range; ToText=replacementText }
+                                | _ -> None)
+                            errors ident.idRange (Some suggestedFix)
                 else
                     Array.empty
             | _ ->
@@ -52,13 +60,13 @@ let runner (args: AstNodeRuleParams) =
 
     let error =
         match args.AstNode with
-        | AstNode.Expression(SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, argExpr, _range)) ->
+        | AstNode.Expression(SynExpr.App(_exprAtomicFlag, _isInfix, funcExpr, argExpr, range)) ->
             match argExpr with
             | SynExpr.App(_) ->
                 // function has extra arguments
                 Array.empty
             | _ ->
-                checkExpr funcExpr (args.GetParents args.NodeIndex)
+                checkExpr funcExpr argExpr range (args.GetParents args.NodeIndex)
         | _ ->
             Array.empty
 

src/FSharpLint.Core/Rules/Conventions/Binding/TupleOfWildcards.fs

@@ -7,7 +7,7 @@ open FSharpLint.Framework.Suggestion
 open FSharpLint.Framework.Ast
 open FSharpLint.Framework.Rules
 
-let private checkTupleOfWildcards pattern identifier =
+let private checkTupleOfWildcards fileContents pattern identifier identifierRange =
     let rec isWildcard = function
         | SynPat.Paren(pattern, _) -> isWildcard pattern
         | SynPat.Wild(_) -> true
@@ -16,15 +16,20 @@ let private checkTupleOfWildcards pattern identifier =
     let constructorString numberOfWildcards =
         let constructorName = identifier |> String.concat "."
         let arguments = Array.create numberOfWildcards "_" |> String.concat ", "
-        $"{constructorName}({arguments})"
+        if numberOfWildcards = 1 then
+            $"%s{constructorName} _"
+        else
+            $"%s{constructorName}(%s{arguments})"
 
     match pattern with
     | SynPat.Tuple(_isStruct, patterns, _, range) when List.length patterns > 1 && patterns |> List.forall isWildcard ->
         let errorFormat = Resources.GetString("RulesTupleOfWildcardsError")
         let refactorFrom = constructorString (List.length patterns)
         let refactorTo = constructorString 1
         let error = String.Format(errorFormat, refactorFrom, refactorTo)
-        { Range = range; Message = error; SuggestedFix = None; TypeChecks = [] } |> Array.singleton
+        let suggestedFix = lazy(
+            Some { SuggestedFix.FromRange = identifierRange; FromText = fileContents; ToText = refactorTo })
+        { Range = range; Message = error; SuggestedFix = Some suggestedFix; TypeChecks = [] } |> Array.singleton
     | _ -> Array.empty
 
 let private isTupleMemberArgs breadcrumbs tupleRange =
@@ -44,11 +49,11 @@ let private isTupleMemberArgs breadcrumbs tupleRange =
 
 let private runner (args:AstNodeRuleParams) =
     match args.AstNode with
-    | AstNode.Pattern(SynPat.LongIdent(identifier, _, _, SynArgPats.Pats([SynPat.Paren(SynPat.Tuple(_, _, _, range) as pattern, _)]), _, _)) ->
+    | AstNode.Pattern(SynPat.LongIdent(identifier, _, _, SynArgPats.Pats([SynPat.Paren(SynPat.Tuple(_, _, _, range) as pattern, _)]), _, identRange)) ->
         let breadcrumbs = args.GetParents 2
         if (not << isTupleMemberArgs breadcrumbs) range then
             let identifier = identifier.LongIdent |> List.map (fun x -> x.idText)
-            checkTupleOfWildcards pattern identifier
+            checkTupleOfWildcards args.FileContent pattern identifier identRange
         else
             Array.empty
     | _ -> Array.empty

src/FSharpLint.Core/Rules/Conventions/Binding/WildcardNamedWithAsPattern.fs

@@ -7,19 +7,23 @@ open FSharp.Compiler.Syntax
 open FSharpLint.Framework.Ast
 open FSharpLint.Framework.Rules
 
-let private checkForWildcardNamedWithAsPattern pattern =
+let private checkForWildcardNamedWithAsPattern fileContents pattern =
     match pattern with
-    | SynPat.Wild(range) ->
+    | SynPat.As(SynPat.Wild(wildcardRange), SynPat.Named(SynIdent(identifier, _), _, _, _), range)
+        when wildcardRange <> range ->
+        let suggestedFix = 
+            lazy(
+                Some { FromRange = range; FromText = fileContents; ToText = identifier.idText })
         { Range = range
           Message = Resources.GetString("RulesWildcardNamedWithAsPattern")
-          SuggestedFix = None
+          SuggestedFix = Some suggestedFix
           TypeChecks = [] } |> Array.singleton
     | _ -> Array.empty
 
 let private runner (args:AstNodeRuleParams) =
     match args.AstNode with
-    | AstNode.Pattern(SynPat.As(leftHandSide, _, _)) ->
-        checkForWildcardNamedWithAsPattern leftHandSide
+    | AstNode.Pattern(SynPat.As(SynPat.Wild(_), SynPat.Named(_), _) as pattern) ->
+        checkForWildcardNamedWithAsPattern args.FileContent pattern
     | _ -> Array.empty
 
 let rule =

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

@@ -24,21 +24,28 @@ let private getStaticEmptyErrorMessage  (range:FSharp.Compiler.Text.Range) (empt
 
     errorMessageKey |> formatError
 
-let private generateError (range:FSharp.Compiler.Text.Range) (emptyLiteralType: EmptyLiteralType) =
+let private generateError (fileContents: string) (range:FSharp.Compiler.Text.Range) (emptyLiteralType: EmptyLiteralType) =
+    let suggestedFix = lazy(
+        let replacementText =
+            match emptyLiteralType with
+            | EmptyStringLiteral -> "String.Empty"
+            | EmptyListLiteral -> "List.Empty"
+            | EmptyArrayLiteral -> "Array.empty"
+        Some({ FromRange = range; FromText = fileContents; ToText = replacementText }))
     { Range = range
       Message = getStaticEmptyErrorMessage range emptyLiteralType
-      SuggestedFix = None
+      SuggestedFix = Some suggestedFix
       TypeChecks = List.Empty }
     |> Array.singleton
 
 let private runner (args: AstNodeRuleParams) =
     match args.AstNode with
     | AstNode.Expression(SynExpr.Const (SynConst.String ("", _, range), _)) -> 
-        generateError range EmptyStringLiteral
+        generateError args.FileContent range EmptyStringLiteral
     | AstNode.Expression(SynExpr.ArrayOrList (isArray, [], range)) ->
         let emptyLiteralType =
             if isArray then EmptyArrayLiteral else EmptyListLiteral
-        generateError range emptyLiteralType
+        generateError args.FileContent range emptyLiteralType
     | AstNode.Expression(SynExpr.Record(_, _, synExprRecordField, _)) ->
         synExprRecordField
         |> List.map (fun field ->
@@ -47,11 +54,11 @@ let private runner (args: AstNodeRuleParams) =
                 match expr with
                 | Some(SynExpr.ArrayOrList(isArray, [], range)) ->
                     let emptyLiteralType = if isArray then EmptyArrayLiteral else EmptyListLiteral
-                    generateError range emptyLiteralType
+                    generateError args.FileContent range emptyLiteralType
                 | Some(SynExpr.Const (SynConst.String ("", _, range), _)) ->
-                    generateError range EmptyStringLiteral
+                    generateError args.FileContent range EmptyStringLiteral
                 | Some(SynExpr.App(_, _, _, SynExpr.Const (SynConst.String ("", _, range), _), _)) ->
-                    generateError range EmptyStringLiteral
+                    generateError args.FileContent range EmptyStringLiteral
                 | _ -> Array.empty)
         |> Array.concat
     | _ -> Array.empty

src/FSharpLint.Core/Rules/Conventions/FunctionReimplementation/CanBeReplacedWithComposition.fs

@@ -8,11 +8,11 @@ open FSharpLint.Framework.Ast
 open FSharpLint.Framework.Rules
 open FSharpLint.Framework.ExpressionUtilities
 
-let private validateLambdaCannotBeReplacedWithComposition _ lambda range =
-    let canBeReplacedWithFunctionComposition expression =
+let private validateLambdaCannotBeReplacedWithComposition fileContents _ lambda range =
+    let tryReplaceWithFunctionComposition expression =
         let getLastElement = List.rev >> List.head
 
-        let rec lambdaArgumentIsLastApplicationInFunctionCalls expression (lambdaArgument:Ident) numFunctionCalls =
+        let rec lambdaArgumentIsLastApplicationInFunctionCalls expression (lambdaArgument:Ident) (calledFunctionIdents: List<string>) =
             let rec appliedValuesAreConstants appliedValues =
                 match appliedValues with
                 | (SynExpr.Const(_)| SynExpr.Null(_))::rest -> appliedValuesAreConstants rest
@@ -22,34 +22,56 @@ let private validateLambdaCannotBeReplacedWithComposition _ lambda range =
             match AstNode.Expression expression with
             | FuncApp(exprs, _) ->
                 match List.map removeParens exprs with
-                | (SynExpr.Ident(_) | SynExpr.LongIdent(_))::appliedValues
+                | (ExpressionUtilities.Identifier(idents, _))::appliedValues
                         when appliedValuesAreConstants appliedValues ->
-
+                    
+                    let funcName = String.Join('.', idents)
+                    let funcStringParts = 
+                        Seq.append
+                            (Seq.singleton funcName)
+                            (appliedValues 
+                                |> Seq.take (appliedValues.Length - 1) 
+                                |> Seq.choose (fun value -> ExpressionUtilities.tryFindTextOfRange value.Range fileContents))
+                    let funcString = String.Join(' ', funcStringParts)
+                    
                     match getLastElement appliedValues with
-                    | SynExpr.Ident(lastArgument) when numFunctionCalls > 1 ->
-                        lastArgument.idText = lambdaArgument.idText
+                    | SynExpr.Ident(lastArgument) when calledFunctionIdents.Length > 1 ->
+                        if lastArgument.idText = lambdaArgument.idText then
+                            funcString :: calledFunctionIdents
+                        else
+                            []
                     | SynExpr.App(_, false, _, _, _) as nextFunction ->
-                        lambdaArgumentIsLastApplicationInFunctionCalls nextFunction lambdaArgument (numFunctionCalls + 1)
-                    | _ -> false
-                | _ -> false
-            | _ -> false
+                        lambdaArgumentIsLastApplicationInFunctionCalls 
+                            nextFunction 
+                            lambdaArgument 
+                            (funcString :: calledFunctionIdents)
+                    | _ -> []
+                | _ -> []
+            | _ -> []
 
         match lambda.Arguments with
         | [singleParameter] ->
-            Helper.FunctionReimplementation.getLambdaParamIdent singleParameter
-            |> Option.exists (fun paramIdent -> lambdaArgumentIsLastApplicationInFunctionCalls expression paramIdent 1)
-        | _ -> false
+            match Helper.FunctionReimplementation.getLambdaParamIdent singleParameter with
+            | Some paramIdent -> 
+                match lambdaArgumentIsLastApplicationInFunctionCalls expression paramIdent [] with
+                | [] -> None
+                | funcStrings -> Some funcStrings
+            | None -> None
+        | _ -> None
 
-    if canBeReplacedWithFunctionComposition lambda.Body then
+    match tryReplaceWithFunctionComposition lambda.Body with
+    | None -> Array.empty
+    | Some funcStrings ->
+        let suggestedFix =
+            lazy(
+                Some { FromRange = range; FromText = fileContents; ToText = String.Join(" >> ", funcStrings) })
         { Range = range
           Message = Resources.GetString("RulesCanBeReplacedWithComposition")
-          SuggestedFix = None
+          SuggestedFix = Some suggestedFix
           TypeChecks = [] } |> Array.singleton
-    else
-        Array.empty
 
 let runner (args:AstNodeRuleParams) =
-    Helper.FunctionReimplementation.checkLambda args validateLambdaCannotBeReplacedWithComposition
+    Helper.FunctionReimplementation.checkLambda args (validateLambdaCannotBeReplacedWithComposition args.FileContent)
 
 let rule =
     { Name = "CanBeReplacedWithComposition"

tests/FSharpLint.Core.Tests/Rules/Binding/TupleOfWildcards.fs

@@ -21,6 +21,38 @@ match Persian(1, 3) with
 
         Assert.IsTrue(this.ErrorExistsAt(7, 10))
 
+    [<Test>]
+    member this.``Suggested fix for tuple of wildcards should be single wildcard``() = 
+        let source = """
+match cat with
+| Persian(_, _) -> ()"""
+
+        let expected = """
+match cat with
+| Persian _ -> ()"""
+
+        this.Parse source
+
+        let result = this.ApplyQuickFix source
+
+        Assert.AreEqual(expected, result)
+
+    [<Test>]
+    member this.``Suggested fix for tuple of wildcards in nested pattern should be single wildcard``() = 
+        let source = """
+match maybeCat with
+| Some(Persian(_, _)) -> ()"""
+
+        let expected = """
+match maybeCat with
+| Some(Persian _) -> ()"""
+
+        this.Parse source
+
+        let result = this.ApplyQuickFix source
+
+        Assert.AreEqual(expected, result)
+
     [<Test>]
     member this.``Method's parameter list of wildcards should not be treated as tuple of wildcards.``() = 
         this.Parse """

tests/FSharpLint.Core.Tests/Rules/Binding/WildcardNamedWithAsPattern.fs

@@ -10,15 +10,28 @@ type TestBindingWildcardNamedWithAsPattern() =
 
     [<Test>]
     member this.WildcardNamedWithAsPattern() =
-        this.Parse """
+        let source = """
 module Program
 
 match [] with
 | _ as x -> ()
 """
 
+        let expected = """
+module Program
+
+match [] with
+| x -> ()
+"""
+        
+        this.Parse source
+
         Assert.IsTrue(this.ErrorExistsAt(5, 2))
 
+        let result = this.ApplyQuickFix source
+
+        Assert.AreEqual(expected, result)
+
     [<Test>]
     member this.NamedPattern() =
         this.Parse """

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

@@ -200,6 +200,23 @@ module Foo
 
         Assert.IsTrue this.NoErrorsExist
 
+    [<Test>]
+    member this.``Suggest not using pipe operator if it's used once``() =
+        let source = """
+let someFunc someParam =
+    someParam
+    |> someOtherFunc
+"""
+        let expected = """
+let someFunc someParam =
+    someOtherFunc someParam
+"""
+        
+        this.Parse source
+        let fixedSource = this.ApplyQuickFix source
+
+        Assert.AreEqual(expected, fixedSource)
+
     [<Test>]
     member this.``Use pipe operator for function with more than 1 argument``() =
         this.Parse """