This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add more documentation about check development
ClosedPublic

Authored by LegalizeAdulthood on Jan 21 2022, 5:55 PM.

Details

Summary
  • Mention pp-trace
  • CMake configuration
  • Overriding registerPPCallbacks
  • Check development tips
    • Guide to useful documentation
    • Using the Transformer library
    • Developing your check incrementally
    • Creating private matchers
    • Unit testing helper code
    • Making your check robust
    • Documenting your check
  • Describe the Inputs test folder

Diff Detail

Event Timeline

LegalizeAdulthood requested review of this revision.Jan 21 2022, 5:55 PM
LegalizeAdulthood edited the summary of this revision. (Show Details)

It's also make sense to mention isLanguageVersionSupported.

clang-tools-extra/docs/clang-tidy/Contributing.rst
79

Clang.

232

Excessive newline.

237

Clang.

243

Clang.

265

Clang.

339

API.

385

Link to Release Notes?

LegalizeAdulthood marked 7 inline comments as done.Jan 22 2022, 10:52 AM

It's also make sense to mention isLanguageVersionSupported.

Good idea.

Update from review comments

clang-tools-extra/docs/clang-tidy/Contributing.rst
295

Should be functions

338

chunks plural, "an intention revealing name" singular mismatch

Eugene.Zelenko added inline comments.Jan 22 2022, 5:58 PM
clang-tools-extra/docs/clang-tidy/Contributing.rst
232

Other languages and their versions are applicable too.

LegalizeAdulthood marked an inline comment as done.

Update from review comments

Clarify ninja build example

Eugene.Zelenko added inline comments.Jan 22 2022, 9:05 PM
clang-tools-extra/docs/clang-tidy/Contributing.rst
232

Objective-C is also supported.

salman-javed-nz added inline comments.
clang-tools-extra/docs/clang-tidy/Contributing.rst
76

Don't you need to enable both clang and clang-tools-extra? Otherwise the clang-tidy CMake target doesn't appear. That has been my experience.

LegalizeAdulthood marked 2 inline comments as done.Jan 23 2022, 7:01 AM
LegalizeAdulthood added inline comments.
clang-tools-extra/docs/clang-tidy/Contributing.rst
232

I copied the doxygen language for LangOptions from LangOptions.h.

LegalizeAdulthood marked an inline comment as done.

Update from review comments

Thank you so much for working on this documentation, I really like the direction it's going!

clang-tools-extra/docs/clang-tidy/Contributing.rst
78
227
228

As usual, we're not super consistent, but most of our documentation is single-spaced (can you correct this throughout your changes?).

232

I made it more generic, you can use this for more than just checking languages (you can check for other language features like whether char is signed or unsigned, etc).

261

I'd argue the most important thing you interact with from Clang are the AST nodes. Maybe instead of "most important", we can be vague and just say "Some commonly used features are" or something like that?

300–316

I think this documentation is really good, but at the same time, I don't think we have any clang-tidy checks that make use of the transformer library currently. I don't see a reason we shouldn't document this more prominently, but I'd like to hear from @ymandel and/or @alexfh whether they think the library is ready for officially supported use within clang-tidy and whether we need any sort of broader community discussion. (I don't see any issues here, just crossing t's and dotting i's in terms of process.)

338–339
342
359–360

Do we have any private matchers that are unit tested? My understanding was that the public matchers were all unit tested, but that the matchers which are local to a check are not exposed via a header file, and so they're not really unit testable.

369

Reworded to avoid a loaded term and not make it sound C++ specific; needs re-flowing to 80-col limits

371–372
382

Another one that catches people out is testing on Windows vs non-Windows targets (as targets which are compatible with MSVC default to a different template instantiation behavior).

One key thing that's not discussed explicitly are false positive rates. Clang-tidy can have significantly higher false positive rates than diagnostics in Clang or the static analyzer, but we still care about not being overly chatty. But "overly chatty" depends on the check -- if the check is for a coding standard and the coding standard says "no calls to 'foo()'", then it's not chatty to diagnose every call to "foo()". But if the check is not following a coding standard, but is instead trying to help someone modernize their code, find bugs, or make it more readable (as examples), then perhaps diagnosing every call to "foo()" will be too chatty. So we ask people to test their code against real world code bases to try to gauge what the false positive and true positive rates are for the check, just to make sure they seem reasonable.

Another thing we may want to mention is that checks can be configured. This helps not only with exposing different functionality for the check, but also can help to control perceived false positives for some use cases.

@aaron.ballman I think this has exposed some limitations with the add-new-check script. Maybe there is merit for the script to be updated to support preprocessor callbacks and options, WDYT?

clang-tools-extra/docs/clang-tidy/Contributing.rst
228

People keep asking for this, but it doesn't matter for the final output and it isn't
specified anywhere in the style guide.

232

This came up in another review, but if you have a check that applies
to C++11 or later, do you have to check all the versions or can you
assume that the C++11 flag will be set when C++14 is requested
via command-line options?

300–316

There are at least two checks that use the Transformer library currently.

359–360

I just did this for my switch/case/label update to simplify boolean expressions.
You do have to expose them via a header file, which isn't a big deal.

@aaron.ballman I think this has exposed some limitations with the add-new-check script. Maybe there is merit for the script to be updated to support preprocessor callbacks and options, WDYT?

It could certainly use an option to specify that your check is Transformer
based.

@aaron.ballman I think this has exposed some limitations with the add-new-check script. Maybe there is merit for the script to be updated to support preprocessor callbacks and options, WDYT?

I wouldn't be opposed to it. I think it currently serves the majority of the needs (there are far less preprocessor checks than there are AST checks), but making it more full-featured would be a win for some folks.

clang-tools-extra/docs/clang-tidy/Contributing.rst
228

We're consistently inconsistent, but the reason why I think we tend to prefer single space is because it's less bytes for everyone to download (as you say, single vs double space doesn't matter for the generated output) and generally we're all reading this on computer screens rather than in print (where double spacing actually helped readability).

232

Later modes will also set earlier modes. e.g., passing -std=c++20 on the command line will set CPlusPlus, CPlusPlus11, CPlusPlus14, CPlusPlus17, and CPlusPlus20 in LangOptions.

300–316

The only mentions of TransformerClangTidyCheck.h that I can find are in ClangTransformerTutorial.rst and clang-formatted-files.txt, so if there are other checks using this functionality, they're not following what's documented here.

359–360

You do have to expose them via a header file, which isn't a big deal.

Then they become part of the public interface for the check and anything which includes the header file has to do heavy template instantiation work that contributes more symbols to the built library. In general, we don't expect private matchers to be unit tested -- we expect the tests for the check to exercise the matcher appropriately.

LegalizeAdulthood marked 16 inline comments as done.Jan 24 2022, 7:14 PM
LegalizeAdulthood added inline comments.
clang-tools-extra/docs/clang-tidy/Contributing.rst
300–316

CleanupCtadCheck, StringFindStrContainsCheck, and StringviewNullptrCheck all derive from TransformerClangTidyCheck.

The first two are in the abseil module and the last is in the bugprone module.

359–360

Look at my review to see how I handled it; the matchers are in a seperate
header file, in my case SimplifyBooleanExprMatchers.h and aren't exposed
to consumers of the check. The matchers header is only included by the
implementation and the matcher tests.

LegalizeAdulthood marked 2 inline comments as done.

Update from review comments

Aside from the unit testing bit, I think this is fantastic! (And the unit testing bit may also be fantastic, I just think it needs more explicit discussion with a wider audience.)

clang-tools-extra/docs/clang-tidy/Contributing.rst
300–316

Oh, interesting, thanks for pointing that out! It turns out that that clang and clang-tools-extra are different sibling directories and searching clang for things you expect to find in clang-tools-extra is not very helpful. :-D

My concerns are no longer a concern here.

359–360

Thanks for pointing out how you're doing it in one of your checks -- I still don't think we should document that we expect people to unit test private matchers unless clang-tidy reviewers are on board with the idea in general. IMO, that's putting a burden on check authors and all the reviewers to do something that's never been suggested before (let alone documented as a best practice) -- that's worthy of open discussion instead of adding it to a patch intended to document current practices.

Overall, this looks fantastic! You may want to consider (in a separate patch) mentioning godbolt.org, which is a great UI for interacting with clang-query and the AST. Example configuration: https://godbolt.org/z/v88W8ET19

@aaron.ballman I think this has exposed some limitations with the add-new-check script. Maybe there is merit for the script to be updated to support preprocessor callbacks and options, WDYT?

It could certainly use an option to specify that your check is Transformer
based.

Agreed. Happy to do this if there's interest. I already have a (google) internal version of this script that does exactly that. In fact, we set it as the default, and require users to explicitly opt out. It is our preference for all new clang tidy checks.

clang-tools-extra/docs/clang-tidy/Contributing.rst
232–233

nit: "be sure"? (just to vary the language a bit)

300–316

My 2cents: definitely comfortable encouraging adoption. In fact, we require it on internal checks unless the user has a strong reason to opt out.

359–360

Agreed on this point -- that we should push this off to discussion on a separate patch. I'm definitely fine with pointing out that unit testing is possible, since that may not be obvious. But, I'd have to be convinced that we want to insist on it.

388–389

Is it worth giving a rule-of-thumb? Personally I'd go with < 10%, all things being equal. A check for a serious bug may reasonably have a higher false positive rate, and trivial checks might not justify *any* false positives. But, if neither of these apply, I'd recommend 10% as the default.

LegalizeAdulthood marked an inline comment as done.Jan 25 2022, 2:00 PM
LegalizeAdulthood added inline comments.
clang-tools-extra/docs/clang-tidy/Contributing.rst
361

If I change the wording from "is best tested with a unit test" to "can be tested with a unit test",
would that alleviate the concern? I want to encourage appropriate testing and unit testing
complex helper code is better than integration testing helper code.

I find it easier to have confidence in private matchers if they are unit tested and I've recently
had a few situations where I had to write relatively complex helper functions to analyze raw
text that I felt would have been better tested with a unit test than an integration test.

388–389

I'm OK with rule-of-thumb 10% advice.

LegalizeAdulthood marked 4 inline comments as done.

Update from review comments

aaron.ballman added inline comments.Jan 26 2022, 7:01 AM
clang-tools-extra/docs/clang-tidy/Contributing.rst
361

If I change the wording from "is best tested with a unit test" to "can be tested with a unit test", would that alleviate the concern?

I largely addresses mine -- saying it can be done is great, saying it should be done is what gave me pause.

388–389

FWIW, I think 10% is pretty arbitrary and I'd rather not see us try to nail it down to a concrete number. In practical terms, it really depends on the check.

Also, clang-tidy is where we put things with a "high" false positive rate already, so this statement has implications on what an acceptable false positive rate is for Clang or the static analyzer.

How about something along these lines:

- Watch out for high false positive rates. Ideally, a check would have no false positives, but given that matching against an AST is not control- or data flow-sensitive, a number of false positives are expected. The higher the false positive rate, the less likely the check will be adopted in practice. Mechanisms should be put in place to help the user manage false positives.
- There are two primary mechanisms for managing false positives: supporting a code pattern which allows the programmer to silence the diagnostic in an ad hoc manner and check configuration options to control the behavior of the check.
- Consider supporting a code pattern to allow the programmer to silence the diagnostic whenever such a code pattern can clearly express the programmer's intent. For example, allowing an explicit cast to `void` to silence an unused variable diagnostic.
- Consider adding check configuration options to allow the user to opt into more aggressive checking behavior without burdening users for the common high-confidence cases.

(or something along those lines). The basic idea I have there is: false positives are expected, try to keep them to a minimum, and here are the two most common ways code reviewers will ask you to handle false positives when they're a concern.

ymandel added inline comments.Jan 26 2022, 10:53 AM
clang-tools-extra/docs/clang-tidy/Contributing.rst
361

+1

388–389

Strongly agree. 10% has served as well in practice for the threshold at which we disable/fix checks, but it's definitely arbitrary. I much prefer your suggested approach.

LegalizeAdulthood marked 2 inline comments as done.Jan 26 2022, 1:05 PM
LegalizeAdulthood added inline comments.
clang-tools-extra/docs/clang-tidy/Contributing.rst
388–389

Yeah, I wasn't a fan of a magic number style piece of advice. I like the reworded suggestion better.

Update from review comments

ymandel accepted this revision.Jan 27 2022, 5:49 AM
This revision is now accepted and ready to land.Jan 27 2022, 5:49 AM
aaron.ballman accepted this revision.Jan 27 2022, 6:13 AM

LGTM, thank you for these fantastic improvements to the docs!