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 94d9c7de0b9..337509d4e2e 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 @@ -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 + diff --git a/codan/org.eclipse.cdt.codan.checkers/plugin.xml b/codan/org.eclipse.cdt.codan.checkers/plugin.xml index a3143436ca8..d36c0d2e977 100644 --- a/codan/org.eclipse.cdt.codan.checkers/plugin.xml +++ b/codan/org.eclipse.cdt.codan.checkers/plugin.xml @@ -657,5 +657,20 @@ name="%problem.name.SymbolShadowing"> + + + + 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 956b74f032f..acfb71f970e 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 @@ -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; 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 adb31299f68..ad9bfe7ac3f 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 @@ -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 diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/MagicNumberChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/MagicNumberChecker.java new file mode 100644 index 00000000000..54d202a6f61 --- /dev/null +++ b/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 allowedLongValues = new HashSet<>(); + /** + * As default we allow the use of zero, one, minus one as float + */ + private Set 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; + } +} diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/MagicNumberCheckerTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/MagicNumberCheckerTest.java new file mode 100644 index 00000000000..52d3fdbeb1c --- /dev/null +++ b/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); + } +} 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 23b0661de9c..52ac2c453d3 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 @@ -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