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

[castler]

Description

Two categories of false positive are addressed:

1. Pure virtual private member functions — RULE-0-2-4 and A0-1-3
   require that private member functions be used. A pure virtual
   function (= 0) is an interface contract that must be overridden
   by derived classes; it has no body of its own and cannot be
   "called" directly. Excluding PureVirtualFunction from the
   LocalFunction class prevents spurious reports on these.

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 CRTP-style wrappers
   that is only ever instantiated by
   downstream consumers outside of 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-template pattern has any instantiation either, so
   genuinely dead private helpers in class templates that are
   instantiated elsewhere are still correctly reported.

Fixes #1168

Change request type

• Release or process automation (GitHub workflows, internal scripts)
• Internal documentation
• External documentation
• Query files (.ql, .qll, .qls or unit tests)
• External scripts (analysis report or other code shipped as part of a release)

Rules with added or modified queries

• No rules added
• Queries have been added for the following rules:
  • rule number here
• Queries have been modified for the following rules:
  • RULE-0-2-4
  • A0-1-3

Release change checklist

A change note (development_handbook.md#change-notes) is required for any pull request which modifies:

• The structure or layout of the release artifacts.
• The evaluation performance (memory, execution time) of an existing query.
• The results of an existing query in any circumstance.

If you are only adding new rule queries, a change note is not required.

Author: Is a change note required?

• Yes
• No

🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.

• Confirmed

Reviewer: Confirm that either a change note is not required or the change note is required and has been added.

• Confirmed

Query development review checklist

For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:

Author

• Have all the relevant rule package description files been checked in?
• Have you verified that the metadata properties of each new query is set appropriately?
• Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
• Are the alert messages properly formatted and consistent with the style guide?
• Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
  As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
• Does the query have an appropriate level of in-query comments/documentation?
• Have you considered/identified possible edge cases?
• Does the query not reinvent features in the standard library?
• Can the query be simplified further (not golfed!)

Reviewer

• Have all the relevant rule package description files been checked in?
• Have you verified that the metadata properties of each new query is set appropriately?
• Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
• Are the alert messages properly formatted and consistent with the style guide?
• Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
  As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
• Does the query have an appropriate level of in-query comments/documentation?
• Have you considered/identified possible edge cases?
• Does the query not reinvent features in the standard library?
• Can the query be simplified further (not golfed!)

[MichaelRFairhurst]

Mostly looks good! I just wanted to say I couldn't reproduce this issue:

1. **Static call targets not recognised** — the original check used
   only `getTarget(call)` from DynamicCallGraph, which resolves
   virtual dispatch.  For private (non-virtual) member functions and
   private static functions that are called directly, CodeQL sometimes
   cannot resolve a dynamic target and the function was incorrectly
   reported as unused.  The new `functionIsCalled(fn)` predicate
   combines the dynamic-graph result with a direct
   `FunctionCall.getTarget()` lookup so that statically-dispatched
   calls are always counted as a use.

and I couldn't get these // COMPLIANT test cases to fail when I commented out the change.

I tried to get the eclipse/communication project database to confirm the difference myself, but I ran into issues there that I'll have to figure out. It looked somewhat like the database I got was maybe built with the wrong CodeQL version, though I looked through the project and saw it looked like you're using the correct version everywhere, so I'm not totally sure why I was seeing issues on my side.

I also don't see exactly how the FunctionCall::getTarget will fix the issues with statics that you're seeing. If you look at VirtualDispatch::getAViableTarget you'll see that for non-virtual calls, it already uses Call::getTarget(), which FunctionCall::getTarget() overrides. The only difference I'm seeing on first look here is that DynamicCallGraph::getTarget1() requires the result of VirtualDisaptch::getAViaibleTarget to have a definition.

Thank you for the PR!

[castler]

I tried to get the eclipse/communication project database to confirm the difference myself, but I ran into issues there that I'll have to figure out. It looked somewhat like the database I got was maybe built with the wrong CodeQL version, though I looked through the project and saw it looked like you're using the correct version everywhere, so I'm not totally sure why I was seeing issues on my side.

To be frank, we used the wrong CodeQL version for some amount of time. Maybe you had a CodeQL database from that? But since around a month (https://github.com/eclipse-score/communication/pull/757/changes#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdcL440) we should use the correct one.

Regarding your finding - I have to apologize - I think you are right.
I will review this today or tomorrow and push a revised version

[MichaelRFairhurst]

Thanks @castler, that would plausibly explain it. I'm definitely not against merging, since these fixes are not harmful, if we find that the mystery continues. 👍

[castler]

Thanks @castler, that would plausibly explain it. I'm definitely not against merging, since these fixes are not harmful, if we find that the mystery continues. 👍

@MichaelRFairhurst I tried to revisit this topic from ground. I think the changes for the pure-virtual interface are valid, but I am not to sure about: https://github.com/github/codeql-coding-standards/pull/1169/changes#diff-67e20819af17d4736a107e5c7147cf6c52cd27479b579c9a96a26adc004feb39R186-R187 producing false negatives? But I also have no clue how I could test this right now.