Replace libdparse with DMD in MismatchedArgumentCheck (#161)

src/dscanner/analysis/mismatched_args.d

@@ -1,263 +1,186 @@
 module dscanner.analysis.mismatched_args;
 
 import dscanner.analysis.base;
-import dscanner.utils : safeAccess;
+import std.format : format;
-import dsymbol.scope_;
+import dmd.arraytypes : Identifiers;
-import dsymbol.symbol;
-import dparse.ast;
-import dparse.lexer : tok, Token;
-import dsymbol.builtin.names;
 
 /// Checks for mismatched argument and parameter names
-final class MismatchedArgumentCheck : BaseAnalyzer
+extern (C++) class MismatchedArgumentCheck(AST) : BaseAnalyzerDmd
 {
+	alias visit = BaseAnalyzerDmd.visit;
 	mixin AnalyzerInfo!"mismatched_args_check";
 
-	///
+	private enum string KEY = "dscanner.confusing.argument_parameter_mismatch";
-	this(BaseAnalyzerArguments args)
+	private enum string MSG = "Argument %d is named '%s', but this is the name of parameter %d";
+
+	private string[][string] funcsWithParams;
+	private bool inFunction;
+
+	extern (D) this(string fileName, bool skipTests = false)
 	{
-		super(args);
+		super(fileName, skipTests);
 	}
 
-	override void visit(const FunctionCallExpression fce)
+	override void visit(AST.Module moduleNode)
 	{
-		import std.typecons : scoped;
+		import dmd.astcodegen : ASTCodegen;
-		import std.algorithm.iteration : each, map;
-		import std.array : array;
 
-		if (fce.arguments is null)
+		auto argVisitor = new ParameterVisitor!(ASTCodegen)(fileName, skipTests);
+		argVisitor.visit(moduleNode);
+		funcsWithParams = argVisitor.funcsWithParams;
+
+		super.visit(moduleNode);
+	}
+
+	override void visit(AST.CallExp callExpr)
+	{
+		super.visit(callExpr);
+
+		auto funcIdent = callExpr.e1.isIdentifierExp();
+		if (callExpr.arguments is null || funcIdent is null || funcIdent.ident is null)
 			return;
-		auto argVisitor = scoped!ArgVisitor;
-		argVisitor.visit(fce.arguments);
-		const istring[] args = argVisitor.args;
 
-		auto identVisitor = scoped!IdentVisitor;
+		string funcName = cast(string) funcIdent.ident.toString();
-		if (fce.unaryExpression !is null)
+		if ((funcName in funcsWithParams) is null || (*callExpr.arguments).length != funcsWithParams[funcName].length)
-			identVisitor.visit(fce.unaryExpression);
+			return;
-		else if (fce.type !is null)
-			identVisitor.visit(fce.type);
 
-		const(DSymbol)*[] symbols = resolveSymbol(sc, identVisitor.names.length > 0
+		auto namedArgsPositions = getNamedArgsPositions(callExpr.names, funcName);
-				? identVisitor.names : [CONSTRUCTOR_SYMBOL_NAME]);
+		string[] args;
 
-		static struct ErrorMessage
+		foreach (int idx, arg; *callExpr.arguments)
 		{
-			const(Token)[] range;
+			if (auto identifier = arg.isIdentifierExp())
-			string message;
+			{
+				if (identifier.ident)
+				{
+					if ((idx in namedArgsPositions) is null)
+						args ~= cast(string) identifier.ident.toString();
+					else
+						args ~= "";
 				}
-
-		ErrorMessage[] messages;
-		bool matched;
-
-		foreach (sym; symbols)
-		{
-			// The cast is a hack because .array() confuses the compiler's overload
-			// resolution code.
-			const(istring)[] params = sym is null ? [] : sym.argNames[].map!(a => cast() a).array();
-			const ArgMismatch[] mismatches = compareArgsToParams(params, args);
-			if (mismatches.length == 0)
-				matched = true;
 				else
 				{
-				foreach (ref const mm; mismatches)
-				{
-					messages ~= ErrorMessage(argVisitor.tokens[mm.argIndex], createWarningFromMismatch(mm));
-				}
-			}
-		}
-
-		if (!matched)
-			foreach (m; messages)
-				addErrorMessage(m.range, KEY, m.message);
-	}
-
-	alias visit = ASTVisitor.visit;
-
-private:
-
-	enum string KEY = "dscanner.confusing.argument_parameter_mismatch";
-}
-
-final class IdentVisitor : ASTVisitor
-{
-	override void visit(const IdentifierOrTemplateInstance ioti)
-	{
-		import dsymbol.string_interning : internString;
-
-		if (ioti.identifier != tok!"")
-			names ~= internString(ioti.identifier.text);
-		else
-			names ~= internString(ioti.templateInstance.identifier.text);
-	}
-
-	override void visit(const Arguments)
-	{
-	}
-
-	override void visit(const IndexExpression ie)
-	{
-		if (ie.unaryExpression !is null)
-			visit(ie.unaryExpression);
-	}
-
-	alias visit = ASTVisitor.visit;
-
-	istring[] names;
-}
-
-final class ArgVisitor : ASTVisitor
-{
-	override void visit(const NamedArgumentList al)
-	{
-		foreach (a; al.items)
-		{
-			auto u = cast(UnaryExpression) a.assignExpression;
-			size_t prevArgs = args.length;
-			if (u !is null && !a.name.text.length)
-				visit(u);
-
-			if (args.length == prevArgs)
-			{
-				// if we didn't get an identifier in the unary expression,
-				// assume it's a good argument
-				args ~= istring.init;
-				tokens ~= a.tokens;
-			}
-		}
-	}
-
-	override void visit(const UnaryExpression unary)
-	{
-		import dsymbol.string_interning : internString;
-
-		if (auto iot = unary.safeAccess.primaryExpression.identifierOrTemplateInstance.unwrap)
-		{
-			if (iot.identifier == tok!"")
 					return;
-			immutable t = iot.identifier;
-			tokens ~= [t];
-			args ~= internString(t.text);
 				}
 			}
-
+			else
-	alias visit = ASTVisitor.visit;
-
-	const(Token[])[] tokens;
-	istring[] args;
-}
-
-const(DSymbol)*[] resolveSymbol(const Scope* sc, const istring[] symbolChain)
-{
-	import std.array : empty;
-
-	const(DSymbol)*[] matchingSymbols = sc.getSymbolsByName(symbolChain[0]);
-	if (matchingSymbols.empty)
-		return null;
-
-	foreach (ref symbol; matchingSymbols)
 			{
-		inner: foreach (i; 1 .. symbolChain.length)
+				args ~= "";
-		{
-			if (symbol.kind == CompletionKind.variableName
-					|| symbol.kind == CompletionKind.memberVariableName
-					|| symbol.kind == CompletionKind.functionName
-					|| symbol.kind == CompletionKind.aliasName)
-				symbol = symbol.type;
-			if (symbol is null)
-			{
-				symbol = null;
-				break inner;
-			}
-			auto p = symbol.getPartsByName(symbolChain[i]);
-			if (p.empty)
-			{
-				symbol = null;
-				break inner;
-			}
-			symbol = p[0];
 			}
 		}
-	return matchingSymbols;
-}
 
-struct ArgMismatch
+		foreach_reverse (argIdx, arg; args)
-{
-	size_t argIndex;
-	size_t paramIndex;
-	string name;
-}
-
-immutable(ArgMismatch[]) compareArgsToParams(const istring[] params, const istring[] args) pure
-{
-	import std.exception : assumeUnique;
-
-	if (args.length != params.length)
-		return [];
-	ArgMismatch[] retVal;
-	foreach (i, arg; args)
 		{
-		if (arg is null || arg == params[i])
+			foreach_reverse (paramIdx, param; funcsWithParams[funcName])
+			{
+				if (arg == param)
+				{
+					if (argIdx == paramIdx)
+						break;
+
+					addErrorMessage(callExpr.loc.linnum, callExpr.loc.charnum,KEY,
+						MSG.format(argIdx + 1, arg, paramIdx + 1));
+
+					return;
+				}
+			}
+		}
+	}
+
+	private extern (D) bool[int] getNamedArgsPositions(Identifiers* names, string funcName)
+	{
+		bool[int] namedArgsPositions;
+
+		if (names is null || (funcName in funcsWithParams) is null)
+			return namedArgsPositions;
+
+		auto funcParams = funcsWithParams[funcName];
+
+		foreach (name; *names)
+		{
+			if (name is null)
 				continue;
-		foreach (j, param; params)
+
-			if (param == arg)
+			string argName = cast(string) name.toString();
-				retVal ~= ArgMismatch(i, j, arg);
+			int idx;
+			for (idx = 0; idx < funcParams.length; idx++)
+				if (funcParams[idx] == argName)
+					break;
+
+			namedArgsPositions[idx] = true;
+		}
+
+		return namedArgsPositions;
 	}
-	return assumeUnique(retVal);
 }
 
-string createWarningFromMismatch(const ArgMismatch mismatch) pure
+extern (C++) class ParameterVisitor(AST) : BaseAnalyzerDmd
 {
-	import std.format : format;
+	alias visit = BaseAnalyzerDmd.visit;
 
-	return "Argument %d is named '%s', but this is the name of parameter %d".format(
+	public string[][string] funcsWithParams;
-			mismatch.argIndex + 1, mismatch.name, mismatch.paramIndex + 1);
+	private string currentFunc;
-}
+	private bool ignoreCurrentFunc;
-
+	private bool inFunction;
-unittest
-{
-	import dsymbol.string_interning : internString;
-	import std.algorithm.iteration : map;
-	import std.array : array;
-	import std.conv : to;
 
+	extern (D) this(string fileName, bool skipTests = false)
 	{
-		istring[] args = ["a", "b", "c"].map!internString().array();
+		super(fileName, skipTests);
-		istring[] params = ["a", "b", "c"].map!internString().array();
-		immutable res = compareArgsToParams(params, args);
-		assert(res == []);
 	}
 
+	override void visit(AST.TemplateDeclaration templateDecl)
 	{
-		istring[] args = ["a", "c", "b"].map!internString().array();
+		if (inFunction)
-		istring[] params = ["a", "b", "c"].map!internString().array();
+			return;
-		immutable res = compareArgsToParams(params, args);
+
-		assert(res == [ArgMismatch(1, 2, "c"), ArgMismatch(2, 1, "b")], to!string(res));
+		inFunction = true;
+		super.visit(templateDecl);
 	}
 
+	override void visit(AST.FuncDeclaration funcDecl)
 	{
-		istring[] args = ["a", "c", "b"].map!internString().array();
+		if (inFunction)
-		istring[] params = ["alpha", "bravo", "c"].map!internString().array();
+			return;
-		immutable res = compareArgsToParams(params, args);
+
-		assert(res == [ArgMismatch(1, 2, "c")]);
+		inFunction = true;
+		string lastFunc = currentFunc;
+		currentFunc = cast(string) funcDecl.ident.toString();
+		funcsWithParams[currentFunc] = [];
+
+		bool ignoreLast = ignoreCurrentFunc;
+		ignoreCurrentFunc = false;
+
+		super.visit(funcDecl);
+
+		if (ignoreCurrentFunc)
+			funcsWithParams.remove(currentFunc);
+
+		ignoreCurrentFunc = ignoreLast;
+		currentFunc = lastFunc;
 	}
 
+	override void visit(AST.Parameter parameter)
 	{
-		istring[] args = ["a", "b"].map!internString().array();
+		if (parameter.ident is null)
-		istring[] params = [null, "b"].map!internString().array();
+		{
-		immutable res = compareArgsToParams(params, args);
+			ignoreCurrentFunc = true;
-		assert(res == []);
+			return;
+		}
+
+		funcsWithParams[currentFunc] ~= cast(string) parameter.ident.toString();
 	}
 }
 
 unittest
 {
-	import dscanner.analysis.helpers : assertAnalyzerWarnings;
+	import dscanner.analysis.helpers : assertAnalyzerWarningsDMD;
 	import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
 	import std.stdio : stderr;
 
 	StaticAnalysisConfig sac = disabledConfig();
 	sac.mismatched_args_check = Check.enabled;
-	assertAnalyzerWarnings(q{
+
+	assertAnalyzerWarningsDMD(q{
 		void foo(int x, int y)
 		{
 		}
@@ -266,20 +189,17 @@ unittest
 		{
 			int x = 1;
 			int y = 2;
-			foo(y, x); /+
+			foo(y, x); // [warn]: Argument 2 is named 'x', but this is the name of parameter 1
-			       ^ [warn]: Argument 2 is named 'x', but this is the name of parameter 1 +/
+			foo(y + 1, x); // [warn]: Argument 2 is named 'x', but this is the name of parameter 1
-			foo(y + 1, x); /+
-			           ^ [warn]: Argument 2 is named 'x', but this is the name of parameter 1 +/
 			foo(y + 1, f(x));
 			foo(x: y, y: x);
-			foo(y, 0); /+
+			foo(y, 0); // [warn]: Argument 1 is named 'y', but this is the name of parameter 2
-			    ^ [warn]: Argument 1 is named 'y', but this is the name of parameter 2 +/
 
 			// foo(y: y, x); // TODO: this shouldn't error
 			foo(x, y: x); // TODO: this should error
-			foo(y, y: x); /+
+			foo(y, y: x); // [warn]: Argument 1 is named 'y', but this is the name of parameter 2
-			    ^ [warn]: Argument 1 is named 'y', but this is the name of parameter 2 +/
 		}
 	}c, sac);
+
 	stderr.writeln("Unittest for MismatchedArgumentCheck passed.");
 }

src/dscanner/analysis/run.d

@@ -659,10 +659,6 @@ BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
 		checks ~= new FunctionAttributeCheck(args.setSkipTests(
 		analysisConfig.function_attribute_check == Check.skipTests && !ut));
 
-	if (moduleName.shouldRun!MismatchedArgumentCheck(analysisConfig))
-		checks ~= new MismatchedArgumentCheck(args.setSkipTests(
-		analysisConfig.mismatched_args_check == Check.skipTests && !ut));
-
 	if (moduleName.shouldRun!UndocumentedDeclarationCheck(analysisConfig))
 		checks ~= new UndocumentedDeclarationCheck(args.setSkipTests(
 		analysisConfig.undocumented_declaration_check == Check.skipTests && !ut));

src/dscanner/analysis/rundmd.d

@@ -34,6 +34,7 @@ import dscanner.analysis.length_subtraction : LengthSubtractionCheck;
 import dscanner.analysis.line_length : LineLengthCheck;
 import dscanner.analysis.local_imports : LocalImportCheck;
 import dscanner.analysis.logic_precedence : LogicPrecedenceCheck;
+import dscanner.analysis.mismatched_args : MismatchedArgumentCheck;
 import dscanner.analysis.numbers : NumberStyleCheck;
 import dscanner.analysis.objectconst : ObjectConstCheck;
 import dscanner.analysis.opequals_without_tohash : OpEqualsWithoutToHashCheck;
@@ -325,6 +326,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
 			config.allman_braces_check == Check.skipTests && !ut
 		);
 
+	if (moduleName.shouldRunDmd!(MismatchedArgumentCheck!ASTCodegen)(config))
+		visitors ~= new MismatchedArgumentCheck!ASTCodegen(
+			fileName,
+			config.mismatched_args_check == Check.skipTests && !ut
+		);
+
 	foreach (visitor; visitors)
 	{
 		m.accept(visitor);