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"> + + + + + + 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 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 diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/cpp/SemanticQueries.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/cpp/SemanticQueries.java index dfb20d44dfc..56ba8b20456 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/cpp/SemanticQueries.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/cpp/SemanticQueries.java @@ -14,6 +14,7 @@ package org.eclipse.cdt.core.dom.ast.cpp; import static org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil.CVTYPE; +import static org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil.REF; import static org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil.TDEF; import java.util.ArrayList; @@ -39,23 +40,76 @@ import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil; */ public class SemanticQueries { + private static final String OPERATOR_EQ = "operator ="; //$NON-NLS-1$ + public static boolean isCopyOrMoveConstructor(ICPPConstructor constructor) { - return isCopyOrMoveConstructor(constructor, CopyOrMoveConstructorKind.COPY_OR_MOVE); + return isCopyOrMoveConstructor(constructor, CopyOrMoveKind.COPY_OR_MOVE); } public static boolean isMoveConstructor(ICPPConstructor constructor) { - return isCopyOrMoveConstructor(constructor, CopyOrMoveConstructorKind.MOVE); + return isCopyOrMoveConstructor(constructor, CopyOrMoveKind.MOVE); } public static boolean isCopyConstructor(ICPPConstructor constructor) { - return isCopyOrMoveConstructor(constructor, CopyOrMoveConstructorKind.COPY); + return isCopyOrMoveConstructor(constructor, CopyOrMoveKind.COPY); } - private enum CopyOrMoveConstructorKind { + private enum CopyOrMoveKind { COPY, MOVE, COPY_OR_MOVE } - private static boolean isCopyOrMoveConstructor(ICPPConstructor constructor, CopyOrMoveConstructorKind kind) { + /** + * @since 6.9 + */ + public static boolean isCopyAssignmentOperator(ICPPMethod method) { + return isAssignmentOperator(method, CopyOrMoveKind.COPY); + } + + /** + * @since 6.9 + */ + public static boolean isCopyOrMoveAssignmentOperator(ICPPMethod method) { + return isAssignmentOperator(method, CopyOrMoveKind.COPY_OR_MOVE); + } + + /** + * @since 6.9 + */ + public static boolean isMoveAssignmentOperator(ICPPMethod method) { + return isAssignmentOperator(method, CopyOrMoveKind.MOVE); + } + + /** + * Check if the method is a copy assignment operator, i.e. an overload of "operator=" + * with one parameter which is of the same class type. + * @param method The method to be checked + * @return True if the method is a copy assignment operator, false otherwise + */ + private static boolean isAssignmentOperator(ICPPMethod method, CopyOrMoveKind kind) { + if (!OPERATOR_EQ.equals(method.getName())) + return false; + if (method instanceof ICPPFunctionTemplate) + return false; + if (!isCallableWithNumberOfArguments(method, 1)) + return false; + IType firstArgumentType = method.getType().getParameterTypes()[0]; + firstArgumentType = SemanticUtil.getNestedType(firstArgumentType, TDEF); + if (!(firstArgumentType instanceof ICPPReferenceType) && kind == CopyOrMoveKind.MOVE) + return false; + if (firstArgumentType instanceof ICPPReferenceType) { + if (kind == CopyOrMoveKind.MOVE && !((ICPPReferenceType) firstArgumentType).isRValueReference()) + return false; + if (kind == CopyOrMoveKind.COPY && ((ICPPReferenceType) firstArgumentType).isRValueReference()) + return false; + } + firstArgumentType = SemanticUtil.getNestedType(firstArgumentType, REF | CVTYPE); + ICPPClassType classType = method.getClassOwner(); + if (classType instanceof ICPPClassTemplate) + classType = CPPTemplates.createDeferredInstance((ICPPClassTemplate) classType); + return firstArgumentType.isSameType(classType); + } + + private static boolean isCopyOrMoveConstructor(ICPPConstructor constructor, CopyOrMoveKind kind) { // 12.8/2-3 [class.copy]: // "A non-template constructor for class X is a copy [move] constructor // if its first parameter is of type X&[&], const X&[&], volatile X&[&] @@ -71,9 +125,9 @@ public class SemanticQueries { return false; ICPPReferenceType firstArgReferenceType = (ICPPReferenceType) firstArgumentType; boolean isRvalue = firstArgReferenceType.isRValueReference(); - if (isRvalue && kind == CopyOrMoveConstructorKind.COPY) + if (isRvalue && kind == CopyOrMoveKind.COPY) return false; - if (!isRvalue && kind == CopyOrMoveConstructorKind.MOVE) + if (!isRvalue && kind == CopyOrMoveKind.MOVE) return false; firstArgumentType = firstArgReferenceType.getType(); firstArgumentType = SemanticUtil.getNestedType(firstArgumentType, CVTYPE);