replace libdparse in objectconst functionality + unittests integration with dmd (#17)

* replace libdparse in objectconst functionality + unittests integration with dmd

* updated dmd

* run tests

* use templates

* visit aggregate declaration

* updated dmd

* solve linter seg fault

* get rid of dup + refactor

* fix typo

dmd

@@ -1 +1 @@
-Subproject commit ae6261888e10e8072033369a9bce60d7be31ab1c
+Subproject commit ac5f925c8b942c2b338f2831c21b11ddfd1aad87

src/dscanner/analysis/helpers.d

@@ -20,6 +20,9 @@ import dsymbol.modulecache : ModuleCache;
 import std.experimental.allocator;
 import std.experimental.allocator.mallocator;
 
+import dmd.parse : Parser;
+import dmd.astbase : ASTBase;
+
 S between(S)(S value, S before, S after) if (isSomeString!S)
 {
 	return value.after(before).before(after);
@@ -350,3 +353,121 @@ void assertAutoFix(string before, string after, const StaticAnalysisConfig confi
 			file, line);
 	}
 }
+
+void assertAnalyzerWarningsDMD(string code, const StaticAnalysisConfig config, bool semantic = false,
+		string file = __FILE__, size_t line = __LINE__)
+{
+	import dmd.globals : global;
+	import dscanner.utils : getModuleName;
+	import std.file : remove, exists;
+	import std.stdio : File;
+	import std.path : dirName;
+	import dmd.arraytypes : Strings;
+
+	import std.stdio : File;
+	import std.file : exists, remove;
+
+	auto deleteme = "test.txt";
+	File f = File(deleteme, "w");
+	scope(exit)
+	{
+		assert(exists(deleteme));
+        remove(deleteme);
+	}
+
+	f.write(code);
+	f.close();
+
+	auto dmdParentDir = dirName(dirName(dirName(dirName(__FILE_FULL_PATH__))));
+
+	global.params.useUnitTests = true;
+	global.path = new Strings();
+	global.path.push((dmdParentDir ~ "/dmd" ~ "\0").ptr);
+	global.path.push((dmdParentDir ~ "/dmd/druntime/src" ~ "\0").ptr);
+
+	initDMD();
+
+	auto input = cast(char[]) code;
+	input ~= '\0';
+	auto t = dmd.frontend.parseModule(cast(const(char)[]) file, cast(const (char)[]) input);
+	if (semantic)
+		t.module_.fullSemantic();
+
+	MessageSet rawWarnings = analyzeDmd("test.txt", t.module_, getModuleName(t.module_.md), config);
+
+	string[] codeLines = code.splitLines();
+
+	// Get the warnings ordered by line
+	string[size_t] warnings;
+	foreach (rawWarning; rawWarnings[])
+	{
+		// Skip the warning if it is on line zero
+		immutable size_t rawLine = rawWarning.line;
+		if (rawLine == 0)
+		{
+			stderr.writefln("!!! Skipping warning because it is on line zero:\n%s",
+					rawWarning.message);
+			continue;
+		}
+
+		size_t warnLine = line - 1 + rawLine;
+		warnings[warnLine] = format("[warn]: %s", rawWarning.message);
+	}
+
+	// Get all the messages from the comments in the code
+	string[size_t] messages;
+	foreach (i, codeLine; codeLines)
+	{
+		// Skip if no [warn] comment
+		if (codeLine.indexOf("// [warn]:") == -1)
+			continue;
+
+		// Skip if there is no comment or code
+		immutable string codePart = codeLine.before("// ");
+		immutable string commentPart = codeLine.after("// ");
+		if (!codePart.length || !commentPart.length)
+			continue;
+
+		// Get the line of this code line
+		size_t lineNo = i + line;
+
+		// Get the message
+		messages[lineNo] = commentPart;
+	}
+
+	// Throw an assert error if any messages are not listed in the warnings
+	foreach (lineNo, message; messages)
+	{
+		// No warning
+		if (lineNo !in warnings)
+		{
+			immutable string errors = "Expected warning:\n%s\nFrom source code at (%s:?):\n%s".format(messages[lineNo],
+					lineNo, codeLines[lineNo - line]);
+			throw new AssertError(errors, file, lineNo);
+		}
+		// Different warning
+		else if (warnings[lineNo] != messages[lineNo])
+		{
+			immutable string errors = "Expected warning:\n%s\nBut was:\n%s\nFrom source code at (%s:?):\n%s".format(
+					messages[lineNo], warnings[lineNo], lineNo, codeLines[lineNo - line]);
+			throw new AssertError(errors, file, lineNo);
+		}
+	}
+
+	// Throw an assert error if there were any warnings that were not expected
+	string[] unexpectedWarnings;
+	foreach (lineNo, warning; warnings)
+	{
+		// Unexpected warning
+		if (lineNo !in messages)
+		{
+			unexpectedWarnings ~= "%s\nFrom source code at (%s:?):\n%s".format(warning,
+					lineNo, codeLines[lineNo - line]);
+		}
+	}
+	if (unexpectedWarnings.length)
+	{
+		immutable string message = "Unexpected warnings:\n" ~ unexpectedWarnings.join("\n");
+		throw new AssertError(message, file, line);
+	}
+}

src/dscanner/analysis/objectconst.d

@@ -5,102 +5,107 @@
 
 module dscanner.analysis.objectconst;
 
-import std.stdio;
-import std.regex;
-import dparse.ast;
-import dparse.lexer;
 import dscanner.analysis.base;
 import dscanner.analysis.helpers;
-import dsymbol.scope_ : Scope;
+import std.stdio;
 
-/**
+extern(C++) class ObjectConstCheck(AST) : BaseAnalyzerDmd
- * Checks that opEquals, opCmp, toHash, 'opCast', and toString are either const,
- * immutable, or inout.
- */
-final class ObjectConstCheck : BaseAnalyzer
 {
-	alias visit = BaseAnalyzer.visit;
-
 	mixin AnalyzerInfo!"object_const_check";
+	alias visit = BaseAnalyzerDmd.visit;
 
-	///
+	extern(D) this(string fileName)
-	this(BaseAnalyzerArguments args)
 	{
-		super(args);
+		super(fileName);
 	}
 
-	mixin visitTemplate!ClassDeclaration;
+	void visitAggregate(AST.AggregateDeclaration ad)
-	mixin visitTemplate!InterfaceDeclaration;
-	mixin visitTemplate!UnionDeclaration;
-	mixin visitTemplate!StructDeclaration;
-
-	override void visit(const AttributeDeclaration d)
 	{
-		if (d.attribute.attribute == tok!"const" && inAggregate)
+		import dmd.astenums : MODFlags, STC;
-		{
-			constColon = true;
-		}
-		d.accept(this);
-	}
 
-	override void visit(const Declaration d)
+		if (!ad.members)
-	{
+			return;
-		import std.algorithm : any;
-		bool setConstBlock;
-		if (inAggregate && d.attributes && d.attributes.any!(a => a.attribute == tok!"const"))
-		{
-			setConstBlock = true;
-			constBlock = true;
-		}
 
-		bool containsDisable(A)(const A[] attribs)
+		foreach(member; *ad.members)
 		{
-			import std.algorithm.searching : canFind;
+			if (auto fd = member.isFuncDeclaration())
-			return attribs.canFind!(a => a.atAttribute !is null &&
-				a.atAttribute.identifier.text == "disable");
-		}
-
-		if (const FunctionDeclaration fd = d.functionDeclaration)
-		{
-			const isDeclationDisabled = containsDisable(d.attributes) ||
-				containsDisable(fd.memberFunctionAttributes);
-
-			if (inAggregate && !constColon && !constBlock && !isDeclationDisabled
-					&& isInteresting(fd.name.text) && !hasConst(fd.memberFunctionAttributes))
 			{
-				addErrorMessage(d.functionDeclaration.name, KEY,
+				if (isInteresting(fd.ident.toString()) && !isConstFunc(fd) &&
-						"Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.");
+					!(fd.storage_class & STC.disable))
+						addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY,
+							"Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.");
+				
+				member.accept(this);
 			}
+			else if (auto scd = member.isStorageClassDeclaration())
+			{
+				foreach (smember; *scd.decl)
+				{
+					if (auto fd2 = smember.isFuncDeclaration())
+					{
+						if (isInteresting(fd2.ident.toString()) && !isConstFunc(fd2, scd) &&
+							!(fd2.storage_class & STC.disable))
+								addErrorMessage(cast(ulong) fd2.loc.linnum, cast(ulong) fd2.loc.charnum, KEY,
+									"Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.");
+						
+						smember.accept(this);
+					}
+					else
+						smember.accept(this);
+				}
+			}
+			else
+				member.accept(this);
 		}
-
-		d.accept(this);
-
-		if (!inAggregate)
-			constColon = false;
-		if (setConstBlock)
-			constBlock = false;
 	}
 
-private:
+	override void visit(AST.ClassDeclaration cd)
-
-	enum string KEY = "dscanner.suspicious.object_const";
-
-	static bool hasConst(const MemberFunctionAttribute[] attributes)
 	{
-		import std.algorithm : any;
+		visitAggregate(cd);	
-
-		return attributes.any!(a => a.tokenType == tok!"const"
-				|| a.tokenType == tok!"immutable" || a.tokenType == tok!"inout");
 	}
 
-	static bool isInteresting(string name)
+	override void visit(AST.StructDeclaration sd)
+	{
+		visitAggregate(sd);
+	}
+
+	override void visit(AST.InterfaceDeclaration id)
+	{
+		visitAggregate(id);
+	}
+
+	override void visit(AST.UnionDeclaration ud)
+	{
+		visitAggregate(ud);
+	}
+
+	extern(D) private static bool isInteresting(const char[] name)
 	{
 		return name == "opCmp" || name == "toHash" || name == "opEquals"
 			|| name == "toString" || name == "opCast";
 	}
 
-	bool constBlock;
+	/**
-	bool constColon;
+	 * Checks if a function has either one of attributes `const`, `immutable`, `inout`
+	 */
+	private bool isConstFunc(AST.FuncDeclaration fd, AST.StorageClassDeclaration scd = null)
+	{
+		import dmd.astenums : MODFlags, STC;
+		import std.stdio : writeln;
+
+		if (scd && (scd.stc & STC.const_ || scd.stc & STC.immutable_ || scd.stc & STC.wild))
+			return true;
+
+		if(fd.type && (fd.type.mod == MODFlags.const_ ||
+			fd.type.mod == MODFlags.immutable_ || fd.type.mod == MODFlags.wild))
+				return true;
+
+		return false; 
+	}
+
+	private enum KEY = "dscanner.suspicious.object_const";
+	
+	AST.AggregateDeclaration deleteme;
 }
 
 unittest
@@ -109,7 +114,7 @@ unittest
 
 	StaticAnalysisConfig sac = disabledConfig();
 	sac.object_const_check = Check.enabled;
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
 		void testConsts()
 		{
 			// Will be ok because all are declared const/immutable
@@ -125,7 +130,7 @@ unittest
 					return 1;
 				}
 
-				const hash_t toHash() // ok
+				immutable hash_t toHash() // ok
 				{
 					return 0;
 				}
@@ -143,7 +148,7 @@ unittest
 
 			class Fox
 			{
-				const{ override string toString() { return "foo"; }} // ok
+				inout { override string toString() { return "foo"; } } // ok
 			}
 
 			class Rat
@@ -159,26 +164,22 @@ unittest
 			// Will warn, because none are const
 			class Dog
 			{
-				bool opEquals(Object a, Object b) /+
+				bool opEquals(Object a, Object b) // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.
-				     ^^^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/
 				{
 					return true;
 				}
 
-				int opCmp(Object o) /+
+				int opCmp(Object o) // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.
-				    ^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/
 				{
 					return 1;
 				}
 
-				hash_t toHash() /+
+				hash_t toHash() // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.
-				       ^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/
 				{
 					return 0;
 				}
 
-				string toString() /+
+				string toString() // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const.
-				       ^^^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/
 				{
 					return "Dog";
 				}

src/dscanner/analysis/run.d

@@ -13,7 +13,6 @@ import dparse.parser;
 import dparse.rollback_allocator;
 import std.algorithm;
 import std.array;
-import std.array;
 import std.conv;
 import std.file : mkdirRecurse;
 import std.functional : toDelegate;
@@ -96,6 +95,7 @@ import dscanner.utils;
 import dscanner.reports : DScannerJsonReporter, SonarQubeGenericIssueDataReporter;
 
 import dmd.astbase : ASTBase;
+import dmd.parse : Parser;
 
 bool first = true;
 
@@ -404,9 +404,9 @@ bool analyze(string[] fileNames, const StaticAnalysisConfig config, string error
 		auto ast_m = new ASTBase.Module(fileName.toStringz, id, false, false);
 		auto input = cast(char[]) code;
 		input ~= '\0';
-		scope p = new Parser!ASTBase(ast_m, input, false);
+		scope astbaseParser = new Parser!ASTBase(ast_m, input, false);
-		p.nextToken();
+		astbaseParser.nextToken();
-		ast_m.members = p.parseModule();
+		ast_m.members = astbaseParser.parseModule();
 
 		// Skip files that could not be read and continue with the rest
 		if (code.length == 0)
@@ -421,8 +421,8 @@ bool analyze(string[] fileNames, const StaticAnalysisConfig config, string error
 		if (errorCount > 0 || (staticAnalyze && warningCount > 0))
 			hasErrors = true;
 		MessageSet results = analyze(fileName, m, config, moduleCache, tokens, staticAnalyze);
-		MessageSet resultsDmd = analyzeDmd(fileName, ast_m);
+		MessageSet resultsDmd = analyzeDmd(fileName, ast_m, getModuleName(astbaseParser.md), config);
-		foreach(result; resultsDmd[])
+		foreach (result; resultsDmd[])
 		{
 			results.insert(result);
 		}
@@ -726,6 +726,46 @@ bool shouldRun(check : BaseAnalyzer)(string moduleName, const ref StaticAnalysis
 	return true;
 }
 
+/**
+ * Checks whether a module is part of a user-specified include/exclude list.
+ *
+ * The user can specify a comma-separated list of filters, everyone needs to start with
+ * either a '+' (inclusion) or '-' (exclusion).
+ *
+ * If no includes are specified, all modules are included.
+*/
+bool shouldRunDmd(check : BaseAnalyzerDmd!ASTBase)(const char[] moduleName, const ref StaticAnalysisConfig config)
+{
+	enum string a = check.name;
+
+	if (mixin("config." ~ a) == Check.disabled)
+		return false;
+
+	// By default, run the check
+	if (!moduleName.length)
+		return true;
+
+	auto filters = mixin("config.filters." ~ a);
+
+	// Check if there are filters are defined
+	// filters starting with a comma are invalid
+	if (filters.length == 0 || filters[0].length == 0)
+		return true;
+
+	auto includers = filters.filter!(f => f[0] == '+').map!(f => f[1..$]);
+	auto excluders = filters.filter!(f => f[0] == '-').map!(f => f[1..$]);
+
+	// exclusion has preference over inclusion
+	if (!excluders.empty && excluders.any!(s => moduleName.canFind(s)))
+		return false;
+
+	if (!includers.empty)
+		return includers.any!(s => moduleName.canFind(s));
+
+	// by default: include all modules
+	return true;
+}
+
 ///
 unittest
 {
@@ -856,10 +896,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
 		checks ~= new NumberStyleCheck(args.setSkipTests(
 		analysisConfig.number_style_check == Check.skipTests && !ut));
 
-	if (moduleName.shouldRun!ObjectConstCheck(analysisConfig))
-		checks ~= new ObjectConstCheck(args.setSkipTests(
-		analysisConfig.object_const_check == Check.skipTests && !ut));
-
 	if (moduleName.shouldRun!OpEqualsWithoutToHashCheck(analysisConfig))
 		checks ~= new OpEqualsWithoutToHashCheck(args.setSkipTests(
 		analysisConfig.opequals_tohash_check == Check.skipTests && !ut));
@@ -1285,14 +1321,24 @@ version (unittest)
 	}
 }
 
-MessageSet analyzeDmd(string fileName, ASTBase.Module m)
+MessageSet analyzeDmd(string fileName, ASTBase.Module m, const char[] moduleName, const StaticAnalysisConfig config)
 {
-	scope vis = new EnumArrayVisitor!ASTBase(fileName);
-	m.accept(vis);
-
 	MessageSet set = new MessageSet;
-	foreach(message; vis.messages)
+	BaseAnalyzerDmd!ASTBase[] visitors;
-		set.insert(message);
+
+	if (moduleName.shouldRunDmd!(ObjectConstCheck!ASTBase)(config))
+		visitors ~= new ObjectConstCheck!ASTBase(fileName);
+
+	if (moduleName.shouldRunDmd!(EnumArrayVisitor!ASTBase)(config))
+		visitors ~= new EnumArrayVisitor!ASTBase(fileName);
+
+	foreach (visitor; visitors)
+	{
+		m.accept(visitor);
+		
+		foreach (message; visitor.messages)
+			set.insert(message);
+	}
 
 	return set;
 }

src/dscanner/utils.d

@@ -8,6 +8,9 @@ import std.format : format;
 import std.file : exists, read;
 import std.path: isValidPath;
 
+import dmd.astbase : ASTBase;
+import dmd.parse : Parser;
+
 private void processBOM(ref ubyte[] sourceCode, string fname)
 {
 	enum spec = "D-Scanner does not support %s-encoded files (%s)";
@@ -309,3 +312,24 @@ auto ref safeAccess(M)(M m)
 {
 	return SafeAccess!M(m);
 }
+
+/**
+ * Return the module name from a ModuleDeclaration instance with the following format: `foo.bar.module`
+ */
+const(char[]) getModuleName(ASTBase.ModuleDeclaration *mdptr)
+{
+	import std.array : array, join;
+
+	if (mdptr !is null)
+	{
+		import std.algorithm : map;
+		ASTBase.ModuleDeclaration md = *mdptr;
+		
+		if (md.packages.length != 0)
+			return join(md.packages.map!(e => e.toString()).array ~ md.id.toString().dup, ".");
+		else
+			return md.id.toString(); 
+	}
+
+	return "";
+}