mirrors/cdt
1
0
Fork 0
mirror of https://github.com/eclipse-cdt/cdt synced 2025-07-25 09:55:29 +02:00
Code Issues Wiki Activity

Bug 545714 - Added a shallow copy checker

Browse source 
Change-Id: I8998a98f9d7836405afcaf54bf241b0acbd9edea
Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
This commit is contained in:
Marco Stornelli 2019-03-24 15:04:49 +01:00
parent 9cbe95695c
commit 2e761a5359
7 changed files with 511 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties
View file

@ -202,3 +202,8 @@ checker.name.SymbolShadowing = Symbol shadowing checker
problem.description.SymbolShadowing = This rule will flag symbols, like local variables, class fields or method parameters, shadowing another symbol in upper scope
problem.messagePattern.SymbolShadowing = Symbol "{0}" is masking symbol declared in upper scope
problem.name.SymbolShadowing = Symbol shadowing
checker.name.ShallowCopyChecker = Copy ctor and assignment operator checker
problem.name.ShallowCopyProblem = Miss copy constructor or assignment operator
problem.messagePattern.ShallowCopyProblem = Shallow copy can have negative run-time effects, explicit copy constructors/assignment operators for classes with references/pointers should be used
problem.description.ShallowCopyProblem = This rule will flag classes with pointer members without copy constructor or assignment operator

15
codan/org.eclipse.cdt.codan.checkers/plugin.xml
View file

@ -601,6 +601,21 @@
markerType="org.eclipse.cdt.codan.core.codanProblem"
messagePattern="%problem.messagePattern.BlacklistProblem"
name="%problem.name.BlacklistProblem">
</problem>
</checker>
<checker
class="org.eclipse.cdt.codan.internal.checkers.ShallowCopyChecker"
id="org.eclipse.cdt.codan.internal.checkers.ShallowCopyChecker"
name="%checker.name.ShallowCopyChecker">
<problem
category="org.eclipse.cdt.codan.core.categories.ProgrammingProblems"
defaultEnabled="false"
defaultSeverity="Warning"
description="%problem.description.ShallowCopyProblem"
id="org.eclipse.cdt.codan.internal.checkers.ShallowCopyProblem"
markerType="org.eclipse.cdt.codan.core.codanProblem"
messagePattern="%problem.messagePattern.ShallowCopyProblem"
name="%problem.name.ShallowCopyProblem">
</problem>
</checker>
<checker

2
codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.java
View file

@ -44,6 +44,8 @@ public class CheckersMessages extends NLS {
public static String Copyright_regex;
public static String ShallowCopyChecker_OnlyNew;
static {
NLS.initializeMessages(CheckersMessages.class.getName(), CheckersMessages.class);
}

2
codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.properties
View file

@ -32,6 +32,8 @@ SwitchCaseChecker_ParameterDefaultAllEnums=Mark even switches with complete enum
BlacklistChecker_list=List of methods/functions to be checked
BlacklistChecker_list_item=Fully qualified method name or function name
ShallowCopyChecker_OnlyNew=Check only classes using operator 'new' in constructor
ProblemBindingChecker_Candidates=Candidates are:
UnusedSymbolInFileScopeChecker_CharacterSequence=Identifier character sequence:
UnusedSymbolInFileScopeChecker_Exceptions=Exceptions (identifier character sequence)

275
codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/ShallowCopyChecker.java Normal file
View file

@ -0,0 +1,275 @@
/*******************************************************************************
* 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.Stack;
import org.eclipse.cdt.codan.core.cxx.model.AbstractIndexAstChecker;
import org.eclipse.cdt.codan.core.model.IProblemWorkingCopy;
import org.eclipse.cdt.core.dom.ast.ASTVisitor;
import org.eclipse.cdt.core.dom.ast.IASTBinaryExpression;
import org.eclipse.cdt.core.dom.ast.IASTDeclSpecifier;
import org.eclipse.cdt.core.dom.ast.IASTDeclaration;
import org.eclipse.cdt.core.dom.ast.IASTExpression;
import org.eclipse.cdt.core.dom.ast.IASTIdExpression;
import org.eclipse.cdt.core.dom.ast.IASTName;
import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit;
import org.eclipse.cdt.core.dom.ast.IBinding;
import org.eclipse.cdt.core.dom.ast.ICompositeType;
import org.eclipse.cdt.core.dom.ast.IPointerType;
import org.eclipse.cdt.core.dom.ast.IType;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTCompositeTypeSpecifier;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTConstructorChainInitializer;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTFieldReference;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTFunctionDefinition;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTNewExpression;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPClassType;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPConstructor;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPField;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPMethod;
import org.eclipse.cdt.core.dom.ast.cpp.ICPPReferenceType;
import org.eclipse.cdt.core.dom.ast.cpp.SemanticQueries;
import org.eclipse.cdt.internal.core.dom.parser.cpp.ICPPDeferredClassInstance;
import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.CPPSemantics;
import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil;
/**
* Checker to find that class has pointers but no copy constructor
*/
@SuppressWarnings("restriction")
public class ShallowCopyChecker extends AbstractIndexAstChecker {
public static final String PROBLEM_ID = "org.eclipse.cdt.codan.internal.checkers.ShallowCopyProblem"; //$NON-NLS-1$
public static final String PARAM_ONLY_NEW = "onlynew"; //$NON-NLS-1$
private boolean fOnlyNew = false;
@Override
public void initPreferences(IProblemWorkingCopy problem) {
super.initPreferences(problem);
addPreference(problem, PARAM_ONLY_NEW, CheckersMessages.ShallowCopyChecker_OnlyNew, Boolean.FALSE);
}
@Override
public void processAst(IASTTranslationUnit ast) {
fOnlyNew = (Boolean) getPreference(getProblemById(PROBLEM_ID, getFile()), PARAM_ONLY_NEW);
if (fOnlyNew) {
ast.accept(new OnlyNewVisitor());
} else {
if (!ast.isHeaderUnit())
return;
ast.accept(new AllPtrsVisitor());
}
}
private boolean isPointerType(IType type) {
type = SemanticUtil.getNestedType(type, SemanticUtil.TDEF);
return type instanceof IPointerType;
}
private boolean isReferenceType(IType type) {
type = SemanticUtil.getNestedType(type, SemanticUtil.TDEF);
return type instanceof ICPPReferenceType;
}
private static boolean hasCopyMethods(ICPPClassType classType) {
boolean hasCopyCtor = false;
boolean hasCopyAssignment = false;
for (ICPPMethod method : classType.getDeclaredMethods()) {
if (!hasCopyCtor && method instanceof ICPPConstructor
&& SemanticQueries.isCopyConstructor((ICPPConstructor) method)) {
hasCopyCtor = true;
} else if (!hasCopyAssignment && SemanticQueries.isCopyAssignmentOperator(method)) {
hasCopyAssignment = true;
}
if (hasCopyAssignment && hasCopyCtor)
return true;
}
return false;
}
private static class CtorInfo {
ICPPConstructor ctor;
boolean hasProblem;
boolean hasCopyMethods;
CtorInfo() {
hasProblem = false;
hasCopyMethods = false;
ctor = null;
}
}
class OnlyNewVisitor extends ASTVisitor {
// NOTE: Classes can be nested and even can be declared in constructors of the other classes
private final Stack<CtorInfo> constructorsStack = new Stack<>();
OnlyNewVisitor() {
shouldVisitDeclarations = true;
shouldVisitNames = true;
shouldVisitExpressions = true;
}
@Override
public int visit(IASTDeclaration declaration) {
ICPPConstructor constructor = getConstructor(declaration);
if (constructor != null) {
CtorInfo info = constructorsStack.push(new CtorInfo());
info.ctor = constructor;
info.hasProblem = false;
try {
CPPSemantics.pushLookupPoint(declaration);
ICPPClassType classType = constructor.getClassOwner();
info.hasCopyMethods = hasCopyMethods(classType);
return PROCESS_CONTINUE;
} finally {
CPPSemantics.popLookupPoint();
}
}
return PROCESS_CONTINUE;
}
@Override
public int leave(IASTDeclaration declaration) {
if (getConstructor(declaration) != null) {
CtorInfo info = constructorsStack.pop();
if (info.hasProblem) {
reportProblem(PROBLEM_ID, declaration);
}
}
return PROCESS_CONTINUE;
}
@Override
public int visit(IASTExpression expression) {
if (!constructorsStack.isEmpty() && expression instanceof IASTBinaryExpression
&& ((IASTBinaryExpression) expression).getOperator() == IASTBinaryExpression.op_assign) {
CtorInfo info = constructorsStack.peek();
if (info.hasCopyMethods)
return PROCESS_CONTINUE;
IASTExpression exp1 = ((IASTBinaryExpression) expression).getOperand1();
IASTExpression exp2 = ((IASTBinaryExpression) expression).getOperand2();
if (exp1 instanceof ICPPASTNewExpression) {
IBinding fBinding = null;
if (exp2 instanceof IASTIdExpression) {
IASTIdExpression fName = (IASTIdExpression) exp2;
fBinding = fName.getName().resolveBinding();
} else if (exp2 instanceof ICPPASTFieldReference) {
ICPPASTFieldReference fName = (ICPPASTFieldReference) exp2;
fBinding = fName.getFieldName().resolveBinding();
}
if (fBinding != null && fBinding instanceof ICPPField) {
ICPPField field = (ICPPField) fBinding;
if (info.ctor.getClassOwner().equals(field.getClassOwner())) {
info.hasProblem = true;
}
}
} else if (exp2 instanceof ICPPASTNewExpression) {
IBinding fBinding = null;
if (exp1 instanceof IASTIdExpression) {
IASTIdExpression fName = (IASTIdExpression) exp1;
fBinding = fName.getName().resolveBinding();
} else if (exp1 instanceof ICPPASTFieldReference) {
ICPPASTFieldReference fName = (ICPPASTFieldReference) exp1;
fBinding = fName.getFieldName().resolveBinding();
}
if (fBinding != null && fBinding instanceof ICPPField) {
ICPPField field = (ICPPField) fBinding;
if (info.ctor.getClassOwner().equals(field.getClassOwner())) {
info.hasProblem = true;
}
}
}
}
return PROCESS_CONTINUE;
}
/**
* Checks that specified declaration is a class constructor
* (it is a class member and its name is equal to the class name)
*/
private ICPPConstructor getConstructor(IASTDeclaration decl) {
if (decl instanceof ICPPASTFunctionDefinition) {
ICPPASTFunctionDefinition functionDefinition = (ICPPASTFunctionDefinition) decl;
if (functionDefinition.isDeleted())
return null;
IBinding binding = functionDefinition.getDeclarator().getName().resolveBinding();
if (binding instanceof ICPPConstructor) {
ICPPConstructor constructor = (ICPPConstructor) binding;
// Skip defaulted copy and move constructors.
if (functionDefinition.isDefaulted() && SemanticQueries.isCopyOrMoveConstructor(constructor))
return null;
if (constructor.getClassOwner().getKey() == ICompositeType.k_union)
return null;
// Skip delegating constructors.
for (ICPPASTConstructorChainInitializer memberInitializer : functionDefinition
.getMemberInitializers()) {
IASTName memberName = memberInitializer.getMemberInitializerId();
if (memberName != null) {
IBinding memberBinding = memberName.resolveBinding();
ICPPClassType classType = null;
if (memberBinding instanceof ICPPClassType) {
classType = (ICPPClassType) memberBinding;
} else if (memberBinding instanceof ICPPConstructor) {
classType = ((ICPPConstructor) memberBinding).getClassOwner();
}
if (classType instanceof ICPPDeferredClassInstance) {
classType = ((ICPPDeferredClassInstance) classType).getClassTemplate();
}
if (classType != null && classType.isSameType(constructor.getClassOwner()))
return null;
}
}
return constructor;
}
}
return null;
}
}
private class AllPtrsVisitor extends ASTVisitor {
AllPtrsVisitor() {
shouldVisitDeclSpecifiers = true;
}
@Override
public int visit(IASTDeclSpecifier decl) {
if (decl instanceof ICPPASTCompositeTypeSpecifier) {
ICPPASTCompositeTypeSpecifier spec = (ICPPASTCompositeTypeSpecifier) decl;
IASTName className = spec.getName();
IBinding binding = className.resolveBinding();
if (!(binding instanceof ICPPClassType)) {
return PROCESS_CONTINUE;
}
try {
CPPSemantics.pushLookupPoint(className);
ICPPClassType classType = (ICPPClassType) binding;
boolean hasCopyMethods = hasCopyMethods(classType);
ICPPField[] fields = classType.getDeclaredFields();
boolean hasPointers = false;
for (ICPPField f : fields) {
if (isPointerType(f.getType()) || isReferenceType(f.getType())) {
hasPointers = true;
break;
}
}
if (!hasCopyMethods && hasPointers)
reportProblem(PROBLEM_ID, decl);
return PROCESS_CONTINUE;
} finally {
CPPSemantics.popLookupPoint();
}
}
return PROCESS_CONTINUE;
}
}
}

210
codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/ShallowCopyCheckerTest.java Normal file
View file

@ -0,0 +1,210 @@
/*******************************************************************************
* 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.param.IProblemPreference;
import org.eclipse.cdt.codan.core.tests.CheckerTestCase;
import org.eclipse.cdt.codan.internal.checkers.ShallowCopyChecker;
/**
* Test for {@link ShallowCopyChecker} class
*/
public class ShallowCopyCheckerTest extends CheckerTestCase {
public static final String ERR_ID = ShallowCopyChecker.PROBLEM_ID;
@Override
public void setUp() throws Exception {
super.setUp();
enableProblems(ERR_ID);
}
private void setOnlyNew(boolean val) {
IProblemPreference pref = getPreference(ShallowCopyChecker.PROBLEM_ID, ShallowCopyChecker.PARAM_ONLY_NEW);
pref.setValue(val);
}
@Override
public boolean isCpp() {
return true;
}
@Override
public boolean isHeader() {
return true;
}
//class Foo {
//public:
//void* p;
//};
public void testWithPointerOnly() throws Exception {
loadCodeAndRun(getAboveComment());
checkErrorLine(1, ERR_ID);
}
//class Foo {
//public:
//Foo(const Foo& o);
//void* p;
//};
public void testWithCopyConstrOnly() throws Exception {
loadCodeAndRun(getAboveComment());
checkErrorLine(1, ERR_ID);
}
//class Foo {
//public:
//Foo& operator=(const Foo& f);
//void* p;
//};
public void testWithAssignOnly() throws Exception {
loadCodeAndRun(getAboveComment());
checkErrorLine(1, ERR_ID);
}
//class Foo {
//public:
//Foo(const Foo& o);
//Foo& operator=(const Foo& f);
//void* p;
//};
public void testWithCopyMethods() throws Exception {
loadCodeAndRun(getAboveComment());
checkNoErrorsOfKind(ERR_ID);
}
//class Foo {
//public:
//Foo();
//};
public void testWithoutPointers() throws Exception {
loadCodeAndRun(getAboveComment());
checkNoErrorsOfKind(ERR_ID);
}
/****************/
//class Foo {
//public:
//Foo(int& f);
//int& p;
//};
public void testWithRefOnly() throws Exception {
loadCodeAndRun(getAboveComment());
checkErrorLine(1, ERR_ID);
}
//class Foo {
//public:
//Foo(const Foo& o);
//Foo(int& f);
//int& p;
//};
public void testWithRefCopyConstrOnly() throws Exception {
loadCodeAndRun(getAboveComment());
checkErrorLine(1, ERR_ID);
}
//class Foo {
//public:
//Foo& operator=(const Foo& f);
//Foo(int& f);
//int& p;
//};
public void testWithRefAssignOnly() throws Exception {
loadCodeAndRun(getAboveComment());
checkErrorLine(1, ERR_ID);
}
//class Foo {
//public:
//Foo(const Foo& o);
//Foo& operator=(const Foo& f);
//Foo(int& f);
//int& p;
//};
public void testWithRefCopyMethods() throws Exception {
loadCodeAndRun(getAboveComment());
checkNoErrorsOfKind(ERR_ID);
}
//class Foo {
//public:
//Foo();
//int* p;
//};
//Foo::Foo() {
//p = new int;
//}
public void testOnlyNewWithFieldPtr() throws Exception {
setOnlyNew(true);
loadCodeAndRun(getAboveComment());
checkErrorLine(6, ERR_ID);
}
//class Foo {
//public:
//Foo();
//};
//Foo::Foo() {
//int* p = new int;
//}
public void testOnlyNewNoField() throws Exception {
setOnlyNew(true);
loadCodeAndRun(getAboveComment());
checkNoErrorsOfKind(ERR_ID);
}
//class Foo {
//public:
//Foo();
//Foo(const Foo& e);
//Foo& operator=(const Foo& e);
//int* p;
//};
//Foo::Foo() {
//p = new int;
//}
public void testOnlyNewWithFieldPtrCopyPresent() throws Exception {
setOnlyNew(true);
loadCodeAndRun(getAboveComment());
checkNoErrorsOfKind(ERR_ID);
}
//class Foo {
//public:
//Foo();
//Foo(const Foo& e);
//Foo& operator=(const int& e);
//int* p;
//};
//Foo::Foo() {
//p = new int;
//}
public void testOnlyNewWithOtherCopyAssignment() throws Exception {
setOnlyNew(true);
loadCodeAndRun(getAboveComment());
checkErrorLine(8, ERR_ID);
}
//class Foo {
//public:
//Foo();
//Foo(const Foo& e);
//Foo& operator=(const int& e);
//int* p;
//};
public void testWithOtherCopyAssignment() throws Exception {
setOnlyNew(false);
loadCodeAndRun(getAboveComment());
checkErrorLine(1, ERR_ID);
}
}

2
codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java
View file

@ -33,6 +33,7 @@ import org.eclipse.cdt.codan.core.internal.checkers.NonVirtualDestructorCheckerT
import org.eclipse.cdt.codan.core.internal.checkers.ProblemBindingCheckerTest;
import org.eclipse.cdt.codan.core.internal.checkers.ReturnCheckerTest;
import org.eclipse.cdt.codan.core.internal.checkers.ReturnStyleCheckerTest;
import org.eclipse.cdt.codan.core.internal.checkers.ShallowCopyCheckerTest;
import org.eclipse.cdt.codan.core.internal.checkers.StatementHasNoEffectCheckerTest;
import org.eclipse.cdt.codan.core.internal.checkers.SuggestedParenthesisCheckerTest;
import org.eclipse.cdt.codan.core.internal.checkers.SuspiciousSemicolonCheckerTest;
@ -105,6 +106,7 @@ public class AutomatedIntegrationSuite extends TestSuite {
suite.addTestSuite(UsingInHeaderCheckerTest.class);
suite.addTestSuite(FloatCompareCheckerTest.class);
suite.addTestSuite(VariableShadowingCheckerTest.class);
suite.addTestSuite(ShallowCopyCheckerTest.class);
// framework
suite.addTest(CodanFastTestSuite.suite());
// quick fixes