add ignoreReturnAssignments to InnerAssignmentCheck (#260)

* allow `return this.value = value` in setter functions for inner assignement check, closes #259

* format

resources/default-config.json

@@ -161,6 +161,7 @@
 		},
 		{
 			"props": {
+				"ignoreReturnAssignments": false,
 				"severity": "IGNORE"
 			},
 			"type": "InnerAssignment"

src/checkstyle/checks/coding/InnerAssignmentCheck.hx

@@ -8,8 +8,11 @@ import haxeparser.Data.TokenDef;
 @desc("Checks for assignments in subexpressions, such as in `if ((a=b) > 0) return;`.")
 class InnerAssignmentCheck extends Check {
 
+	public var ignoreReturnAssignments:Bool;
+
 	public function new() {
 		super(TOKEN);
+		ignoreReturnAssignments = false;
 		categories = [Category.COMPLEXITY, Category.CLARITY, Category.BUG_RISK];
 		points = 5;
 	}
@@ -47,7 +50,7 @@ class InnerAssignmentCheck extends Check {
 			case Kwd(KwdVar): false;
 			case Kwd(KwdFunction): false;
 			case Kwd(KwdSwitch): true;
-			case Kwd(KwdReturn): true;
+			case Kwd(KwdReturn): filterReturn(token);
 			case BrOpen, DblDot: false;
 			case POpen: filterPOpen(token.parent);
 			default: filterAssignment(token.parent);
@@ -66,4 +69,45 @@ class InnerAssignmentCheck extends Check {
 			default: true;
 		}
 	}
+
+	function filterReturn(token:TokenTree):Bool {
+		if (!ignoreReturnAssignments) return true;
+
+		// only ignore return this.value = value when
+		// - there are no other Binops apart from =
+		// - return is only statement inside block
+		// - it is inside of setter function
+
+		var allBinops:Array<TokenTree> = token.filterCallback(function(token:TokenTree, depth:Int):FilterResult {
+			if (token.tok == null) return GO_DEEPER;
+			return switch (token.tok) {
+				case Binop(_): FOUND_GO_DEEPER;
+				case Unop(_): FOUND_GO_DEEPER;
+				case POpen, BkOpen, BrOpen: FOUND_GO_DEEPER;
+				default: GO_DEEPER;
+			}
+		});
+
+		if (allBinops.length != 1) return true;
+		var parent:TokenTree;
+		if (Type.enumEq(token.parent.tok, BrOpen)) {
+			var brOpen:TokenTree = token.parent;
+			// parent is a block and has more than two childs
+			if (brOpen.childs.length > 2) return true;
+			parent = brOpen.parent;
+		}
+		else {
+			parent = token.parent;
+			// parent is no block and has more than one child
+			if (parent.getLastChild() != token) return true;
+		}
+		if (!Type.enumEq(parent.parent.tok, Kwd(KwdFunction))) return true;
+		switch (parent.tok) {
+			case Const(CIdent(name)):
+				if (!StringTools.startsWith(name, "set_")) return true;
+			default: return true;
+		}
+
+		return false;
+	}
 }

test/checks/coding/InnerAssignmentCheckTest.hx

@@ -21,6 +21,28 @@ class InnerAssignmentCheckTest extends CheckTestCase<InnerAssignmentCheckTests>
 		assertMsg(check, IF_RETURN_EXPR, MSG_INNER_ASSIGNMENT);
 		assertMsg(check, WHILE_COND_RETURN, MSG_INNER_ASSIGNMENT);
 		assertMsg(check, SWITCH, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, SETTER_GETTER_ISSUE_259, MSG_INNER_ASSIGNMENT);
+	}
+
+	public function testIgnoreReturnAssignments () {
+		var check = new InnerAssignmentCheck();
+		check.ignoreReturnAssignments = true;
+		assertNoMsg(check, IF_EXPR);
+		assertNoMsg(check, WHILE_COND);
+		assertNoMsg(check, MEMBER_DEF);
+		assertNoMsg(check, METHOD_DEF);
+		assertNoMsg(check, BRACELESS_ANON_FUNC_ISSUE_113);
+		assertNoMsg(check, SETTER_GETTER_ISSUE_259);
+
+		assertMsg(check, IF_COND, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, IF_RETURN_EXPR, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, WHILE_COND_RETURN, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, SWITCH, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, INCORRECT_SETTER_GETTER_MULTIPLE_STATEMENTS_ISSUE_259, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, INCORRECT_SETTER_GETTER_MULTIPLE_BINOP_ISSUE_259, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, INCORRECT_SETTER_GETTER_UNOP_ISSUE_259, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, INCORRECT_SETTER_GETTER_ARRAY_ACCESS_ISSUE_259, MSG_INNER_ASSIGNMENT);
+		assertMsg(check, INCORRECT_SETTER_GETTER_CALL_ISSUE_259, MSG_INNER_ASSIGNMENT);
 	}
 }
 
@@ -99,6 +121,39 @@ abstract InnerAssignmentCheckTests(String) to String {
 			var b = false;
 			trace(function() b = true);
 		}
-	}
-	";
+	}";
+
+	var SETTER_GETTER_ISSUE_259  = "
+	class Test {
+		@:isVar
+		public var value(get, set) : String;
+		private function get_value() : String { return this.value; }
+		private function set_value(value : String) : String { return this.value = value; }
+		private function set_value(value : String) : String return this.value = value;
+	}";
+
+	var INCORRECT_SETTER_GETTER_MULTIPLE_STATEMENTS_ISSUE_259  = "
+	class Test {
+		private function set_value(value : String) : String { StringTools.trim(value); return this.value = value; }
+	}";
+
+	var INCORRECT_SETTER_GETTER_MULTIPLE_BINOP_ISSUE_259  = "
+	class Test {
+		private function set_value(value : String) : String { return this.value = value + 1; }
+	}";
+
+	var INCORRECT_SETTER_GETTER_UNOP_ISSUE_259  = "
+	class Test {
+		private function set_value(value : Int) : Int { return this.value = ++value; }
+	}";
+
+	var INCORRECT_SETTER_GETTER_ARRAY_ACCESS_ISSUE_259  = "
+	class Test {
+		private function set_value(value : Array<String>) : String { return this.value = value[0]; }
+	}";
+
+	var INCORRECT_SETTER_GETTER_CALL_ISSUE_259  = "
+	class Test {
+		private function set_value(value : String) : String { return this.value = StringTools.trim(value); }
+	}";
 }