diff --git a/src/FSharpLint.Core/Rules/Conventions/CyclomaticComplexity.fs b/src/FSharpLint.Core/Rules/Conventions/CyclomaticComplexity.fs index 147f8d6f4..67862b4aa 100644 --- a/src/FSharpLint.Core/Rules/Conventions/CyclomaticComplexity.fs +++ b/src/FSharpLint.Core/Rules/Conventions/CyclomaticComplexity.fs @@ -1,7 +1,8 @@ module FSharpLint.Rules.CyclomaticComplexity open System -open System.IO +open System.Collections.Generic +open System.Linq open FSharp.Compiler.Syntax open FSharpLint.Framework open FSharpLint.Framework.Suggestion @@ -16,7 +17,7 @@ type Config = { } /// The scope of a binding. -type BindingScope = +type private BindingScope = { /// The Node corresponding to the binding. Node: AstNode @@ -26,8 +27,68 @@ type BindingScope = Complexity: int } +type private BindingScopeComparer() = + interface IComparer with + member this.Compare(left, right) = + let leftStart = left.Binding.RangeOfBindingWithoutRhs.Start + let rightStart = right.Binding.RangeOfBindingWithoutRhs.Start + let leftTuple : ValueTuple = leftStart.Line, leftStart.Column, left.Complexity + let rightTuple : ValueTuple = rightStart.Line, rightStart.Column, right.Complexity + leftTuple.CompareTo rightTuple + +/// A two-tiered stack-like structure for containing BindingScopes. +type private BindingStack(maxComplexity: int) = + let mutable tier1_ = [] + let mutable tier2_ = SortedSet(BindingScopeComparer()) + + member this.Push (args:AstNodeRuleParams) (bs: BindingScope) = + // if the node is a child of the current binding scope + let isChildOfCurrent = if List.isEmpty tier1_ then + false + else + args.GetParents args.NodeIndex |> List.tryFind (fun x -> Object.ReferenceEquals(tier1_.Head.Node, x)) |> Option.isSome + // if the node is not a child and the stack isn't empty, we're finished with the current head of tier1, so move it from tier1 to tier2 + if not isChildOfCurrent && not (List.isEmpty tier1_) then + let popped = tier1_.Head + tier1_ <- tier1_.Tail + if popped.Complexity > maxComplexity then + tier2_.Add popped |> ignore + // finally, push the item on to the stack + tier1_ <- bs::tier1_ + + member this.IncrComplexityOfCurrentScope incr = + let h = tier1_.Head + let complexity = h.Complexity + incr + tier1_ <- {h with Complexity = complexity}::tier1_.Tail + + interface IEnumerable with + member this.GetEnumerator() = + let cleanedUp = + tier1_ + |> List.filter (fun scope -> scope.Complexity > maxComplexity) + |> List.sortByDescending (fun scope -> scope.Complexity) // sort in descending order by complexity + |> List.distinctBy (fun scope -> scope.Binding.RangeOfBindingWithRhs.Start) // throw away any extraneous elements with the same start position but a lower complexity + let enum1 = cleanedUp :> IEnumerable + let enum2 = tier2_ :> IEnumerable + Enumerable.Concat(enum1, enum2).GetEnumerator() + + member this.GetEnumerator(): Collections.IEnumerator = (this :> IEnumerable :> System.Collections.IEnumerable).GetEnumerator() + + /// Clears the stack. + member this.Clear() = + tier1_ <- [] + tier2_.Clear() + /// A stack to track the current cyclomatic complexity of a binding scope. -let mutable private BindingStack : BindingScope list = [] +let mutable private bindingStackOpt : BindingStack option = None + +/// gets the global binding stack +let private getBindingStack (maxComplexity: int) = + match bindingStackOpt with + | Some bs -> bs + | None -> let bs = BindingStack maxComplexity + bindingStackOpt <- Some bs + bs /// Determines the number of cases in a match clause. let private countCasesInMatchClause (clause: SynMatchClause) = @@ -42,11 +103,6 @@ let private countCasesInMatchClause (clause: SynMatchClause) = match clause with | SynMatchClause(pat, _, _, _, _) -> countCases pat 0 -let private increaseComplexity incr = - let h = BindingStack.Head - let complexity = h.Complexity + incr - BindingStack <- {h with Complexity = complexity}::BindingStack - /// Boolean operator functions. let private boolFunctions = Set.ofList ["op_BooleanOr"; "op_BooleanAnd"] @@ -89,20 +145,8 @@ let private countBooleanOperators expression = /// Runner for the rule. let runner (config:Config) (args:AstNodeRuleParams) : WarningDetails[] = - let popOffBindingStack() = - if BindingStack.Length > 0 then - let popped = BindingStack.Head - BindingStack <- BindingStack.Tail - // if the complexity within the scope of the binding is greater than the max complexity, report it - if popped.Complexity > config.MaxComplexity then - let errorFormatString = Resources.GetString("RulesCyclomaticComplexityError") - let errMsg = String.Format(errorFormatString, popped.Complexity, config.MaxComplexity) - Some { Range = popped.Binding.RangeOfBindingWithRhs; Message = errMsg; SuggestedFix = None; TypeChecks = [] } - else - None - else - None - + let bindingStack = getBindingStack config.MaxComplexity + let mutable warningDetails = None let node = args.AstNode let parentIndex = args.SyntaxArray.[args.NodeIndex].ParentIndex @@ -115,18 +159,10 @@ let runner (config:Config) (args:AstNodeRuleParams) : WarningDetails[] = let bindingOpt = match node with | AstNode.Binding binding -> Some binding | _ -> None - // if the node is a child of the current binding scope - let isChildOfCurrent = if List.isEmpty BindingStack then - false - else - args.GetParents args.NodeIndex |> List.tryFind (fun x -> Object.ReferenceEquals(BindingStack.Head.Node, x)) |> Option.isSome - // if the node is not a child and the stack isn't empty, we're finished with the current binding scope at the head of the stack, so pop it off - if not isChildOfCurrent && List.length BindingStack > 0 then - warningDetails <- popOffBindingStack() // if the node is a binding, push it onto the stack match bindingOpt with - | Some binding -> BindingStack <- { Node = node; Binding = binding; Complexity = 0 }::BindingStack + | Some binding -> bindingStack.Push args { Node = node; Binding = binding; Complexity = 0 } | None -> () // if not metadata, match the node against an expression which increments the complexity @@ -135,40 +171,43 @@ let runner (config:Config) (args:AstNodeRuleParams) : WarningDetails[] = | AstNode.Expression expression -> match expression with | SynExpr.For _ -> - increaseComplexity 1 + bindingStack.IncrComplexityOfCurrentScope 1 | SynExpr.ForEach _ -> - increaseComplexity 1 + bindingStack.IncrComplexityOfCurrentScope 1 | SynExpr.While(_, condition, _, _) -> - increaseComplexity (1 + countBooleanOperators condition) // include the number of boolean operators in the while condition + bindingStack.IncrComplexityOfCurrentScope (1 + countBooleanOperators condition) // include the number of boolean operators in the while condition | SynExpr.IfThenElse(condition, _, _, _, _, _, _) -> - increaseComplexity (1 + countBooleanOperators condition) // include the number of boolean operators in the condition + bindingStack.IncrComplexityOfCurrentScope (1 + countBooleanOperators condition) // include the number of boolean operators in the condition | SynExpr.MatchBang(_, _, clauses, _) | SynExpr.MatchLambda(_, _, clauses, _, _) | SynExpr.Match(_, _, clauses, _) -> let numCases = clauses |> List.sumBy countCasesInMatchClause // determine the number of cases in the match expression - increaseComplexity (numCases + countBooleanOperators expression) // include the number of boolean operators in any when expressions, if applicable + bindingStack.IncrComplexityOfCurrentScope (numCases + countBooleanOperators expression) // include the number of boolean operators in any when expressions, if applicable | _ -> () | _ -> () // if the last node to be processed, pop everything off the stack if args.NodeIndex >= args.SyntaxArray.Length-1 then - seq { - match warningDetails with - | Some x -> yield x - | None -> () - while not BindingStack.IsEmpty do - match popOffBindingStack() with - | Some x -> yield x - | None -> () - } |> Seq.toArray + let fromStack = bindingStack + |> Seq.sortBy (fun scope -> // sort by order of start position, for reporting + let pos = scope.Binding.RangeOfBindingWithRhs.Start + pos.Column, pos.Line) + |> Seq.map (fun scope -> // transform into WarningDetails + let errMsg = String.Format(Resources.GetString("RulesCyclomaticComplexityError"), scope.Complexity, config.MaxComplexity) + { Range = scope.Binding.RangeOfBindingWithRhs; Message = errMsg; SuggestedFix = None; TypeChecks = [] }) + |> Seq.toList + let ret = match warningDetails with + | Some x -> x::fromStack + | None -> fromStack + ret |> List.toArray else - match warningDetails with - | Some x -> [|x|] - | None -> Array.empty + Array.empty /// Resets call stack after a call to runner. let cleanup () = - BindingStack <- [] + match bindingStackOpt with + | Some bs -> bs.Clear() + | None _ -> () /// Generator function for a rule instance. let rule config = diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/CyclomaticComplexity.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/CyclomaticComplexity.fs index 8cb4140ed..a0bab0f0d 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/CyclomaticComplexity.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/CyclomaticComplexity.fs @@ -2,7 +2,6 @@ module FSharpLint.Core.Tests.Rules.Conventions.CyclomaticComplexity open NUnit.Framework open FSharpLint.Rules.CyclomaticComplexity -open System /// The max cyclomatic complexity as configured in these tests. [] @@ -149,7 +148,7 @@ type TestConventionsCyclomaticComplexity() = [] member this.EnsureNoErrorWhenComplexityBelowThreshold(code) = this.Parse code - Assert.IsFalse(this.ErrorsExist) + Assert.IsTrue(this.NoErrorsExist) /// Verifies that flags are raised on source code that has cyclomatic complexity > maxComplexity. [] @@ -196,7 +195,7 @@ let f() = let g() = {makeMatchSnippetWithLogicalOperatorsInWhenClause (MaxComplexity / 2) |> indent 8}""" this.Parse code - Assert.AreEqual(0, this.ErrorRanges.Length) + Assert.IsTrue this.NoErrorsExist /// Verifies that the cyclomatic complexity is calculated on functions independently by checking that a function that comes after a function with a cyclomatic complexity that is flagged as too high need not be flagged. [] @@ -221,7 +220,7 @@ let f() = {makeMatchSnippet MaxComplexity |> indent 8} ()""" this.Parse code - Assert.AreEqual(0, this.ErrorRanges.Length) + Assert.IsTrue this.NoErrorsExist /// Verifies that the cyclomatic complexity is calculated on nested functions independently by checking that a nested function that comes after another nested function with a cyclomatic complexity that is flagged as too high need not be flagged. @@ -232,7 +231,22 @@ let f() = let g() = {(makeMatchSnippet (MaxComplexity+1)) |> indent 8} let h() = -{makeMatchSnippet (MaxComplexity) |> indent 8} +{makeMatchSnippet MaxComplexity |> indent 8} {makeMatchSnippet (MaxComplexity+1) |> indent 4}""" this.Parse code - Assert.AreEqual(2, this.ErrorRanges.Length) \ No newline at end of file + Assert.AreEqual(2, this.ErrorRanges.Length) + + /// Verifies that the multiple messages are not provided for a single function. + [] + member this.EnsureRedundantWarningsNotReported() = + // generates a vapid match clause + let genMatchClause i = $"""| "{i}" -> match str with + | "A" -> () + | "B" -> ()""" + // create a snippet of code with 10 match clauses + let matchClauses = [ 1..10 ] |> List.map genMatchClause |> List.map (indent 4) |> String.concat NewLine + let code = """Module Program +let f (str: string) = + match str with""" + NewLine + matchClauses + this.Parse code + Assert.AreEqual(1, this.ErrorRanges.Length) \ No newline at end of file