From ee2ef752d703a89e85d83f0a5486efc1a69cc72a Mon Sep 17 00:00:00 2001 From: Jan Schlosser Date: Mon, 3 Aug 2026 16:19:41 +0200 Subject: [PATCH] Fix RULE-0-2-4/A0-1-3 false positives for private functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two categories of false positive are addressed: 1. **Private pure virtual functions used via the NVI idiom** — `DynamicCallGraph::getTarget()` resolves a virtual call to the implementations that may actually *run*. A pure virtual function (`= 0`) has no body, so it is never a viable dispatch target and is therefore never returned by `getTarget()` — not even when it is unambiguously named by a call, as in the non-virtual interface (NVI) idiom where a public member calls a private pure virtual. It was consequently reported as unused even though a sibling member calls it. `functionIsCalled` now also counts the *statically named* callee (`FunctionCall.getTarget()`), which recovers exactly that case. This is deliberately narrower than excluding `PureVirtualFunction` from `LocalFunction` outright: a private pure virtual that is genuinely never called and never overridden is still reported. 2. **Private members of never-instantiated class templates** — when a class template is never instantiated with a concrete type anywhere in the analyzed compilation units, Clang never elaborates a body for its member functions, so `Call`/`FunctionCall` targets within that pattern's own text cannot be resolved by `DynamicCallGraph::getTarget()` or `VirtualDispatch`, even for calls between sibling members of the very same class (e.g. a public entry point calling a private helper). This is common for generic "plumbing" library code (CRTP-style wrappers and the like) that is only ever instantiated by downstream consumers outside the analyzed codebase. The new `hasNoVisibleInstantiation(fn)` predicate conservatively treats such private members as "used" (out of scope for this analysis) rather than reporting them as dead code — but only when *no* sibling member of the same class has any instantiation either, so genuinely dead private helpers in class templates that *are* instantiated elsewhere are still correctly reported. The sibling-instantiation check is factored into a `pragma[noinline]` predicate over the declaring `Class` rather than over the member `Function`. Inlined into the caller, the join orderer loses the fact that the class is functionally determined and materialises the full (member, sibling) cross product per class before projecting it away. On a large real-world database this cut the peak intermediate relation for that predicate from 4,831,818 tuples to 7,700 with identical results. Fixes #1168 --- ...3-unused-local-function-false-positives.md | 16 ++++ .../UnusedLocalFunction.qll | 80 ++++++++++++++++++- .../UnusedLocalFunction.expected | 2 + .../test/rules/unusedlocalfunction/test.cpp | 80 ++++++++++++++++++- 4 files changed, 175 insertions(+), 3 deletions(-) create mode 100644 change_notes/2026-08-03-unused-local-function-false-positives.md diff --git a/change_notes/2026-08-03-unused-local-function-false-positives.md b/change_notes/2026-08-03-unused-local-function-false-positives.md new file mode 100644 index 0000000000..88300791ab --- /dev/null +++ b/change_notes/2026-08-03-unused-local-function-false-positives.md @@ -0,0 +1,16 @@ +- `A0-1-3`, `RULE-0-2-4` - `UnusedLocalFunction.ql`: + - Fixed false positives for private pure virtual functions used through the non-virtual + interface (NVI) idiom. `DynamicCallGraph::getTarget()` resolves a virtual call to the + implementations that may actually run; a pure virtual function has no body, so it is + never a viable dispatch target and was reported as unused even when a sibling member + called it. A call is now also counted when the function is the statically named + callee. Pure virtual functions that are genuinely never called and never overridden + are still reported. + - Excluded private member functions of class templates that are never concretely + instantiated anywhere in the database (and where no sibling member of the same + class-template pattern is instantiated either). Clang never elaborates a body for the + members of such patterns, so calls between sibling members of the same + never-instantiated class (e.g. a public entry point calling a private helper) cannot be + resolved by the call graph. This is common for generic "plumbing" library code + (CRTP-style wrappers, etc.) that is only ever + instantiated by downstream consumers outside of the analyzed codebase. diff --git a/cpp/common/src/codingstandards/cpp/rules/unusedlocalfunction/UnusedLocalFunction.qll b/cpp/common/src/codingstandards/cpp/rules/unusedlocalfunction/UnusedLocalFunction.qll index 39715e484f..258f25aa94 100644 --- a/cpp/common/src/codingstandards/cpp/rules/unusedlocalfunction/UnusedLocalFunction.qll +++ b/cpp/common/src/codingstandards/cpp/rules/unusedlocalfunction/UnusedLocalFunction.qll @@ -20,11 +20,84 @@ predicate overloadedFunctionIsCalled(Function unusedFunction) { exists(Function f | f = unusedFunction.getAnOverload() and f = getTarget(_)) } +/** + * Holds if `fn` is the target of some call, either statically or according to the + * dynamic call graph. + * + * `DynamicCallGraph::getTarget()` resolves a virtual call to the functions that may + * actually run, i.e. the overriding implementations. A pure virtual function has no + * body, so it is never a viable dispatch target and is therefore *never* returned by + * `getTarget()` -- even when it is unambiguously named by a call, as in the + * non-virtual interface (NVI) idiom where a public member calls a private pure + * virtual. The additional static `FunctionCall.getTarget()` disjunct recovers exactly + * that case: the callee as written in the source. + */ +predicate functionIsCalled(Function fn) { + fn = getTarget(_) + or + // The statically named callee, which the dynamic call graph drops for calls that + // dispatch to an override (notably pure virtual functions, which have no body). + exists(FunctionCall fc | fc.getTarget() = fn) +} + /** Checks if a Function's address was taken. */ predicate addressBeenTaken(Function unusedFunction) { exists(FunctionAccess fa | fa.getTarget() = unusedFunction) } +/** + * Holds if some member of the class `c` has at least one concrete instantiation anywhere in the + * database. + * + * If this holds for the declaring type of a member function `fn`, the class template is genuinely + * "alive" (used with a concrete type somewhere), and the fact that `fn` itself was never + * instantiated is real evidence that it is unused: for a member function to lack a concrete + * instantiation while sibling members do have one, it must never have been called from any of + * those sibling bodies. + * + * `pragma[noinline]` keeps this a standalone relation of arity one. Inlined into the caller, the + * join orderer loses the fact that `c` is functionally determined and materialises the full + * (member, sibling) cross product per class before projecting it away, which is quadratic in the + * size of the largest class. + */ +pragma[noinline] +private predicate classHasAnyInstantiatedMember(Class c) { + exists(Function sibling, Function siblingInstantiation | + sibling.getDeclaringType() = c and + siblingInstantiation.isConstructedFrom(sibling) + ) +} + +/** + * Holds if `fn` is a function from an uninstantiated template for which no concrete + * instantiation exists anywhere in the database, and no other member of the same + * class-template pattern is instantiated either. + * + * When a class template is never instantiated with a concrete type in the analyzed + * compilation units, Clang never elaborates a body for its member functions, so + * `Call`/`FunctionCall` targets within that pattern's own text cannot be resolved by + * `DynamicCallGraph::getTarget()` or `VirtualDispatch`, even for calls between sibling members + * of the very same class (e.g. a constructor calling a private helper). This is common for + * generic "plumbing" library code (CRTP-style wrappers, etc.) + * that is only ever instantiated by downstream consumers outside of this codebase. In that + * situation we have no visibility at all into the call graph, so we conservatively treat the + * function as "used" (out of scope for this analysis) rather than report it as dead code. + * + * We only do this when *no* sibling member of the class pattern has an instantiation either + * (see `classHasAnyInstantiatedMember`): if some sibling *is* instantiated, the class is + * genuinely used, and `fn` lacking an instantiation is real (not merely missing) evidence that + * it is unused. + */ +predicate hasNoVisibleInstantiation(Function fn) { + // Restricted to class-template members: a standalone function template that is never + // instantiated anywhere is genuinely dead code, and detecting that does not suffer from the + // "sibling member of the same class" ambiguity this predicate is designed for. + fn instanceof MemberFunction and + fn.isFromUninstantiatedTemplate(_) and + not exists(Function instantiation | instantiation.isConstructedFrom(fn)) and + not classHasAnyInstantiatedMember(fn.getDeclaringType()) +} + /** A `Function` nested in an anonymous namespace. */ class AnonymousNamespaceFunction extends Function { AnonymousNamespaceFunction() { getNamespace().getParentNamespace*().isAnonymous() } @@ -74,7 +147,7 @@ module UnusedLocalFunction { query predicate problems(LocalFunction unusedLocalFunction, string message) { not isExcluded(unusedLocalFunction, Config::getQuery()) and // No static or dynamic call target for this function - not unusedLocalFunction = getTarget(_) and + not functionIsCalled(unusedLocalFunction) and // If this is a TemplateFunction or an instantiation of a template, then only report it as unused // if all other instantiations of the template are unused not exists( @@ -88,7 +161,7 @@ module UnusedLocalFunction { | // There exists an instantiation which is called functionFromInstantiatedTemplate.isConstructedFrom(functionFromUninstantiatedTemplate) and - functionFromInstantiatedTemplate = getTarget(_) + functionIsCalled(functionFromInstantiatedTemplate) ) and // A function is defined as "used" if any one of the following holds true: // - It's an explicitly deleted functions e.g. =delete @@ -100,6 +173,9 @@ module UnusedLocalFunction { not unusedLocalFunction.getAnAttribute().getName() = "maybe_unused" and not overloadedFunctionIsCalled(unusedLocalFunction) and not addressBeenTaken(unusedLocalFunction) and + // We have no visibility into the call graph of a template that is never instantiated + // anywhere in the database, so we cannot reliably tell it is unused. + not hasNoVisibleInstantiation(unusedLocalFunction) and message = unusedLocalFunction.getLocalFunctionType() + " function " + unusedLocalFunction.getName() + " is not statically called, or is in an unused template." diff --git a/cpp/common/test/rules/unusedlocalfunction/UnusedLocalFunction.expected b/cpp/common/test/rules/unusedlocalfunction/UnusedLocalFunction.expected index d3de068e9c..df9322e7d7 100644 --- a/cpp/common/test/rules/unusedlocalfunction/UnusedLocalFunction.expected +++ b/cpp/common/test/rules/unusedlocalfunction/UnusedLocalFunction.expected @@ -6,3 +6,5 @@ | test.cpp:85:6:85:7 | h3 | Anonymous namespace function h3 is not statically called, or is in an unused template. | | test.cpp:144:8:144:8 | f | Anonymous namespace class member function f is not statically called, or is in an unused template. | | test.cpp:150:8:150:8 | f | Anonymous namespace class member function f is not statically called, or is in an unused template. | +| test.cpp:214:9:214:18 | deadHelper | Private member function deadHelper is not statically called, or is in an unused template. | +| test.cpp:236:16:236:28 | neverUsedPure | Private member function neverUsedPure is not statically called, or is in an unused template. | diff --git a/cpp/common/test/rules/unusedlocalfunction/test.cpp b/cpp/common/test/rules/unusedlocalfunction/test.cpp index 9ce37dcaae..45f28665fc 100644 --- a/cpp/common/test/rules/unusedlocalfunction/test.cpp +++ b/cpp/common/test/rules/unusedlocalfunction/test.cpp @@ -157,4 +157,82 @@ class C3 { public: void f() {} // COMPLIANT - public external linkage }; -} // namespace N1 \ No newline at end of file +} // namespace N1 + +class PureVirtualBase { +public: + void callImpl() { impl(); } + +private: + virtual void impl() = 0; // COMPLIANT - pure virtual contract. +}; + +class PureVirtualDerived : public PureVirtualBase { +private: + void impl() override {} +}; + +void test_pure_virtual_private_member() { + PureVirtualDerived derived; + derived.callImpl(); +} + +/** + * Class templates that are never instantiated anywhere in the analyzed + * compilation units. + * + * Clang never elaborates a body for the members of such patterns, so calls + * between sibling members (even genuine ones, like a public entry point calling + * a private helper) cannot be resolved by the call graph. We conservatively + * treat all of them as used, rather than risk reporting them as dead code. + */ +template class NeverInstantiatedFactory { +public: + static void Create() { instanceHelper(); } + +private: + static void instanceHelper() { + } // COMPLIANT - class template is never instantiated anywhere in this + // translation unit, so the analysis has no visibility into whether + // `Create` (also never instantiated) really calls it; conservatively + // not reported. +}; + +/** + * A class template that *is* instantiated (and its caller genuinely used), so + * the ordinary per-instantiation call-graph reasoning applies and a + * truly-unused private helper is still correctly reported. + */ +template class InstantiatedFactory { +public: + UsedT get() { return makeValue(); } + +private: + UsedT makeValue() { + return UsedT(); + } // COMPLIANT - called by get(), which is instantiated. + UsedT deadHelper() { // NON_COMPLIANT - never called, and the class template + // is instantiated, so the analysis does have visibility + // into this member. + return UsedT(); + } +}; + +void test_instantiated_factory() { + InstantiatedFactory factory; + factory.get(); +} +/** + * A private pure virtual that is genuinely dead: it is never called through the + * non-virtual interface, and no derived class ever overrides it. Pure virtual + * functions are deliberately in scope for this query (see + * `UnusedFunctions::UsableFunction`), so this must still be reported. + */ +class DeadPureVirtualBase { +public: + void unrelated() {} + +private: + virtual void neverUsedPure() = 0; // NON_COMPLIANT - never called, never + // overridden. +};