replace libdparse in constructor check (#43)

changelog/dscanner.struct-ctor-check.dd

@@ -0,0 +1,19 @@
+Remove the check regarding structs with no arguments constructors.
+
+The check is implemented in constructors.d and it warns against the usage
+of both constructors with all parameters with default values and constructors
+without any arguments, as this might be confusing. This scenario, for structs,
+is no longer D valid code and that's why it is being deprecated.
+
+Let's consider the following code:
+
+---
+struct Dog
+{
+	this() {}
+	this(string name = "doggie") {} // [warn]: This struct constructor can never be called with its default argument.
+}
+---
+
+D-Scanner would throw and error for this particular struct, but this code
+does not compile anymore hence this check is not needed anymore/

src/dscanner/analysis/constructors.d

@@ -1,104 +1,63 @@
 module dscanner.analysis.constructors;
 
-import dparse.ast;
-import dparse.lexer;
 import std.stdio;
-import std.typecons : Rebindable;
 import dscanner.analysis.base;
 import dscanner.analysis.helpers;
-import dsymbol.scope_ : Scope;
 
-final class ConstructorCheck : BaseAnalyzer
+extern(C++) class ConstructorCheck(AST) : BaseAnalyzerDmd
 {
-	alias visit = BaseAnalyzer.visit;
+	alias visit = BaseAnalyzerDmd.visit;
-
 	mixin AnalyzerInfo!"constructor_check";
 
-	this(BaseAnalyzerArguments args)
+	extern(D) this(string fileName)
 	{
-		super(args);
+		super(fileName);
 	}
 
-	override void visit(const ClassDeclaration classDeclaration)
+	override void visit(AST.ClassDeclaration d)
 	{
-		const oldHasDefault = hasDefaultArgConstructor;
+		bool hasDefaultArgConstructor = false;
-		const oldHasNoArg = hasNoArgConstructor;
+		bool hasNoArgConstructor = false;
-		hasNoArgConstructor = null;
+
-		hasDefaultArgConstructor = null;
+		if (d.members)
-		immutable State prev = state;
+		{
-		state = State.inClass;
+			foreach (s; *d.members)
-		classDeclaration.accept(this);
+			{
+				if (auto cd = s.isCtorDeclaration())
+				{
+					auto tf = cd.type.isTypeFunction();
+
+					if (tf)
+					{
+						if (tf.parameterList.parameters.length == 0)
+							hasNoArgConstructor = true;
+						else
+						{
+							// Check if all parameters have a default value
+							hasDefaultArgConstructor = true;
+
+							foreach (param; *tf.parameterList.parameters)
+								if (param.defaultArg is null)
+									hasDefaultArgConstructor = false;
+						}
+					}
+				}
+
+				s.accept(this);
+			}
+		}
+
 		if (hasNoArgConstructor && hasDefaultArgConstructor)
 		{
-			addErrorMessage(
+			addErrorMessage(cast(ulong) d.loc.linnum,
-				Message.Diagnostic.from(fileName, classDeclaration.name,
+					cast(ulong) d.loc.charnum, KEY, MESSAGE);
-					"This class has a zero-argument constructor as well as a"
-					~ " constructor with one default argument. This can be confusing."),
-				[
-					Message.Diagnostic.from(fileName, hasNoArgConstructor, "zero-argument constructor defined here"),
-					Message.Diagnostic.from(fileName, hasDefaultArgConstructor, "default argument constructor defined here")
-				],
-				"dscanner.confusing.constructor_args"
-			);
-		}
-		hasDefaultArgConstructor = oldHasDefault;
-		hasNoArgConstructor = oldHasNoArg;
-		state = prev;
-	}
-
-	override void visit(const StructDeclaration structDeclaration)
-	{
-		immutable State prev = state;
-		state = State.inStruct;
-		structDeclaration.accept(this);
-		state = prev;
-	}
-
-	override void visit(const Constructor constructor)
-	{
-		final switch (state)
-		{
-		case State.inStruct:
-			if (constructor.parameters.parameters.length == 1
-					&& constructor.parameters.parameters[0].default_ !is null)
-			{
-				const(Token)[] tokens = constructor.parameters.parameters[0].default_.tokens;
-				assert(tokens.length);
-				// we extend the token range to the `=` sign, since it's continuous
-				tokens = (tokens.ptr - 1)[0 .. tokens.length + 1];
-				addErrorMessage(tokens,
-						"dscanner.confusing.struct_constructor_default_args",
-						"This struct constructor can never be called with its "
-						~ "default argument.");
-			}
-			break;
-		case State.inClass:
-			if (constructor.parameters.parameters.length == 1
-					&& constructor.parameters.parameters[0].default_ !is null)
-			{
-				hasDefaultArgConstructor = constructor;
-			}
-			else if (constructor.parameters.parameters.length == 0)
-				hasNoArgConstructor = constructor;
-			break;
-		case State.ignoring:
-			break;
 		}
 	}
 	
 private:
-
+	enum MESSAGE = "This class has a zero-argument constructor as well as a"
-	enum State : ubyte
+					~ " constructor with default arguments. This can be confusing.";
-	{
+	enum KEY = "dscanner.confusing.constructor_args";
-		ignoring,
-		inClass,
-		inStruct
-	}
-
-	State state;
-
-	Rebindable!(const Constructor) hasNoArgConstructor;
-	Rebindable!(const Constructor) hasDefaultArgConstructor;
 }
 
 unittest
@@ -107,20 +66,23 @@ unittest
 
 	StaticAnalysisConfig sac = disabledConfig();
 	sac.constructor_check = Check.enabled;
-	// TODO: test supplemental diagnostics
+	assertAnalyzerWarningsDMD(q{
-	assertAnalyzerWarnings(q{
+		class Cat // [warn]: This class has a zero-argument constructor as well as a constructor with default arguments. This can be confusing.
-		class Cat /+
-		      ^^^ [warn]: This class has a zero-argument constructor as well as a constructor with one default argument. This can be confusing. +/
 		{
 			this() {}
 			this(string name = "kittie") {}
 		}
 
-		struct Dog
+		class Cat // [warn]: This class has a zero-argument constructor as well as a constructor with default arguments. This can be confusing.
 		{
 			this() {}
-			this(string name = "doggie") {} /+
+			this(string name = "kittie", int x = 2) {}
-			                 ^^^^^^^^^^ [warn]: This struct constructor can never be called with its default argument. +/
+		}
+
+		class Cat
+		{
+			this() {}
+			this(string name = "kittie", int x) {}
 		}
 	}c, sac);
 

src/dscanner/analysis/run.d

@@ -840,10 +840,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName,
 		checks ~= new CommaExpressionCheck(args.setSkipTests(
 		analysisConfig.comma_expression_check == Check.skipTests && !ut));
 
-	if (moduleName.shouldRun!ConstructorCheck(analysisConfig))
-		checks ~= new ConstructorCheck(args.setSkipTests(
-		analysisConfig.constructor_check == Check.skipTests && !ut));
-
 	if (moduleName.shouldRun!UnmodifiedFinder(analysisConfig))
 		checks ~= new UnmodifiedFinder(args.setSkipTests(
 		analysisConfig.could_be_immutable_check == Check.skipTests && !ut));
@@ -1324,6 +1320,9 @@ MessageSet analyzeDmd(string fileName, ASTBase.Module m, const char[] moduleName
 	if (moduleName.shouldRunDmd!(ExplicitlyAnnotatedUnittestCheck!ASTBase)(config))
 		visitors ~= new ExplicitlyAnnotatedUnittestCheck!ASTBase(fileName);
 
+	if (moduleName.shouldRunDmd!(ConstructorCheck!ASTBase)(config))
+		visitors ~= new ConstructorCheck!ASTBase(fileName);
+
 	foreach (visitor; visitors)
 	{
 		m.accept(visitor);