[clang-tools-extra] [clang-tidy] Add modernize-nlohmann-json-explicit-conversions check (PR #126425)

[Mike Crowe via cfe-commits]

mikecrowe wrote:

I had anticipated some pushback for this check.

The capabilities that clang-tidy provides give library writers the ability to make breaking changes whilst keeping their users happy because avoiding the breakage can be (at least mostly) automated. I think that such changes are far more common outside the standard library and C++ language itself and they allow correcting the mistakes of the past. There are many heavily-used libraries that would benefit from clang-tidy checks. I believe that such automation is planned to be an integral part of the Carbon compatibility story.

Until now I hadn't realised that clang-tidy plugins were possible - support for them has been added since I started writing clang-tidy checks and I clearly haven't been paying attention! I will investigate turning this check into a plugin instead. I think that the situation for "modernize" checks like this one is different to other checks too: such checks are more likely to only be run a few times for testing and then finally once for real. They won't be incorporated into CI pipelines. Once the code has been modernised the checks will never be used on that code again. This means that having to build the check by hand is more acceptable.

Nevertheless, I think that it would be worth having a policy on clang-tidy support for libraries to manage expectations. Perhaps that policy could even mean moving some of the existing built-in checks out to plugins in the long term?

Even assuming that this check will never land, I'm still interested in review of the code that will hopefully eventually end up in a plugin if anyone wishes to look at it.

@HerrCai0907 wrote:
> I think we should a guideline this kind of cases to avoid to waste contributors effort. It is frustrating to write code and find out it can't possibly be merged.

In this case I'm not particularly worried. I wrote the check so I could use it. The amount of extra work required for the release notes etc. included in this change was small.

@carosgalvezp wrote:
> It feels like instead nlohmann/json should be the one responsible for providing a "migration path" / tooling for these types of deprecated features.

clang-tidy provides an excellent basis for doing that so it should be no surprise that library authors and users would want to build on it. Hopefully a plugin can work well enough, but I am slightly worried about compatibility with different versions of clang-tidy. I'll find out!

Thanks to everyone for their comments.


https://github.com/llvm/llvm-project/pull/126425