Split unused variable and unused parameter checks (#768)

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;
 

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);

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()
+	Regex!char re;
-	{
-		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];
-	}
 
-	void pushScope()
+	bool inAggregateScope;
-	{
-		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;
 
 	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.");
-}
-

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.");
+}

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.");
+}