Bug 273252 - Added checker for variable masking

Added new checker for variable masking another in a parent scope

Change-Id: Icff6b6499a1d38cc5a719d143552bbe17d57b15c
Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>

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

codan/org.eclipse.cdt.codan.checkers/plugin.xml

@@ -628,5 +628,19 @@
                name="%problem.name.MultipleDeclarationsProblem">
          </problem>
       </checker>
+      <checker
+           class="org.eclipse.cdt.codan.internal.checkers.SymbolShadowingChecker"
+           id="org.eclipse.cdt.codan.internal.checkers.SymbolShadowing"
+           name="%checker.name.SymbolShadowing">
+           <problem
+              category="org.eclipse.cdt.codan.core.categories.ProgrammingProblems"
+              defaultEnabled="false"
+              defaultSeverity="Warning"
+              description="%problem.description.SymbolShadowing"
+              id="org.eclipse.cdt.codan.internal.checkers.SymbolShadowingProblem"
+              messagePattern="%problem.messagePattern.SymbolShadowing"
+              name="%problem.name.SymbolShadowing">
+	       </problem>
+       </checker>
    </extension>
 </plugin>

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;
 

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
+Copyright_regex=Regex to search for copyright information
+
+SymbolShadowingChecker_CheckFunctionParameters=Check even function/method parameters

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<IProblemLocation> 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<IProblemLocation> 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;
+		}
+	}
+}

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

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