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 5ca5f106e93..94d9c7de0b9 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 @@ -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 diff --git a/codan/org.eclipse.cdt.codan.checkers/plugin.xml b/codan/org.eclipse.cdt.codan.checkers/plugin.xml index a75f6ca5980..a3143436ca8 100644 --- a/codan/org.eclipse.cdt.codan.checkers/plugin.xml +++ b/codan/org.eclipse.cdt.codan.checkers/plugin.xml @@ -601,6 +601,21 @@ markerType="org.eclipse.cdt.codan.core.codanProblem" messagePattern="%problem.messagePattern.BlacklistProblem" name="%problem.name.BlacklistProblem"> + + + + 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; + } + } +} diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/ShallowCopyCheckerTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/ShallowCopyCheckerTest.java new file mode 100644 index 00000000000..d27e677239b --- /dev/null +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/ShallowCopyCheckerTest.java @@ -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); + } +} 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 fe806334e26..23b0661de9c 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 @@ -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