Merge pull request #495 from wilzbach/assert_without_msg

Add check for asserts without an explanatory message
merged-on-behalf-of: Sebastian Wilzbach <sebi.wilzbach@gmail.com>

README.md

@@ -139,6 +139,7 @@ Note that the "--skipTests" option is the equivalent of changing each
 * Allman brace style
 * Redundant visibility attributes
 * Public declarations without a documented unittest. By default disabled.
+* Asserts without an explanatory message. By default disabled.
 
 #### Wishlist
 

src/analysis/assert_without_msg.d

@@ -0,0 +1,155 @@
+// Distributed under the Boost Software License, Version 1.0.
+//    (See accompanying file LICENSE_1_0.txt or copy at
+//          http://www.boost.org/LICENSE_1_0.txt)
+
+module analysis.assert_without_msg;
+
+import analysis.base : BaseAnalyzer;
+import dsymbol.scope_ : Scope;
+import dparse.lexer;
+import dparse.ast;
+
+import std.stdio;
+import std.algorithm;
+
+/**
+ * Check that all asserts have an explanatory message.
+ */
+class AssertWithoutMessageCheck : BaseAnalyzer
+{
+	enum string KEY = "dscanner.style.assert_without_msg";
+	enum string MESSAGE = "An assert should have an explanatory message";
+
+	///
+	this(string fileName, const(Scope)* sc, bool skipTests = false)
+	{
+		super(fileName, sc, skipTests);
+	}
+
+	override void visit(const AssertExpression expr)
+	{
+		if (expr.message is null)
+			addErrorMessage(expr.line, expr.column, KEY, MESSAGE);
+	}
+
+	override void visit(const FunctionCallExpression expr)
+	{
+		if (!isStdExceptionImported)
+			return;
+
+		if (expr.unaryExpression !is null &&
+			expr.unaryExpression.primaryExpression !is null &&
+			expr.unaryExpression.primaryExpression.identifierOrTemplateInstance !is null)
+		{
+			auto ident = expr.unaryExpression.primaryExpression.identifierOrTemplateInstance.identifier;
+			if (ident.text == "enforce" && expr.arguments !is null && expr.arguments.argumentList !is null &&
+					expr.arguments.argumentList.items.length < 2)
+				addErrorMessage(ident.line, ident.column, KEY, MESSAGE);
+		}
+	}
+
+	override void visit(const SingleImport sImport)
+	{
+		static immutable stdException = ["std", "exception"];
+		if (sImport.identifierChain.identifiers.map!(a => a.text).equal(stdException))
+			isStdExceptionImported = true;
+	}
+
+	// revert the stack after new scopes
+	override void visit(const Declaration decl)
+	{
+		// be careful - ImportDeclarations don't introduce a new scope
+		if (decl.importDeclaration is null)
+		{
+			bool tmp = isStdExceptionImported;
+			scope(exit) isStdExceptionImported = tmp;
+			decl.accept(this);
+		}
+		else
+			decl.accept(this);
+	}
+
+	mixin ScopedVisit!IfStatement;
+	mixin ScopedVisit!BlockStatement;
+
+	alias visit = BaseAnalyzer.visit;
+
+private:
+	bool isStdExceptionImported;
+
+	template ScopedVisit(NodeType)
+	{
+		override void visit(const NodeType n)
+		{
+			bool tmp = isStdExceptionImported;
+			scope(exit) isStdExceptionImported = tmp;
+			n.accept(this);
+		}
+	}
+}
+
+unittest
+{
+	import std.stdio : stderr;
+	import std.format : format;
+	import analysis.config : StaticAnalysisConfig, Check, disabledConfig;
+	import analysis.helpers : assertAnalyzerWarnings;
+
+	StaticAnalysisConfig sac = disabledConfig();
+	sac.assert_without_msg = Check.enabled;
+
+	assertAnalyzerWarnings(q{
+	unittest {
+		assert(0, "foo bar");
+		assert(0); // [warn]: %s
+	}
+	}c.format(
+		AssertWithoutMessageCheck.MESSAGE,
+	), sac);
+
+	assertAnalyzerWarnings(q{
+	unittest {
+		static assert(0, "foo bar");
+		static assert(0); // [warn]: %s
+	}
+	}c.format(
+		AssertWithoutMessageCheck.MESSAGE,
+	), sac);
+
+	// check for std.exception.enforce
+	assertAnalyzerWarnings(q{
+	unittest {
+		enforce(0); // std.exception not imported yet - could be a user-defined symbol
+		import std.exception;
+		enforce(0, "foo bar");
+		enforce(0); // [warn]: %s
+	}
+	}c.format(
+		AssertWithoutMessageCheck.MESSAGE,
+	), sac);
+
+	// check for std.exception.enforce
+	assertAnalyzerWarnings(q{
+	unittest {
+		import exception;
+		class C {
+			import std.exception;
+		}
+		enforce(0); // std.exception not imported yet - could be a user-defined symbol
+		struct S {
+			import std.exception;
+		}
+		enforce(0); // std.exception not imported yet - could be a user-defined symbol
+		if (false) {
+			import std.exception;
+		}
+		enforce(0); // std.exception not imported yet - could be a user-defined symbol
+		{
+			import std.exception;
+		}
+		enforce(0); // std.exception not imported yet - could be a user-defined symbol
+	}
+	}c, sac);
+
+	stderr.writeln("Unittest for AssertWithoutMessageCheck passed.");
+}

src/analysis/config.d

@@ -189,6 +189,9 @@ struct StaticAnalysisConfig
 	@INI("Check public declarations without a documented unittest")
 	string has_public_example = Check.disabled;
 
+	@INI("Check for asserts without an explanatory message")
+	string assert_without_msg = Check.disabled;
+
 	@INI("Module-specific filters")
 	ModuleFilters filters;
 }

src/analysis/run.d

@@ -70,6 +70,7 @@ import analysis.useless_initializer;
 import analysis.allman;
 import analysis.redundant_attributes;
 import analysis.has_public_example;
+import analysis.assert_without_msg;
 
 import dsymbol.string_interning : internString;
 import dsymbol.scope_;
@@ -483,6 +484,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a
 		checks ~= new HasPublicExampleCheck(fileName, moduleScope,
 		analysisConfig.has_public_example == Check.skipTests && !ut);
 
+	if (moduleName.shouldRun!"assert_without_msg"(analysisConfig))
+		checks ~= new AssertWithoutMessageCheck(fileName, moduleScope,
+		analysisConfig.assert_without_msg == Check.skipTests && !ut);
+
 	version (none)
 		if (moduleName.shouldRun!"redundant_if_check"(analysisConfig))
 			checks ~= new IfStatementCheck(fileName, moduleScope,