From 9502af24945d2db88e20072da76c34549b646d17 Mon Sep 17 00:00:00 2001 From: Eugene Wissner Date: Tue, 2 Jul 2019 22:56:52 +0200 Subject: [PATCH] Split unused variable and unused parameter checks (#768) --- src/dscanner/analysis/config.d | 5 +- src/dscanner/analysis/run.d | 7 +- src/dscanner/analysis/unused.d | 231 ++++------------------- src/dscanner/analysis/unused_parameter.d | 122 ++++++++++++ src/dscanner/analysis/unused_variable.d | 126 +++++++++++++ 5 files changed, 290 insertions(+), 201 deletions(-) create mode 100644 src/dscanner/analysis/unused_parameter.d create mode 100644 src/dscanner/analysis/unused_variable.d diff --git a/src/dscanner/analysis/config.d b/src/dscanner/analysis/config.d index 0caecc9..b0e5be3 100644 --- a/src/dscanner/analysis/config.d +++ b/src/dscanner/analysis/config.d @@ -86,12 +86,15 @@ struct StaticAnalysisConfig @INI("Checks for some problems with constructors") string constructor_check = Check.enabled; - @INI("Checks for unused variables and function parameters") + @INI("Checks for unused variables") string unused_variable_check = Check.enabled; @INI("Checks for unused labels") string unused_label_check = Check.enabled; + @INI("Checks for unused function parameters") + string unused_parameter_check = Check.enabled; + @INI("Checks for duplicate attributes") string duplicate_attribute = Check.enabled; diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index a95b237..6f19b54 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -37,8 +37,9 @@ import dscanner.analysis.objectconst; import dscanner.analysis.range; import dscanner.analysis.ifelsesame; import dscanner.analysis.constructors; -import dscanner.analysis.unused; +import dscanner.analysis.unused_variable; import dscanner.analysis.unused_label; +import dscanner.analysis.unused_parameter; import dscanner.analysis.duplicate_attribute; import dscanner.analysis.opequals_without_tohash; import dscanner.analysis.length_subtraction; @@ -441,6 +442,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new UnusedVariableCheck(fileName, moduleScope, analysisConfig.unused_variable_check == Check.skipTests && !ut); + if (moduleName.shouldRun!"unused_parameter_check"(analysisConfig)) + checks ~= new UnusedParameterCheck(fileName, moduleScope, + analysisConfig.unused_parameter_check == Check.skipTests && !ut); + if (moduleName.shouldRun!"long_line_check"(analysisConfig)) checks ~= new LineLengthCheck(fileName, tokens, analysisConfig.long_line_check == Check.skipTests && !ut); diff --git a/src/dscanner/analysis/unused.d b/src/dscanner/analysis/unused.d index 2686faa..97ac8dc 100644 --- a/src/dscanner/analysis/unused.d +++ b/src/dscanner/analysis/unused.d @@ -10,13 +10,12 @@ import dscanner.analysis.base; import std.container; import std.regex : Regex, regex, matchAll; import dsymbol.scope_ : Scope; -import std.algorithm.iteration : map; import std.algorithm : all; /** * Checks for unused variables. */ -final class UnusedVariableCheck : BaseAnalyzer +abstract class UnusedIdentifierCheck : BaseAnalyzer { alias visit = BaseAnalyzer.visit; @@ -266,13 +265,6 @@ final class UnusedVariableCheck : BaseAnalyzer inAggregateScope = sb; } - override void visit(const VariableDeclaration variableDeclaration) - { - foreach (d; variableDeclaration.declarators) - this.variableDeclared(d.name.text, d.name.line, d.name.column, false, false); - variableDeclaration.accept(this); - } - override void visit(const Type2 tp) { if (tp.typeIdentifierPart && @@ -293,13 +285,6 @@ final class UnusedVariableCheck : BaseAnalyzer tp.accept(this); } - override void visit(const AutoDeclaration autoDeclaration) - { - foreach (t; autoDeclaration.parts.map!(a => a.identifier)) - this.variableDeclared(t.text, t.line, t.column, false, false); - autoDeclaration.accept(this); - } - override void visit(const WithStatement withStatetement) { interestDepth++; @@ -310,32 +295,6 @@ final class UnusedVariableCheck : BaseAnalyzer withStatetement.declarationOrStatement.accept(this); } - override void visit(const Parameter parameter) - { - import std.algorithm : among; - import std.algorithm.iteration : filter; - import std.range : empty; - import std.array : array; - - if (parameter.name != tok!"") - { - immutable bool isRef = !parameter.parameterAttributes - .filter!(a => a.idType.among(tok!"ref", tok!"out")).empty; - immutable bool isPtr = parameter.type && !parameter.type - .typeSuffixes.filter!(a => a.star != tok!"").empty; - - variableDeclared(parameter.name.text, parameter.name.line, - parameter.name.column, true, isRef | isPtr); - - if (parameter.default_ !is null) - { - interestDepth++; - parameter.default_.accept(this); - interestDepth--; - } - } - } - override void visit(const StructBody structBody) { immutable bool sb = inAggregateScope; @@ -371,8 +330,37 @@ final class UnusedVariableCheck : BaseAnalyzer // issue #270: Ignore unused variables inside of `typeof` expressions } + abstract protected void popScope(); + + protected uint interestDepth; + + protected Tree[] tree; + + protected void variableDeclared(string name, size_t line, size_t column, bool isRef) + { + if (inAggregateScope || name.all!(a => a == '_')) + return; + tree[$ - 1].insert(new UnUsed(name, line, column, isRef)); + } + + protected void pushScope() + { + tree ~= new Tree; + } + private: + struct UnUsed + { + string name; + size_t line; + size_t column; + bool isRef; + bool uncertain; + } + + alias Tree = RedBlackTree!(UnUsed*, "a.name < b.name"); + mixin template PartsUseVariables(NodeType) { override void visit(const NodeType node) @@ -383,13 +371,6 @@ private: } } - void variableDeclared(string name, size_t line, size_t column, bool isParameter, bool isRef) - { - if (inAggregateScope || name.all!(a => a == '_')) - return; - tree[$ - 1].insert(new UnUsed(name, line, column, isParameter, isRef)); - } - void variableUsed(string name) { size_t treeIndex = tree.length - 1; @@ -402,161 +383,13 @@ private: } } - void popScope() - { - foreach (uu; tree[$ - 1]) - { - if (!uu.isRef && tree.length > 1) - { - if (uu.uncertain) - continue; - immutable string certainty = uu.uncertain ? " might not be used." - : " is never used."; - immutable string errorMessage = (uu.isParameter ? "Parameter " : "Variable ") - ~ uu.name ~ certainty; - addErrorMessage(uu.line, uu.column, uu.isParameter ? "dscanner.suspicious.unused_parameter" - : "dscanner.suspicious.unused_variable", errorMessage); - } - } - tree = tree[0 .. $ - 1]; - } + Regex!char re; - void pushScope() - { - tree ~= new RedBlackTree!(UnUsed*, "a.name < b.name"); - } - - struct UnUsed - { - string name; - size_t line; - size_t column; - bool isParameter; - bool isRef; - bool uncertain; - } - - RedBlackTree!(UnUsed*, "a.name < b.name")[] tree; - - uint interestDepth; + bool inAggregateScope; uint mixinDepth; bool isOverride; - bool inAggregateScope; - bool blockStatementIntroducesScope = true; - - Regex!char re; } - -@system unittest -{ - import std.stdio : stderr; - import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; - - StaticAnalysisConfig sac = disabledConfig(); - sac.unused_variable_check = Check.enabled; - assertAnalyzerWarnings(q{ - - // Issue 274 - unittest - { - size_t byteIndex = 0; - *(cast(FieldType*)(retVal.ptr + byteIndex)) = item; - } - - // bug encountered after correct DIP 1009 impl in dparse - version (StdDdoc) - { - bool isAbsolute(R)(R path) pure nothrow @safe - if (isRandomAccessRange!R && isSomeChar!(ElementType!R) || - is(StringTypeOf!R)); - } - - unittest - { - int a; // [warn]: Variable a is never used. - } - - void inPSC(in int a){} // [warn]: Parameter a is never used. - - // Issue 380 - int templatedEnum() - { - enum a(T) = T.init; - return a!int; - } - - // Issue 380 - int otherTemplatedEnum() - { - auto a(T) = T.init; // [warn]: Variable a is never used. - return 0; - } - - void doStuff(int a, int b) // [warn]: Parameter b is never used. - { - return a; - } - - // Issue 364 - void test364_1() - { - enum s = 8; - immutable t = 2; - int[s][t] a; - a[0][0] = 1; - } - - void test364_2() - { - enum s = 8; - alias a = e!s; - a = 1; - } - - // Issue 352 - void test352_1() - { - void f(int *x) {*x = 1;} - } - - void test352_2() - { - void f(Bat** bat) {*bat = bats.ptr + 8;} - } - - // Issue 490 - void test490() - { - auto cb1 = delegate(size_t _) {}; - cb1(3); - auto cb2 = delegate(size_t a) {}; // [warn]: Parameter a is never used. - cb2(3); - } - - void oops () - { - class Identity { int val; } - Identity v; - v.val = 0; - } - - bool hasDittos(int decl) - { - mixin("decl++;"); - } - - void main() - { - const int testValue; - testValue.writeln; - } - - }c, sac); - stderr.writeln("Unittest for UnusedVariableCheck passed."); -} - diff --git a/src/dscanner/analysis/unused_parameter.d b/src/dscanner/analysis/unused_parameter.d new file mode 100644 index 0000000..01f3b29 --- /dev/null +++ b/src/dscanner/analysis/unused_parameter.d @@ -0,0 +1,122 @@ +// Copyright Brian Schott (Hackerpilot) 2014-2015. +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) +module dscanner.analysis.unused_parameter; + +import dparse.ast; +import dparse.lexer; +import dscanner.analysis.unused; +import dsymbol.scope_ : Scope; + +/** + * Checks for unused variables. + */ +final class UnusedParameterCheck : UnusedIdentifierCheck +{ + alias visit = UnusedIdentifierCheck.visit; + + /** + * Params: + * fileName = the name of the file being analyzed + */ + this(string fileName, const(Scope)* sc, bool skipTests = false) + { + super(fileName, sc, skipTests); + } + + override void visit(const Parameter parameter) + { + import std.algorithm : among; + import std.algorithm.iteration : filter; + import std.range : empty; + + if (parameter.name != tok!"") + { + immutable bool isRef = !parameter.parameterAttributes + .filter!(a => a.idType.among(tok!"ref", tok!"out")).empty; + immutable bool isPtr = parameter.type && !parameter.type + .typeSuffixes.filter!(a => a.star != tok!"").empty; + + variableDeclared(parameter.name.text, parameter.name.line, + parameter.name.column, isRef | isPtr); + + if (parameter.default_ !is null) + { + interestDepth++; + parameter.default_.accept(this); + interestDepth--; + } + } + } + + override protected void popScope() + { + foreach (uu; tree[$ - 1]) + { + if (!uu.isRef && tree.length > 1) + { + if (uu.uncertain) + continue; + immutable string errorMessage = "Parameter " ~ uu.name ~ " is never used."; + addErrorMessage(uu.line, uu.column, + "dscanner.suspicious.unused_parameter", errorMessage); + } + } + tree = tree[0 .. $ - 1]; + } +} + +@system unittest +{ + import std.stdio : stderr; + import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings; + + StaticAnalysisConfig sac = disabledConfig(); + sac.unused_parameter_check = Check.enabled; + assertAnalyzerWarnings(q{ + + // bug encountered after correct DIP 1009 impl in dparse + version (StdDdoc) + { + bool isAbsolute(R)(R path) pure nothrow @safe + if (isRandomAccessRange!R && isSomeChar!(ElementType!R) || + is(StringTypeOf!R)); + } + + void inPSC(in int a){} // [warn]: Parameter a is never used. + + void doStuff(int a, int b) // [warn]: Parameter b is never used. + { + return a; + } + + // Issue 352 + void test352_1() + { + void f(int *x) {*x = 1;} + } + + void test352_2() + { + void f(Bat** bat) {*bat = bats.ptr + 8;} + } + + // Issue 490 + void test490() + { + auto cb1 = delegate(size_t _) {}; + cb1(3); + auto cb2 = delegate(size_t a) {}; // [warn]: Parameter a is never used. + cb2(3); + } + + bool hasDittos(int decl) + { + mixin("decl++;"); + } + + }c, sac); + stderr.writeln("Unittest for UnusedParameterCheck passed."); +} diff --git a/src/dscanner/analysis/unused_variable.d b/src/dscanner/analysis/unused_variable.d new file mode 100644 index 0000000..8b03cb1 --- /dev/null +++ b/src/dscanner/analysis/unused_variable.d @@ -0,0 +1,126 @@ +// Copyright Brian Schott (Hackerpilot) 2014-2015. +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) +module dscanner.analysis.unused_variable; + +import dparse.ast; +import dscanner.analysis.unused; +import dsymbol.scope_ : Scope; +import std.algorithm.iteration : map; + +/** + * Checks for unused variables. + */ +final class UnusedVariableCheck : UnusedIdentifierCheck +{ + alias visit = UnusedIdentifierCheck.visit; + + /** + * Params: + * fileName = the name of the file being analyzed + */ + this(string fileName, const(Scope)* sc, bool skipTests = false) + { + super(fileName, sc, skipTests); + } + + override void visit(const VariableDeclaration variableDeclaration) + { + foreach (d; variableDeclaration.declarators) + this.variableDeclared(d.name.text, d.name.line, d.name.column, false); + variableDeclaration.accept(this); + } + + override void visit(const AutoDeclaration autoDeclaration) + { + foreach (t; autoDeclaration.parts.map!(a => a.identifier)) + this.variableDeclared(t.text, t.line, t.column, false); + autoDeclaration.accept(this); + } + + override protected void popScope() + { + foreach (uu; tree[$ - 1]) + { + if (!uu.isRef && tree.length > 1) + { + if (uu.uncertain) + continue; + immutable string errorMessage = "Variable " ~ uu.name ~ " is never used."; + addErrorMessage(uu.line, uu.column, + "dscanner.suspicious.unused_variable", errorMessage); + } + } + tree = tree[0 .. $ - 1]; + } +} + +@system unittest +{ + import std.stdio : stderr; + import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings; + + StaticAnalysisConfig sac = disabledConfig(); + sac.unused_variable_check = Check.enabled; + assertAnalyzerWarnings(q{ + + // Issue 274 + unittest + { + size_t byteIndex = 0; + *(cast(FieldType*)(retVal.ptr + byteIndex)) = item; + } + + unittest + { + int a; // [warn]: Variable a is never used. + } + + // Issue 380 + int templatedEnum() + { + enum a(T) = T.init; + return a!int; + } + + // Issue 380 + int otherTemplatedEnum() + { + auto a(T) = T.init; // [warn]: Variable a is never used. + return 0; + } + + // Issue 364 + void test364_1() + { + enum s = 8; + immutable t = 2; + int[s][t] a; + a[0][0] = 1; + } + + void test364_2() + { + enum s = 8; + alias a = e!s; + a = 1; + } + + void oops () + { + class Identity { int val; } + Identity v; + v.val = 0; + } + + void main() + { + const int testValue; + testValue.writeln; + } + + }c, sac); + stderr.writeln("Unittest for UnusedVariableCheck passed."); +}