mirror of
https://github.com/jfmengels/elm-review.git
synced 2024-12-23 09:42:49 +03:00
714 lines
23 KiB
Elm
714 lines
23 KiB
Elm
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 )
|