Merge pull request #1169 from castler/fix-rule-0-2-4-unused-limited-visibility-fp

Fix RULE-0-2-4/A0-1-3 false positives for private functions

Author: MichaelRFairhurst

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.

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<UnusedLocalFunctionConfigSig Config> {
   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<UnusedLocalFunctionConfigSig Config> {
     |
       // 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<UnusedLocalFunctionConfigSig Config> {
     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."

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. |

cpp/common/test/rules/unusedlocalfunction/test.cpp

@@ -157,4 +157,82 @@ class C3 {
 public:
   void f() {} // COMPLIANT - public external linkage
 };
-} // namespace N1
+} // 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 NeverUsedT> 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 UsedT> 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<int> 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.
+};