Skip to content

Fix #4270: preserve Qt property accessor references for unusedFunction - #8838

Open
tzi4 wants to merge 4 commits into
cppcheck-opensource:mainfrom
tzi4:fix/trac-4270-qt-properties
Open

tzi4 wants to merge 4 commits into
cppcheck-opensource:mainfrom
tzi4:fix/trac-4270-qt-properties

Conversation

@tzi4

@tzi4 tzi4 commented Sep 8, 2026

Copy link
Copy Markdown

This fixes the false unusedFunction warnings reported in Trac #4270. Cppcheck can report a method as unused even when it is referenced by Q_PROPERTY, because the Qt configuration removes the declaration during preprocessing.

The patch keeps those references through macro expansion using Qt's QT_ANNOTATE_CLASS hook and passes them to the unused function check. I added tests for wrapper macros, inherited accessors and cached results. I also checked that unrelated unused methods still produce a warning.

All 46 new integration cases pass, along with Cppcheck's full C++ test suite.

I also tried a few property examples with Qt 5 and Qt 6.11.2 moc, and both generated the expected accessor calls.

Comment thread lib/preprocessor.cpp Outdated
Comment on lines +905 to +906
static const simplecpp::Token* qtPropertyAttributes(const simplecpp::Token* tok)
{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an AI review. Take it with a grain of salt and feel free to reject it by resolving the comment.

I built the PR and confirmed the ticket example: value/setValue/resetValue/name referenced from Q_PROPERTY are no longer reported, while a really unused method still is. The 60 tests in test/cli/unused_function_test.py pass.

Design question: qtPropertyAttributes() + qtPropertyFunctions() hard-code Qt's grammar (type parsing with templates/>>, CONSTANT/FINAL/REVISION/MEMBER, ...) in the generic Preprocessor, and they are the only reason preprocessor.cpp now includes token.h (for Token::isStandardType), which pulls in many headers (see the oss-fuzz/Makefile diff). The <exporter prefix=...> list in qt.cfg already describes what matters.

As an experiment, I replaced qtPropertyFunctions() with the generic rule "a name (optionally ::-qualified or parenthesized) following an exporter prefix keyword at depth 0". All but 4 of the 60 CLI tests still pass. The 4 failures are all test_unused_functions_qt_property_keyword_names, where a type or the property itself is named READ/WRITE (class READ {}; Q_PROPERTY(READ property READ value ...)). Is handling such names worth around 60 extra lines of Qt-specific parsing in the preprocessor? In that rare case the worst outcome is that a function named like the type is treated as used. With the generic rule, qtPropertyAttributes() and the token.h include could go away, and the same code could serve any library exporter, not just Q_PROPERTY.

@danmar danmar Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we cleanup this PR? Personally, I think it would sound better to have a cleaner simpler code but a small set of false negatives?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow up and sorry for the slow reply. I can see that the simpler approach could be better trade off here after you mentioned it.

I tried a shared exporter-keyword scan locally. It removes the Qt-specific grammar parsing and the token.h dependency while keeping the annotation hook and macro/cache handling. The original tests only differ in the four keyword-name cases you have pointed out, so I am happy to accept those false negatives and simplify the PR along those lines.

@dmcppcheck

Copy link
Copy Markdown
Contributor

Test results for commit 8825355 (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8838/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: +0.0% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8838/2026-09-09_14-08-41-8825355d017e_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

@dmcppcheck

Copy link
Copy Markdown
Contributor

Test results for commit c768129 (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8838/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: +0.1% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8838/2026-10-10_14-46-12-c768129dffa2_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

I found no new correctness problems. The ticket case works, including:

  • accessors declared in a header and defined in a separate .cpp
  • -j1, and -j2 with a reused --cppcheck-build-dir
  • wrapper, ## and duplicating macros
  • malformed Q_PROPERTY arguments, which caused no crash and no leftover __cppcheck_qt_annotation__ tokens

test/cfg/qt.cpp gives the same results with --enable=all on base and on the PR.

One limitation is left, and it is not a regression. When the source defines Q_PROPERTY itself as empty and calls it through a wrapper macro, the fallback in getExportedFunctions() misses the reference. The false positive that base already reports stays.

Testing

Builds: make -j8 cppcheck testrunner succeeded in pr/, and make -j8 cppcheck succeeded in base/.

Tests:

  • ./testrunner TestUnusedFunctions TestPreprocessor TestLibrary: 0 failures.
  • python3 -m pytest test/cli/unused_function_test.py: 64 passed.

Base and PR compared: cppcheck -q --enable=all --inconclusive --library=qt test/cfg/qt.cpp gave the same 229 lines on both.

My own tests (all in /work/scratch):

  • Header and separate .cpp: a Q_OBJECT class in a header with Q_PROPERTY(int value READ value WRITE setValue NOTIFY valueChanged), with the methods defined in a.cpp and the class used from main.cpp. With -j1 --no-cppcheck-build-dir, -j2 --cppcheck-build-dir=bd and -j1 --cppcheck-build-dir=bd, only unused is reported.
  • Macro variants: P(Q_PROPERTY(...)), TWICE(Q_PROPERTY(...)) and CAT(Q_,PROPERTY)(...). The -E output contains no annotation tokens, and only unused is reported.
  • Malformed input: Q_PROPERTY(), Q_PROPERTY(READ), READ ::, READ (::), READ (), READ (x::) and a bare WRITE. No crash; the result is as expected.
  • Limitation, not a regression: base gives the same result here.
#define Q_PROPERTY(...)
#define QP(...) Q_PROPERTY(__VA_ARGS__)
class A {
    QP(int a READ getA)
public:
    int getA() const { return 0; }
};
int main() { A a; }

cppcheck -q --enable=unusedFunction --library=qt f.cpp prints:

f.cpp:6:9: style: The function 'getA' is never used. [unusedFunction]

Automated review of c768129 by Claude. The pull request was built and tested locally.

Comment thread lib/preprocessor.cpp
// and not already handled by the hook, excluding inactive branches.
const auto collect = [&](const simplecpp::TokenList& tokens) {
for (const simplecpp::Token* tok = tokens.cfront(); tok; tok = tok->next) {
if (locations.find(tok->location) == locations.end() ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

This fallback only catches exporter invocations that are written literally in the source. For a wrapper such as #define Q_PROPERTY(...) plus #define QP(...) Q_PROPERTY(__VA_ARGS__) and QP(int a READ getA), the use location of Q_PROPERTY holds the raw token QP, so getA is still reported as unused. This is no worse than base. If the case should not be supported, a comment here would say so; if it should, a test for it would document the behaviour.

@dmcppcheck

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants