From dbea951e5d03e2548264322eb5d8e3c0d0c6a54c Mon Sep 17 00:00:00 2001
From: Hackerpilot <briancschott@gmail.com>
Date: Sat, 21 Feb 2015 02:13:38 -0800
Subject: [PATCH] Implement #232

---
 src/analysis/config.d        |   3 +
 src/analysis/if_statements.d | 131 +++++++++++++++++++++++++++++++++++
 src/analysis/run.d           |   6 +-
 3 files changed, 138 insertions(+), 2 deletions(-)
 create mode 100644 src/analysis/if_statements.d

diff --git a/src/analysis/config.d b/src/analysis/config.d
index 4a4984e..1c90a69 100644
--- a/src/analysis/config.d
+++ b/src/analysis/config.d
@@ -83,4 +83,7 @@ struct StaticAnalysisConfig
 
 	@INI("Checks for variables that could be declared immutable")
 	bool could_be_immutable_check = false; // disabled by default for now
+
+	@INI("Checks for redundant expressions in if statements")
+	bool redundant_if_check;
 }
diff --git a/src/analysis/if_statements.d b/src/analysis/if_statements.d
new file mode 100644
index 0000000..cfed690
--- /dev/null
+++ b/src/analysis/if_statements.d
@@ -0,0 +1,131 @@
+//          Copyright Brian Schott (Hackerpilot) 2015.
+// 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.if_statements;
+
+import std.d.ast;
+import std.d.lexer;
+import std.d.formatter;
+import analysis.base;
+
+class IfStatementCheck : BaseAnalyzer
+{
+	alias visit = BaseAnalyzer.visit;
+	this(string fileName)
+	{
+		import std.container.binaryheap : heapify;
+
+		super(fileName);
+	}
+
+	override void visit(const IfStatement ifStatement)
+	{
+		import std.format : format;
+		import std.algorithm : sort, countUntil;
+		import std.array : appender;
+
+		++depth;
+
+		if (ifStatement.expression.items.length == 1
+			&& (cast(AndAndExpression) ifStatement.expression.items[0]) is null)
+		{
+			redundancyCheck(ifStatement.expression, ifStatement.expression.line,
+				ifStatement.expression.column);
+		}
+		inIfExpresson = true;
+		ifStatement.expression.accept(this);
+		inIfExpresson = false;
+		ifStatement.thenStatement.accept(this);
+		if (expressions.length)
+			expressions = expressions[0 .. expressions.countUntil!(a => a.depth + 1 >= depth)];
+		if (ifStatement.elseStatement)
+			ifStatement.elseStatement.accept(this);
+		--depth;
+	}
+
+	override void visit(const AndAndExpression andAndExpression)
+	{
+		if (inIfExpresson)
+		{
+			redundancyCheck(andAndExpression, andAndExpression.line,
+				andAndExpression.column);
+			redundancyCheck(andAndExpression.left, andAndExpression.line,
+				andAndExpression.column);
+			redundancyCheck(andAndExpression.right, andAndExpression.line,
+				andAndExpression.column);
+		}
+		andAndExpression.accept(this);
+	}
+
+	override void visit(const OrOrExpression orOrExpression)
+	{
+		// intentionally does nothing
+	}
+
+private:
+	invariant
+	{
+		assert(depth >= 0);
+	}
+
+	void redundancyCheck(const ExpressionNode expression, size_t line,
+		size_t column)
+	{
+		import std.format : format;
+		import std.array : appender;
+		import std.algorithm : sort;
+
+		if (expression is null)
+			return;
+		auto app = appender!string();
+		std.d.formatter.format(app, expression);
+		immutable size_t prevLocation = alreadyChecked(app.data, line, column);
+		if (prevLocation != size_t.max)
+		{
+			addErrorMessage(line, column, KEY,
+				"Expression %s is true: already checked on line %d.".format(
+				expressions[prevLocation].formatted,
+				expressions[prevLocation].line));
+		}
+		else
+		{
+			expressions ~= ExpressionInfo(app.data, line, column, depth);
+			sort(expressions);
+		}
+	}
+
+	size_t alreadyChecked(string expressionText, size_t line, size_t column)
+	{
+		foreach (i, ref info; expressions)
+		{
+			if (info.line == line && info.column == column)
+				continue;
+			if (info.formatted == expressionText)
+				return i;
+		}
+		return size_t.max;
+	}
+
+	bool inIfExpresson;
+	int depth;
+	ExpressionInfo[] expressions;
+	private enum KEY = "dscanner.if_statement";
+}
+
+private struct ExpressionInfo
+{
+	int opCmp(ref const ExpressionInfo other) const nothrow
+	{
+		if (line < other.line || (line == other.line && column < other.column))
+			return 1;
+		if (line == other.line && column == other.column)
+			return 0;
+		return -1;
+	}
+
+	string formatted;
+	size_t line;
+	size_t column;
+	int depth;
+}
diff --git a/src/analysis/run.d b/src/analysis/run.d
index d94e27f..1b0a7bc 100644
--- a/src/analysis/run.d
+++ b/src/analysis/run.d
@@ -40,6 +40,7 @@ import analysis.comma_expression;
 import analysis.function_attributes;
 import analysis.local_imports;
 import analysis.unmodified;
+import analysis.if_statements;
 
 bool first = true;
 
@@ -112,8 +113,8 @@ void generateReport(string[] fileNames, const StaticAnalysisConfig config)
 	writeln("}");
 }
 
-// For multiple files
-// Returns: true if there were errors
+/// For multiple files
+/// Returns: true if there were errors
 bool analyze(string[] fileNames, const StaticAnalysisConfig config, bool staticAnalyze = true)
 {
 	bool hasErrors = false;
@@ -187,6 +188,7 @@ MessageSet analyze(string fileName, const Module m,
 	if (analysisConfig.comma_expression_check) checks ~= new CommaExpressionCheck(fileName);
 	if (analysisConfig.local_import_check) checks ~= new LocalImportCheck(fileName);
 	if (analysisConfig.could_be_immutable_check) checks ~= new UnmodifiedFinder(fileName);
+	if (analysisConfig.redundant_if_check) checks ~= new IfStatementCheck(fileName);
 
 	foreach (check; checks)
 	{