Bug 546609 - Added checker for magic numbers

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

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

@@ -207,3 +207,9 @@ 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
+
+checker.name.MagicNumberChecker = Magic numbers checker
+problem.name.MagicNumberProblem = Avoid magic numbers
+problem.messagePattern.MagicNumberProblem = Avoid constant literals
+problem.description.MagicNumberProblem = This rule will flag all constant literals numbers
+

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

@@ -657,5 +657,20 @@
               name="%problem.name.SymbolShadowing">
 	       </problem>
        </checker>
+      <checker
+            class="org.eclipse.cdt.codan.internal.checkers.MagicNumberChecker"
+            id="org.eclipse.cdt.codan.internal.checkers.MagicNumberChecker"
+            name="%checker.name.MagicNumberChecker">
+         <problem
+               category="org.eclipse.cdt.codan.core.categories.CodeStyle"
+               defaultEnabled="false"
+               defaultSeverity="Warning"
+               description="%problem.description.MagicNumberProblem"
+               id="org.eclipse.cdt.codan.internal.checkers.MagicNumberProblem"
+               markerType="org.eclipse.cdt.codan.core.codanProblem"
+               messagePattern="%problem.messagePattern.MagicNumberProblem"
+               name="%problem.name.MagicNumberProblem">
+         </problem>
+      </checker>
    </extension>
 </plugin>

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.java

@@ -38,6 +38,8 @@ public class CheckersMessages extends NLS {
 	public static String SwitchCaseChecker_ParameterDefaultAllEnums;
 	public static String BlacklistChecker_list;
 	public static String BlacklistChecker_list_item;
+	public static String MagicNumberChecker_ParameterArray;
+	public static String MagicNumberChecker_ParameterOperatorParen;
 	public static String UnusedSymbolInFileScopeChecker_CharacterSequence;
 	public static String UnusedSymbolInFileScopeChecker_Exceptions;
 	public static String SymbolShadowingChecker_CheckFunctionParameters;

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/CheckersMessages.properties

@@ -26,6 +26,8 @@ StatementHasNoEffectChecker_ParameterMacro=Report problem in statements that com
 SuggestedParenthesisChecker_SuggestParanthesesAroundNot=Suggest parenthesis around 'not' operator
 SuspiciousSemicolonChecker_ParamAfterElse=Report an error if semicolon is right after 'else' statement
 SuspiciousSemicolonChecker_ParamElse=Do not report an error after 'if' when 'else' exists
+MagicNumberChecker_ParameterArray=Check literals in array expressions
+MagicNumberChecker_ParameterOperatorParen=Check literals in calls of class operator()
 
 SwitchCaseChecker_ParameterDefaultAllEnums=Mark even switches with complete enum items
 

codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/MagicNumberChecker.java

@@ -0,0 +1,212 @@
+/*******************************************************************************
+ * 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.HashSet;
+import java.util.Set;
+
+import org.eclipse.cdt.codan.core.cxx.model.AbstractIndexAstChecker;
+import org.eclipse.cdt.codan.core.model.IProblem;
+import org.eclipse.cdt.codan.core.model.IProblemWorkingCopy;
+import org.eclipse.cdt.codan.core.param.ListProblemPreference;
+import org.eclipse.cdt.core.dom.ast.ASTVisitor;
+import org.eclipse.cdt.core.dom.ast.IASTArrayDeclarator;
+import org.eclipse.cdt.core.dom.ast.IASTArrayModifier;
+import org.eclipse.cdt.core.dom.ast.IASTArraySubscriptExpression;
+import org.eclipse.cdt.core.dom.ast.IASTEnumerationSpecifier.IASTEnumerator;
+import org.eclipse.cdt.core.dom.ast.IASTExpression;
+import org.eclipse.cdt.core.dom.ast.IASTIdExpression;
+import org.eclipse.cdt.core.dom.ast.IASTInitializer;
+import org.eclipse.cdt.core.dom.ast.IASTLiteralExpression;
+import org.eclipse.cdt.core.dom.ast.IASTNode;
+import org.eclipse.cdt.core.dom.ast.IASTTranslationUnit;
+import org.eclipse.cdt.core.dom.ast.IASTUnaryExpression;
+import org.eclipse.cdt.core.dom.ast.IType;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTFieldReference;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTFunctionCallExpression;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTUnaryExpression;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPClassType;
+import org.eclipse.cdt.core.dom.ast.cpp.ICPPMethod;
+import org.eclipse.cdt.internal.core.dom.parser.ASTQueries;
+import org.eclipse.cdt.internal.core.dom.parser.ValueFactory;
+import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.SemanticUtil;
+
+@SuppressWarnings("restriction")
+public class MagicNumberChecker extends AbstractIndexAstChecker {
+	public static final String ERR_ID = "org.eclipse.cdt.codan.internal.checkers.MagicNumberProblem"; //$NON-NLS-1$
+	private static final String OPERATOR_PAREN = "operator ()"; //$NON-NLS-1$
+	public static final String PARAM_ARRAY = "checkArray"; //$NON-NLS-1$
+	public static final String PARAM_EXCEPTIONS = "exceptions"; //$NON-NLS-1$
+	/**
+	 * Operator() is often used for matrix manipulation
+	 */
+	public static final String PARAM_OPERATOR_PAREN = "checkOperatorParen"; //$NON-NLS-1$
+	/**
+	 * As default we allow the use of zero, one, minus one and two (used often as modulo value and other bit operations) as int
+	 */
+	private Set<Long> allowedLongValues = new HashSet<>();
+	/**
+	 * As default we allow the use of zero, one, minus one as float
+	 */
+	private Set<Double> allowedDoubleValues = new HashSet<>();
+	private boolean checkArray;
+	private boolean checkOperatorParen;
+
+	@Override
+	public void initPreferences(IProblemWorkingCopy problem) {
+		super.initPreferences(problem);
+		addPreference(problem, PARAM_ARRAY, CheckersMessages.MagicNumberChecker_ParameterArray, Boolean.TRUE);
+		addPreference(problem, PARAM_OPERATOR_PAREN, CheckersMessages.MagicNumberChecker_ParameterOperatorParen,
+				Boolean.TRUE);
+		ListProblemPreference list = addListPreference(problem, PARAM_EXCEPTIONS,
+				CheckersMessages.GenericParameter_ParameterExceptions,
+				CheckersMessages.GenericParameter_ParameterExceptionsItem);
+		list.addChildValue("1"); //$NON-NLS-1$
+		list.addChildValue("0"); //$NON-NLS-1$
+		list.addChildValue("-1"); //$NON-NLS-1$
+		list.addChildValue("2"); //$NON-NLS-1$
+		list.addChildValue("1.0"); //$NON-NLS-1$
+		list.addChildValue("0.0"); //$NON-NLS-1$
+		list.addChildValue("-1.0"); //$NON-NLS-1$
+	}
+
+	private void initExceptions() {
+		allowedLongValues.clear();
+		allowedDoubleValues.clear();
+		Object[] arr = (Object[]) getPreference(getProblemById(ERR_ID, getFile()), PARAM_EXCEPTIONS);
+		for (Object o : arr) {
+			String s = (String) o;
+			try {
+				allowedLongValues.add(Long.parseLong(s));
+			} catch (NumberFormatException e) {
+				try {
+					allowedDoubleValues.add(Double.parseDouble(s));
+				} catch (NumberFormatException e1) {
+				}
+			}
+		}
+	}
+
+	@Override
+	public void processAst(IASTTranslationUnit ast) {
+		final IProblem pt = getProblemById(ERR_ID, getFile());
+		checkArray = (Boolean) getPreference(pt, PARAM_ARRAY);
+		checkOperatorParen = (Boolean) getPreference(pt, PARAM_OPERATOR_PAREN);
+		initExceptions();
+		ast.accept(new ASTVisitor() {
+			{
+				shouldVisitExpressions = true;
+			}
+
+			@Override
+			public int visit(IASTExpression expression) {
+				if (expression instanceof IASTLiteralExpression) {
+					/**
+					 * Check if the expression is already inside a macro, if so it's ok
+					 */
+					if (enclosedInMacroExpansion(expression))
+						return PROCESS_CONTINUE;
+					/**
+					 * Check if we are inside an initializer, if so it's ok
+					 */
+					IASTInitializer node = ASTQueries.findAncestorWithType(expression, IASTInitializer.class);
+					if (node != null)
+						return PROCESS_CONTINUE;
+					/**
+					 * Check if the literal is used for an array
+					 */
+					IASTNode parent = expression.getParent();
+					if (!checkArray && (parent instanceof IASTArrayModifier || parent instanceof IASTArrayDeclarator
+							|| parent instanceof IASTArraySubscriptExpression))
+						return PROCESS_CONTINUE;
+					/**
+					 * Check if the literal is just a value assigned to an enum
+					 */
+					IASTEnumerator enums = ASTQueries.findAncestorWithType(expression, IASTEnumerator.class);
+					if (enums != null)
+						return PROCESS_CONTINUE;
+					/**
+					 * Check if the literal is used in operator() of a class instance
+					 */
+					if (!checkOperatorParen && isOperatorParen(expression)) {
+						return PROCESS_CONTINUE;
+					}
+					IASTLiteralExpression literal = (IASTLiteralExpression) expression;
+					int kind = literal.getKind();
+					Number value;
+					switch (kind) {
+					case IASTLiteralExpression.lk_float_constant:
+						value = ValueFactory.getConstantNumericalValue(expression);
+						if (!allowedDoubleValues.contains(value.doubleValue()))
+							reportProblem(ERR_ID, expression);
+						break;
+					case IASTLiteralExpression.lk_integer_constant:
+						value = ValueFactory.getConstantNumericalValue(expression);
+						if (!allowedLongValues.contains(value.longValue()))
+							reportProblem(ERR_ID, expression);
+						break;
+					default:
+						return PROCESS_CONTINUE;
+					}
+				}
+				return PROCESS_CONTINUE;
+			}
+		});
+	}
+
+	/**
+	 * Checks whether expression references this (directly, by pointer or by reference)
+	 */
+	private 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 boolean isOperatorParen(IASTExpression expression) {
+		ICPPASTFunctionCallExpression func = ASTQueries.findAncestorWithType(expression,
+				ICPPASTFunctionCallExpression.class);
+		if (func == null)
+			return false;
+
+		IASTExpression funcName = func.getFunctionNameExpression();
+		if (funcName instanceof ICPPASTFieldReference && Arrays.equals(OPERATOR_PAREN.toCharArray(),
+				((ICPPASTFieldReference) funcName).getFieldName().getSimpleID()))
+			return true;
+
+		if (!(funcName instanceof IASTIdExpression) && !referencesThis(funcName))
+			return false;
+
+		IType type = SemanticUtil.getUltimateType(funcName.getExpressionType(), true);
+		if (type instanceof ICPPClassType) {
+			ICPPMethod[] methods = ((ICPPClassType) type).getAllDeclaredMethods();
+			for (ICPPMethod m : methods) {
+				if (OPERATOR_PAREN.equals(m.getName())) {
+					return true;
+				}
+			}
+		}
+		return false;
+	}
+}

codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/MagicNumberCheckerTest.java

@@ -0,0 +1,248 @@
+/*******************************************************************************
+ * 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.MagicNumberChecker;
+
+/**
+ * Test for {@link MagicNumberChecker} class
+ */
+public class MagicNumberCheckerTest extends CheckerTestCase {
+
+	public static final String ERR_ID = MagicNumberChecker.ERR_ID;
+
+	@Override
+	public void setUp() throws Exception {
+		super.setUp();
+		enableProblems(ERR_ID);
+	}
+
+	@Override
+	public boolean isCpp() {
+		return true;
+	}
+
+	//void bar() {
+	//	int a = -18;
+	//}
+	public void testWithInit() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//void bar() {
+	//	int a = -18;
+	//	a = 15;
+	//}
+	public void testMagicNumberAssignment() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(3, ERR_ID);
+	}
+
+	//void foo(int p) {
+	//}
+	//void bar() {
+	//	int a = -18;
+	//	foo(15);
+	//}
+	public void testMagicNumberFunctionCall() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(5, ERR_ID);
+	}
+
+	//void bar() {
+	//	int a[100];
+	//}
+	public void testMagicNumberArray() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(2, ERR_ID);
+	}
+
+	//void bar() {
+	//	int a = -18;
+	//	a = 2;
+	//}
+	public void testException() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//void bar() {
+	//	int a = -18;
+	//	a = 238;
+	//}
+	public void tesCustomException() throws Exception {
+		setPreferenceValue(ERR_ID, MagicNumberChecker.PARAM_EXCEPTIONS, new String[] { "238" });
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//void bar() {
+	//	int a[100];
+	//	++a[8];
+	//}
+	public void testArrayDisabled() throws Exception {
+		setPreferenceValue(ERR_ID, MagicNumberChecker.PARAM_ARRAY, false);
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//class Foo {
+	//private:
+	//	int a[2];
+	//public:
+	//	Foo(int val) {
+	//		a[0] = val;
+	//	}
+	//	int operator()(int idx) const {
+	//		return a[idx];
+	//	}
+	//};
+	//void bar() {
+	//	Foo f(111);
+	//	f(88);
+	//}
+	public void testOperatorParenDisabled() throws Exception {
+		setPreferenceValue(ERR_ID, MagicNumberChecker.PARAM_OPERATOR_PAREN, false);
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//class Foo {
+	//private:
+	//	int a[2];
+	//public:
+	//	Foo(int val) {
+	//		a[0] = val;
+	//	}
+	//	int operator()(int idx) const {
+	//		return a[idx];
+	//	}
+	//};
+	//void bar() {
+	//	Foo f(111);
+	//	f(88);
+	//}
+	public void testOperatorParenEnabled() throws Exception {
+		setPreferenceValue(ERR_ID, MagicNumberChecker.PARAM_OPERATOR_PAREN, true);
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(14, ERR_ID);
+	}
+
+	//class Foo {
+	//private:
+	//	int a[2];
+	//public:
+	//	Foo(int val) {
+	//		a[0] = val;
+	//	}
+	//	int operator()(int idx) const {
+	//		return a[idx];
+	//	}
+	//};
+	//class Baz : public Foo {}
+	//void bar() {
+	//	Baz f(111);
+	//	f(88);
+	//}
+	public void testOperatorParenDisabledHierarchy() throws Exception {
+		setPreferenceValue(ERR_ID, MagicNumberChecker.PARAM_OPERATOR_PAREN, false);
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//class Foo {
+	//private:
+	//	int a[2];
+	//public:
+	//	Foo(int val) {
+	//		a[0] = val;
+	//	}
+	//	int operator()(int idx) const {
+	//		return a[idx];
+	//	}
+	//};
+	//class Baz : public Foo {}
+	//void bar() {
+	//	Baz f(111);
+	//	f.operator()(88);
+	//}
+	public void testOperatorParenExplicitDisabledHierarchy() throws Exception {
+		setPreferenceValue(ERR_ID, MagicNumberChecker.PARAM_OPERATOR_PAREN, false);
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//class Foo {
+	//private:
+	//	int a[2];
+	//public:
+	//	Foo(int val) {
+	//		a[0] = val;
+	//	}
+	//	int operator()(int idx) const {
+	//		return a[idx];
+	//	}
+	//	int set(int idx) const {
+	//		a[idx] = 0;
+	//	}
+	//};
+	//class Baz : public Foo {}
+	//void bar() {
+	//	Baz f(111);
+	//	f.set(9999);
+	//}
+	public void testClassMethod() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkErrorLine(18, ERR_ID);
+	}
+
+	//class Foo {
+	//public:
+	//	int operator()(int idx) {
+	//		return 0;
+	//	}
+	//	void pippo(int i) {
+	//		(*this)(111111);
+	//	}
+	//};
+	//int main() {
+	//	Foo* f = new Foo();
+	//	f->operator ()(11111);
+	//	Foo f2;
+	//	f2.operator ()(11111);
+	//	return 0;
+	//}
+	public void testImplicitOperatorParenMethod() throws Exception {
+		setPreferenceValue(ERR_ID, MagicNumberChecker.PARAM_OPERATOR_PAREN, false);
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//enum Foo {
+	//	BAR = 11
+	//}
+	public void testWithEnum() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+
+	//#define MACRO 44
+	//void foo() {
+	//	int a = 0;
+	//	a = MACRO;
+	//}
+	public void testWithMacro() throws Exception {
+		loadCodeAndRun(getAboveComment());
+		checkNoErrorsOfKind(ERR_ID);
+	}
+}

codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/tests/AutomatedIntegrationSuite.java

@@ -29,6 +29,7 @@ import org.eclipse.cdt.codan.core.internal.checkers.DecltypeAutoCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.FloatCompareCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.FormatStringCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.GotoStatementCheckerTest;
+import org.eclipse.cdt.codan.core.internal.checkers.MagicNumberCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.NonVirtualDestructorCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.ProblemBindingCheckerTest;
 import org.eclipse.cdt.codan.core.internal.checkers.ReturnCheckerTest;
@@ -107,6 +108,7 @@ public class AutomatedIntegrationSuite extends TestSuite {
 		suite.addTestSuite(FloatCompareCheckerTest.class);
 		suite.addTestSuite(VariableShadowingCheckerTest.class);
 		suite.addTestSuite(ShallowCopyCheckerTest.class);
+		suite.addTestSuite(MagicNumberCheckerTest.class);
 		// framework
 		suite.addTest(CodanFastTestSuite.suite());
 		// quick fixes