llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.14k stars 12.02k forks source link

[clang-tidy] RFC: Add generic check that takes match expression like clang-query #107680

Open olologin opened 2 months ago

olologin commented 2 months ago

A wild idea, but maybe someone else will also find it useful, you can close this issue if it doesn't make sense or raises too many concerns.

Can we have clang-tidy check that takes its AST match expressions from clang-query input file? I can explain why this might be useful: Clang-tidy has a lot of infrastructure built around it, like plugins for IDE, wrapper-scripts like https://github.com/Ericsson/codechecker which we use at work, and I believe also a lot of other things. And unfortunately some projects need to have their own weird (but simple) checks, which are useless for upstream clang-tidy. A lot of such checks could be easily expressed as clang-query match expressions, but we dont want to build infrastructure around clang-query. So we have to to build our simple custom checks into clang-tidy/clang-plugin and then build it ourselves. But rebuilding clang-tidy to add simple match expression seems like overkill, especially because you need to build it for different platforms, distribute it, find some way for your developers to keep it updated, and you have to do all this every time some developer decides to improve existing check or add new check.

What if we just develop a new check like clang-query-based which in its configuration takes path to the clang-query file? It seems clang-query parser is already implemented, maybe it is easy to reuse it for this purpose.

Usecase example, in some weird project:

.clang-tidy:

Checks: >
    clang-query-based

CheckOptions:
    clang-query-based.QueryFile: clang-query/custom_query.txt

clang-query/custom_query.txt contents:

match callExpr(
    callee(functionDecl(
        hasAnyName("sort", "nth_element", "partial_sort", "partition"),
        anyOf(
            hasDeclContext(namespaceDecl(hasName("std"))),
            unless(hasParent(namespaceDecl()))
        )
    )),
    isExpansionInFileMatching(".*my_project/dev/.*")
).bind("Please use sort functions from platform-stable library instead of std (std sort functions lead to different results on different platforms)")

match callExpr(
    callee(functionDecl(
        hasAnyName("acos", "asin", "atan", "atan2", "cos", "sin", "tan", "cosh", "sinh", "tanh", "ctan"),
        anyOf(
            hasDeclContext(namespaceDecl(hasName("std"))),
            unless(hasParent(namespaceDecl()))
        )
    )),
    isExpansionInFileMatching(".*my_project/dev/.*")
).bind("Please use math functions from platform-stable library instead of std (std math functions lead to different results on different platforms)")

Alternatively, putting match expressions right into .clang-tidy yaml file (as multiline field) should also work, it is probably even better because caching tools will not have to consider another file.

olologin commented 2 months ago

Something to think about: How should NOLINT work with these custom matches.

MichelleCDjunaidi commented 2 months ago

If I'm understanding you right, you want to funnel a clang-query matcher into a clang-tidy check. A possible concern is https://github.com/llvm/llvm-project/issues/106656; that is, the AST implementation of clang-query is not exactly the same as clang-tidy, so there will be cases where your clang-query matcher can't translate into clang-tidy without further modification. Granted, this doesn't happen often, but the underlying issue should probably be fixed before this feature can take off.

olologin commented 2 months ago

@MichelleCDjunaidi I think slightly adapting matcher for clang-tidy is also fine, I can live with that. My main issue is inability to add some checks without rebuilding and redeploying new clang-tidy.