Repository navigation
Conversation
9c4b4ab to
d7ddfb1
Compare
|
I've made a small improvement to the check, improved the documentation and tried to placate the REUSE licence checks. I think that the Codacy check failures probably just need to be ignored somehow. We don't really want to modify LLVM's I'm afraid that I don't really understand the Windows failures. |
|
This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions! |
github-actions
Bot
added
the
state: stale
d7ddfb1 to
2403373
Compare
github-actions
Bot
removed
the
state: stale
|
This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions! |
github-actions
Bot
added
the
state: stale
There was a problem hiding this comment.
Thanks a lot for this, @mikecrowe , and sorry it took so long to get a review. The LLVM reviewers had a point: migration tooling for dropping implicit conversions belongs with the library. I'd like to merge this. A few things first:
Build / CMake
project(nlohmann_json VERSION 3.12.0 LANGUAGES C CXX)makes every consumer of the library need a C compiler. Please keepLANGUAGES CXXand callenable_language(C)inside theif (JSON_ClangTidyPlugin)block, if the LLVM/Clang CMake config needs it. This may also be what broke the Windows jobs.- The branch now conflicts with
developinCMakeLists.txt. Could you rebase? That will also trigger a fresh CI run; the current head has none.
CI workflow
- The workflow is called
name: Ubuntu, the same as the existing Ubuntu workflow. Please rename it, for instance toclang-tidy plugin. - Please update the matrix to current clang releases (at least the oldest supported and the newest available), and bump the pinned
actions/checkoutto the version the other workflows use.
Tests
- The test uses a hand-written stand-in for
basic_json. The real class lives in an ABI-tagged inline namespace (nlohmann::json_abi_v3_12_0::basic_json), which users can also change viaNLOHMANN_JSON_NAMESPACE_*. Could you add at least one test that runs the check on the realsingle_include/nlohmann/json.hpp? It should coverjson,ordered_json, and a custombasic_jsoninstantiation. Even a simple "fix, then compile withJSON_USE_IMPLICIT_CONVERSIONS=0" test would give a lot of confidence. - A few edge cases I'd like either tested or documented as limitations:
- conversions inside macro expansions (a fix-it there would rewrite the macro body)
- conversions inside function templates, where the same line can be reported once per instantiation and could get conflicting fixes
- a custom
string_t: thestd::basic_string<char>→std::stringsubstitution only covers the default string type.
Codacy
- No need to change LLVM's
check_clang_tidy.pyor the test file for Codacy. I'll exclude those two paths from the Codacy analysis instead.
Docs
- Please state in
clang-tidy.mdwhich clang versions are supported, and say that the plugin is best-effort tooling: it's tested in CI but not covered by the library's API stability guarantees. The clang plugin API changes between releases, and I'd rather set expectations up front.
Your open question
Implicit conversion to std::string_view is explicitly disabled in operator ValueType(), as are std::any and std::optional. Conversion to std::wstring isn't possible either, because there is no way to read JSON into it. So the check doesn't need to handle any of these. A negative test case would be nice, though.
If you don't have time
It's been a while since you opened this, so if you no longer have the time or interest to work through the points above, that's completely fine. Just let me know, and I'd be happy to pick up the PR myself and finish it. Your commits would stay intact (or at least carry a Co-authored-by trailer), and you'd be credited in the release notes.
Thanks again, this will make disabling implicit conversions by default a lot more realistic.
This review was written with Claude Code.
|
Thanks for looking at these patches. I will try to make the changes that you suggest, but it might take me a while and I suspect that it will take a few more rounds of review.
When I wrote #4610 I believed that it was not possible to include system headers from the test cases, which is why I was unable to use the real header and had to create a facsimile type. I'll check whether that's still true. |
github-actions
Bot
removed
the
state: stale
|
@nlohmann, would you prefer me to rebase and rewrite the existing commits to address the review comments or rebase only to resolve the conflict with current develop and then add new commits to my branch to address your review comments? I checked your contributing guidelines but I couldn't see a preference specified there. |
Whatever you like :) |
2403373 to
5fe5b8f
Compare
|
I've done the easy stuff and I've pushed the results to get another CI run and see whether the newer Clang versions work too. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
reuse==6.2.0 (pinned in cmake/requirements/requirements-reuse.txt) warns "'.reuse/dep5' is deprecated. You are recommended to instead use REUSE.toml. Use `reuse convert-dep5` to convert." on every ci_reuse_compliance run. A later reuse release that drops DEP5 support would break that CI job outright. Generate REUSE.toml from the 9 Files: stanzas with `reuse convert-dep5` and remove .reuse/dep5. `reuse lint` still reports full compliance with spec 3.3 (1268/1268 files with copyright and license information, unchanged), and `reuse spdx` produces the same per-file copyright and license entries as before the conversion. Update the three places that referenced the old file: FILES.md now documents REUSE.toml with a spec 3.3 link, README.md's REUSE section names REUSE.toml, and the comment in gen_hedley_undef_check.cmake now refers to REUSE.toml's blanket path = "**" rule. Overlaps #4663, which adds an entry to .reuse/dep5; that PR will need to move its entry into REUSE.toml when rebased on this. #5716 item 4 Signed-off-by: Niels Lohmann <mail@nlohmann.me>
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated and/or formatted correctly, or 📎 A ready-to-apply patch is attached to the failed workflow run as the git apply amalgamation.patchThis does not require installing astyle yourself. @mikecrowe Please read and follow the Contribution Guidelines. |
e62f30e to
acbc87a
Compare
Add a clang-tidy plugin containing one check to replace implicit conversions (as enabled by default with JSON_USE_IMPLICIT_CONVERSIONS) with explicit ones. This will make it easier for library users to switch away from using implicit conversions which should make it possible for the library to start disallowing them sooner. Being able to test the plugin in a similar way to how checks are tested within clang-tidy itself requires copying the check_clang_tidy.py script from the LLVM code itself. The check itself is virtually identical to the one proposed for inclusion in clang-tidy itself at llvm/llvm-project#126425 . Unfortunately it is necessary to add "C" to the languages for the project in CMakeLists.txt for find_package to work for LLVM. Signed-off-by: Mike Crowe <mac@mcrowe.com>
The purpose of these tests is to ensure that anyone attempting to compile the clang-tidy plugin on the tested distributions will be able to and that the resulting plugin will work. Clang-tidy plugins were only introduced with Clang 16, so we can't test with any earlier versions than that. Signed-off-by: Mike Crowe <mac@mcrowe.com>
Now that the plugin is included in the source tree it can be linked to directly. Signed-off-by: Mike Crowe <mac@mcrowe.com>
acbc87a to
9f61f75
Compare
|
I'm sorry that it has taken so long but I believe that I've addressed most of the review comments. The exception is the custom string type. I think that the check is useful even without that. I've also made some other changes:
|
| bool operator!=(const char*, const std::string_view&); | ||
| #endif | ||
|
|
||
| size_t strlen(const char* str); |
nlohmann
left a comment
nlohmann
left a comment
There was a problem hiding this comment.
Thanks a lot, @mikecrowe! This is a big step forward: the macro and template handling is careful, the cast removal is a nice touch, and the test file is now very thorough. Dropping the custom string_t support is fine with me as long as the docs say so (see the inline comment on clang-tidy.md).
I built the plugin at 9f61f75 with clang 19 (debian:13) and clang 22 (silkeh/clang:22). Both build cleanly and pass both tests when ctest actually runs them. I also tried a few extra cases with clang 19. Comments marked "(verified)" come from that run. Details are in the inline comments. In short:
- The workflow doesn't run any tests (the
ctest -Rregex matches nothing, andctestexits 0). This is the important one. - The compile check runs clang-tidy on the stand-in instead of the real header. The check does work on the real header (same fixes, and the result compiles with implicit conversions off), but the test should show it, including
ordered_jsonand a custombasic_json. - Five fixes produce code that doesn't compile with
JSON_USE_IMPLICIT_CONVERSIONS=0:(*p),**pp, a conditionalf ? j["a"] : j["b"], a binaryoperator*returningjson&, and types in an anonymous namespace. - I'd like the plugin to live in
tools/clang_tidy_plugin/, next totools/gdb_pretty_printer/. - Smaller points on naming, CMake, the workflow matrix (pinned
silkeh/clangimages) and the docs.
These came out right in my run: std::string, std::map<std::string, int>, if (j) (becomes j.get<bool>()), and (j = k) (becomes (j = k).get<int>()).
Thanks again for sticking with this!
This review was written with Claude Code.
| - name: Build clang-tidy plugin | ||
| run: cmake --build build --target clang_tidy_plugin/libNlohmannJsonClangTidyPlugin.so | ||
| - name: Test clang-tidy plugin | ||
| run: cd build && ctest -V --output-on-failure -R modernize-nlohmann-json-explicit-conversions-check |
There was a problem hiding this comment.
(verified) The tests are now called ...-conversions-output-check and ...-conversions-compile-check, so this regex matches neither. ctest prints "No tests were found!!!" and exits 0, so every job would pass without testing anything. How about this?
| run: cd build && ctest -V --output-on-failure -R modernize-nlohmann-json-explicit-conversions-check | |
| run: cd build && ctest -V --output-on-failure --no-tests=error -R modernize-nlohmann-json-explicit-conversions |
With the right regex, both tests pass locally. (The Actions workflows haven't run on this head yet. I'll approve them once this is fixed.)
| jobs: | ||
| ci_test_clang_tidy: | ||
| runs-on: ubuntu-latest | ||
| strategy: |
There was a problem hiding this comment.
17 container jobs on every push and PR is a lot for a plugin that rarely changes. Also, distro images leave it to the distro which clang minor version you get. How about the silkeh/clang:<version> images that ubuntu.yml already uses? Each tag pins one clang version, and the tags span Debian 12 (:16) to Debian 13 (:22). They contain clang-tidy and the LLVM/Clang CMake config but not the clang-tidy headers. A single apt-get install -y cmake ninja-build libclang-${{ matrix.clang }}-dev clang-tidy-${{ matrix.clang }} installs those at exactly the image's version (verified for 16.0.6 and 22.1.8). With container: silkeh/clang:${{ matrix.clang }}, I think clang: [16, 22] is enough. The plugin API breaks between major releases (like the ClangTidyModuleRegistry move in 22). The oldest version catches code that needs a newer API, and the newest catches code that uses a removed one. When clang 23 comes out, we bump the newer one. I built the plugin and ran both tests in silkeh/clang:22 that way, and they pass.
Please also add a paths: filter (the plugin, its tests, this workflow, and include/**, since the conversion operator lives there), and the concurrency: block the other workflows have.
| container: ${{ matrix.distro }} | ||
| steps: | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| - name: List files |
| # Run clang-tidy | ||
| message(STATUS "Running clang-tidy to fix implicit conversions in ${TEST_FILE}") | ||
| execute_process( | ||
| COMMAND ${CLANG_TIDY_TOOL} -load "${PLUGIN_PATH}" "-checks=-*,modernize-nlohmann-json-explicit-conversions" "-config={}" --fix "${TEST_FILE}" -- ${CXX_STANDARD_FLAG} |
There was a problem hiding this comment.
The compiler gets -DUSE_REAL_HEADER -I<single_include>, but clang-tidy only gets -std=c++17. So clang-tidy parses the stand-in basic_json, and the real header is only used to compile the fixed result.
The good news (verified): when I added -DUSE_REAL_HEADER -I single_include after --, the fixes were exactly the same, and the result compiles with JSON_USE_IMPLICIT_CONVERSIONS=0. So the check finds the real templated operator ValueType() in its ABI namespace. Please pass -DUSE_REAL_HEADER "-I${INCLUDE_PATH}" here too, so the test covers that.
| COMMAND ${CMAKE_COMMAND} | ||
| "-DTEMPLATE_FILE=${CMAKE_CURRENT_LIST_DIR}/modernize-nlohmann-json-explicit-conversions.cpp" | ||
| "-DTEST_WORK_DIR=${CMAKE_BINARY_DIR}/test_artifacts" | ||
| "-DCLANG_TIDY_TOOL=${CLANG_TIDY_TOOL}" |
There was a problem hiding this comment.
CLANG_TIDY_TOOL comes from cmake/ci.cmake, which is only included with JSON_CI, so this test only works with -DJSON_CI=ON. It also picks the first clang-tidy it finds, which isn't necessarily the version the plugin was built against. The output check already uses LLVM_TOOLS_BINARY_DIR. Could this use something like find_program(NLOHMANN_CLANG_TIDY NAMES clang-tidy HINTS ${LLVM_TOOLS_BINARY_DIR} NO_DEFAULT_PATH)?
| void f(const nlohmann::json &j1, const nlohmann::json &j2, const nlohmann::json *pj) | ||
| { | ||
| int i = j1.at("value1").get<int>(); | ||
| double d = j1["value2").get<double>(); |
There was a problem hiding this comment.
| double d = j1["value2").get<double>(); | |
| double d = j1["value2"].get<double>(); |
| int i = j1.at("value1").get<int>(); | ||
| double d = j1["value2").get<double>(); | ||
| bool b1 = j1.find("valid")->get<bool>(); | ||
| bool b2 = j.find("valid")->get<bool>(); |
There was a problem hiding this comment.
| bool b2 = j.find("valid")->get<bool>(); | |
| bool b2 = j2.get<bool>(); |
| // ... | ||
| } | ||
| ``` | ||
| After you have run this check you can set [JSON_USE_IMPLICIT_CONVERSIONS=0](../api/macros/json_use_implicit_conversions.md) to find any occurrences that the check have not been fixed automatically. |
There was a problem hiding this comment.
| After you have run this check you can set [JSON_USE_IMPLICIT_CONVERSIONS=0](../api/macros/json_use_implicit_conversions.md) to find any occurrences that the check have not been fixed automatically. | |
| After you have run this check you can set [JSON_USE_IMPLICIT_CONVERSIONS=0](../api/macros/json_use_implicit_conversions.md) to find any occurrences that the check has not fixed automatically. |
| const auto value3 = pj2->get<std::string>(); | ||
| const auto value4 = j4["value"]->get<std::string>(); |
There was a problem hiding this comment.
The input uses pj3, and value4 should use .get, not ->get. Please also give this code block a language and add blank lines around the code blocks on this page.
| const auto value3 = pj2->get<std::string>(); | |
| const auto value4 = j4["value"]->get<std::string>(); | |
| const auto value3 = pj3->get<std::string>(); | |
| const auto value4 = j4["value"].get<std::string>(); |
| [`JSON_DELETE_DEPRECATED_FUNCTIONS`](../api/macros/json_delete_deprecated_functions.md). This option is `OFF` by | ||
| default. | ||
|
|
||
| ### `JSON_ClangTidyPlugin` |
There was a problem hiding this comment.
To keep the options in alphabetical order, this belongs before JSON_DeleteDeprecatedFunctions.
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated and/or formatted correctly, or 📎 A ready-to-apply patch is attached to the failed workflow run as the git apply amalgamation.patchThis does not require installing astyle yourself. @mikecrowe Please read and follow the Contribution Guidelines. |
Add a clang-tidy plugin containing one check to replace implicit conversions (as enabled by default with JSON_USE_IMPLICIT_CONVERSIONS) with explicit ones. This will make it easier for library users to switch away from using implicit conversions which should make it possible for the library to start disallowing them sooner.
The skeleton of the check was generated by the clang-tidy add_new_check.py script. It added headers indicating that the files were under the LLVM Apache-2.0-with-LLVM-exception licence. I've replaced these with the MIT licence to match the rest of the library but would be happy to change them back.
The check was written using LLVM's clang-format style. I can modify that if you'd prefer.
The check_clang_tidy.py script was copied in its entirety from the LLVM repository so it must keep its existing Apache-2.0-with-LLVM-exception licence.
The test file is written in the style of clang-tidy's own tests. This means that it cannot include system headers. We might be able to relax that for a plugin so that the actual single_include header could be used. That might make cross-platform CI harder though and I haven't looked into doing so.
I know very little about CMake or GitHub workflows. What I have implemented appears to work, but I don't know whether both work the way you would like. I have a Linux and Debian bias.
A reviewer of the original check asked about implicit conversions to
std::wstringandstd::string_view. I think that neither of those are possible. Is that true?make amalgamate. N/A