From 37d50eeb7fb5c6daa182158d60b8a77d9f312d23 Mon Sep 17 00:00:00 2001 From: Mike Crowe Date: Fri, 21 Feb 2025 21:02:38 +0000 Subject: [PATCH 1/3] Add clang-tidy plugin for modernize-nlohmann-json-explicit-conversions 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 https://github.com/llvm/llvm-project/pull/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 --- .reuse/dep5 | 4 + CMakeLists.txt | 9 +- LICENSES/LLVM-exception.txt | 15 + clang_tidy_plugin/CMakeLists.txt | 11 + ...zeNlohmannJsonExplicitConversionsCheck.cpp | 78 ++++ ...nizeNlohmannJsonExplicitConversionsCheck.h | 31 ++ clang_tidy_plugin/Module.cpp | 31 ++ tests/CMakeLists.txt | 4 + tests/clang_tidy_plugin/CMakeLists.txt | 10 + tests/clang_tidy_plugin/check_clang_tidy.py | 397 ++++++++++++++++++ ...ize-nlohmann-json-explicit-conversions.cpp | 318 ++++++++++++++ 11 files changed, 907 insertions(+), 1 deletion(-) create mode 100644 LICENSES/LLVM-exception.txt create mode 100644 clang_tidy_plugin/CMakeLists.txt create mode 100644 clang_tidy_plugin/ModernizeNlohmannJsonExplicitConversionsCheck.cpp create mode 100644 clang_tidy_plugin/ModernizeNlohmannJsonExplicitConversionsCheck.h create mode 100644 clang_tidy_plugin/Module.cpp create mode 100644 tests/clang_tidy_plugin/CMakeLists.txt create mode 100755 tests/clang_tidy_plugin/check_clang_tidy.py create mode 100644 tests/clang_tidy_plugin/modernize-nlohmann-json-explicit-conversions.cpp diff --git a/.reuse/dep5 b/.reuse/dep5 index 06b1d37c1b..de43739036 100644 --- a/.reuse/dep5 +++ b/.reuse/dep5 @@ -34,3 +34,7 @@ License: BSD-3-Clause Files: tools/gdb_pretty_printer/* Copyright: 2020 Hannes Domani License: MIT + +Files: tests/clang_tidy_plugin/check_clang_tidy.py +Copyright: 2003-2019 University of Illinois at Urbana-Champaign. +License: Apache-2.0 WITH LLVM-exception diff --git a/CMakeLists.txt b/CMakeLists.txt index 25ae287070..469c161c8d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,7 +4,7 @@ cmake_minimum_required(VERSION 3.1...3.14) ## PROJECT ## name and version ## -project(nlohmann_json VERSION 3.11.3 LANGUAGES CXX) +project(nlohmann_json VERSION 3.11.3 LANGUAGES C CXX) ## ## MAIN_PROJECT CHECK @@ -48,6 +48,7 @@ option(JSON_LegacyDiscardedValueComparison "Enable legacy discarded value compar option(JSON_Install "Install CMake targets during install step." ${MAIN_PROJECT}) option(JSON_MultipleHeaders "Use non-amalgamated version of the library." ON) option(JSON_SystemInclude "Include as system headers (skip for clang-tidy)." OFF) +option(JSON_ClangTidyPlugin "Build clang-tidy plug-in." OFF) if (JSON_CI) include(ci) @@ -158,6 +159,12 @@ CONFIGURE_FILE( @ONLY ) +if (JSON_ClangTidyPlugin) + find_package(LLVM REQUIRED CONFIG) + find_package(Clang REQUIRED CONFIG) + add_subdirectory(clang_tidy_plugin) +endif() + ## ## TESTS ## create and configure the unit test target diff --git a/LICENSES/LLVM-exception.txt b/LICENSES/LLVM-exception.txt new file mode 100644 index 0000000000..fa4b725a0e --- /dev/null +++ b/LICENSES/LLVM-exception.txt @@ -0,0 +1,15 @@ +---- LLVM Exceptions to the Apache 2.0 License ---- + + As an exception, if, as a result of your compiling your source code, portions + of this Software are embedded into an Object form of such source code, you + may redistribute such embedded portions in such Object form without complying + with the conditions of Sections 4(a), 4(b) and 4(d) of the License. + + In addition, if you combine or link compiled forms of this Software with + software that is licensed under the GPLv2 ("Combined Software") and if a + court of competent jurisdiction determines that the patent provision (Section + 3), the indemnity provision (Section 9) or other Section of the License + conflicts with the conditions of the GPLv2, you may retroactively and + prospectively choose to deem waived or otherwise exclude such Section(s) of + the License, but only in their entirety and only with respect to the Combined + Software. diff --git a/clang_tidy_plugin/CMakeLists.txt b/clang_tidy_plugin/CMakeLists.txt new file mode 100644 index 0000000000..9ef6e40868 --- /dev/null +++ b/clang_tidy_plugin/CMakeLists.txt @@ -0,0 +1,11 @@ +add_library(NlohmannJsonClangTidyPlugin MODULE "") +target_compile_options(NlohmannJsonClangTidyPlugin PRIVATE -fno-rtti) +target_include_directories(NlohmannJsonClangTidyPlugin + PRIVATE + ${CLANG_INCLUDE_DIRS} + ${LLVM_INCLUDE_DIRS} +) +target_sources(NlohmannJsonClangTidyPlugin PRIVATE + ${CMAKE_CURRENT_LIST_DIR}/Module.cpp + ${CMAKE_CURRENT_LIST_DIR}/ModernizeNlohmannJsonExplicitConversionsCheck.cpp +) diff --git a/clang_tidy_plugin/ModernizeNlohmannJsonExplicitConversionsCheck.cpp b/clang_tidy_plugin/ModernizeNlohmannJsonExplicitConversionsCheck.cpp new file mode 100644 index 0000000000..4bc338da1e --- /dev/null +++ b/clang_tidy_plugin/ModernizeNlohmannJsonExplicitConversionsCheck.cpp @@ -0,0 +1,78 @@ +// SPDX-FileCopyrightText: 2025 Mike Crowe +// SPDX-License-Identifier: MIT + +#include "ModernizeNlohmannJsonExplicitConversionsCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace { + void replaceAll(std::string &Str, const std::string &From, const std::string &To) { + assert(!From.empty()); + + std::string::size_type StartPos = 0; + while ((StartPos = Str.find(from, StartPos)) != std::string::npos) { + Str.replace(StartPos, From.length(), To); + StartPos += To.length(); + } + } +} + +namespace clang::tidy::modernize { + +void NlohmannJsonExplicitConversionsCheck::registerMatchers( + MatchFinder *Finder) { + auto Matcher = + cxxMemberCallExpr( + on(expr().bind("arg")), + callee(cxxConversionDecl(ofClass(hasName("nlohmann::basic_json"))) + .bind("conversionDecl"))) + .bind("conversionCall"); + Finder->addMatcher(Matcher, this); +} + +void NlohmannJsonExplicitConversionsCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Decl = + Result.Nodes.getNodeAs("conversionDecl"); + const auto *Call = + Result.Nodes.getNodeAs("conversionCall"); + const auto *Arg = Result.Nodes.getNodeAs("arg"); + + const QualType DestinationType = Decl->getConversionType(); + std::string DestinationTypeStr = + DestinationType.getAsString(Result.Context->getPrintingPolicy()); + replaceAll(DestinationTypeStr, "std::basic_string", "std::string"); + + const SourceRange ExprRange = Call->getSourceRange(); + if (!ExprRange.isValid()) + return; + + bool Deref = false; + if (const auto *Op = llvm::dyn_cast(Arg); + Op && Op->getOpcode() == UO_Deref) + Deref = true; + else if (const auto *Op = llvm::dyn_cast(Arg); + Op && Op->getOperator() == OO_Star) + Deref = true; + + llvm::StringRef SourceText = clang::Lexer::getSourceText( + clang::CharSourceRange::getTokenRange(ExprRange), *Result.SourceManager, + Result.Context->getLangOpts()); + + if (Deref) + SourceText.consume_front("*"); + + const std::string ReplacementText = + (llvm::Twine(SourceText) + (Deref ? "->" : ".") + "get<" + + DestinationTypeStr + ">()") + .str(); + diag(Call->getExprLoc(), + "implicit nlohmann::json conversion to %0 should be explicit") + << DestinationTypeStr + << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(ExprRange), + ReplacementText); +} + +} // namespace clang::tidy::modernize diff --git a/clang_tidy_plugin/ModernizeNlohmannJsonExplicitConversionsCheck.h b/clang_tidy_plugin/ModernizeNlohmannJsonExplicitConversionsCheck.h new file mode 100644 index 0000000000..11eb482e5e --- /dev/null +++ b/clang_tidy_plugin/ModernizeNlohmannJsonExplicitConversionsCheck.h @@ -0,0 +1,31 @@ +// SPDX-FileCopyrightText: 2025 Mike Crowe +// SPDX-License-Identifier: MIT + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_NLOHMANNJSONEXPLICITCONVERSIONSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_NLOHMANNJSONEXPLICITCONVERSIONSCHECK_H + +#include "clang-tidy/ClangTidyCheck.h" + +namespace clang::tidy::modernize { + +/// Convert implicit conversions via operator ValueType() from nlohmann::json to +/// explicit calls to the get<> method because the next major version of the +/// library will remove support for implicit conversions. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/modernize/nlohmann-json-explicit-conversions.html +class NlohmannJsonExplicitConversionsCheck : public ClangTidyCheck { +public: + NlohmannJsonExplicitConversionsCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } +}; + +} // namespace clang::tidy::modernize + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_NLOHMANNJSONEXPLICITCONVERSIONSCHECK_H diff --git a/clang_tidy_plugin/Module.cpp b/clang_tidy_plugin/Module.cpp new file mode 100644 index 0000000000..1ec0d852ae --- /dev/null +++ b/clang_tidy_plugin/Module.cpp @@ -0,0 +1,31 @@ +// SPDX-FileCopyrightText: 2025 Mike Crowe +// SPDX-License-Identifier: MIT + +#include "clang-tidy/ClangTidyModule.h" +#include "clang-tidy/ClangTidyModuleRegistry.h" +#include "ModernizeNlohmannJsonExplicitConversionsCheck.h" + +namespace { + + using namespace clang::tidy; + +class NlohmannJsonChecksModule : public ClangTidyModule +{ +public: + void addCheckFactories(ClangTidyCheckFactories& CheckFactories) override + { + CheckFactories.registerCheck( + "modernize-nlohmann-json-explicit-conversions"); + } +}; + +} // namespace + +namespace clang::tidy { + +// Register the module using this statically initialized variable. + static ClangTidyModuleRegistry::Add<::NlohmannJsonChecksModule> nlohmannJsonChecksInit( + "nlohmann-json-checks-module", + "Adds 'modernize-nlohmann-json-explicit-conversions' check."); + +} // namespace clang::tidy diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a153a69248..47ee19086c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -168,6 +168,10 @@ json_test_add_test_for(src/unit-comparison.cpp add_subdirectory(abi) +if (JSON_ClangTidyPlugin) + add_subdirectory(clang_tidy_plugin) +endif() + ############################################################################# # Test the generated build configs ############################################################################# diff --git a/tests/clang_tidy_plugin/CMakeLists.txt b/tests/clang_tidy_plugin/CMakeLists.txt new file mode 100644 index 0000000000..330153a2c3 --- /dev/null +++ b/tests/clang_tidy_plugin/CMakeLists.txt @@ -0,0 +1,10 @@ +include(CTest) + +if(NOT LLVM_TOOLS_BINARY_DIR) + message(FATAL_ERROR "Could not determine LLVM binary directory") +endif() + +add_test(NAME modernize-nlohmann-json-explicit-conversions-check COMMAND ${CMAKE_COMMAND} -E env PATH=${LLVM_TOOLS_BINARY_DIR}:$ENV{PATH} + ${CMAKE_CURRENT_LIST_DIR}/check_clang_tidy.py -std=c++17 + -plugin=$ + ${CMAKE_CURRENT_LIST_DIR}/modernize-nlohmann-json-explicit-conversions.cpp modernize-nlohmann-json-explicit-conversions temp) diff --git a/tests/clang_tidy_plugin/check_clang_tidy.py b/tests/clang_tidy_plugin/check_clang_tidy.py new file mode 100755 index 0000000000..f5f6f17afb --- /dev/null +++ b/tests/clang_tidy_plugin/check_clang_tidy.py @@ -0,0 +1,397 @@ +#!/usr/bin/env python3 +# +# ===- check_clang_tidy.py - ClangTidy Test Helper ------------*- python -*--===# +# +# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +# See https://llvm.org/LICENSE.txt for license information. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +# +# ===------------------------------------------------------------------------===# +# +# This file was taken from llvm-project and tweaked very slightly to +# add the ability to load a plugin so that it can be used for testing +# clang-tidy plugins. + +""" +ClangTidy Test Helper +===================== + +This script is used to simplify writing, running, and debugging tests compatible +with llvm-lit. By default it runs clang-tidy in fix mode and uses FileCheck to +verify messages and/or fixes. + +For debugging, with --export-fixes, the tool simply exports fixes to a provided +file and does not run FileCheck. + +Extra arguments, those after the first -- if any, are passed to either +clang-tidy or clang: +* Arguments between the first -- and second -- are clang-tidy arguments. + * May be only whitespace if there are no clang-tidy arguments. + * clang-tidy's --config would go here. +* Arguments after the second -- are clang arguments + +Examples +-------- + + // RUN: %check_clang_tidy %s llvm-include-order %t -- -- -isystem %S/Inputs + +or + + // RUN: %check_clang_tidy %s llvm-include-order --export-fixes=fixes.yaml %t -std=c++20 + +Notes +----- + -std=c++(98|11|14|17|20)-or-later: + This flag will cause multiple runs within the same check_clang_tidy + execution. Make sure you don't have shared state across these runs. +""" + +import argparse +import os +import pathlib +import re +import subprocess +import sys + + +def write_file(file_name, text): + with open(file_name, "w", encoding="utf-8") as f: + f.write(text) + f.truncate() + + +def try_run(args, raise_error=True): + try: + process_output = subprocess.check_output(args, stderr=subprocess.STDOUT).decode( + errors="ignore" + ) + except subprocess.CalledProcessError as e: + process_output = e.output.decode(errors="ignore") + print("%s failed:\n%s" % (" ".join(args), process_output)) + if raise_error: + raise + return process_output + + +# This class represents the appearance of a message prefix in a file. +class MessagePrefix: + def __init__(self, label): + self.has_message = False + self.prefixes = [] + self.label = label + + def check(self, file_check_suffix, input_text): + self.prefix = self.label + file_check_suffix + self.has_message = self.prefix in input_text + if self.has_message: + self.prefixes.append(self.prefix) + return self.has_message + + +class CheckRunner: + def __init__(self, args, extra_args): + self.resource_dir = args.resource_dir + self.assume_file_name = args.assume_filename + self.input_file_name = args.input_file_name + self.check_name = args.check_name + self.temp_file_name = args.temp_file_name + self.original_file_name = self.temp_file_name + ".orig" + self.expect_clang_tidy_error = args.expect_clang_tidy_error + self.std = args.std + self.plugin = args.plugin + self.check_suffix = args.check_suffix + self.input_text = "" + self.has_check_fixes = False + self.has_check_messages = False + self.has_check_notes = False + self.expect_no_diagnosis = False + self.export_fixes = args.export_fixes + self.fixes = MessagePrefix("CHECK-FIXES") + self.messages = MessagePrefix("CHECK-MESSAGES") + self.notes = MessagePrefix("CHECK-NOTES") + + file_name_with_extension = self.assume_file_name or self.input_file_name + _, extension = os.path.splitext(file_name_with_extension) + if extension not in [".c", ".hpp", ".m", ".mm"]: + extension = ".cpp" + self.temp_file_name = self.temp_file_name + extension + + self.clang_extra_args = [] + self.clang_tidy_extra_args = extra_args + if "--" in extra_args: + i = self.clang_tidy_extra_args.index("--") + self.clang_extra_args = self.clang_tidy_extra_args[i + 1 :] + self.clang_tidy_extra_args = self.clang_tidy_extra_args[:i] + + # If the test does not specify a config style, force an empty one; otherwise + # auto-detection logic can discover a ".clang-tidy" file that is not related to + # the test. + if not any( + [re.match("^-?-config(-file)?=", arg) for arg in self.clang_tidy_extra_args] + ): + self.clang_tidy_extra_args.append("--config={}") + + if extension in [".m", ".mm"]: + self.clang_extra_args = [ + "-fobjc-abi-version=2", + "-fobjc-arc", + "-fblocks", + ] + self.clang_extra_args + + if extension in [".cpp", ".hpp", ".mm"]: + self.clang_extra_args.append("-std=" + self.std) + + # Tests should not rely on STL being available, and instead provide mock + # implementations of relevant APIs. + self.clang_extra_args.append("-nostdinc++") + + if self.resource_dir is not None: + self.clang_extra_args.append("-resource-dir=%s" % self.resource_dir) + + def read_input(self): + with open(self.input_file_name, "r", encoding="utf-8") as input_file: + self.input_text = input_file.read() + + def get_prefixes(self): + for suffix in self.check_suffix: + if suffix and not re.match("^[A-Z0-9\\-]+$", suffix): + sys.exit( + 'Only A..Z, 0..9 and "-" are allowed in check suffixes list,' + + ' but "%s" was given' % suffix + ) + + file_check_suffix = ("-" + suffix) if suffix else "" + + has_check_fix = self.fixes.check(file_check_suffix, self.input_text) + self.has_check_fixes = self.has_check_fixes or has_check_fix + + has_check_message = self.messages.check(file_check_suffix, self.input_text) + self.has_check_messages = self.has_check_messages or has_check_message + + has_check_note = self.notes.check(file_check_suffix, self.input_text) + self.has_check_notes = self.has_check_notes or has_check_note + + if has_check_note and has_check_message: + sys.exit( + "Please use either %s or %s but not both" + % (self.notes.prefix, self.messages.prefix) + ) + + if not has_check_fix and not has_check_message and not has_check_note: + self.expect_no_diagnosis = True + + expect_diagnosis = ( + self.has_check_fixes or self.has_check_messages or self.has_check_notes + ) + if self.expect_no_diagnosis and expect_diagnosis: + sys.exit( + "%s, %s or %s not found in the input" + % ( + self.fixes.prefix, + self.messages.prefix, + self.notes.prefix, + ) + ) + assert expect_diagnosis or self.expect_no_diagnosis + + def prepare_test_inputs(self): + # Remove the contents of the CHECK lines to avoid CHECKs matching on + # themselves. We need to keep the comments to preserve line numbers while + # avoiding empty lines which could potentially trigger formatting-related + # checks. + cleaned_test = re.sub("// *CHECK-[A-Z0-9\\-]*:[^\r\n]*", "//", self.input_text) + write_file(self.temp_file_name, cleaned_test) + write_file(self.original_file_name, cleaned_test) + + def run_clang_tidy(self): + args = ( + [ + "clang-tidy", + self.temp_file_name, + ] + + [ + ( + "-fix" + if self.export_fixes is None + else "--export-fixes=" + self.export_fixes + ) + ] + + ([ "--load=%s" % self.plugin ] if self.plugin else []) + + [ + "--checks=-*," + self.check_name, + ] + + self.clang_tidy_extra_args + + ["--"] + + self.clang_extra_args + ) + if self.expect_clang_tidy_error: + args.insert(0, "not") + print("Running " + repr(args) + "...") + clang_tidy_output = try_run(args) + print("------------------------ clang-tidy output -----------------------") + print( + clang_tidy_output.encode(sys.stdout.encoding, errors="replace").decode( + sys.stdout.encoding + ) + ) + print("------------------------------------------------------------------") + + diff_output = try_run( + ["diff", "-u", self.original_file_name, self.temp_file_name], False + ) + print("------------------------------ Fixes -----------------------------") + print(diff_output) + print("------------------------------------------------------------------") + return clang_tidy_output + + def check_no_diagnosis(self, clang_tidy_output): + if clang_tidy_output != "": + sys.exit("No diagnostics were expected, but found the ones above") + + def check_fixes(self): + if self.has_check_fixes: + try_run( + [ + "FileCheck", + "-input-file=" + self.temp_file_name, + self.input_file_name, + "-check-prefixes=" + ",".join(self.fixes.prefixes), + "-strict-whitespace", + ] + ) + + def check_messages(self, clang_tidy_output): + if self.has_check_messages: + messages_file = self.temp_file_name + ".msg" + write_file(messages_file, clang_tidy_output) + try_run( + [ + "FileCheck", + "-input-file=" + messages_file, + self.input_file_name, + "-check-prefixes=" + ",".join(self.messages.prefixes), + "-implicit-check-not={{warning|error}}:", + ] + ) + + def check_notes(self, clang_tidy_output): + if self.has_check_notes: + notes_file = self.temp_file_name + ".notes" + filtered_output = [ + line + for line in clang_tidy_output.splitlines() + if not ("note: FIX-IT applied" in line) + ] + write_file(notes_file, "\n".join(filtered_output)) + try_run( + [ + "FileCheck", + "-input-file=" + notes_file, + self.input_file_name, + "-check-prefixes=" + ",".join(self.notes.prefixes), + "-implicit-check-not={{note|warning|error}}:", + ] + ) + + def run(self): + self.read_input() + if self.export_fixes is None: + self.get_prefixes() + self.prepare_test_inputs() + clang_tidy_output = self.run_clang_tidy() + if self.expect_no_diagnosis: + self.check_no_diagnosis(clang_tidy_output) + elif self.export_fixes is None: + self.check_fixes() + self.check_messages(clang_tidy_output) + self.check_notes(clang_tidy_output) + + +CPP_STANDARDS = [ + "c++98", + "c++11", + ("c++14", "c++1y"), + ("c++17", "c++1z"), + ("c++20", "c++2a"), + ("c++23", "c++2b"), + ("c++26", "c++2c"), +] +C_STANDARDS = ["c99", ("c11", "c1x"), "c17", ("c23", "c2x"), "c2y"] + + +def expand_std(std): + split_std, or_later, _ = std.partition("-or-later") + + if not or_later: + return [split_std] + + for standard_list in (CPP_STANDARDS, C_STANDARDS): + item = next( + ( + i + for i, v in enumerate(standard_list) + if (split_std in v if isinstance(v, (list, tuple)) else split_std == v) + ), + None, + ) + if item is not None: + return [split_std] + [ + x if isinstance(x, str) else x[0] for x in standard_list[item + 1 :] + ] + return [std] + + +def csv(string): + return string.split(",") + + +def parse_arguments(): + parser = argparse.ArgumentParser( + prog=pathlib.Path(__file__).stem, + description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + parser.add_argument("-expect-clang-tidy-error", action="store_true") + parser.add_argument("-resource-dir") + parser.add_argument("-assume-filename") + parser.add_argument("input_file_name") + parser.add_argument("check_name") + parser.add_argument("temp_file_name") + parser.add_argument( + "-check-suffix", + "-check-suffixes", + default=[""], + type=csv, + help="comma-separated list of FileCheck suffixes", + ) + parser.add_argument( + "-export-fixes", + default=None, + type=str, + metavar="file", + help="A file to export fixes into instead of fixing.", + ) + parser.add_argument( + "-std", + type=csv, + default=["c++11-or-later"], + help="Passed to clang. Special -or-later values are expanded.", + ) + parser.add_argument( + "-plugin", + ) + return parser.parse_known_args() + + +def main(): + args, extra_args = parse_arguments() + + abbreviated_stds = args.std + for abbreviated_std in abbreviated_stds: + for std in expand_std(abbreviated_std): + args.std = std + CheckRunner(args, extra_args).run() + + +if __name__ == "__main__": + main() diff --git a/tests/clang_tidy_plugin/modernize-nlohmann-json-explicit-conversions.cpp b/tests/clang_tidy_plugin/modernize-nlohmann-json-explicit-conversions.cpp new file mode 100644 index 0000000000..4533f6da34 --- /dev/null +++ b/tests/clang_tidy_plugin/modernize-nlohmann-json-explicit-conversions.cpp @@ -0,0 +1,318 @@ +// SPDX-FileCopyrightText: 2025 Mike Crowe +// SPDX-License-Identifier: MIT + +// The test is limited to C++17 because the version of +// check_clang_tidy.py here would like to test with -std=c++23, but +// Clang 16 doesn't support that. + +// RUN: %check_clang_tidy -std=c++17 %s modernize-nlohmann-json-explicit-conversions %t -- -- -isystem %clang_tidy_headers + +typedef __PTRDIFF_TYPE__ ptrdiff_t; +typedef __SIZE_TYPE__ size_t; + +typedef unsigned __INT16_TYPE__ char16; +typedef unsigned __INT32_TYPE__ char32; + +namespace std { +template +class allocator {}; + +template +class char_traits {}; + +template > +struct basic_string_view; + +template , typename A = allocator> +struct basic_string { + typedef size_t size_type; + typedef basic_string _Type; + basic_string(); + basic_string(const C *p, const A &a = A()); + basic_string(const C *p, size_type count); + basic_string(const C *b, const C *e); + + ~basic_string(); + + const C *c_str() const; + const C *data() const; + + bool empty() const; + size_type size() const; + size_type length() const; + + _Type& append(const C *s); + _Type& append(const C *s, size_type n); + _Type& assign(const C *s); + _Type& assign(const C *s, size_type n); + + int compare(const _Type&) const; + int compare(const C* s) const; + int compare(size_type pos, size_type len, const _Type&) const; + int compare(size_type pos, size_type len, const C* s) const; + template + int compare(size_type pos1, size_type count1, const StringViewLike& t) const; + + size_type find(const _Type& str, size_type pos = 0) const; + size_type find(const C* s, size_type pos = 0) const; + size_type find(const C* s, size_type pos, size_type n) const; + + size_type rfind(const _Type& str, size_type pos = npos) const; + size_type rfind(const C* s, size_type pos, size_type count) const; + size_type rfind(const C* s, size_type pos = npos) const; + size_type rfind(C ch, size_type pos = npos) const; + + _Type& insert(size_type pos, const _Type& str); + _Type& insert(size_type pos, const C* s); + _Type& insert(size_type pos, const C* s, size_type n); + + _Type substr(size_type pos = 0, size_type count = npos) const; + + constexpr bool starts_with(std::basic_string_view sv) const noexcept; + constexpr bool starts_with(C ch) const noexcept; + constexpr bool starts_with(const C* s) const; + + constexpr bool ends_with(std::basic_string_view sv) const noexcept; + constexpr bool ends_with(C ch) const noexcept; + constexpr bool ends_with(const C* s) const; + + _Type& operator[](size_type); + const _Type& operator[](size_type) const; + + _Type& operator+=(const _Type& str); + _Type& operator+=(const C* s); + _Type& operator=(const _Type& str); + _Type& operator=(const C* s); + + static constexpr size_t npos = -1; +}; + +typedef basic_string string; +typedef basic_string wstring; +typedef basic_string u16string; +typedef basic_string u32string; + +template +struct basic_string_view { + typedef size_t size_type; + typedef basic_string_view _Type; + + const C *str; + constexpr basic_string_view(const C* s) : str(s) {} + + const C *data() const; + + bool empty() const; + size_type size() const; + size_type length() const; + + size_type find(_Type v, size_type pos = 0) const; + size_type find(C ch, size_type pos = 0) const; + size_type find(const C* s, size_type pos, size_type count) const; + size_type find(const C* s, size_type pos = 0) const; + + size_type rfind(_Type v, size_type pos = npos) const; + size_type rfind(C ch, size_type pos = npos) const; + size_type rfind(const C* s, size_type pos, size_type count) const; + size_type rfind(const C* s, size_type pos = npos) const; + + constexpr bool starts_with(basic_string_view sv) const noexcept; + constexpr bool starts_with(C ch) const noexcept; + constexpr bool starts_with(const C* s) const; + + constexpr bool ends_with(basic_string_view sv) const noexcept; + constexpr bool ends_with(C ch) const noexcept; + constexpr bool ends_with(const C* s) const; + + constexpr int compare(basic_string_view sv) const noexcept; + + static constexpr size_t npos = -1; +}; + +typedef basic_string_view string_view; +typedef basic_string_view wstring_view; +typedef basic_string_view u16string_view; +typedef basic_string_view u32string_view; + +std::string operator+(const std::string&, const std::string&); +std::string operator+(const std::string&, const char*); +std::string operator+(const char*, const std::string&); + +bool operator==(const std::string&, const std::string&); +bool operator==(const std::string&, const char*); +bool operator==(const char*, const std::string&); + +bool operator==(const std::wstring&, const std::wstring&); +bool operator==(const std::wstring&, const wchar_t*); +bool operator==(const wchar_t*, const std::wstring&); + +bool operator==(const std::string_view&, const std::string_view&); +bool operator==(const std::string_view&, const char*); +bool operator==(const char*, const std::string_view&); + +#if __cplusplus < 202002L +bool operator!=(const std::string&, const std::string&); +bool operator!=(const std::string&, const char*); +bool operator!=(const char*, const std::string&); + +bool operator!=(const std::string_view&, const std::string_view&); +bool operator!=(const std::string_view&, const char*); +bool operator!=(const char*, const std::string_view&); +#endif + +size_t strlen(const char* str); + +template +class vector +{ +}; + +} + +struct MyStruct { + int i1; + int i2; +}; + +namespace nlohmann +{ + class basic_json + { + public: + template + ValueType get() const + { + return ValueType{}; + } + + // nlohmann::json uses SFINAE to limit the types that can be converted to. + // Rather than do that here, let's just provide the overloads we need + // instead. + operator int() const + { + return get(); + } + + operator double() const + { + return get(); + } + + operator std::string() const + { + return get(); + } + + operator MyStruct() const + { + return get(); + } + + operator std::vector() const + { + return get>(); + } + + int otherMember() const; + }; + + class iterator + { + public: + basic_json &operator*(); + basic_json *operator->(); + }; + + using json = basic_json; +} + +using nlohmann::json; +using nlohmann::iterator; + +int get_int(json &j) +{ + return j; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to int should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return j.get(); +} + +std::string get_string(json &j) +{ + return j; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to std::string should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return j.get(); +} + +std::vector get_string_vector(json &j) +{ + return j; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to std::vector should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return j.get>(); +} + +MyStruct get_struct(json &j) +{ + return j; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to MyStruct should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return j.get(); +} + +int get_int_ptr(json *j) +{ + return *j; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to int should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return j->get(); +} + +int get_int_ptr_expr(json *j) +{ + return *(j+1); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to int should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return (j+1)->get(); +} + +int get_int_iterator(iterator i) +{ + return *i; + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to int should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return i->get(); +} + +int get_int_fn() +{ + extern json get_json(); + return get_json(); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to int should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return get_json().get(); +} + +double get_double_fn_ref() +{ + extern nlohmann::json &get_json_ref(); + return get_json_ref(); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to double should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return get_json_ref().get(); +} + +std::string get_string_fn_ptr() +{ + extern json *get_json_ptr(); + return *get_json_ptr(); + // CHECK-MESSAGES: [[@LINE-1]]:10: warning: implicit nlohmann::json conversion to std::string should be explicit [modernize-nlohmann-json-explicit-conversions] + // CHECK-FIXES: return get_json_ptr()->get(); +} + +int call_other_member(nlohmann::json &j) +{ + return j.otherMember(); +} + +int call_other_member_ptr(nlohmann::json *j) +{ + return j->otherMember(); +} + +int call_other_member_iterator(iterator i) +{ + return i->otherMember(); +} From e93ca82123ff4271954c0847e984733c61f17d4b Mon Sep 17 00:00:00 2001 From: Mike Crowe Date: Sat, 22 Feb 2025 15:33:46 +0000 Subject: [PATCH 2/3] ci: Test clang-tidy plugin 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 --- .github/workflows/clang-tidy-plugin.yml | 60 +++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 .github/workflows/clang-tidy-plugin.yml diff --git a/.github/workflows/clang-tidy-plugin.yml b/.github/workflows/clang-tidy-plugin.yml new file mode 100644 index 0000000000..5faf6738ea --- /dev/null +++ b/.github/workflows/clang-tidy-plugin.yml @@ -0,0 +1,60 @@ +name: Ubuntu + +on: + push: + branches: + - develop + - master + - release/* + pull_request: + workflow_dispatch: + +permissions: + contents: read + +jobs: + ci_test_clang_tidy: + runs-on: ubuntu-latest + strategy: + matrix: + # clang-tidy plugins require at least Clang 16. This means that + # anything older than Ubuntu 24.04 and Debian 12 can't work at all. + include: + - distro: ubuntu:24.04 + clang_version: 16 + - distro: ubuntu:24.04 + clang_version: 17 + - distro: ubuntu:24.04 + clang_version: 18 + - distro: debian:12 + clang_version: 16 + - distro: debian:12 + clang_version: 19 + - distro: debian:trixie + clang_version: 17 + - distro: debian:trixie + clang_version: 18 + - distro: debian:trixie + clang_version: 19 + container: ${{ matrix.distro }} + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - name: List files + run: ls -l + - name: Install packages + run: >- + echo distro=${{matrix.distro}} && + echo clang_version=${{matrix.clang_version}} && + apt-get update && + apt-get install -y + build-essential + clang-${{ matrix.clang_version }} + clang-tidy-${{ matrix.clang_version }} + libclang-${{matrix.clang_version}}-dev + cmake ninja-build + - name: Run CMake + run: cmake -S . -B build -G Ninja -DJSON_CI=On -DJSON_ClangTidyPlugin=ON + - 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 From d7ddfb1469129721da279563dd58d919a78d3612 Mon Sep 17 00:00:00 2001 From: Mike Crowe Date: Sun, 23 Feb 2025 17:30:32 +0000 Subject: [PATCH 3/3] Add documentation for clang-tidy plugin Signed-off-by: Mike Crowe --- .../macros/json_use_implicit_conversions.md | 2 + docs/mkdocs/docs/integration/clang-tidy.md | 113 ++++++++++++++++++ docs/mkdocs/docs/integration/cmake.md | 4 + docs/mkdocs/mkdocs.yml | 1 + 4 files changed, 120 insertions(+) create mode 100644 docs/mkdocs/docs/integration/clang-tidy.md diff --git a/docs/mkdocs/docs/api/macros/json_use_implicit_conversions.md b/docs/mkdocs/docs/api/macros/json_use_implicit_conversions.md index 0a5ad4df46..3fe684c667 100644 --- a/docs/mkdocs/docs/api/macros/json_use_implicit_conversions.md +++ b/docs/mkdocs/docs/api/macros/json_use_implicit_conversions.md @@ -23,6 +23,8 @@ By default, implicit conversions are enabled. You can prepare existing code by already defining `JSON_USE_IMPLICIT_CONVERSIONS` to `0` and replace any implicit conversions with calls to [`get`](../basic_json/get.md). + The [clang-tidy modernize-nlohmann-json-explicit-conversions](../../integration/clang-tidy.md#modernize-nlohmann-json-explicit-conversions) + check can help to do this automatically. !!! hint "CMake option" diff --git a/docs/mkdocs/docs/integration/clang-tidy.md b/docs/mkdocs/docs/integration/clang-tidy.md new file mode 100644 index 0000000000..3381f83be6 --- /dev/null +++ b/docs/mkdocs/docs/integration/clang-tidy.md @@ -0,0 +1,113 @@ +# Clang-Tidy + +The library comes with a [clang-tidy](https://clang.llvm.org/extra/clang-tidy/) plugin. +It is disabled by default but can be enabled by enabling the `JSON_ClangTidyPlugin` [CMake option](cmake.md#json_clangtidyplugin). +Clang-tidy [Plugins](https://clang.llvm.org/extra/clang-tidy/ExternalClang-TidyExamples.html) are only supported by Clang 16 and later. + +## Building the plugin + +You will need to have the development files matching your version of clang-tidy installed to build the plugin. +For example, if you are running on a Debian-derived Linux distribution: + +```sh +apt install clang-tidy libclang-dev +``` +but if this installs a version that is older than Clang 16 then you might be able to specify a newer version. For example: +```sh +apt install clang-tidy-19 libclang-19-dev +``` + +```sh +mkdir build +cd build +cmake -DJSON_ClangTidyPlugin=ON .. +cmake -build . +``` + +# Running the plugin +To tell clang-tidy to use the plugin you must pass a path to it as an argument to the `-load` option. +For example, you can run clang-tidy with only the _modernize-nlohmann-json-explicit-conversion_ check using the plugin on a single file with: +```sh +clang-tidy -load .../path/to/build/clang_tidy_plugin/libNlohmannJsonClangTidyPlugin.so -checks='-*,modernize-nlohmann-json-explicit-conversions` -fix source.cpp +clang-tidy +``` +or you can create a `.clang-tidy` file to enable the checks you require. + +# Checks + +At the moment the plugin contains only a single check. + +## modernize-nlohmann-json-explicit-conversions + +This check converts code that takes advantage of [implicit conversions](../api/macros/json_use_implicit_conversions.md) to use explicit `get()` calls using the correct templated type. +For example, it turns: +```c++ +void f(const nlohmann::json &j1, const nlohmann::json &j2) +{ + int i = j1; + double d = j2.at("value"); + bool b = *j2.find("valid"); + std::cout << i << " " << d << " " << b << "\n"; +} +``` +into +```c++ +void f(const nlohmann::json &j1, const nlohmann::json &j2) +{ + int i = j1.get(); + double d = j2.at("value").get(); + bool b = j2.find("valid")->get(); + std::cout << i << " " << d << " " << b << "\n"; +} +``` +by knowing what the target type is for the implicit conversion and turning +that into an explicit call to the `get()` method with that type as the +template parameter. + +Unfortunately the check does not work very well if the implicit conversion +occurs in templated code or in a system header. For example, the following +won't be fixed because the implicit conversion will happen inside +`std::optional`'s constructor: +```c++ +void f(const nlohmann::json &j) +{ + std::optional oi; + const auto &it = j.find("value"); + if (it != j.end()) + oi = *it; + // ... +} +``` +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. + +### Limitations + +#### Typedefs may leak platform specifics + +The check sees through `typedef` which can cause types that ought to vary by platform to become fixed by the check. This is particularly a problem when using the fixed-sized types from `` or other platform-specific types like `size_t` and `off_t`. For example, converting: +```c++ +uint64_t u64 = j.at("value"); +``` +in a 64-bit Linux environment will result in: +```c++ +uint64_t u64 = j.at("value").get(); +``` +but in a 32-bit Linux environment will result in: +```c++ +uint64_t u64 = j.at("value").get(); +``` + +This could cause serious bugs. If you use such types in code you apply the check to it is recommended that you run on all your target platforms and compare the results. + +Other more-benign uses of `typedef` types may also cause the resultant code to look uglier. The check automatically turns `std::basic_string` back into `std::string` since that is so common. + +#### Redundant casts + +Code using the library may use casts to force the correct type. Such code will be fixed but the unnecessary cast will remain, which could cause confusion and be brittle when the code is changed. For example, converting: +```c++ +const auto value = static_cast(j["value"]) +``` +will yield: +``` +const auto value = static_cast(j["value"].get()) +``` diff --git a/docs/mkdocs/docs/integration/cmake.md b/docs/mkdocs/docs/integration/cmake.md index 69c8030f1f..17bc39cd10 100644 --- a/docs/mkdocs/docs/integration/cmake.md +++ b/docs/mkdocs/docs/integration/cmake.md @@ -131,6 +131,10 @@ Build the unit tests when [`BUILD_TESTING`](https://cmake.org/cmake/help/latest/ Enable CI build targets. The exact targets are used during the several CI steps and are subject to change without notice. This option is `OFF` by default. +### `JSON_ClangTidyPlugin` + +Enable building the [clang-tidy plugin](clang-tidy.md). This option is `OFF` by default. + ### `JSON_Diagnostics` Enable [extended diagnostic messages](../home/exceptions.md#extended-diagnostic-messages) by defining macro [`JSON_DIAGNOSTICS`](../api/macros/json_diagnostics.md). This option is `OFF` by default. diff --git a/docs/mkdocs/mkdocs.yml b/docs/mkdocs/mkdocs.yml index 7a89219fbc..b0254943e3 100644 --- a/docs/mkdocs/mkdocs.yml +++ b/docs/mkdocs/mkdocs.yml @@ -93,6 +93,7 @@ nav: - Integration: - integration/index.md - integration/migration_guide.md + - integration/clang-tidy.md - integration/cmake.md - integration/package_managers.md - integration/pkg-config.md