Replace libdparse with DMD in UndocumentedDeclarationCheck (#123)

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!UndocumentedDeclarationCheck(analysisConfig))
-		checks ~= new UndocumentedDeclarationCheck(args.setSkipTests(
-		analysisConfig.undocumented_declaration_check == Check.skipTests && !ut));
-
 	return checks;
 }
 

src/dscanner/analysis/rundmd.d

@@ -48,6 +48,7 @@ import dscanner.analysis.redundant_storage_class : RedundantStorageClassCheck;
 import dscanner.analysis.static_if_else : StaticIfElse;
 import dscanner.analysis.style : StyleChecker;
 import dscanner.analysis.trust_too_much : TrustTooMuchCheck;
+import dscanner.analysis.undocumented : UndocumentedDeclarationCheck;
 import dscanner.analysis.unmodified : UnmodifiedFinder;
 import dscanner.analysis.unused_label : UnusedLabelCheck;
 import dscanner.analysis.unused_parameter : UnusedParameterCheck;
@@ -339,6 +340,12 @@ MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleN
 			config.if_constraints_indent == Check.skipTests && !ut
 		);
 
+	if (moduleName.shouldRunDmd!(UndocumentedDeclarationCheck!ASTCodegen)(config))
+		visitors ~= new UndocumentedDeclarationCheck!ASTCodegen(
+			fileName,
+			config.undocumented_declaration_check == Check.skipTests && !ut
+		);
+
 	foreach (visitor; visitors)
 	{
 		m.accept(visitor);

src/dscanner/analysis/undocumented.d

@@ -6,338 +6,220 @@
 module dscanner.analysis.undocumented;
 
 import dscanner.analysis.base;
-import dsymbol.scope_ : Scope;
+import dmd.astenums : STC;
-import dparse.ast;
+import std.format : format;
-import dparse.lexer;
-
 import std.regex : ctRegex, matchAll;
-import std.stdio;
 
 /**
  * Checks for undocumented public declarations. Ignores some operator overloads,
  * main functions, and functions whose name starts with "get" or "set".
  */
-final class UndocumentedDeclarationCheck : BaseAnalyzer
+extern (C++) class UndocumentedDeclarationCheck(AST) : BaseAnalyzerDmd
 {
-	alias visit = BaseAnalyzer.visit;
+	alias visit = BaseAnalyzerDmd.visit;
-
 	mixin AnalyzerInfo!"undocumented_declaration_check";
 
-	this(BaseAnalyzerArguments args)
+	private enum KEY = "dscanner.style.undocumented_declaration";
+	private enum DEFAULT_MSG = "Public declaration is undocumented.";
+	private enum MSG = "Public declaration '%s' is undocumented.";
+
+	private immutable string[] ignoredFunctionNames = [
+		"opCmp", "opEquals", "toString", "toHash", "main"
+	];
+	private enum getSetRe = ctRegex!`^(?:get|set)(?:\p{Lu}|_).*`;
+
+	extern (D) this(string fileName, bool skipTests = false)
 	{
-		super(args);
+		super(fileName, skipTests);
 	}
 
-	override void visit(const Module mod)
+	override void visit(AST.VisibilityDeclaration visibilityDecl)
 	{
-		push(tok!"public");
+		import dmd.dsymbol : Visibility;
-		mod.accept(this);
+
+		if (visibilityDecl.visibility.kind == Visibility.Kind.public_)
+			super.visit(visibilityDecl);
 	}
 
-	override void visit(const Declaration dec)
+	override void visit(AST.StorageClassDeclaration storageClassDecl)
 	{
-		if (dec.attributeDeclaration)
+		if (!hasIgnorableStorageClass(storageClassDecl.stc))
+			super.visit(storageClassDecl);
+	}
+
+	override void visit(AST.DeprecatedDeclaration _) {}
+
+	override void visit(AST.FuncDeclaration funcDecl)
+	{
+		if (funcDecl.comment() !is null || funcDecl.ident is null)
+			return;
+
+		string funcName = cast(string) funcDecl.ident.toString();
+		bool canBeUndocumented = hasIgnorableStorageClass(funcDecl.storage_class) || isIgnorableFunctionName(funcName);
+
+		if (!canBeUndocumented)
 		{
-			auto attr = dec.attributeDeclaration.attribute;
+			addErrorMessage(funcDecl.loc.linnum, funcDecl.loc.charnum, KEY, MSG.format(funcName));
-			if (isProtection(attr.attribute.type))
+			super.visit(funcDecl);
-				set(attr.attribute.type);
-			else if (attr.attribute == tok!"override")
-				setOverride(true);
-			else if (attr.deprecated_ !is null)
-				setDeprecated(true);
-			else if (attr.atAttribute !is null && attr.atAttribute.identifier.text == "disable")
-				setDisabled(true);
 		}
+	}
 
-		immutable bool shouldPop = dec.attributeDeclaration is null;
+	private extern (D) bool isIgnorableFunctionName(string funcName)
-		immutable bool prevOverride = getOverride();
+	{
-		immutable bool prevDisabled = getDisabled();
+		import std.algorithm : canFind;
-		immutable bool prevDeprecated = getDeprecated();
+
-		bool dis;
+		return ignoredFunctionNames.canFind(funcName) || !matchAll(funcName, getSetRe).empty;
-		bool dep;
+	}
-		bool ovr;
+
-		bool pushed;
+	override void visit(AST.CtorDeclaration ctorDecl)
-		foreach (attribute; dec.attributes)
+	{
+		if (ctorDecl.comment() !is null)
+			return;
+
+		addErrorMessage(ctorDecl.loc.linnum, ctorDecl.loc.charnum, KEY, DEFAULT_MSG);
+	}
+
+	override void visit(AST.TemplateDeclaration templateDecl)
+	{
+		if (templateDecl.comment() !is null || templateDecl.ident is null)
+			return;
+
+		if (!templateDecl.isDeprecated())
 		{
-			if (isProtection(attribute.attribute.type))
+			string templateName = cast(string) templateDecl.ident.toString();
+			addErrorMessage(templateDecl.loc.linnum, templateDecl.loc.charnum, KEY, MSG.format(templateName));
+		}
+	}
+
+	mixin VisitDeclaration!(AST.ClassDeclaration);
+	mixin VisitDeclaration!(AST.InterfaceDeclaration);
+	mixin VisitDeclaration!(AST.StructDeclaration);
+	mixin VisitDeclaration!(AST.UnionDeclaration);
+	mixin VisitDeclaration!(AST.EnumDeclaration);
+	mixin VisitDeclaration!(AST.EnumMember);
+	mixin VisitDeclaration!(AST.VarDeclaration);
+
+	private template VisitDeclaration(NodeType)
+	{
+		override void visit(NodeType decl)
+		{
+			if (decl.comment() !is null || decl.ident is null)
 			{
-				if (shouldPop)
+				super.visit(decl);
-				{
+				return;
-					pushed = true;
-					push(attribute.attribute.type);
-				}
-				else
-					set(attribute.attribute.type);
-			}
-			else if (attribute.attribute == tok!"override")
-				ovr = true;
-			else if (attribute.deprecated_ !is null)
-				dep = true;
-			else if (attribute.atAttribute !is null
-					&& attribute.atAttribute.identifier.text == "disable")
-				dis = true;
-		}
-		if (ovr)
-			setOverride(true);
-		if (dis)
-			setDisabled(true);
-		if (dep)
-			setDeprecated(true);
-		dec.accept(this);
-		if (shouldPop && pushed)
-			pop();
-		if (ovr)
-			setOverride(prevOverride);
-		if (dis)
-			setDisabled(prevDisabled);
-		if (dep)
-			setDeprecated(prevDeprecated);
-	}
-
-	override void visit(const VariableDeclaration variable)
-	{
-		if (!currentIsInteresting() || variable.comment.ptr !is null)
-			return;
-		if (variable.autoDeclaration !is null)
-		{
-			addMessage(variable.autoDeclaration.parts[0].identifier,
-					variable.autoDeclaration.parts[0].identifier.text);
-			return;
-		}
-		foreach (dec; variable.declarators)
-		{
-			if (dec.comment.ptr is null)
-				addMessage(dec.name, dec.name.text);
-			return;
-		}
-	}
-
-	override void visit(const ConditionalDeclaration cond)
-	{
-		const VersionCondition ver = cond.compileCondition.versionCondition;
-		if (ver is null || (ver.token != tok!"unittest" && ver.token.text != "none"))
-			cond.accept(this);
-		else if (cond.falseDeclarations.length > 0)
-			foreach (f; cond.falseDeclarations)
-				visit(f);
-	}
-
-	override void visit(const FunctionBody fb)
-	{
-	}
-
-	override void visit(const Unittest u)
-	{
-	}
-
-	override void visit(const TraitsExpression t)
-	{
-	}
-
-	mixin V!AnonymousEnumMember;
-	mixin V!ClassDeclaration;
-	mixin V!EnumDeclaration;
-	mixin V!InterfaceDeclaration;
-	mixin V!StructDeclaration;
-	mixin V!UnionDeclaration;
-	mixin V!TemplateDeclaration;
-	mixin V!FunctionDeclaration;
-	mixin V!Constructor;
-
-private:
-
-	enum string KEY = "dscanner.style.undocumented_declaration";
-
-	mixin template V(T)
-	{
-		override void visit(const T declaration)
-		{
-			import std.traits : hasMember;
-			static if (hasMember!(T, "storageClasses"))
-			{
-				// stop at declarations with a deprecated in their storage classes
-				foreach (sc; declaration.storageClasses)
-					if (sc.deprecated_ !is null)
-						return;
 			}
 
-			if (currentIsInteresting())
+			bool canBeUndocumented;
+			static if (__traits(hasMember, NodeType, "storage_class"))
+				canBeUndocumented = hasIgnorableStorageClass(decl.storage_class);
+
+			if (!canBeUndocumented)
 			{
-				if (declaration.comment.ptr is null)
+				string declName = cast(string) decl.ident.toString();
-				{
+				addErrorMessage(decl.loc.linnum, decl.loc.charnum, KEY, MSG.format(declName));
-					static if (hasMember!(T, "name"))
+				super.visit(decl);
-					{
-						static if (is(T == FunctionDeclaration))
-						{
-							import std.algorithm : canFind;
-
-							if (!(ignoredFunctionNames.canFind(declaration.name.text)
-									|| isGetterOrSetter(declaration.name.text)
-									|| isProperty(declaration)))
-							{
-								addMessage(declaration.name, declaration.name.text);
-							}
-						}
-						else
-						{
-							if (declaration.name.type != tok!"")
-								addMessage(declaration.name, declaration.name.text);
-						}
-					}
-					else
-					{
-						import std.algorithm : countUntil;
-
-						// like constructors
-						auto tokens = declaration.tokens.findTokenForDisplay(tok!"this");
-						auto earlyEnd = tokens.countUntil!(a => a == tok!"{" || a == tok!"(" || a == tok!";");
-						if (earlyEnd != -1)
-							tokens = tokens[0 .. earlyEnd];
-						addMessage(tokens, null);
-					}
-				}
-				static if (!(is(T == TemplateDeclaration) || is(T == FunctionDeclaration)))
-				{
-					declaration.accept(this);
-				}
 			}
 		}
 	}
 
-	static bool isGetterOrSetter(string name)
+	private bool hasIgnorableStorageClass(ulong storageClass)
 	{
-		return !matchAll(name, getSetRe).empty;
+		return (storageClass & STC.deprecated_) || (storageClass & STC.override_)
+			|| (storageClass & STC.disable) || (storageClass & STC.property);
 	}
 
-	static bool isProperty(const FunctionDeclaration dec)
+	override void visit(AST.UnitTestDeclaration _) {}
+
+	override void visit(AST.TraitsExp _) {}
+
+	override void visit(AST.ConditionalDeclaration conditionalDecl)
 	{
-		if (dec.memberFunctionAttributes is null)
+		auto versionCond = conditionalDecl.condition.isVersionCondition();
+
+		if (versionCond is null)
+			super.visit(conditionalDecl);
+
+		if (isIgnorableVersion(versionCond) && conditionalDecl.elsedecl)
+		{
+			foreach (decl; *(conditionalDecl.elsedecl))
+				super.visit(decl);
+		}
+	}
+
+	override void visit(AST.ConditionalStatement conditionalStatement)
+	{
+		auto versionCond = conditionalStatement.condition.isVersionCondition();
+
+		if (versionCond is null)
+			super.visit(conditionalStatement);
+
+		if (isIgnorableVersion(versionCond) && conditionalStatement.elsebody)
+		{
+			super.visit(conditionalStatement.elsebody);
+		}
+	}
+
+	private bool isIgnorableVersion(AST.VersionCondition versionCond)
+	{
+		if (versionCond is null || versionCond.ident is null)
 			return false;
-		foreach (attr; dec.memberFunctionAttributes)
-		{
-			if (attr.atAttribute && attr.atAttribute.identifier.text == "property")
-				return true;
-		}
-		return false;
-	}
 
-	void addMessage(T)(T range, string name)
+		string versionStr = cast(string) versionCond.ident.toString();
-	{
-		import std.string : format;
 
-		addErrorMessage(range, KEY, name is null
+		return versionStr == "unittest" || versionStr == "none";
-				? "Public declaration is undocumented."
-				: format("Public declaration '%s' is undocumented.", name));
 	}
-
-	bool getOverride()
-	{
-		return stack[$ - 1].isOverride;
-	}
-
-	void setOverride(bool o = true)
-	{
-		stack[$ - 1].isOverride = o;
-	}
-
-	bool getDisabled()
-	{
-		return stack[$ - 1].isDisabled;
-	}
-
-	void setDisabled(bool d = true)
-	{
-		stack[$ - 1].isDisabled = d;
-	}
-
-	bool getDeprecated()
-	{
-		return stack[$ - 1].isDeprecated;
-	}
-
-	void setDeprecated(bool d = true)
-	{
-		stack[$ - 1].isDeprecated = d;
-	}
-
-	bool currentIsInteresting()
-	{
-		return stack[$ - 1].protection == tok!"public"
-			&& !getOverride() && !getDisabled() && !getDeprecated();
-	}
-
-	void set(IdType p)
-	in
-	{
-		assert(isProtection(p));
-	}
-	do
-	{
-		stack[$ - 1].protection = p;
-	}
-
-	void push(IdType p)
-	in
-	{
-		assert(isProtection(p));
-	}
-	do
-	{
-		stack ~= ProtectionInfo(p, false);
-	}
-
-	void pop()
-	{
-		assert(stack.length > 1);
-		stack = stack[0 .. $ - 1];
-	}
-
-	static struct ProtectionInfo
-	{
-		IdType protection;
-		bool isOverride;
-		bool isDeprecated;
-		bool isDisabled;
-	}
-
-	ProtectionInfo[] stack;
 }
 
-// Ignore undocumented symbols with these names
-private immutable string[] ignoredFunctionNames = [
-	"opCmp", "opEquals", "toString", "toHash", "main"
-];
-
-private enum getSetRe = ctRegex!`^(?:get|set)(?:\p{Lu}|_).*`;
-
 unittest
 {
 	import std.stdio : stderr;
-	import std.format : format;
 	import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig;
-	import dscanner.analysis.helpers : assertAnalyzerWarnings;
+	import dscanner.analysis.helpers : assertAnalyzerWarningsDMD;
 
 	StaticAnalysisConfig sac = disabledConfig();
 	sac.undocumented_declaration_check = Check.enabled;
 
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
-		class C{} /+
+		private int x;
-		      ^ [warn]: Public declaration 'C' is undocumented. +/
+		int y; // [warn]: Public declaration 'y' is undocumented.
-		interface I{} /+
+		public int z; // [warn]: Public declaration 'z' is undocumented.
-		          ^ [warn]: Public declaration 'I' is undocumented. +/
+
-		enum e = 0; /+
+		///
-		     ^ [warn]: Public declaration 'e' is undocumented. +/
+		class C
-		void f(){} /+
+		{
-		     ^ [warn]: Public declaration 'f' is undocumented. +/
+			int h; // [warn]: Public declaration 'h' is undocumented.
-		struct S{} /+
+
-		       ^ [warn]: Public declaration 'S' is undocumented. +/
+		public:
-		template T(){} /+
+			int g; // [warn]: Public declaration 'g' is undocumented.
-		         ^ [warn]: Public declaration 'T' is undocumented. +/
+			void f() {} // [warn]: Public declaration 'f' is undocumented.
-		union U{} /+
+
-		      ^ [warn]: Public declaration 'U' is undocumented. +/
+		private:
+			int a;
+			int b;
+		}
+	}c, sac);
+
+	assertAnalyzerWarningsDMD(q{
+		deprecated int y;
+
+		///
+		class C
+		{
+			private int b;
+		}
+	}c, sac);
+
+	assertAnalyzerWarningsDMD(q{
+		class C{} // [warn]: Public declaration 'C' is undocumented.
+		interface I{} // [warn]: Public declaration 'I' is undocumented.
+		enum e = 0; // [warn]: Public declaration 'e' is undocumented.
+		void f(){} // [warn]: Public declaration 'f' is undocumented.
+		struct S{} // [warn]: Public declaration 'S' is undocumented.
+		template T(){} // [warn]: Public declaration 'T' is undocumented.
+		union U{} // [warn]: Public declaration 'U' is undocumented.
 	}, sac);
 
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
 		/// C
 		class C{}
 		/// I
@@ -355,12 +237,13 @@ unittest
 	}, sac);
 
 	// https://github.com/dlang-community/D-Scanner/issues/760
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
+		deprecated("This has been deprecated") auto func(){}
 		deprecated auto func(){}
 		deprecated auto func()(){}
 	}, sac);
 
-	assertAnalyzerWarnings(q{
+	assertAnalyzerWarningsDMD(q{
 		class C{} /// a
 		interface I{} /// b
 		enum e = 0; /// c
@@ -370,5 +253,34 @@ unittest
 		union U{} /// g
 	}, sac);
 
+	assertAnalyzerWarningsDMD(q{
+		int x; // [warn]: Public declaration 'x' is undocumented.
+		int y; ///
+
+		///
+		class C
+		{
+			private int a;
+			int b; ///
+			int c; // [warn]: Public declaration 'c' is undocumented.
+			protected int d;
+		}
+
+		///
+		class D
+		{
+			///
+			void fun()
+			{
+				class Inner
+				{
+					int z;
+				}
+
+				int a1, a2, a3;
+			}
+		}
+	}c, sac);
+
 	stderr.writeln("Unittest for UndocumentedDeclarationCheck passed.");
 }