diff --git a/src/dscanner/analysis/cyclomatic_complexity.d b/src/dscanner/analysis/cyclomatic_complexity.d index 27e6d2dc..f6013682 100644 --- a/src/dscanner/analysis/cyclomatic_complexity.d +++ b/src/dscanner/analysis/cyclomatic_complexity.d @@ -4,12 +4,8 @@ module dscanner.analysis.cyclomatic_complexity; -import dparse.ast; -import dparse.lexer; -import dsymbol.scope_ : Scope; import dscanner.analysis.base; -import dscanner.analysis.helpers; - +import dmd.location : Loc; import std.format; /// Implements a basic cyclomatic complexity algorithm using the AST. @@ -35,12 +31,9 @@ import std.format; /// See: https://en.wikipedia.org/wiki/Cyclomatic_complexity /// Rules based on http://cyvis.sourceforge.net/cyclomatic_complexity.html /// and https://github.com/fzipp/gocyclo -final class CyclomaticComplexityCheck : BaseAnalyzer +extern (C++) class CyclomaticComplexityCheck(AST) : BaseAnalyzerDmd { - /// Message key emitted when the threshold is reached - enum string KEY = "dscanner.metric.cyclomatic_complexity"; - /// Human readable message emitted when the threshold is reached - enum string MESSAGE = "Cyclomatic complexity of this function is %s."; + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"cyclomatic_complexity"; /// Maximum cyclomatic complexity. Once the cyclomatic complexity is greater @@ -50,52 +43,44 @@ final class CyclomaticComplexityCheck : BaseAnalyzer /// unmaintainable / untestable. /// /// For clean development a threshold like 20 can be used instead. - int maxCyclomaticComplexity; + immutable int maxCyclomaticComplexity; - /// - this(BaseAnalyzerArguments args, int maxCyclomaticComplexity = 50) + private enum string KEY = "dscanner.metric.cyclomatic_complexity"; + private enum string MESSAGE = "Cyclomatic complexity of this function is %s."; + + private int[] complexityStack = [0]; + private bool[] inLoop = [false]; + + extern (D) this(string fileName, bool skipTests = false, int maxCyclomaticComplexity = 50) { - super(args); + super(fileName, skipTests); this.maxCyclomaticComplexity = maxCyclomaticComplexity; } - mixin VisitComplex!IfStatement; - mixin VisitComplex!CaseStatement; - mixin VisitComplex!CaseRangeStatement; - mixin VisitLoop!DoStatement; - mixin VisitLoop!WhileStatement; - mixin VisitLoop!ForStatement; - mixin VisitLoop!ForeachStatement; - mixin VisitComplex!AndAndExpression; - mixin VisitComplex!OrOrExpression; - mixin VisitComplex!TernaryExpression; - mixin VisitComplex!ThrowExpression; - mixin VisitComplex!Catch; - mixin VisitComplex!LastCatch; - mixin VisitComplex!ReturnStatement; - mixin VisitComplex!FunctionLiteralExpression; - mixin VisitComplex!GotoStatement; - mixin VisitComplex!ContinueStatement; - - override void visit(const SwitchStatement n) + override void visit(AST.TemplateDeclaration templateDecl) { - inLoop.assumeSafeAppend ~= false; - scope (exit) - inLoop.length--; - n.accept(this); + foreach (member; *templateDecl.members) + member.accept(this); } - override void visit(const BreakStatement b) + override void visit(AST.FuncDeclaration funDecl) { - if (b.label !is Token.init || inLoop[$ - 1]) - complexityStack[$ - 1]++; + if (funDecl.fbody is null) + return; + + analyzeFunctionBody(funDecl.fbody, funDecl.loc); } - override void visit(const FunctionDeclaration fun) + override void visit(AST.UnitTestDeclaration unitTestDecl) { - if (!fun.functionBody) + if (skipTests) return; + analyzeFunctionBody(unitTestDecl.fbody, unitTestDecl.loc); + } + + private void analyzeFunctionBody(AST.Statement functionBody, Loc location) + { complexityStack.assumeSafeAppend ~= 1; inLoop.assumeSafeAppend ~= false; scope (exit) @@ -103,58 +88,100 @@ final class CyclomaticComplexityCheck : BaseAnalyzer complexityStack.length--; inLoop.length--; } - fun.functionBody.accept(this); - testComplexity(fun.name); + + functionBody.accept(this); + testComplexity(location.linnum, location.charnum); } - override void visit(const Unittest unittest_) + private void testComplexity(size_t line, size_t column) { - if (!skipTests) - { - complexityStack.assumeSafeAppend ~= 1; - inLoop.assumeSafeAppend ~= false; - scope (exit) - { - complexityStack.length--; - inLoop.length--; - } - unittest_.accept(this); - testComplexity(unittest_.findTokenForDisplay(tok!"unittest")); - } + auto complexity = complexityStack[$ - 1]; + if (complexity > maxCyclomaticComplexity) + addErrorMessage(line, column, KEY, format!MESSAGE(complexity)); } - alias visit = BaseAnalyzer.visit; -private: - int[] complexityStack = [0]; - bool[] inLoop = [false]; + override void visit(AST.FuncExp funcExp) + { + if (funcExp.fd is null) + return; + + complexityStack[$ - 1]++; + funcExp.fd.accept(this); + } - void testComplexity(T)(T annotatable) + mixin VisitComplex!(AST.IfStatement); + mixin VisitComplex!(AST.LogicalExp); + mixin VisitComplex!(AST.CondExp); + mixin VisitComplex!(AST.CaseStatement); + mixin VisitComplex!(AST.CaseRangeStatement); + mixin VisitComplex!(AST.ReturnStatement); + mixin VisitComplex!(AST.ContinueStatement); + mixin VisitComplex!(AST.GotoStatement); + mixin VisitComplex!(AST.TryFinallyStatement); + mixin VisitComplex!(AST.ThrowExp); + + private template VisitComplex(NodeType, int increase = 1) { - auto complexity = complexityStack[$ - 1]; - if (complexity > maxCyclomaticComplexity) + override void visit(NodeType nodeType) { - addErrorMessage(annotatable, KEY, format!MESSAGE(complexity)); + complexityStack[$ - 1] += increase; + super.visit(nodeType); } } - template VisitComplex(NodeType, int increase = 1) + override void visit(AST.SwitchStatement switchStatement) { - override void visit(const NodeType n) + inLoop.assumeSafeAppend ~= false; + scope (exit) + inLoop.length--; + + switchStatement.condition.accept(this); + switchStatement._body.accept(this); + } + + override void visit(AST.BreakStatement breakStatement) + { + if (inLoop[$ - 1]) + complexityStack[$ - 1]++; + } + + override void visit(AST.TryCatchStatement tryCatchStatement) + { + tryCatchStatement._body.accept(this); + + if (tryCatchStatement.catches !is null) { - complexityStack[$ - 1] += increase; - n.accept(this); + foreach (catchStatement; *(tryCatchStatement.catches)) + { + complexityStack[$ - 1]++; + catchStatement.handler.accept(this); + } } } - template VisitLoop(NodeType, int increase = 1) + override void visit(AST.StaticForeachStatement staticForeachStatement) { - override void visit(const NodeType n) + // StaticForeachStatement visit has to be overridden in order to avoid visiting + // its forEachStatement member, which would increase the complexity. + return; + } + + mixin VisitLoop!(AST.DoStatement); + mixin VisitLoop!(AST.WhileStatement); + mixin VisitLoop!(AST.ForStatement); + mixin VisitLoop!(AST.ForeachRangeStatement); + mixin VisitLoop!(AST.ForeachStatement); + + private template VisitLoop(NodeType, int increase = 1) + { + override void visit(NodeType nodeType) { inLoop.assumeSafeAppend ~= true; scope (exit) inLoop.length--; + complexityStack[$ - 1] += increase; - n.accept(this); + super.visit(nodeType); } } } @@ -162,34 +189,34 @@ private: unittest { import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarningsDMD; import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.cyclomatic_complexity = Check.enabled; sac.max_cyclomatic_complexity = 0; - assertAnalyzerWarnings(q{ -unittest /+ -^^^^^^^^ [warn]: Cyclomatic complexity of this function is 1. +/ + + // TODO: Remove redundant tests and break down remaining tests in individual assertions + assertAnalyzerWarningsDMD(q{ +unittest // [warn]: Cyclomatic complexity of this function is 1. { } -unittest /+ -^^^^^^^^ [warn]: Cyclomatic complexity of this function is 1. +/ +// unit test +unittest // [warn]: Cyclomatic complexity of this function is 1. { writeln("hello"); writeln("world"); } -void main(string[] args) /+ - ^^^^ [warn]: Cyclomatic complexity of this function is 3. +/ +void main(string[] args) // [warn]: Cyclomatic complexity of this function is 3. { if (!args.length) return; writeln("hello ", args); } -unittest /+ -^^^^^^^^ [warn]: Cyclomatic complexity of this function is 1. +/ +unittest // [warn]: Cyclomatic complexity of this function is 1. { // static if / static foreach does not increase cyclomatic complexity static if (stuff) @@ -197,8 +224,7 @@ unittest /+ int a; } -unittest /+ -^^^^^^^^ [warn]: Cyclomatic complexity of this function is 2. +/ +unittest // [warn]: Cyclomatic complexity of this function is 2. { foreach (i; 0 .. 2) { @@ -206,8 +232,7 @@ unittest /+ int a; } -unittest /+ -^^^^^^^^ [warn]: Cyclomatic complexity of this function is 3. +/ +unittest // [warn]: Cyclomatic complexity of this function is 3. { foreach (i; 0 .. 2) { @@ -216,8 +241,7 @@ unittest /+ int a; } -unittest /+ -^^^^^^^^ [warn]: Cyclomatic complexity of this function is 2. +/ +unittest // [warn]: Cyclomatic complexity of this function is 2. { switch (x) { @@ -229,8 +253,8 @@ unittest /+ int a; } -bool shouldRun(check : BaseAnalyzer)( /+ - ^^^^^^^^^ [warn]: Cyclomatic complexity of this function is 20. +/ +// Template, other (tested) stuff +bool shouldRun(check : BaseAnalyzer)( // [warn]: Cyclomatic complexity of this function is 20. string moduleName, const ref StaticAnalysisConfig config) { enum string a = check.name; @@ -262,7 +286,157 @@ bool shouldRun(check : BaseAnalyzer)( /+ // by default: include all modules return true; } - }c, sac); + + assertAnalyzerWarningsDMD(q{ + // goto, return + void returnGoto() // [warn]: Cyclomatic complexity of this function is 3. + { + goto hello; + int a = 0; + a += 9; + + hello: + return; + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + // if, else, ternary operator + void ifElseTernary() // [warn]: Cyclomatic complexity of this function is 4. + { + if (1 > 2) + { + int a; + } + else if (2 > 1) + { + int b; + } + else + { + int c; + } + + int d = true ? 1 : 2; + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + // static if and static foreach don't increase cyclomatic complexity + void staticIfFor() // [warn]: Cyclomatic complexity of this function is 1. + { + static if (stuff) + int a; + + int b; + + static foreach(i; 0 .. 10) + { + pragma(msg, i); + } + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + // function literal (lambda) + void lambda() // [warn]: Cyclomatic complexity of this function is 2. + { + auto x = (int a) => a + 1; + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + // loops: for, foreach, while, do - while + void controlFlow() // [warn]: Cyclomatic complexity of this function is 7. + { + int x = 0; + + for (int i = 0; i < 100; i++) + { + i++; + } + + foreach (i; 0 .. 2) + { + x += i; + continue; + } + + while (true) + { + break; + } + + do + { + int x = 0; + } while (true); + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + // switch - case + void switchCaseCaseRange() // [warn]: Cyclomatic complexity of this function is 5. + { + switch (x) + { + case 1: + break; + case 2: + case 3: + break; + case 7: .. case 10: + break; + default: + break; + } + int a; + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + // if, else, logical expressions + void ifConditions() // [warn]: Cyclomatic complexity of this function is 5. + { + if (true && false) + { + doX(); + } + else if (true || false) + { + doY(); + } + } + }c, sac); + + assertAnalyzerWarningsDMD(q{ + // catch, throw + void throwCatch() // [warn]: Cyclomatic complexity of this function is 5. + { + int x; + try + { + x = 5; + } + catch (Exception e) + { + x = 7; + } + catch (Exception a) + { + x = 8; + } + catch (Exception x) + { + throw new Exception("Exception"); + } + finally + { + x = 9; + } + } + }c, sac); + stderr.writeln("Unittest for CyclomaticComplexityCheck passed."); } diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 8df7980a..9483b0d8 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -908,11 +908,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, checks ~= new UnusedResultChecker(args.setSkipTests( analysisConfig.unused_result == Check.skipTests && !ut)); - if (moduleName.shouldRun!CyclomaticComplexityCheck(analysisConfig)) - checks ~= new CyclomaticComplexityCheck(args.setSkipTests( - analysisConfig.cyclomatic_complexity == Check.skipTests && !ut), - analysisConfig.max_cyclomatic_complexity.to!int); - if (moduleName.shouldRun!BodyOnDisabledFuncsCheck(analysisConfig)) checks ~= new BodyOnDisabledFuncsCheck(args.setSkipTests( analysisConfig.body_on_disabled_func_check == Check.skipTests && !ut)); @@ -1349,6 +1344,13 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN config.if_else_same_check == Check.skipTests && !ut ); + if (moduleName.shouldRunDmd!(CyclomaticComplexityCheck!ASTCodegen)(config)) + visitors ~= new CyclomaticComplexityCheck!ASTCodegen( + fileName, + config.cyclomatic_complexity == Check.skipTests && !ut, + config.max_cyclomatic_complexity.to!int + ); + foreach (visitor; visitors) { m.accept(visitor);