Replace libdparse with DMD in UnmodifiedFinder (#117)

src/dscanner/analysis/run.d

@@ -829,10 +829,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
 		moduleScope
 	);
 
-	if (moduleName.shouldRun!UnmodifiedFinder(analysisConfig))
-		checks ~= new UnmodifiedFinder(args.setSkipTests(
-		analysisConfig.could_be_immutable_check == Check.skipTests && !ut));
-
 	if (moduleName.shouldRun!FunctionAttributeCheck(analysisConfig))
 		checks ~= new FunctionAttributeCheck(args.setSkipTests(
 		analysisConfig.function_attribute_check == Check.skipTests && !ut));
@@ -1354,6 +1350,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
 			config.unused_variable_check == Check.skipTests && !ut
 		);
 
+	if (moduleName.shouldRunDmd!(UnmodifiedFinder!ASTCodegen)(config))
+		visitors ~= new UnmodifiedFinder!ASTCodegen(
+			fileName,
+			config.could_be_immutable_check == Check.skipTests && !ut
+		);
+
 	foreach (visitor; visitors)
 	{
 		m.accept(visitor);

src/dscanner/analysis/unmodified.d

@@ -5,378 +5,295 @@
 module dscanner.analysis.unmodified;
 
 import dscanner.analysis.base;
-import dscanner.analysis.nolint;
-import dscanner.utils : safeAccess;
-import dsymbol.scope_ : Scope;
-import std.container;
-import dparse.ast;
-import dparse.lexer;
 
 /**
  * Checks for variables that could have been declared const or immutable
  */
-final class UnmodifiedFinder : BaseAnalyzer
+// TODO: many similarities to unused_param.d, maybe refactor into a common base class
+extern (C++) class UnmodifiedFinder(AST) : BaseAnalyzerDmd
 {
-	alias visit = BaseAnalyzer.visit;
+	alias visit = BaseAnalyzerDmd.visit;
-
 	mixin AnalyzerInfo!"could_be_immutable_check";
 
-	///
+	private enum KEY = "dscanner.suspicious.unmodified";
-	this(BaseAnalyzerArguments args)
+	private enum MSG = "Variable %s is never modified and could have been declared const or immutable.";
-	{
-		super(args);
-	}
 
-	override void visit(const Module mod)
+	private static struct VarInfo
-	{
-		pushScope();
-		mod.accept(this);
-		popScope();
-	}
-
-	override void visit(const BlockStatement blockStatement)
-	{
-		pushScope();
-		blockStatementDepth++;
-		blockStatement.accept(this);
-		blockStatementDepth--;
-		popScope();
-	}
-
-	override void visit(const StructBody structBody)
-	{
-		pushScope();
-		immutable oldBlockStatementDepth = blockStatementDepth;
-		blockStatementDepth = 0;
-		structBody.accept(this);
-		blockStatementDepth = oldBlockStatementDepth;
-		popScope();
-	}
-
-	override void visit(const VariableDeclaration dec)
-	{
-		if (dec.autoDeclaration is null && blockStatementDepth > 0
-				&& isImmutable <= 0 && !canFindImmutable(dec))
-		{
-			foreach (d; dec.declarators)
-			{
-				if (initializedFromCast(d.initializer))
-					continue;
-				if (initializedFromNew(d.initializer))
-					continue;
-				tree[$ - 1].insert(new VariableInfo(d.name.text, d.name, isValueTypeSimple(dec.type)));
-			}
-		}
-		dec.accept(this);
-	}
-
-	override void visit(const AutoDeclaration autoDeclaration)
-	{
-		import std.algorithm : canFind;
-
-		if (blockStatementDepth > 0 && isImmutable <= 0
-				&& (!autoDeclaration.storageClasses.canFind!(a => a.token == tok!"const"
-					|| a.token == tok!"enum" || a.token == tok!"immutable")))
-		{
-			foreach (part; autoDeclaration.parts)
-			{
-				if (initializedFromCast(part.initializer))
-					continue;
-				if (initializedFromNew(part.initializer))
-					continue;
-				tree[$ - 1].insert(new VariableInfo(part.identifier.text, part.identifier));
-			}
-		}
-		autoDeclaration.accept(this);
-	}
-
-	override void visit(const AssignExpression assignExpression)
-	{
-		if (assignExpression.operator != tok!"")
-		{
-			interest++;
-			guaranteeUse++;
-			assignExpression.ternaryExpression.accept(this);
-			guaranteeUse--;
-			interest--;
-
-			if (assignExpression.operator == tok!"~=")
-				interest++;
-			assignExpression.expression.accept(this);
-			if (assignExpression.operator == tok!"~=")
-				interest--;
-		}
-		else
-			assignExpression.accept(this);
-	}
-
-	override void visit(const Declaration dec)
-	{
-		if (canFindImmutableOrConst(dec))
-		{
-			isImmutable++;
-			with (noLint.push(NoLintFactory.fromDeclaration(dec)))
-				dec.accept(this);
-			isImmutable--;
-		}
-		else
-		{
-			with (noLint.push(NoLintFactory.fromDeclaration(dec)))
-				dec.accept(this);
-		}
-	}
-
-	override void visit(const IdentifierChain ic)
-	{
-		if (ic.identifiers.length && interest > 0)
-			variableMightBeModified(ic.identifiers[0].text);
-		ic.accept(this);
-	}
-
-	override void visit(const IdentifierOrTemplateInstance ioti)
-	{
-		if (ioti.identifier != tok!"" && interest > 0)
-			variableMightBeModified(ioti.identifier.text);
-		ioti.accept(this);
-	}
-
-	mixin PartsMightModify!AsmPrimaryExp;
-	mixin PartsMightModify!IndexExpression;
-	mixin PartsMightModify!FunctionCallExpression;
-	mixin PartsMightModify!NewExpression;
-	mixin PartsMightModify!IdentifierOrTemplateChain;
-	mixin PartsMightModify!ReturnStatement;
-
-	override void visit(const UnaryExpression unary)
-	{
-		if (unary.prefix == tok!"++" || unary.prefix == tok!"--"
-				|| unary.suffix == tok!"++" || unary.suffix == tok!"--"
-				|| unary.prefix == tok!"*" || unary.prefix == tok!"&")
-		{
-			interest++;
-			guaranteeUse++;
-			unary.accept(this);
-			guaranteeUse--;
-			interest--;
-		}
-		else
-			unary.accept(this);
-	}
-
-	override void visit(const ForeachStatement foreachStatement)
-	{
-		if (foreachStatement.low !is null)
-		{
-			interest++;
-			foreachStatement.low.accept(this);
-			interest--;
-		}
-		if (foreachStatement.declarationOrStatement !is null)
-			foreachStatement.declarationOrStatement.accept(this);
-	}
-
-	override void visit(const TraitsExpression)
-	{
-		// issue #266: Ignore unmodified variables inside of `__traits` expressions
-	}
-
-	override void visit(const TypeofExpression)
-	{
-		// issue #270: Ignore unmodified variables inside of `typeof` expressions
-	}
-
-	override void visit(const AsmStatement a)
-	{
-		inAsm = true;
-		a.accept(this);
-		inAsm = false;
-	}
-
-private:
-
-	enum string KEY = "dscanner.suspicious.unmodified";
-
-	template PartsMightModify(T)
-	{
-		override void visit(const T t)
-		{
-			interest++;
-			t.accept(this);
-			interest--;
-		}
-	}
-
-	void variableMightBeModified(string name)
-	{
-		size_t index = tree.length - 1;
-		auto vi = VariableInfo(name);
-		if (guaranteeUse == 0)
-		{
-			auto r = tree[index].equalRange(&vi);
-			if (!r.empty && r.front.isValueType && !inAsm)
-				return;
-		}
-		while (true)
-		{
-			if (tree[index].removeKey(&vi) != 0 || index == 0)
-				break;
-			index--;
-		}
-	}
-
-	bool initializedFromNew(const Initializer initializer)
-	{
-		if (const UnaryExpression ue = cast(UnaryExpression) safeAccess(initializer)
-			.nonVoidInitializer.assignExpression)
-		{
-			return ue.newExpression !is null;
-		}
-		return false;
-	}
-
-	bool initializedFromCast(const Initializer initializer)
-	{
-		import std.typecons : scoped;
-
-		static class CastFinder : ASTVisitor
-		{
-			alias visit = ASTVisitor.visit;
-			override void visit(const CastExpression castExpression)
-			{
-				foundCast = true;
-				castExpression.accept(this);
-			}
-
-			bool foundCast;
-		}
-
-		if (initializer is null)
-			return false;
-		auto finder = scoped!CastFinder();
-		finder.visit(initializer);
-		return finder.foundCast;
-	}
-
-	bool canFindImmutableOrConst(const Declaration dec)
-	{
-		import std.algorithm : canFind, map, filter;
-
-		return !dec.attributes.map!(a => a.attribute)
-			.filter!(a => a == tok!"immutable" || a == tok!"const").empty;
-	}
-
-	bool canFindImmutable(const VariableDeclaration dec)
-	{
-		import std.algorithm : canFind;
-
-		foreach (storageClass; dec.storageClasses)
-		{
-			if (storageClass.token == tok!"enum")
-				return true;
-		}
-		foreach (sc; dec.storageClasses)
-		{
-			if (sc.token == tok!"immutable" || sc.token == tok!"const")
-				return true;
-		}
-		if (dec.type !is null)
-		{
-			foreach (tk; dec.type.typeConstructors)
-				if (tk == tok!"immutable" || tk == tok!"const")
-					return true;
-			if (dec.type.type2)
-			{
-				const tk = dec.type.type2.typeConstructor;
-				if (tk == tok!"immutable" || tk == tok!"const")
-					return true;
-			}
-		}
-		return false;
-	}
-
-	static struct VariableInfo
 	{
 		string name;
-		Token token;
+		ulong lineNum;
-		bool isValueType;
+		ulong charNum;
+		bool isUsed = false;
 	}
 
-	void popScope()
+	private alias VarSet = VarInfo[string];
+	private VarSet[] usedVars;
+	private bool inAggregate;
+
+	extern (D) this(string fileName, bool skipTests = false)
 	{
-		foreach (vi; tree[$ - 1])
+		super(fileName, skipTests);
+		pushScope();
+	}
+
+	override void visit(AST.CompoundStatement compoundStatement)
+	{
+		pushScope();
+		super.visit(compoundStatement);
+		popScope();
+	}
+
+	override void visit(AST.TemplateDeclaration templateDeclaration)
+	{
+		auto oldInTemplate = inAggregate;
+		inAggregate = true;
+		super.visit(templateDeclaration);
+		inAggregate = oldInTemplate;
+	}
+
+	override void visit(AST.StructDeclaration structDecl)
+	{
+		auto oldInAggregate = inAggregate;
+		inAggregate = true;
+		super.visit(structDecl);
+		inAggregate = oldInAggregate;
+	}
+
+	override void visit(AST.VarDeclaration varDeclaration)
+	{
+		import dmd.astenums : STC;
+
+		super.visit(varDeclaration);
+
+		if (varDeclaration.ident is null)
+			return;
+
+		string varName = cast(string) varDeclaration.ident.toString();
+		bool isConst = varDeclaration.storage_class & STC.const_ || varDeclaration.storage_class & STC.immutable_
+			|| varDeclaration.storage_class & STC.manifest || isConstType(varDeclaration.type);
+
+		bool markAsUsed = isConst || isFromCastOrNew(varDeclaration._init) || inAggregate;
+		currentScope[varName] = VarInfo(varName, varDeclaration.loc.linnum, varDeclaration.loc.charnum, markAsUsed);
+	}
+
+	private bool isConstType(AST.Type type)
+	{
+		import dmd.astenums : MODFlags;
+
+		if (type is null)
+			return false;
+
+		bool isConst = type.mod & MODFlags.const_ || type.mod & MODFlags.immutable_;
+
+		if (auto typePtr = type.isTypePointer())
+			isConst = isConst || typePtr.next.mod & MODFlags.const_ || typePtr.next.mod & MODFlags.immutable_;
+
+		return isConst;
+	}
+
+	private bool isFromCastOrNew(AST.Initializer initializer)
+	{
+		if (initializer is null)
+			return false;
+
+		auto initExpr = initializer.isExpInitializer();
+		if (initExpr is null)
+			return false;
+
+		return initExpr.exp.isNewExp() !is null || initExpr.exp.isCastExp() !is null;
+	}
+
+	override void visit(AST.IntervalExp intervalExp)
+	{
+		super.visit(intervalExp);
+
+		auto identifier1 = intervalExp.lwr.isIdentifierExp();
+		if (identifier1 && identifier1.ident)
+			markAsUsed(cast(string) identifier1.ident.toString());
+
+		auto identifier2 = intervalExp.upr.isIdentifierExp();
+		if (identifier2 && identifier2.ident)
+			markAsUsed(cast(string) identifier2.ident.toString());
+	}
+
+	override void visit(AST.IndexExp indexExpression)
+	{
+		super.visit(indexExpression);
+
+		auto identifier1 = indexExpression.e1.isIdentifierExp();
+		if (identifier1 && identifier1.ident)
+			markAsUsed(cast(string) identifier1.ident.toString());
+
+		auto identifier2 = indexExpression.e2.isIdentifierExp();
+		if (identifier2 && identifier2.ident)
+			markAsUsed(cast(string) identifier2.ident.toString());
+	}
+
+	mixin VisitAssignNode!(AST.AssignExp);
+	mixin VisitAssignNode!(AST.BinAssignExp);
+	mixin VisitAssignNode!(AST.PtrExp);
+	mixin VisitAssignNode!(AST.AddrExp);
+	mixin VisitAssignNode!(AST.PreExp);
+	mixin VisitAssignNode!(AST.PostExp);
+
+	private template VisitAssignNode(NodeType)
+	{
+		override void visit(NodeType node)
 		{
-			immutable string errorMessage = "Variable " ~ vi.name
+			super.visit(node);
-				~ " is never modified and could have been declared const or immutable.";
+
-			addErrorMessage(vi.token, KEY, errorMessage);
+			if (node.e1 is null)
+				return;
+
+			auto identifier = node.e1.isIdentifierExp();
+			if (identifier && identifier.ident)
+				markAsUsed(cast(string) identifier.ident.toString());
 		}
-		tree = tree[0 .. $ - 1];
 	}
 
-	void pushScope()
+	mixin VisitFunctionNode!(AST.CallExp);
+	mixin VisitFunctionNode!(AST.NewExp);
+
+	private template VisitFunctionNode(NodeType)
 	{
-		tree ~= new RedBlackTree!(VariableInfo*, "a.name < b.name");
+		override void visit(NodeType node)
+		{
+			super.visit(node);
+
+			if (node.arguments is null)
+				return;
+
+			foreach (arg; *node.arguments)
+			{
+				auto identifier = arg.isIdentifierExp();
+				if (identifier && identifier.ident)
+					markAsUsed(cast(string) arg.isIdentifierExp().ident.toString());
+			}
+		}
 	}
 
-	int blockStatementDepth;
+	mixin VisitDotExpressionNode!(AST.DotIdExp);
+	mixin VisitDotExpressionNode!(AST.DotTemplateInstanceExp);
 
-	int interest;
+	private template VisitDotExpressionNode(NodeType)
+	{
+		override void visit(NodeType node)
+		{
+			super.visit(node);
+			auto identifierExp = node.e1.isIdentifierExp();
+			if (identifierExp && identifierExp.ident)
+				markAsUsed(cast(string) identifierExp.ident.toString());
+		}
+	}
 
-	int guaranteeUse;
+	private extern (D) void markAsUsed(string varName)
+	{
+		import std.range : retro;
 
-	int isImmutable;
+		foreach (funcScope; usedVars.retro())
+		{
+			if (varName in funcScope)
+			{
+				funcScope[varName].isUsed = true;
+				break;
+			}
+		}
+	}
 
-	bool inAsm;
+	@property private extern (D) VarSet currentScope()
+	{
+		return usedVars[$ - 1];
+	}
 
-	RedBlackTree!(VariableInfo*, "a.name < b.name")[] tree;
+	private void pushScope()
+	{
+		// Error with gdc-12
+		//usedVars ~= new VarSet;
+
+		// Workaround for gdc-12
+		VarSet newScope;
+		newScope["test"] = VarInfo("test", 0, 0);
+		usedVars ~= newScope;
+		currentScope.remove("test");
+	}
+
+	private void popScope()
+	{
+		import std.algorithm : each, filter;
+		import std.format : format;
+
+		currentScope.byValue
+			.filter!(var => !var.isUsed)
+			.each!(var => addErrorMessage(var.lineNum, var.charNum, KEY, MSG.format(var.name)));
+
+		usedVars.length--;
+	}
 }
 
-bool isValueTypeSimple(const Type type) pure nothrow @nogc
+unittest
-{
-	if (type.type2 is null)
-		return false;
-	return type.type2.builtinType != tok!"" && type.typeSuffixes.length == 0;
-}
-
-@system unittest
 {
 	import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
-	import dscanner.analysis.helpers : assertAnalyzerWarnings;
+	import dscanner.analysis.helpers : assertAnalyzerWarningsDMD;
 	import std.stdio : stderr;
-	import std.format : format;
 
 	StaticAnalysisConfig sac = disabledConfig();
 	sac.could_be_immutable_check = Check.enabled;
 
 	// fails
-
+	assertAnalyzerWarningsDMD(q{
-	assertAnalyzerWarnings(q{
+		void foo()
-		void foo(){int i = 1;} /+
+		{
-		               ^ [warn]: Variable i is never modified and could have been declared const or immutable. +/
+			int i = 1; // [warn]: Variable i is never modified and could have been declared const or immutable.
+		}
 	}, sac);
 
+	assertAnalyzerWarningsDMD(q{
+		void foo()
+		{
+			int i = 5; // [warn]: Variable i is never modified and could have been declared const or immutable.
+			int j = 6;
+			j = i + 5;
+		}
+	}c, sac);
+
 	// pass
-
+	assertAnalyzerWarningsDMD(q{
-	assertAnalyzerWarnings(q{
+		void foo()
-		void foo(){const(int) i;}
+		{
+			const(int) i;
+			const int j;
+			const(int)* a;
+			const int* b;
+		}
 	}, sac);
 
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
-		void foo(){immutable(int)* i;}
+		void foo()
+		{
+			immutable(int) i;
+			immutable int j;
+			immutable(int)* b;
+			immutable int* a;
+		}
 	}, sac);
 
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
-		void foo(){enum i = 1;}
+		void foo()
+		{
+			enum i = 1;
+			enum string j = "test";
+		}
 	}, sac);
 
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
-		void foo(){E e = new E;}
+		void foo()
+		{
+			E e = new E;
+			auto f = new F;
+		}
 	}, sac);
 
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
-		void foo(){auto e = new E;}
-	}, sac);
-
-	assertAnalyzerWarnings(q{
 		void issue640()
 		{
 			size_t i1;
@@ -387,11 +304,25 @@ bool isValueTypeSimple(const Type type) pure nothrow @nogc
 		}
 	}, sac);
 
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
-		@("nolint(dscanner.suspicious.unmodified)")
+		void foo()
-		void foo(){
+		{
-			int i = 1;
+			int i = 5; // [warn]: Variable i is never modified and could have been declared const or immutable.
+			int j = 6;
+			j = i + 5;
 		}
-	}, sac);
+	}c, sac);
-}
 
+	assertAnalyzerWarningsDMD(q{
+		void foo()
+		{
+			int i = 5;
+			if (true)
+				--i;
+			else
+				i++;
+		}
+	}c, sac);
+
+	stderr.writeln("Unittest for UnmodifiedFinder passed.");
+}