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 92c33fd3abe..5ca5f106e93 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 @@ -197,3 +197,8 @@ checker.name.FloatCompareChecker = Float compare checker problem.name.FloatCompareProblem = Direct float comparison problem.messagePattern.FloatCompareProblem = Float numbers should be compared using an epsilon problem.description.FloatCompareProblem = This rule will flag direct float or double comparison + +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 diff --git a/codan/org.eclipse.cdt.codan.checkers/plugin.xml b/codan/org.eclipse.cdt.codan.checkers/plugin.xml index 9a1a0ab2c83..a75f6ca5980 100644 --- a/codan/org.eclipse.cdt.codan.checkers/plugin.xml +++ b/codan/org.eclipse.cdt.codan.checkers/plugin.xml @@ -628,5 +628,19 @@ name="%problem.name.MultipleDeclarationsProblem"> + + + + diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.java index f8d630c241f..2f0e48f4f58 100644 --- a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.java +++ b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.java @@ -40,6 +40,7 @@ public class CheckersMessages extends NLS { public static String BlacklistChecker_list_item; public static String UnusedSymbolInFileScopeChecker_CharacterSequence; public static String UnusedSymbolInFileScopeChecker_Exceptions; + public static String SymbolShadowingChecker_CheckFunctionParameters; public static String Copyright_regex; diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.properties b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.properties index 105eec2d977..8b908138c32 100644 --- a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.properties +++ b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.properties @@ -36,4 +36,6 @@ ProblemBindingChecker_Candidates=Candidates are: UnusedSymbolInFileScopeChecker_CharacterSequence=Identifier character sequence: UnusedSymbolInFileScopeChecker_Exceptions=Exceptions (identifier character sequence) -Copyright_regex=Regex to search for copyright information \ No newline at end of file +Copyright_regex=Regex to search for copyright information + +SymbolShadowingChecker_CheckFunctionParameters=Check even function/method parameters diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/SymbolShadowingChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/SymbolShadowingChecker.java new file mode 100644 index 00000000000..4af5d5dd796 --- /dev/null +++ b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/SymbolShadowingChecker.java @@ -0,0 +1,176 @@ +/******************************************************************************* + * 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 + * + * Contributors: + * Marco Stornelli - Initial implementation + *******************************************************************************/ + +package org.eclipse.cdt.codan.internal.checkers; + +import java.util.HashSet; +import java.util.Set; + +import org.eclipse.cdt.codan.checkers.CodanCheckersActivator; +import org.eclipse.cdt.codan.core.cxx.model.AbstractIndexAstChecker; +import org.eclipse.cdt.codan.core.model.IProblemLocation; +import org.eclipse.cdt.codan.core.model.IProblemWorkingCopy; +import org.eclipse.cdt.core.dom.ast.ASTVisitor; +import org.eclipse.cdt.core.dom.ast.DOMException; +import org.eclipse.cdt.core.dom.ast.EScopeKind; +import org.eclipse.cdt.core.dom.ast.IASTDeclarator; +import org.eclipse.cdt.core.dom.ast.IASTFileLocation; +import org.eclipse.cdt.core.dom.ast.IASTImageLocation; +import org.eclipse.cdt.core.dom.ast.IASTName; +import org.eclipse.cdt.core.dom.ast.IASTNode; +import org.eclipse.cdt.core.dom.ast.IASTParameterDeclaration; +import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit; +import org.eclipse.cdt.core.dom.ast.IBinding; +import org.eclipse.cdt.core.dom.ast.IField; +import org.eclipse.cdt.core.dom.ast.IParameter; +import org.eclipse.cdt.core.dom.ast.IProblemBinding; +import org.eclipse.cdt.core.dom.ast.IScope; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPVariableInstance; +import org.eclipse.cdt.core.index.IIndex; +import org.eclipse.cdt.core.index.IIndexName; +import org.eclipse.cdt.internal.core.dom.parser.cpp.ICPPInternalVariable; +import org.eclipse.cdt.internal.core.pdom.dom.cpp.PDOMCPPGlobalScope; +import org.eclipse.core.runtime.CoreException; + +@SuppressWarnings("restriction") +public class SymbolShadowingChecker extends AbstractIndexAstChecker { + + public static final String ERR_ID = "org.eclipse.cdt.codan.internal.checkers.SymbolShadowingProblem"; //$NON-NLS-1$ + public static final String PARAM_FUNC_PARAM = "paramFuncParameters"; //$NON-NLS-1$ + + private IASTTranslationUnit ast; + private IIndex index; + + private boolean checkFuncParams; + + @Override + public void initPreferences(IProblemWorkingCopy problem) { + super.initPreferences(problem); + addPreference(problem, PARAM_FUNC_PARAM, CheckersMessages.SymbolShadowingChecker_CheckFunctionParameters, + Boolean.TRUE); + } + + @Override + public void processAst(IASTTranslationUnit ast) { + this.ast = ast; + index = ast.getIndex(); + checkFuncParams = (Boolean) getPreference(getProblemById(ERR_ID, getFile()), PARAM_FUNC_PARAM); + ast.accept(new VariableDeclarationVisitor()); + } + + /** + * This visitor looks for variable declarations. + */ + class VariableDeclarationVisitor extends ASTVisitor { + + VariableDeclarationVisitor() { + shouldVisitDeclarators = true; + } + + /** + * Check if it's the type we want. We check for fields, variables and parameters. + * @param binding The binding to be checked + * @return True if it's a field or variable, false otherwise + */ + private boolean validBinding(IBinding binding) { + return binding instanceof IField || binding instanceof ICPPInternalVariable + || binding instanceof ICPPVariableInstance || (checkFuncParams && binding instanceof IParameter); + } + + private void report(String id, IASTNode astNode, Set cache, Object... args) { + IProblemLocation loc = getProblemLocation(astNode); + if (loc != null && !cache.contains(loc)) { + reportProblem(id, loc, args); + cache.add(loc); + } + } + + private int getLocation(IASTNode astNode, IASTFileLocation astLocation) { + if (enclosedInMacroExpansion(astNode) && astNode instanceof IASTName) { + IASTImageLocation imageLocation = ((IASTName) astNode).getImageLocation(); + if (imageLocation != null) { + return imageLocation.getNodeOffset(); + } + } + return astLocation.getNodeOffset(); + } + + /** + * Check if at least one of declNames is declared before our declarator, if they + * are all declared after our declarator then there's no shadowing. + * @param declarator The declarator in lower scope to be checked + * @param declNames The declarator names in upper scope + * @return True if at least of upper scopes is declared before declarator, false otherwise + */ + private boolean isParentDeclaredBefore(IASTDeclarator declarator, IASTName[] declNames) { + int start = getLocation(declarator, declarator.getFileLocation()); + for (IASTName n : declNames) { + if (getLocation(n, n.getFileLocation()) < start) + return true; + } + return false; + } + + @Override + public int visit(IASTDeclarator declarator) { + if (!checkFuncParams && declarator.getParent() instanceof IASTParameterDeclaration) + return PROCESS_CONTINUE; + IBinding binding = declarator.getName().resolveBinding(); + + if (binding == null || binding instanceof IProblemBinding) + return PROCESS_CONTINUE; + + /** + * We need a cache here to avoid to report same problem multiple times. + */ + Set cache = new HashSet<>(); + IScope scope; + try { + scope = binding.getScope(); + if (scope.getKind() != EScopeKind.eLocal) + return PROCESS_CONTINUE; + scope = scope.getParent(); + while (scope != null && !(scope instanceof IProblemBinding) && !(scope instanceof PDOMCPPGlobalScope)) { + IBinding[] scopeBindings = scope.find(declarator.getName().toString(), + declarator.getTranslationUnit()); + + IScope current = scope; + scope = scope.getParent(); + for (IBinding scopeBinding : scopeBindings) { + if (scopeBinding != null && validBinding(scopeBinding)) { + IASTName[] declNames = ast.getDeclarationsInAST(scopeBinding); + if (declNames != null && declNames.length != 0) { + if (scope != null && current.getKind() == scope.getKind() + && current.getKind() == EScopeKind.eLocal + && isParentDeclaredBefore(declarator, declNames)) { + report(ERR_ID, declarator, cache, declarator.getName()); + } else if (scope == null || current.getKind() != scope.getKind()) { + report(ERR_ID, declarator, cache, declarator.getName()); + } + } else { + IIndexName[] indexNames = index.findDeclarations(scopeBinding); + if (indexNames != null && indexNames.length != 0) + report(ERR_ID, declarator, cache, declarator.getName()); + } + break; + } + } + } + } catch (DOMException | CoreException e) { + CodanCheckersActivator.log(e); + } + return PROCESS_CONTINUE; + } + } +} \ No newline at end of file diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/VariableShadowingCheckerTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/VariableShadowingCheckerTest.java new file mode 100644 index 00000000000..42e64bb74c2 --- /dev/null +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/VariableShadowingCheckerTest.java @@ -0,0 +1,234 @@ +/******************************************************************************* + * 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 + * + * Contributors: + * Marco Stornelli - Initial implementation + *******************************************************************************/ +package org.eclipse.cdt.codan.core.internal.checkers; + +import org.eclipse.cdt.codan.core.tests.CheckerTestCase; +import org.eclipse.cdt.codan.internal.checkers.SymbolShadowingChecker; + +/** + * Test for {@link#VariableShadowingChecker} class + */ +public class VariableShadowingCheckerTest extends CheckerTestCase { + + public static final String ERR_ID = SymbolShadowingChecker.ERR_ID; + + @Override + public void setUp() throws Exception { + super.setUp(); + enableProblems(ERR_ID); + } + + @Override + public boolean isCpp() { + return true; + } + + //int a; + //void foo(void) { + // int a; + //} + public void testGlobalFuncLoc() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //int a; + //void foo(void) { + // for( int a = 1; a < 2; a++ ) { + // } + //} + public void testGlobalFor() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //class Fpp { + // int a; + // void foo() { + // int a; + // } + //}; + public void testClassVSFuncLoc() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(4, ERR_ID); + } + + //class Fpp { + // int a; + // void foo() { + // for( int a = 1; a < 2; a++ ) { + // } + // } + //}; + public void testClassVSFor() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(4, ERR_ID); + } + + //class Fpp { + // int a; + // void foo() { + // } + //}; + //class Bar { + // void foo() { + // int a; + // } + //}; + public void testOtherClassVSFuncLoc() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ERR_ID); + } + + //void foo() { + // int a; + // for( int a = 1; a < 2; a++ ) { + // } + //} + public void testFuncLocVSFor() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //void bar(int a) { + //} + //void foo(void) { + // for( int a = 1; a < 2; a++ ) { + // } + //} + public void testFuncLocVSForOK() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ERR_ID); + } + + //void foo() { + // for( int a = 1; a < 2; a++ ) { + // } + // for( int a = 1; a < 2; a++ ) { + // } + //} + public void test2ForOK() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ERR_ID); + } + + //void foo() { + // for( int a = 1; a < 2; a++ ) { + // for( int a = 1; a < 2; a++ ) { + // } + // } + //} + public void testInnerFor() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //int a; + //class Foo { + // int a; + // void foo() { + // int a; + // for( int a = 1; a < 2; a++ ) { + // } + // } + //}; + public void test5Hirarchies() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(6, ERR_ID); + } + + //class Foo { + // int a; + // void foo(int a) { + // a = 1; + // } + //}; + public void testParameter() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(3, ERR_ID); + } + + //class Foo { + // void foo(int a) { + // a = 1; + // } + // void bar(int a) { + // a = 1; + // } + //}; + public void testParameterTwoMethods() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ERR_ID); + } + + //class Foo { + // int a; + // Foo() { + // class Local { + // void localMethod(int a) { + // } + // }; + // } + //}; + public void testParameterLocalClassMethod() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(5, ERR_ID); + } + + //class Foo { + // int a; + // Foo() { + // class Local { + // void localMethod() { + // int a = 1; + // } + // }; + // } + //}; + public void testLocalClassAfter() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(6, ERR_ID); + } + + //class Foo { + // Foo() { + // class Local { + // void localMethod() { + // int a = 1; + // } + // }; + // } + // int a; + //}; + public void testLocalClassBefore() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(5, ERR_ID); + } + + //int foo() { + // int c = 0; + // switch(c) { + // case default: { + // int a = c; + // a++; + // } + // } + // int a = c + 1; + // return a; + //}; + public void testSwitch() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(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 35b3f3237b4..fe806334e26 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 @@ -39,6 +39,7 @@ import org.eclipse.cdt.codan.core.internal.checkers.SuspiciousSemicolonCheckerTe import org.eclipse.cdt.codan.core.internal.checkers.SwitchCaseCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.UnusedSymbolInFileScopeCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.UsingInHeaderCheckerTest; +import org.eclipse.cdt.codan.core.internal.checkers.VariableShadowingCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.VariablesCheckerTest; import org.eclipse.cdt.codan.core.internal.checkers.VirtualMethodCallCheckerTest; import org.eclipse.cdt.codan.internal.checkers.ui.quickfix.AssignmentInConditionQuickFixTest; @@ -103,6 +104,7 @@ public class AutomatedIntegrationSuite extends TestSuite { suite.addTestSuite(VariablesCheckerTest.class); suite.addTestSuite(UsingInHeaderCheckerTest.class); suite.addTestSuite(FloatCompareCheckerTest.class); + suite.addTestSuite(VariableShadowingCheckerTest.class); // framework suite.addTest(CodanFastTestSuite.suite()); // quick fixes