diff --git a/change-notes/1.19/analysis-cpp.md b/change-notes/1.19/analysis-cpp.md index 4835afd0c70d..60e038cf557e 100644 --- a/change-notes/1.19/analysis-cpp.md +++ b/change-notes/1.19/analysis-cpp.md @@ -16,6 +16,7 @@ | **Query** | **Expected impact** | **Change** | |----------------------------|------------------------|------------------------------------------------------------------| | Empty branch of conditional | Fewer false positive results | The query now recognizes commented blocks more reliably. | +| Expression has no effect | Fewer false positive results | Expressions in template instantiations are now excluded from this query. | | Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. | | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. | | Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type. | diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql index 9e42cb5d2ea0..2db4616a08a1 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql @@ -100,6 +100,7 @@ where // EQExprs are covered by CompareWhereAssignMeant.ql not accessInInitOfForStmt(peivc) and not peivc.isCompilerGenerated() and not exists(Macro m | peivc = m.getAnInvocation().getAnExpandedElement()) and + not peivc.isFromTemplateInstantiation(_) and parent = peivc.getParent() and not parent.isInMacroExpansion() and not parent instanceof PureExprInVoidContext and diff --git a/cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp b/cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp index d9a71fa7bafa..2acdfcf80f8f 100644 --- a/cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp +++ b/cpp/ql/test/library-tests/queries/expr_has_no_effect/calls.cpp @@ -5,11 +5,11 @@ int external(); class Base { public: virtual int thingy() { - 1; + 1; // BAD } int our_thingy() { - Base::thingy(); + Base::thingy(); // BAD return 2; } }; @@ -17,14 +17,14 @@ class Base { class Derived : public Base { public: virtual int thingy() { - external(); + external(); // GOOD return 3; } }; void internal() { Base* ptr = new Derived(); - ptr->thingy(); + ptr->thingy(); // GOOD } } \ No newline at end of file diff --git a/cpp/ql/test/library-tests/queries/expr_has_no_effect/expr_has_no_effect.expected b/cpp/ql/test/library-tests/queries/expr_has_no_effect/expr_has_no_effect.expected index af2bacade1a2..5982cfa589ef 100644 --- a/cpp/ql/test/library-tests/queries/expr_has_no_effect/expr_has_no_effect.expected +++ b/cpp/ql/test/library-tests/queries/expr_has_no_effect/expr_has_no_effect.expected @@ -1,6 +1,5 @@ | calls.cpp:8:5:8:5 | 1 | This expression has no effect. | calls.cpp:8:5:8:5 | 1 | | | calls.cpp:12:5:12:16 | call to thingy | This expression has no effect (because $@ has no external side effects). | calls.cpp:7:15:7:20 | thingy | thingy | -| templatey.cpp:4:3:4:8 | ... << ... | This expression has no effect. | templatey.cpp:4:3:4:8 | ... << ... | | | templatey.cpp:39:3:39:23 | call to pointless_add_numbers | This expression has no effect (because $@ has no external side effects). | templatey.cpp:29:5:29:25 | pointless_add_numbers | pointless_add_numbers | | volatile.c:9:5:9:5 | c | This expression has no effect. | volatile.c:9:5:9:5 | c | | | volatile.c:12:5:12:9 | access to array | This expression has no effect. | volatile.c:12:5:12:9 | access to array | | diff --git a/cpp/ql/test/library-tests/queries/expr_has_no_effect/templatey.cpp b/cpp/ql/test/library-tests/queries/expr_has_no_effect/templatey.cpp index 114e2052a530..7d2b6b19777e 100644 --- a/cpp/ql/test/library-tests/queries/expr_has_no_effect/templatey.cpp +++ b/cpp/ql/test/library-tests/queries/expr_has_no_effect/templatey.cpp @@ -1,7 +1,7 @@ template void foo(T x, T y) { - x << y; + x << y; // GOOD (effect depends on T) }; struct streamable @@ -15,9 +15,9 @@ void operator<<(streamable& lhs, streamable& rhs) int main() { int x = 3; - foo(x, x); + foo(x, x); // BAD [NOT DETECTED] streamable y; - foo(y, y); + foo(y, y); // BAD [NOT DETECTED] return 0; } @@ -34,7 +34,7 @@ int pointless_add_numbers(int lhs, int rhs) void call_add_numbers() { int accum = 0; - add_numbers(accum, 4); - add_numbers(accum, 10); - pointless_add_numbers(accum, 20); + add_numbers(accum, 4); // GOOD + add_numbers(accum, 10); // GOOD + pointless_add_numbers(accum, 20); // BAD } diff --git a/cpp/ql/test/library-tests/queries/expr_has_no_effect/volatile.c b/cpp/ql/test/library-tests/queries/expr_has_no_effect/volatile.c index 27ee532c99c2..c34e0818f192 100644 --- a/cpp/ql/test/library-tests/queries/expr_has_no_effect/volatile.c +++ b/cpp/ql/test/library-tests/queries/expr_has_no_effect/volatile.c @@ -6,18 +6,18 @@ char *pc; volatile char *pv; void f(void) { - c; - v; + c; // BAD + v; // (accesses to volatile variables are considered impure) - pc[5]; + pc[5]; // BAD pv[5]; ((volatile char *)pc)[5]; - *pc; + *pc; // BAD *pv; *((volatile char *)pc); - *(pc + 5); + *(pc + 5); // BAD *(pv + 5); *((volatile char *)(pc + 5)); *(((volatile char *)pc) + 5); diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected index 20cc25fb99b1..f50c352fe340 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected @@ -1,5 +1,6 @@ | preproc.c:89:2:89:4 | call to fn4 | This expression has no effect (because $@ has no external side effects). | preproc.c:33:5:33:7 | fn4 | fn4 | | preproc.c:94:2:94:4 | call to fn9 | This expression has no effect (because $@ has no external side effects). | preproc.c:78:5:78:7 | fn9 | fn9 | +| template.cpp:19:3:19:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | template.cpp:9:10:9:19 | operator++ | operator++ | | test.c:7:5:7:5 | 0 | This expression has no effect. | test.c:7:5:7:5 | 0 | | | test.c:9:8:9:8 | 1 | This expression has no effect. | test.c:9:8:9:8 | 1 | | | test.c:9:11:9:11 | 2 | This expression has no effect. | test.c:9:11:9:11 | 2 | | @@ -19,11 +20,6 @@ | test.c:26:15:26:16 | 32 | This expression has no effect. | test.c:26:15:26:16 | 32 | | | test.c:27:9:27:10 | 33 | This expression has no effect. | test.c:27:9:27:10 | 33 | | | test.cpp:24:3:24:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ | -| test.cpp:24:3:24:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ | -| test.cpp:24:3:24:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ | -| test.cpp:25:3:25:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ | -| test.cpp:25:3:25:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ | | test.cpp:25:3:25:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ | -| test.cpp:26:3:26:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | test.cpp:9:14:9:23 | operator++ | operator++ | | test.cpp:62:5:62:5 | call to operator= | This expression has no effect (because $@ has no external side effects). | test.cpp:47:14:47:22 | operator= | operator= | | test.cpp:65:5:65:5 | call to operator= | This expression has no effect (because $@ has no external side effects). | test.cpp:55:7:55:7 | operator= | operator= | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c new file mode 100644 index 000000000000..2f7fcc1e05e2 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/macros.c @@ -0,0 +1,7 @@ + +#define UNUSED(x) (x) + +void test2(int param) +{ + UNUSED(param); // GOOD +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp new file mode 100644 index 000000000000..ecc3d6246034 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/template.cpp @@ -0,0 +1,22 @@ + +template +void Increment(T &t) { + t++; // GOOD (sometimes has an effect) +} + +class Nothing { +public: + Nothing operator++(int) { + return *this; // do nothing + } +}; + +void myTemplateTest() { + int i = 0; + Nothing n; + + i++; // GOOD (always has an effect) + n++; // BAD (never has an effect) + Increment(i); + Increment(n); +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp index 33235b5c4454..1658f210cf3b 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp @@ -23,7 +23,7 @@ class MyTemplateClass ++arg1; // pure, does nothing ++arg2; // pure, does nothing - ++arg3; // not pure in all cases (when _It is int this has a side-effect) [FALSE POSITIVE] + ++arg3; // not pure in all cases (when _It is int this has a side-effect) return arg2; }