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 337509d4e2e..7654d0e4098 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 @@ -213,3 +213,7 @@ problem.name.MagicNumberProblem = Avoid magic numbers problem.messagePattern.MagicNumberProblem = Avoid constant literals problem.description.MagicNumberProblem = This rule will flag all constant literals numbers +checker.name.NoDiscard = NoDiscardChecker +problem.description.NoDiscard = This rule will flag the use of functions marked as 'no discard' without taking into account return value +problem.messagePattern.NoDiscard = Return value from ''{0}'' not evaluated +problem.name.NoDiscard = Return value not evaluated diff --git a/codan/org.eclipse.cdt.codan.checkers/plugin.xml b/codan/org.eclipse.cdt.codan.checkers/plugin.xml index d36c0d2e977..08c0b921af2 100644 --- a/codan/org.eclipse.cdt.codan.checkers/plugin.xml +++ b/codan/org.eclipse.cdt.codan.checkers/plugin.xml @@ -672,5 +672,17 @@ name="%problem.name.MagicNumberProblem"> + + + 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 deb2b298ca2..d231288b155 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 @@ -46,6 +46,7 @@ public class CheckersMessages extends NLS { public static String Copyright_regex; public static String ShallowCopyChecker_OnlyNew; public static String CStyleCastCheck_checkInMacro; + public static String NoDiscardChecker_ParameterMacro; static { NLS.initializeMessages(CheckersMessages.class.getName(), CheckersMessages.class); 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 63a42ad201d..b9d4819f34c 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 @@ -29,7 +29,7 @@ SuspiciousSemicolonChecker_ParamAfterElse=Report an error if semicolon is right 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() - +NoDiscardChecker_ParameterMacro=Report problem in statements that come from macro expansion SwitchCaseChecker_ParameterDefaultAllEnums=Mark even switches with complete enum items BlacklistChecker_list=List of methods/functions to be checked diff --git a/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NoDiscardChecker.java b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NoDiscardChecker.java new file mode 100644 index 00000000000..587bbcf9da9 --- /dev/null +++ b/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NoDiscardChecker.java @@ -0,0 +1,179 @@ +/******************************************************************************* + * Copyright (c) 2020 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.Optional; + +import org.eclipse.cdt.codan.core.cxx.model.AbstractAstFunctionChecker; +import org.eclipse.cdt.codan.core.model.IProblemWorkingCopy; +import org.eclipse.cdt.core.dom.ast.ASTVisitor; +import org.eclipse.cdt.core.dom.ast.IASTExpression; +import org.eclipse.cdt.core.dom.ast.IASTExpressionList; +import org.eclipse.cdt.core.dom.ast.IASTExpressionStatement; +import org.eclipse.cdt.core.dom.ast.IASTFieldReference; +import org.eclipse.cdt.core.dom.ast.IASTFunctionCallExpression; +import org.eclipse.cdt.core.dom.ast.IASTFunctionDefinition; +import org.eclipse.cdt.core.dom.ast.IASTIdExpression; +import org.eclipse.cdt.core.dom.ast.IASTName; +import org.eclipse.cdt.core.dom.ast.IASTNode; +import org.eclipse.cdt.core.dom.ast.IASTUnaryExpression; +import org.eclipse.cdt.core.dom.ast.IBinding; +import org.eclipse.cdt.core.dom.ast.IFunction; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTCastExpression; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPASTExpression; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPClassType; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPFunction; +import org.eclipse.cdt.core.dom.ast.cpp.ICPPVariable; +import org.eclipse.cdt.internal.core.dom.parser.cpp.ICPPEvaluation; +import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.EvalFunctionCall; +import org.eclipse.cdt.internal.core.dom.parser.cpp.semantics.EvalTypeId; + +@SuppressWarnings("restriction") +public class NoDiscardChecker extends AbstractAstFunctionChecker { + public static final String ER_ID = "org.eclipse.cdt.codan.internal.checkers.NoDiscardProblem"; //$NON-NLS-1$ + public static final String PARAM_MACRO_ID = "macro"; //$NON-NLS-1$ + + @Override + protected void processFunction(IASTFunctionDefinition func) { + func.accept(new FunctionCallVisitor()); + } + + class FunctionCallVisitor extends ASTVisitor { + + FunctionCallVisitor() { + shouldVisitExpressions = true; + } + + @Override + public int visit(IASTExpression expr) { + if (!shouldReportInMacro() && enclosedInMacroExpansion(expr)) + return PROCESS_SKIP; + if (isDiscardedValueExpression(expr)) { + Optional res = getNodiscardFunction(expr); + if (res.isPresent()) { + reportProblem(ER_ID, expr, res.get()); + } + } + return PROCESS_CONTINUE; + } + + private boolean isDiscardedValueExpression(IASTExpression expr) { + IASTNode parent = expr.getParent(); + IASTNode child = expr; + + while (parent instanceof IASTUnaryExpression) { + int operator = ((IASTUnaryExpression) parent).getOperator(); + if (operator != IASTUnaryExpression.op_bracketedPrimary) { + return false; + } + child = parent; + parent = parent.getParent(); + } + if (parent instanceof IASTExpressionStatement) { + return true; + } + + if (child instanceof IASTExpression && checkNestedLeftSide((IASTExpression) child)) { + return true; + } + + return false; + } + + private Optional getNodiscardFunction(IASTExpression expr) { + if (expr instanceof IASTFunctionCallExpression) { + IASTFunctionCallExpression func = (IASTFunctionCallExpression) expr; + IASTExpression functionNameExpression = func.getFunctionNameExpression(); + + IASTName name; + if (functionNameExpression instanceof IASTIdExpression) { + name = ((IASTIdExpression) functionNameExpression).getName(); + } else if (functionNameExpression instanceof IASTFieldReference) { + name = ((IASTFieldReference) functionNameExpression).getFieldName(); + } else { + return Optional.empty(); + } + IBinding binding = name.resolveBinding(); + + if (binding instanceof IFunction && ((IFunction) binding).isNoDiscard()) { + return Optional.of(name); + } else if ((binding instanceof ICPPClassType || binding instanceof ICPPVariable) + && expr instanceof ICPPASTExpression && checkEvaluation((ICPPASTExpression) expr)) { + return Optional.of(name); + } + + return Optional.empty(); + } else if (expr instanceof ICPPASTCastExpression) { + ICPPASTCastExpression cast = (ICPPASTCastExpression) expr; + if (cast.getOperator() != ICPPASTCastExpression.op_static_cast) { + return Optional.empty(); + } + if (checkEvaluation(cast)) { + return Optional.of(cast.getTypeId()); + } + return Optional.empty(); + } else + return Optional.empty(); + } + + /** + * Check if the expression is 'nodiscard' looking for its evaluation + * @param expr A cpp expression + * @return True if nodiscard, false otherwise + */ + private boolean checkEvaluation(ICPPASTExpression expr) { + ICPPEvaluation eval = expr.getEvaluation(); + if (eval instanceof EvalTypeId) { + ICPPFunction evalFunc = ((EvalTypeId) eval).getConstructor(); + if (evalFunc != null && evalFunc.isNoDiscard()) { + return true; + } + } else if (eval instanceof EvalFunctionCall) { + ICPPFunction evalFunc = ((EvalFunctionCall) eval).resolveFunctionBinding(); + if (evalFunc != null && evalFunc.isNoDiscard()) { + return true; + } + } + return false; + } + + /** + * Check if the function call is a left side of a nested expression. If it's a + * left side we need to go ahead and we need to evaluate the expression since it + * can be a discarded expression. + * @param func The expression list + * @return True if it's a nested expression and it's the left side, false otherwise + */ + private boolean checkNestedLeftSide(IASTExpression expr) { + if (expr.getPropertyInParent() == IASTExpressionList.NESTED_EXPRESSION) { + IASTExpressionList list = (IASTExpressionList) expr.getParent(); + IASTExpression[] allExpr = list.getExpressions(); + if (expr != allExpr[allExpr.length - 1]) + return true; + } + return false; + } + } + + @Override + public void initPreferences(IProblemWorkingCopy problem) { + super.initPreferences(problem); + addPreference(problem, PARAM_MACRO_ID, CheckersMessages.NoDiscardChecker_ParameterMacro, Boolean.TRUE); + } + + /** + * @return + */ + private boolean shouldReportInMacro() { + return (Boolean) getPreference(getProblemById(ER_ID, getFile()), PARAM_MACRO_ID); + } +} diff --git a/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/NoDiscardCheckerTest.java b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/NoDiscardCheckerTest.java new file mode 100644 index 00000000000..140e27ab99a --- /dev/null +++ b/codan/org.eclipse.cdt.codan.core.tests/src/org/eclipse/cdt/codan/core/internal/checkers/NoDiscardCheckerTest.java @@ -0,0 +1,387 @@ +/******************************************************************************* + * Copyright (c) 2020 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.NoDiscardChecker; + +/** + * Test for {@link NoDiscardChecker} class + */ +public class NoDiscardCheckerTest extends CheckerTestCase { + + public static final String ER_ID = NoDiscardChecker.ER_ID; + + @Override + public void setUp() throws Exception { + super.setUp(); + enableProblems(ER_ID); + } + + @Override + public boolean isCpp() { + return true; + } + + //[[nodiscard]] int test() { + // return 2; + //} + //int main() { + // test(); + // return 0; + //} + public void testCppSimpleCall() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(5, ER_ID); + } + + //[[nodiscard]] int test() { + // return 2; + //} + //int main() { + // return test(); + //} + public void testCppInReturn() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //[[nodiscard]] int test() { + // return 2; + //} + //int main() { + // return test() ? 0 : 1; + //} + public void testCppInTernaryReturn() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //[[nodiscard]] int test() { + // return 2; + //} + //void process(int v) {} + //int main() { + // process(test()); + // return 0; + //} + public void testCppInOtherCall() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //[[nodiscard]] int test() { + // return 2; + //} + //void process(int v) {} + //int main() { + // if (test()) { + // } + // return 0; + //} + public void testCppInCondition() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //class Test { + //public: + // Test() {} + // [[nodiscard]] int foo() { + // return 2; + // } + //}; + // + //int main() { + // Test t; + // t.foo(); + // return 0; + //} + public void testCppNoDiscardMethod() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(11, ER_ID); + } + + //int test() __attribute__((warn_unused_result)) { + // return 2; + //} + //int main() { + // test(); + // return 0; + //} + public void testCSimpleCall() throws Exception { + loadCodeAndRunC(getAboveComment()); + checkErrorLine(5, ER_ID); + } + + //int test() __attribute__((warn_unused_result)) { + // return 2; + //} + //int main() { + // return test(); + //} + public void testCInReturn() throws Exception { + loadCodeAndRunC(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //int test() __attribute__((warn_unused_result)) { + // return 2; + //} + //int main() { + // return test() ? 0 : 1; + //} + public void testCInTernaryReturn() throws Exception { + loadCodeAndRunC(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //int test() __attribute__((warn_unused_result)) { + // return 2; + //} + //void process(int v) {} + //int main() { + // process(test()); + // return 0; + //} + public void testCInOtherCall() throws Exception { + loadCodeAndRunC(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //int test() __attribute__((warn_unused_result)) { + // return 2; + //} + //void process(int v) {} + //int main() { + // if (test()) { + // } + // return 0; + //} + public void testCInCondition() throws Exception { + loadCodeAndRunC(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //class Test { + //public: + // Test() {} + // int foo() __attribute__((warn_unused_result)) { + // return 2; + // } + //}; + // + //int main() { + // Test t; + // t.foo(); + // return 0; + //} + public void testCppUnusedNoDiscardMethod() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(11, ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //int main() { + // ((((foo())))); + // return 0; + //} + public void testInParenNoDiscardMethod() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(5, ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //#define MACRO(X) X() + //int main() { + // MACRO(foo); + // return 0; + //} + public void testInMacroEnabled() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(6, ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //#define MACRO(X) X() + //int main() { + // MACRO(foo); + // return 0; + //} + public void testInMacroDisabled() throws Exception { + setPreferenceValue(ER_ID, NoDiscardChecker.PARAM_MACRO_ID, false); + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //int main() { + // (void) foo(); + // return 0; + //} + public void testVoidCast() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //int main() { + // static_cast(foo()); + // return 0; + //} + public void testStaticVoidCast() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //int process() { + // return foo(), 2; + //} + //int main() { + // process(); + // return 0; + //} + public void testCommaLeftHandSide() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(5, ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //int bar() { + // return 4; + //} + //int process() { + // return (foo()), bar(); + //} + //int main() { + // process(); + // return 0; + //} + public void testCommaLeftHandSide2() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(8, ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //int bar() { + // return 4; + //} + //int main() { + // (foo()), bar(); + // return 0; + //} + public void testCommaLeftHandSide3() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(8, ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //int process() { + // return 2, foo(); + //} + //int main() { + // process(); + // return 0; + //} + public void testCommaRightHandSide() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //[[nodiscard]] int foo() { + // return 2; + //} + //int main() { + // int vv = 0; + // vv = 2, foo(); + // return 0; + //} + public void testCommaRightHandSide2() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //class Test { + //public: + // Test(double v) {} + // [[nodiscard]] Test(int v) {} + //}; + //int main() { + // Test(42); + // Test(42.0); + // return 0; + //} + public void testCppNoDiscardCtor() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(7, ER_ID); + } + + //class Test { + //public: + // Test(double v) {} + // [[nodiscard]] Test(int v) {} + //}; + //int main() { + // static_cast(42); + // static_cast(42.0); + // return 0; + //} + public void testCppNoDiscardCtorStaticCast() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(7, ER_ID); + } + + //class Test { + //public: + // Test(double v) {} + // [[nodiscard]] Test(int v) {} + //}; + //int main() { + // reinterpret_cast(42); + // (Test) 42; + // return 0; + //} + public void testCppNoDiscardCtorOtherCast() throws Exception { + loadCodeAndRun(getAboveComment()); + checkNoErrorsOfKind(ER_ID); + } + + //struct Foo { + // [[nodiscard]] bool operator()() { return true; } + //}; + //int main() { + // Foo foo1; + // foo1(); + //} + public void testCppFunctor() throws Exception { + loadCodeAndRun(getAboveComment()); + checkErrorLine(6, ER_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 52ac2c453d3..5f516245bc7 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 @@ -30,6 +30,7 @@ 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.NoDiscardCheckerTest; 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; @@ -109,6 +110,7 @@ public class AutomatedIntegrationSuite extends TestSuite { suite.addTestSuite(VariableShadowingCheckerTest.class); suite.addTestSuite(ShallowCopyCheckerTest.class); suite.addTestSuite(MagicNumberCheckerTest.class); + suite.addTestSuite(NoDiscardCheckerTest.class); // framework suite.addTest(CodanFastTestSuite.suite()); // quick fixes diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/EvalFunctionCall.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/EvalFunctionCall.java index 47d757faf2f..5c30a9bd92e 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/EvalFunctionCall.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/EvalFunctionCall.java @@ -372,7 +372,7 @@ public final class EvalFunctionCall extends CPPDependentEvaluation { return EvalFixed.INCOMPLETE; } - private ICPPFunction resolveFunctionBinding() { + public ICPPFunction resolveFunctionBinding() { ICPPFunction function = getOverload(); if (function == null) { ICPPEvaluation funcEval = fArguments[0];