From 3f4395e95b43793958f2f2d0d70dbaa1d41ad212 Mon Sep 17 00:00:00 2001
From: Hackerpilot <briancschott@gmail.com>
Date: Thu, 21 Aug 2014 16:07:03 -0700
Subject: [PATCH] Add logical operator precedence check

---
 analysis/config.d           |  3 ++
 analysis/logic_precedence.d | 61 +++++++++++++++++++++++++++++++++++++
 analysis/run.d              |  2 ++
 astprinter.d                | 16 +++++++++-
 4 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 analysis/logic_precedence.d

diff --git a/analysis/config.d b/analysis/config.d
index 4bb5fca..13c2c80 100644
--- a/analysis/config.d
+++ b/analysis/config.d
@@ -65,4 +65,7 @@ struct StaticAnalysisConfig
 
 	@INI("Checks for confusing code in inline asm statements")
 	bool asm_style_check;
+
+	@INI("Checks for confusing logical operator precedence")
+	bool logical_precedence_check;
 }
diff --git a/analysis/logic_precedence.d b/analysis/logic_precedence.d
new file mode 100644
index 0000000..00df770
--- /dev/null
+++ b/analysis/logic_precedence.d
@@ -0,0 +1,61 @@
+//          Copyright Brian Schott (Hackerpilot) 2014.
+// 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.logic_precedence;
+
+import std.stdio;
+import std.d.ast;
+import std.d.lexer;
+import analysis.base;
+import analysis.helpers;
+
+/**
+ * Checks for code with confusing && and || operator precedence
+ * ---
+ * if (a && b || c) // bad
+ * if (a && (b || c)) // good
+ * ---
+ */
+class LogicPrecedenceCheck : BaseAnalyzer
+{
+	alias visit = BaseAnalyzer.visit;
+
+	enum string KEY = "dscanner.confusing.logical_precedence";
+
+	this(string fileName)
+	{
+		super(fileName);
+	}
+
+	override void visit(const OrOrExpression orOr)
+	{
+		if (orOr.left is null || orOr.right is null) return;
+		const AndAndExpression left = cast(AndAndExpression) orOr.left;
+		const AndAndExpression right = cast(AndAndExpression) orOr.right;
+		if (left is null && right is null) return;
+		if ((left !is null && left.right is null) && (right !is null && right.right is null)) return;
+		addErrorMessage(orOr.line, orOr.column, KEY,
+			"Use parenthesis to clarify this expression.");
+		orOr.accept(this);
+	}
+}
+
+unittest
+{
+	import analysis.config;
+	StaticAnalysisConfig sac;
+	sac.logic_precedence_check = true;
+	assertAnalyzerWarnings(q{
+		void testFish()
+		{
+			if (a && b || c) {} // [warn]:
+			if ((a && b) || c) {} // Good
+			if (b || c && d) {} // [warn]:
+			if (b || (c && d)) {} // Good
+		}
+	}c, sac);
+	stderr.writeln("Unittest for LogicPrecedenceCheck passed.");
+}
+
diff --git a/analysis/run.d b/analysis/run.d
index 379ddde..edebbf2 100644
--- a/analysis/run.d
+++ b/analysis/run.d
@@ -28,6 +28,7 @@ import analysis.opequals_without_tohash;
 import analysis.length_subtraction;
 import analysis.builtin_property_names;
 import analysis.asm_style;
+import analysis.logic_precedence;
 
 bool first = true;
 
@@ -155,6 +156,7 @@ MessageSet analyze(string fileName, ubyte[] code, const StaticAnalysisConfig ana
 	if (analysisConfig.length_subtraction_check) checks ~= new LengthSubtractionCheck(fileName);
 	if (analysisConfig.builtin_property_names_check) checks ~= new BuiltinPropertyNameCheck(fileName);
 	if (analysisConfig.asm_style_check) checks ~= new AsmStyleCheck(fileName);
+	if (analysisConfig.logical_precedence_check) checks ~= new LogicPrecedenceCheck(fileName);
 
 	foreach (check; checks)
 	{
diff --git a/astprinter.d b/astprinter.d
index 1e7b9af..8302510 100644
--- a/astprinter.d
+++ b/astprinter.d
@@ -625,6 +625,21 @@ class XMLPrinter : ASTVisitor
 		output.writeln("</mulExpression>");
 	}
 
+	override void visit(const OrOrExpression orOrExpression)
+	{
+		output.writeln("<orOrExpression>");
+		output.writeln("<left>");
+		visit(orOrExpression.left);
+		output.writeln("</left>");
+		if (orOrExpression.right !is null)
+		{
+			output.writeln("<right>");
+			visit(orOrExpression.right);
+			output.writeln("</right>");
+		}
+		output.writeln("</orOrExpression>");
+	}
+
 	override void visit(const Parameter param)
 	{
 		output.writeln("<parameter>");
@@ -1037,7 +1052,6 @@ class XMLPrinter : ASTVisitor
 	override void visit(const NonVoidInitializer nonVoidInitializer) { mixin (tagAndAccept!"nonVoidInitializer"); }
 	override void visit(const Operands operands) { mixin (tagAndAccept!"operands"); }
 	override void visit(const OrExpression orExpression) { mixin (tagAndAccept!"orExpression"); }
-	override void visit(const OrOrExpression orOrExpression) { mixin (tagAndAccept!"orOrExpression"); }
 	override void visit(const OutStatement outStatement) { mixin (tagAndAccept!"outStatement"); } override void visit(const MixinDeclaration mixinDeclaration) { mixin (tagAndAccept!"mixinDeclaration"); }
 	override void visit(const Parameters parameters) { mixin (tagAndAccept!"parameters"); }
 	override void visit(const Postblit postblit) { mixin (tagAndAccept!"postblit"); } override void visit(const NewAnonClassExpression newAnonClassExpression) { mixin (tagAndAccept!"newAnonClassExpression"); }