Bug 545959 - Added checker for assignment operator

Change-Id: Ib48742cbc04679ab9e48349f4d68aea5657d38c9
Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>

5 files changed, 407 insertions, 0 deletions

diff --git a/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties b/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties
index 735b445d5e6..5e13f9864c4 100644
--- a/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties
+++ b/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties
@@ -168,3 +168,11 @@ checker.name.VirtualMethodCallChecker = Virtual method call checker
 problem.name.VirtualMethodCallProblem = Virtual method call in constructor/destructor
 problem.messagePattern.VirtualMethodCallProblem = Calling a virtual method in constructor or destructor can cause crashes and unexpected behavior
 problem.description.VirtualMethodCallProblem = This rule will flag constructor and destructor with virtual method calls
+
+checker.name.AssignmentOperatorChecker = Assignment operator checker
+problem.name.MissSelfCheckProblem = Missing self check in assignment operator
+problem.messagePattern.MissSelfCheckProblem = Self assignment checks improves performance and avoid to unexpected behavior
+problem.description.MissSelfCheckProblem = This rule will flag assignment operators without a self assignment check
+problem.name.MissReferenceProblem = Missing reference return value in assignment operator
+problem.messagePattern.MissReferenceProblem = Assignment operators should return by reference
+problem.description.MissReferenceProblem = This rule will flag assignment operators not returning by reference
diff --git a/codan/org.eclipse.cdt.codan.checkers/plugin.xml b/codan/org.eclipse.cdt.codan.checkers/plugin.xml
index 470dbc9bd9a..99fae9b9f46 100644
--- a/codan/org.eclipse.cdt.codan.checkers/plugin.xml
+++ b/codan/org.eclipse.cdt.codan.checkers/plugin.xml
@@ -539,5 +539,30 @@
                name="%problem.name.VirtualMethodCallProblem">
          </problem>
       </checker>
+      <checker
+            class="org.eclipse.cdt.codan.internal.checkers.AssignmentOperatorChecker"
+            id="org.eclipse.cdt.codan.internal.checkers.AssignmentOperatorChecker"
+            name="%checker.name.AssignmentOperatorChecker">
+         <problem
+               category="org.eclipse.cdt.codan.core.categories.ProgrammingProblems"
+               defaultEnabled="false"
+               defaultSeverity="Warning"
+               description="%problem.description.MissSelfCheckProblem"
+               id="org.eclipse.cdt.codan.internal.checkers.MissSelfCheckProblem"
+               markerType="org.eclipse.cdt.codan.core.codanProblem"
+               messagePattern="%problem.messagePattern.MissSelfCheckProblem"
+               name="%problem.name.MissSelfCheckProblem">
+         </problem>
+         <problem
+               category="org.eclipse.cdt.codan.core.categories.CodeStyle"
+               defaultEnabled="false"
+               defaultSeverity="Warning"
+               description="%problem.description.MissReferenceProblem"
+               id="org.eclipse.cdt.codan.internal.checkers.MissReferenceProblem"
+               markerType="org.eclipse.cdt.codan.core.codanProblem"
+               messagePattern="%problem.messagePattern.MissReferenceProblem"
+               name="%problem.name.MissReferenceProblem">
+         </problem>
+      </checker>
    </extension>
 </plugin>
diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/AssignmentOperatorChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/AssignmentOperatorChecker.java
new file mode 100644
index 00000000000..c3196b940fe
--- /dev/null
+++ b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/AssignmentOperatorChecker.java
@@ -0,0 +1,181 @@
+/*******************************************************************************
+ * Copyright (c) 2019 Marco Stornelli
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ *******************************************************************************/
+package org.eclipse.cdt.codan.internal.checkers;
+
+import java.util.Arrays;
+import java.util.Stack;
+
+import org.eclipse.cdt.codan.core.cxx.model.AbstractIndexAstChecker;
+import org.eclipse.cdt.core.dom.ast.ASTVisitor;
+import org.eclipse.cdt.core.dom.ast.IASTBinaryExpression;
+import org.eclipse.cdt.core.dom.ast.IASTDeclaration;
+import org.eclipse.cdt.core.dom.ast.IASTDeclarator;
+import org.eclipse.cdt.core.dom.ast.IASTExpression;
+import org.eclipse.cdt.core.dom.ast.IASTIdExpression;
+import org.eclipse.cdt.core.dom.ast.IASTLiteralExpression;
+import org.eclipse.cdt.core.dom.ast.IASTNode;
+import org.eclipse.cdt.core.dom.ast.IASTPointerOperator;
+import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit;
+import org.eclipse.cdt.core.dom.ast.IASTUnaryExpression;
+import org.eclipse.cdt.core.dom.ast.IBinding;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTFunctionDeclarator;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTFunctionDefinition;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTParameterDeclaration;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTReferenceOperator;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTUnaryExpression;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPMethod;
+import org.eclipse.cdt.core.dom.ast.cpp.SemanticQueries;
+
+public class AssignmentOperatorChecker extends AbstractIndexAstChecker {
+	public static final String MISS_REF_ID = "org.eclipse.cdt.codan.internal.checkers.MissReferenceProblem"; //$NON-NLS-1$
+	public static final String MISS_SELF_CHECK_ID = "org.eclipse.cdt.codan.internal.checkers.MissSelfCheckProblem"; //$NON-NLS-1$
+	private static final String OPERATOR_EQ = "operator ="; //$NON-NLS-1$
+
+	@Override
+	public void processAst(IASTTranslationUnit ast) {
+		ast.accept(new OnEachClass());
+	}
+
+	private static class OperatorEqInfo {
+		IASTDeclarator decl;
+		ICPPASTParameterDeclaration[] parameters;
+	}
+
+	class OnEachClass extends ASTVisitor {
+		private final Stack<OperatorEqInfo> opInfo = new Stack<>();
+
+		OnEachClass() {
+			shouldVisitDeclarations = true;
+			shouldVisitExpressions = true;
+		}
+
+		@Override
+		public int visit(IASTDeclaration declaration) {
+			ICPPMethod method = getOperatorEq(declaration);
+			if (method != null) {
+				OperatorEqInfo info = opInfo.push(new OperatorEqInfo());
+				IASTDeclarator declMethod = ((ICPPASTFunctionDefinition) declaration).getDeclarator();
+				if (!(declMethod instanceof ICPPASTFunctionDeclarator))
+					return PROCESS_CONTINUE;
+				if (((ICPPASTFunctionDefinition) declaration).isDefaulted()
+						|| ((ICPPASTFunctionDefinition) declaration).isDeleted())
+					return PROCESS_CONTINUE;
+				IASTPointerOperator[] pointers = declMethod.getPointerOperators();
+				info.parameters = ((ICPPASTFunctionDeclarator) declMethod).getParameters();
+				if (pointers.length != 1 || !(pointers[0] instanceof ICPPASTReferenceOperator))
+					reportProblem(MISS_REF_ID, declaration);
+
+				if (!SemanticQueries.isCopyOrMoveAssignmentOperator(method))
+					return PROCESS_CONTINUE;
+
+				info.decl = declMethod;
+			}
+			return PROCESS_CONTINUE;
+		}
+
+		@Override
+		public int leave(IASTDeclaration declaration) {
+			if (getOperatorEq(declaration) != null && !opInfo.isEmpty()) {
+				opInfo.pop();
+			}
+			return PROCESS_CONTINUE;
+		}
+
+		@Override
+		public int visit(IASTExpression expression) {
+			if (!opInfo.isEmpty()) {
+				OperatorEqInfo info = opInfo.peek();
+				if (info.decl == null)
+					return PROCESS_CONTINUE;
+				if (expression instanceof IASTBinaryExpression) {
+					IASTBinaryExpression binary = (IASTBinaryExpression) expression;
+					if ((binary.getOperator() == IASTBinaryExpression.op_equals
+							|| binary.getOperator() == IASTBinaryExpression.op_notequals)) {
+						if (referencesThis(binary.getOperand1()) && referencesParameter(info, binary.getOperand2())) {
+							info.decl = null;
+						} else if (referencesThis(binary.getOperand2())
+								&& referencesParameter(info, binary.getOperand1())) {
+							info.decl = null;
+						} else {
+							reportProblem(MISS_SELF_CHECK_ID, info.decl);
+						}
+					} else {
+						reportProblem(MISS_SELF_CHECK_ID, info.decl);
+					}
+					info.decl = null;
+				} else {
+					reportProblem(MISS_SELF_CHECK_ID, info.decl);
+					info.decl = null;
+				}
+			}
+			return PROCESS_CONTINUE;
+		}
+
+		/**
+		 * Checks whether expression references the parameter (directly, by pointer or by reference)
+		 */
+		public boolean referencesParameter(OperatorEqInfo info, IASTNode expr) {
+			if (expr instanceof IASTIdExpression) {
+				IASTIdExpression id = (IASTIdExpression) expr;
+				if (Arrays.equals(id.getName().getSimpleID(),
+						info.parameters[0].getDeclarator().getName().getSimpleID())) {
+					return true;
+				}
+			} else if (expr instanceof ICPPASTUnaryExpression) {
+				ICPPASTUnaryExpression unExpr = (ICPPASTUnaryExpression) expr;
+				switch (unExpr.getOperator()) {
+				case IASTUnaryExpression.op_amper:
+				case IASTUnaryExpression.op_star:
+				case IASTUnaryExpression.op_bracketedPrimary:
+					return referencesParameter(info, unExpr.getOperand());
+				}
+			}
+			return false;
+		}
+
+		/**
+		 * Checks whether expression references this (directly, by pointer or by reference)
+		 */
+		public boolean referencesThis(IASTNode expr) {
+			if (expr instanceof IASTLiteralExpression) {
+				IASTLiteralExpression litArg = (IASTLiteralExpression) expr;
+				if (litArg.getKind() == IASTLiteralExpression.lk_this) {
+					return true;
+				}
+			} else if (expr instanceof ICPPASTUnaryExpression) {
+				ICPPASTUnaryExpression unExpr = (ICPPASTUnaryExpression) expr;
+				switch (unExpr.getOperator()) {
+				case IASTUnaryExpression.op_amper:
+				case IASTUnaryExpression.op_star:
+				case IASTUnaryExpression.op_bracketedPrimary:
+					return referencesThis(unExpr.getOperand());
+				}
+			}
+			return false;
+		}
+
+		private ICPPMethod getOperatorEq(IASTDeclaration decl) {
+			if (decl instanceof ICPPASTFunctionDefinition) {
+				ICPPASTFunctionDefinition functionDefinition = (ICPPASTFunctionDefinition) decl;
+				if (functionDefinition.isDeleted())
+					return null;
+				IBinding binding = functionDefinition.getDeclarator().getName().resolveBinding();
+				if (binding instanceof ICPPMethod) {
+					ICPPMethod method = (ICPPMethod) binding;
+					if (OPERATOR_EQ.equals(method.getName()))
+						return method;
+				}
+			}
+			return null;
+		}
+	}
+}
diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/AssignmentOperatorCheckerTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/AssignmentOperatorCheckerTest.java
new file mode 100644
index 00000000000..3d687bd8c8c
--- /dev/null
+++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/AssignmentOperatorCheckerTest.java
@@ -0,0 +1,191 @@
+/*******************************************************************************
+ * Copyright (c) 2019 Marco Stornelli
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *******************************************************************************/
+package org.eclipse.cdt.codan.core.internal.checkers;
+
+import org.eclipse.cdt.codan.core.tests.CheckerTestCase;
+import org.eclipse.cdt.codan.internal.checkers.AssignmentOperatorChecker;
+
+/**
+ * Test for {@link AssignmentOperatorChecker} class
+ */
+public class AssignmentOperatorCheckerTest extends CheckerTestCase {
+
+	public static final String MISS_REF_ID = AssignmentOperatorChecker.MISS_REF_ID;
+	public static final String MISS_SELF_ID = AssignmentOperatorChecker.MISS_SELF_CHECK_ID;
+
+	@Override
+	public void setUp() throws Exception {
+		super.setUp();
+		enableProblems(MISS_REF_ID, MISS_SELF_ID);
+	}
+
+	@Override
+	public boolean isCpp() {
+		return true;
+	}
+
+	//class Foo {
+	//public:
+	//Foo& operator=(const Foo& f);
+	//};
+	//Foo& Foo::operator=(const Foo& f) {
+	//    if (this != &f) {
+	//        return *this;
+	//    }
+	//}
+	public void testWithNoError() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(MISS_REF_ID);
+		checkNoErrorsOfKind(MISS_SELF_ID);
+	}
+
+	//class Foo {
+	//public:
+	//Foo operator=(const Foo& f);
+	//};
+	//Foo Foo::operator=(const Foo& f) {
+	//    if (this != &f) {
+	//        return *this;
+	//    }
+	//    return *this;
+	//}
+	public void testWithReturnByCopyPassByRef() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(5, MISS_REF_ID);
+		checkNoErrorsOfKind(MISS_SELF_ID);
+	}
+
+	//class Foo {
+	//public:
+	//Foo operator=(Foo f);
+	//};
+	//Foo Foo::operator=(Foo f) {
+	//    if (this != &f) {
+	//        return *this;
+	//    }
+	//    return *this;
+	//}
+	public void testWithReturnByCopyPassByValue() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(5, MISS_REF_ID);
+		checkNoErrorsOfKind(MISS_SELF_ID);
+	}
+
+	//class Foo {
+	//public:
+	//Foo& operator=(const Foo& f);
+	//};
+	//Foo& Foo::operator=(const Foo& f) {
+	//  return *this;
+	//}
+	public void testWithNoSelfCheckPassByRef() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(MISS_REF_ID);
+		checkErrorLine(5, MISS_SELF_ID);
+	}
+
+	//class Foo {
+	//public:
+	//Foo& operator=(Foo f);
+	//};
+	//Foo& Foo::operator=(Foo f) {
+	//  return *this;
+	//}
+	public void testWithNoSelfCheckPassByValue() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(MISS_REF_ID);
+		checkErrorLine(5, MISS_SELF_ID);
+	}
+
+	//class Foo {
+	//private:
+	//int p;
+	//public:
+	//Foo& operator=(const Foo& f);
+	//};
+	//Foo& Foo::operator=(const Foo& f) {
+	//    if (this == &f) {
+	//        return *this;
+	//    }
+	//    p = f.p;
+	//    return *this;
+	//}
+	public void testWithEqSelfCheck() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(MISS_REF_ID);
+		checkNoErrorsOfKind(MISS_SELF_ID);
+	}
+
+	//class Foo {
+	//private:
+	//int p;
+	//public:
+	//void operator=(const int& f);
+	//};
+	//void Foo::operator=(const int& f) {
+	//    p = f.p;
+	//}
+	public void testWithOpEqNoAssignment() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(7, MISS_REF_ID);
+		checkNoErrorsOfKind(MISS_SELF_ID);
+	}
+
+	//class Foo	{
+	//	Foo& operator=(Foo&) {
+	//		return *this;
+	//	}
+	//};
+	public void testWithInlineOpEq() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(2, MISS_SELF_ID);
+		checkNoErrorsOfKind(MISS_REF_ID);
+	}
+
+	//class Foo	{
+	//	class Bar {
+	//		Bar& operator=(Bar&) {
+	//			return *this;
+	//		}
+	//	}
+	//};
+	public void testWithNestedClasses() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(3, MISS_SELF_ID);
+		checkNoErrorsOfKind(MISS_REF_ID);
+	}
+
+	//class Foo {
+	//	Foo& operator=(Foo& a) {
+	//		class Local {
+	//			Local& operator=(Local& a) {
+	//				return *this;
+	//			}
+	//		};
+	//		return *this;
+	//	}
+	//};
+	public void testWithLocalClass() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(2, MISS_SELF_ID);
+		checkErrorLine(4, MISS_SELF_ID);
+		checkNoErrorsOfKind(MISS_REF_ID);
+	}
+
+	//class Foo	{
+	//	Foo& operator=(Foo&) = delete;
+	//};
+	public void testWithDeletedOpEq() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(MISS_SELF_ID);
+		checkNoErrorsOfKind(MISS_REF_ID);
+	}
+}
diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java
index 40120ed32de..b280c1ce2f0 100644
--- a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java
+++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java
@@ -16,6 +16,7 @@ package org.eclipse.cdt.codan.core.tests;
 
 import org.eclipse.cdt.codan.core.internal.checkers.AbstractClassInstantiationCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.AssignmentInConditionCheckerTest;
+import org.eclipse.cdt.codan.core.internal.checkers.AssignmentOperatorCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.AssignmentToItselfCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.CStyleCastCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.CaseBreakCheckerTest;
@@ -95,6 +96,7 @@ public class AutomatedIntegrationSuite extends TestSuite {
 		suite.addTestSuite(CopyrightCheckerTest.class);
 		suite.addTestSuite(SwitchCaseCheckerTest.class);
 		suite.addTestSuite(VirtualMethodCallCheckerTest.class);
+		suite.addTestSuite(AssignmentOperatorCheckerTest.class);
 		// framework
 		suite.addTest(CodanFastTestSuite.suite());
 		// quick fixes