elm-review/tests/CognitiveComplexity.elm

714 lines
23 KiB
Elm
Raw Normal View History

2022-04-17 09:59:11 +03:00
module CognitiveComplexity exposing (rule)
{-|
@docs rule
-}
import Dict exposing (Dict)
import Elm.Syntax.Declaration as Declaration exposing (Declaration)
import Elm.Syntax.Expression as Expression exposing (Expression)
import Elm.Syntax.Node as Node exposing (Node)
import Elm.Syntax.Range exposing (Location, Range)
import Review.Rule as Rule exposing (Rule)
import Set exposing (Set)
{-| Reports functions that have a too high cognitive complexity.
You can configure the threshold above which a function will be reported (`15` in the example configuration below).
config =
[ CognitiveComplexity.rule 15
]
## What is cognitive complexity?
Cognitive complexity is **not to be confused with "Cyclomatic Complexity"**, which has a different way of measuring the
complexity.
Here's an explanation extracted from [free white paper](https://www.sonarsource.com/resources/white-papers/cognitive-complexity/)
provided by SonarSource, the creators of the concept.
> Cognitive complexity tries to measure how hard it is to understand a function, primarily focusing on the control structures
> that hinder the understanding of a function by reading it from top to bottom in one go, like you would for a novel.
> A Cognitive Complexity score is assessed according to three basic rules:
> 1. Ignore structures that allow multiple statements to be readably shorthanded into one
> 2. Increment (add one) for each break in the linear flow of the code
> 3. Increment when flow-breaking structures are nested
Some small differences may be found between the implementation detailed in the paper and this rule, as the idea was
formulated more on imperative programming languages, and may not be applicable to a pure functional language like Elm.
You can read about how is works in the [complexity breakdown section](#complexity-breakdown) below.
## When (not) to enable this rule
This rule is an experiment. I don't know if this will be more useful or detrimental, and I haven't yet figured out what
the ideal complexity threshold for Elm projects is.
I would for now recommend to use it with a very high threshold to find places in your codebase that need refactoring,
and eventually to enable it in your configuration to make sure no new extremely complex functions appear. As you refactor more
and more of your codebase, you can gradually lower the threshold until you reach a level that you feel happy with.
Please let me know how enabling this rule works out for you!
## Complexity breakdown
Following is a breakdown of how the complexity of a function is computed:
- If expression: Increases complexity by 1 + nesting, and increases nesting.
`else if` also increases complexity by 1 + nesting, but doesn't further increase the nesting.
`else` doesn't increase the complexity.
```js
-- Total: 4
a =
if b then -- +1
if c then -- +2, including 1 for nesting
1
else
2
else if d then -- +1
3
else -- +0
4
```
- Case expression: Increases complexity by 1 + nesting, regardless of how many cases are handled, and increases nesting.
```js
-- Total: 3
a =
case b of -- +1
A -> 1
B -> 2
C ->
case c of -- +2, including 1 for nesting
_ -> 3
D -> 4
```
- Let functions: Increases nesting.
```js
-- Total: 2
a =
let
fn b = -- increases nesting
if b then -- +2, including 1 for nesting
1
else
2
constant = -- Not a function, no increase
True
in
fn constant
```
- Anonymous functions: Increases nesting.
```js
-- Total: 2
a things =
List.map
(\thing -> -- increases nesting
case thing of -- +2, including 1 for nesting
Just _ -> 1
Nothing -> 2
)
things
```
- Logical operator suites: Increase complexity by 1 for every discontinuation of the suite
```elm
a && b -- +1
-- This is still the same logical construction as
-- above, and therefore about as hard to understand
a && b && c && d && e -- +1
-- Total: 3
a && b && c -- +1
|| d -- +1 for breaking the chain of && with a ||
|| not (e || f) -- +1 for breaking the chain
-- with a `not` of a binary operation
```
- Recursive function calls (direct or indirect): Increase complexity by 1 for each different call
```js
-- Total: 2
fun1 n =
fun2 n -- +1
+ fun2 n -- +0, already counted
+ fun1 n -- +1
-- Total: 1
fun2 n =
fun1 n -- +1
```
[The original metric](https://www.sonarsource.com/docs/CognitiveComplexity.pdf) increases the complexity for other
structures that the Elm language doesn't have.
## Try it out
You can try this rule out by running the following command:
```bash
elm-review --template jfmengels/elm-review-cognitive-complexity/example --rules CognitiveComplexity
```
The cognitive complexity is set to 15 in the configuration used by the example.
## Thanks
Thanks to the team at SonarSource for designing the metric and for not restricting its use.
Thanks to G. Ann Campbell for the different talks she made on the subject.
-}
rule : Int -> Rule
rule threshold =
Rule.newModuleRuleSchema "CognitiveComplexity" initialContext
|> Rule.withDeclarationExitVisitor declarationExitVisitor
|> Rule.withExpressionEnterVisitor expressionEnterVisitor
|> Rule.withExpressionExitVisitor expressionExitVisitor
|> Rule.withFinalModuleEvaluation (finalEvaluation threshold)
|> Rule.fromModuleRuleSchema
type alias Context =
{ nesting : Int
, operandsToIgnore : List Range
, elseIfToIgnore : List Range
, rangesWhereNestingIncreases : List Range
, increases : List Increase
, references : Dict String Location
, functionsToReport : List FunctionToReport
}
type alias FunctionToReport =
{ functionName : Node String
, increases : List Increase
, references : Dict String Location
}
type alias Increase =
{ line : Location
, increase : Int
, nesting : Int
, kind : IncreaseKind
}
type IncreaseKind
= If
| ElseIf
| Case
| Operator String
| RecursiveCall
| IndirectRecursiveCall String
initialContext : Context
initialContext =
{ nesting = 0
, operandsToIgnore = []
, elseIfToIgnore = []
, rangesWhereNestingIncreases = []
, references = Dict.empty
, increases = []
, functionsToReport = []
}
expressionEnterVisitor : Node Expression -> Context -> ( List nothing, Context )
expressionEnterVisitor node context =
if List.member (Node.range node) context.rangesWhereNestingIncreases then
( [], expressionEnterVisitorHelp node { context | nesting = context.nesting + 1 } )
else
( [], expressionEnterVisitorHelp node context )
expressionEnterVisitorHelp : Node Expression -> Context -> Context
expressionEnterVisitorHelp node context =
case Node.value node of
Expression.IfBlock _ _ else_ ->
if not (List.member (Node.range node) context.elseIfToIgnore) then
{ context
| increases =
{ line = (Node.range node).start
, increase = context.nesting + 1
, nesting = context.nesting
, kind = If
}
:: context.increases
, nesting = context.nesting + 1
, elseIfToIgnore = Node.range else_ :: context.elseIfToIgnore
}
else
-- This if expression is an else if
-- We want to increase the complexity but keep the same nesting as the parent if
{ context
| increases =
{ line = (Node.range node).start
, increase = context.nesting
, nesting = context.nesting - 1
, kind = ElseIf
}
:: context.increases
, elseIfToIgnore = Node.range else_ :: context.elseIfToIgnore
}
Expression.CaseExpression _ ->
{ context
| increases =
{ line = (Node.range node).start
, increase = context.nesting + 1
, nesting = context.nesting
, kind = Case
}
:: context.increases
, nesting = context.nesting + 1
}
Expression.LetExpression { declarations } ->
{ context | rangesWhereNestingIncreases = computeRangesForLetDeclarations declarations ++ context.rangesWhereNestingIncreases }
Expression.OperatorApplication operator _ left right ->
if (operator == "&&" || operator == "||") && not (List.member (Node.range node) context.operandsToIgnore) then
let
( increases, operandsToIgnore ) =
incrementAndIgnoreForOperands
operator
[]
left
right
in
{ context
| increases =
{ line = (Node.range node).start
, increase = 1
, nesting = 0
, kind = Operator operator
}
:: increases
++ context.increases
, operandsToIgnore = operandsToIgnore ++ context.operandsToIgnore
}
else
context
Expression.LambdaExpression _ ->
{ context | nesting = context.nesting + 1 }
Expression.FunctionOrValue [] name ->
{ context
| references =
if Dict.member name context.references then
-- The reference already exists, and we want to keep the first reference
-- for a better presentation
context.references
else
Dict.insert name (Node.range node).start context.references
}
_ ->
context
computeRangesForLetDeclarations : List (Node Expression.LetDeclaration) -> List Range
computeRangesForLetDeclarations declarations =
List.filterMap
(\letDecl ->
case Node.value letDecl of
Expression.LetFunction { declaration } ->
if List.isEmpty (Node.value declaration).arguments then
Nothing
else
Just (declaration |> Node.value |> .expression |> Node.range)
Expression.LetDestructuring _ _ ->
Nothing
)
declarations
incrementAndIgnoreForOperands : String -> List Increase -> Node Expression -> Node Expression -> ( List Increase, List Range )
incrementAndIgnoreForOperands operator increases left right =
let
( leftIncreases, leftIgnore ) =
incrementAndIgnore operator left
( rightIncreases, rightIgnore ) =
incrementAndIgnore operator right
in
( List.concat [ leftIncreases, rightIncreases, increases ]
, Node.range left :: Node.range right :: leftIgnore ++ rightIgnore
)
incrementAndIgnore : String -> Node Expression -> ( List Increase, List Range )
incrementAndIgnore parentOperator node =
case Node.value node of
Expression.OperatorApplication operator _ left right ->
if operator == "&&" || operator == "||" then
let
newOperatorIncrease : List Increase
newOperatorIncrease =
if operator == parentOperator then
[]
else
[ { line = (Node.range node).start
, increase = 1
, nesting = 0
, kind = Operator operator
}
]
in
incrementAndIgnoreForOperands
operator
newOperatorIncrease
left
right
else
( [], [] )
_ ->
( [], [] )
expressionExitVisitor : Node Expression -> Context -> ( List nothing, Context )
expressionExitVisitor node context =
if List.member (Node.range node) context.rangesWhereNestingIncreases then
( [], expressionExitVisitorHelp node { context | nesting = context.nesting - 1 } )
else
( [], expressionExitVisitorHelp node context )
expressionExitVisitorHelp : Node Expression -> Context -> Context
expressionExitVisitorHelp node context =
case Node.value node of
Expression.IfBlock _ _ _ ->
if not (List.member (Node.range node) context.elseIfToIgnore) then
{ context | nesting = context.nesting - 1 }
else
context
Expression.CaseExpression _ ->
{ context | nesting = context.nesting - 1 }
Expression.LambdaExpression _ ->
{ context | nesting = context.nesting - 1 }
_ ->
context
declarationExitVisitor : Node Declaration -> Context -> ( List (Rule.Error {}), Context )
declarationExitVisitor node context =
let
functionsToReport : List FunctionToReport
functionsToReport =
case Node.value node of
Declaration.FunctionDeclaration function ->
{ functionName = function.declaration |> Node.value |> .name
, increases = context.increases
, references = context.references
}
:: context.functionsToReport
_ ->
context.functionsToReport
in
( []
, { nesting = 0
, operandsToIgnore = []
, elseIfToIgnore = []
, rangesWhereNestingIncreases = []
, references = Dict.empty
, increases = []
, functionsToReport = functionsToReport
}
)
finalEvaluation : Int -> Context -> List (Rule.Error {})
finalEvaluation threshold context =
let
potentialRecursiveFunctions : Set String
potentialRecursiveFunctions =
List.map (.functionName >> Node.value) context.functionsToReport
|> Set.fromList
recursiveCalls : RecursiveCalls
recursiveCalls =
context.functionsToReport
|> List.map
(\{ functionName, references } ->
( Node.value functionName, Dict.filter (\name _ -> Set.member name potentialRecursiveFunctions) references )
)
|> Dict.fromList
|> findRecursiveCalls
in
List.filterMap
(\{ functionName, increases, references } ->
let
recursiveCallsForFunctionName : List String
recursiveCallsForFunctionName =
Dict.get (Node.value functionName) recursiveCalls
|> Maybe.withDefault Set.empty
|> Set.toList
allIncreases : List Increase
allIncreases =
List.concat
[ increases
, recursiveCallsForFunctionName
|> List.filterMap
(\referenceToRecursiveFunction ->
Dict.get referenceToRecursiveFunction references
|> Maybe.map (Tuple.pair referenceToRecursiveFunction)
)
|> List.map
(\( reference, location ) ->
{ line = location
, increase = 1
, nesting = 0
, kind =
if Node.value functionName == reference then
RecursiveCall
else
IndirectRecursiveCall reference
}
)
]
finalComplexity : Int
finalComplexity =
List.sum (List.map .increase allIncreases)
in
if finalComplexity > threshold then
Just
(Rule.error
{ message = Node.value functionName ++ " has a cognitive complexity of " ++ String.fromInt finalComplexity ++ ", higher than the allowed " ++ String.fromInt threshold
, details =
if List.isEmpty allIncreases then
explanation
else
explanation
++ [ allIncreases
|> List.sortBy (\{ line } -> ( line.row, line.column ))
|> List.map explain
|> String.join "\n"
]
}
(Node.range functionName)
)
else
Nothing
)
context.functionsToReport
explanation : List String
explanation =
[ "This metric is a heuristic to measure how easy to understand a piece of code is, primarily through increments for breaks in the linear flow and for nesting those breaks."
, "The most common ways to reduce complexity is to extract sections into functions and to unnest control flow structures. Following is a breakdown of where complexity was found:"
]
explain : Increase -> String
explain increase =
"Line " ++ String.fromInt increase.line.row ++ ": +" ++ String.fromInt increase.increase ++ " for the " ++ kindToString increase.kind ++ mentionNesting increase.nesting
mentionNesting : Int -> String
mentionNesting nesting =
if nesting == 0 then
""
else
" (including " ++ String.fromInt nesting ++ " for nesting)"
kindToString : IncreaseKind -> String
kindToString kind =
case kind of
If ->
"if expression"
ElseIf ->
"else if expression"
Case ->
"case expression"
Operator operator ->
"use of `" ++ operator ++ "`"
RecursiveCall ->
"recursive call"
IndirectRecursiveCall fnName ->
"indirect recursive call to " ++ fnName
-- FINDING RECURSIVE FUNCTIONS
-- Inspired by the algorithm found at https://www.baeldung.com/cs/detecting-recursiveCalls-in-directed-graph
type alias RecursiveCalls =
Dict String (Set String)
type alias Visited =
Dict String VisitState
type VisitState
= InStack
| Done
findRecursiveCalls : Dict String (Dict String a) -> RecursiveCalls
findRecursiveCalls graph =
graph
|> Dict.keys
|> List.foldl
(\vertice ( recursiveCalls, visited ) ->
let
res : { recursiveCalls : RecursiveCalls, visited : Visited, stack : List String }
res =
processDFSTree
graph
[ vertice ]
(Dict.insert vertice InStack visited)
in
( mergeRecursiveCallsDict res.recursiveCalls recursiveCalls, res.visited )
)
( Dict.empty, Dict.empty )
|> Tuple.first
mergeRecursiveCallsDict : RecursiveCalls -> RecursiveCalls -> RecursiveCalls
mergeRecursiveCallsDict left right =
Dict.merge
(\functionName calls dict -> Dict.insert functionName calls dict)
(\functionName callsLeft callsRight dict -> Dict.insert functionName (Set.union callsLeft callsRight) dict)
(\functionName calls dict -> Dict.insert functionName calls dict)
left
right
Dict.empty
processDFSTree : Dict String (Dict String a) -> List String -> Visited -> { recursiveCalls : RecursiveCalls, visited : Visited, stack : List String }
processDFSTree graph stack visited =
let
vertices : List String
vertices =
List.head stack
|> Maybe.andThen (\v -> Dict.get v graph)
|> Maybe.withDefault Dict.empty
|> Dict.keys
in
List.foldl
(\vertice acc ->
case Dict.get vertice visited of
Just InStack ->
{ acc | recursiveCalls = insertCycle stack vertice acc.recursiveCalls }
Just Done ->
acc
Nothing ->
let
res : { recursiveCalls : RecursiveCalls, visited : Visited, stack : List String }
res =
processDFSTree
graph
(vertice :: stack)
(Dict.insert vertice InStack visited)
in
{ recursiveCalls = mergeRecursiveCallsDict res.recursiveCalls acc.recursiveCalls, visited = res.visited }
)
{ recursiveCalls = Dict.empty, visited = visited }
vertices
|> (\res ->
{ recursiveCalls = res.recursiveCalls
, visited =
List.head stack
|> Maybe.map (\v -> Dict.insert v Done res.visited)
|> Maybe.withDefault res.visited
, stack = List.drop 1 stack
}
)
insertCycle : List String -> String -> RecursiveCalls -> RecursiveCalls
insertCycle stack vertice recursiveCalls =
case stack of
x :: xs ->
List.foldl
(\( functionName, reference ) acc ->
Dict.update
functionName
(Maybe.withDefault Set.empty >> Set.insert reference >> Just)
acc
)
recursiveCalls
(toTuples x (takeTop xs ( x, [] ) vertice) [])
[] ->
recursiveCalls
toTuples : String -> ( String, List String ) -> List ( String, String ) -> List ( String, String )
toTuples x ( first, xs ) result =
case xs of
[] ->
( x, first ) :: result
firstofXs :: restOfXs ->
toTuples first ( firstofXs, restOfXs ) (( x, first ) :: result)
takeTop : List String -> ( String, List String ) -> String -> ( String, List String )
takeTop stack ( previousValue, previousValues ) stopValue =
case stack of
[] ->
( previousValue, previousValues )
x :: xs ->
if x /= stopValue then
takeTop xs ( x, previousValue :: previousValues ) stopValue
else
( x, previousValue :: previousValues )