1
0
Fork 0
mirror of https://github.com/eclipse-cdt/cdt synced 2025-04-29 19:45:01 +02:00

Bug 545959 - Added checker for assignment operator

Change-Id: Ib48742cbc04679ab9e48349f4d68aea5657d38c9
Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
This commit is contained in:
Marco Stornelli 2019-03-30 17:45:11 +01:00
parent 04350fec0e
commit 4c0e7d9f68
6 changed files with 468 additions and 7 deletions

View file

@ -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

View file

@ -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>

View file

@ -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;
}
}
}

View file

@ -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);
}
}

View file

@ -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

View file

@ -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);