[clang-tidy] add depercation warning for non-whitelisted global options

[HerrCai0907]

We plan to depercate StrictMode and IgnoreMacros global options after 2 major versions and support local options only for them.
This patch introduces the depercation warning.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Congcong Cai (HerrCai0907)

Changes

We plan to depercate StrictMode and IgnoreMacros global options after 2 major versions and support local options only for them.
This patch introduces the depercation warning.

---

Full diff: https://github.com/llvm/llvm-project/pull/121057.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/ClangTidyCheck.cpp (+17-11)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-fmt.cpp (+1-1)
• (added) clang-tools-extra/test/clang-tidy/infrastructure/deprecation-global-option.cpp (+3)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
index 6028bb2258136b..6e190101b220cf 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
@@ -7,11 +7,11 @@
 //===----------------------------------------------------------------------===//
 
 #include "ClangTidyCheck.h"
-#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Error.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/YAMLParser.h"
 #include <optional>
+#include <string>
 
 namespace clang::tidy {
 
@@ -62,16 +62,25 @@ ClangTidyCheck::OptionsView::get(StringRef LocalName) const {
   return std::nullopt;
 }
 
+static const llvm::StringSet<> AllowedGlobalOption{"IncludeStyle"};
+
 static ClangTidyOptions::OptionMap::const_iterator
 findPriorityOption(const ClangTidyOptions::OptionMap &Options,
                    StringRef NamePrefix, StringRef LocalName,
-                   llvm::StringSet<> *Collector) {
+                   ClangTidyContext *Context) {
+  llvm::StringSet<> *Collector = Context->getOptionsCollector();
   if (Collector) {
     Collector->insert((NamePrefix + LocalName).str());
     Collector->insert(LocalName);
   }
   auto IterLocal = Options.find((NamePrefix + LocalName).str());
   auto IterGlobal = Options.find(LocalName);
+  // FIXME: temporary solution for deprecation warnings, should be removed
+  // after 22.x.
+  if (IterGlobal != Options.end() && !AllowedGlobalOption.contains(LocalName))
+    Context->configurationDiag(
+        "deprecation global option '%0', please use '%1%0'.")
+        << LocalName << NamePrefix;
   if (IterLocal == Options.end())
     return IterGlobal;
   if (IterGlobal == Options.end())
@@ -83,8 +92,7 @@ findPriorityOption(const ClangTidyOptions::OptionMap &Options,
 
 std::optional<StringRef>
 ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName) const {
-  auto Iter = findPriorityOption(CheckOptions, NamePrefix, LocalName,
-                                 Context->getOptionsCollector());
+  auto Iter = findPriorityOption(CheckOptions, NamePrefix, LocalName, Context);
   if (Iter != CheckOptions.end())
     return StringRef(Iter->getValue().Value);
   return std::nullopt;
@@ -117,8 +125,7 @@ ClangTidyCheck::OptionsView::get<bool>(StringRef LocalName) const {
 template <>
 std::optional<bool>
 ClangTidyCheck::OptionsView::getLocalOrGlobal<bool>(StringRef LocalName) const {
-  auto Iter = findPriorityOption(CheckOptions, NamePrefix, LocalName,
-                                 Context->getOptionsCollector());
+  auto Iter = findPriorityOption(CheckOptions, NamePrefix, LocalName, Context);
   if (Iter != CheckOptions.end()) {
     if (auto Result = getAsBool(Iter->getValue().Value, Iter->getKey()))
       return Result;
@@ -157,10 +164,9 @@ std::optional<int64_t> ClangTidyCheck::OptionsView::getEnumInt(
     bool IgnoreCase) const {
   if (!CheckGlobal && Context->getOptionsCollector())
     Context->getOptionsCollector()->insert((NamePrefix + LocalName).str());
-  auto Iter = CheckGlobal
-                  ? findPriorityOption(CheckOptions, NamePrefix, LocalName,
-                                       Context->getOptionsCollector())
-                  : CheckOptions.find((NamePrefix + LocalName).str());
+  auto Iter = CheckGlobal ? findPriorityOption(CheckOptions, NamePrefix,
+                                               LocalName, Context)
+                          : CheckOptions.find((NamePrefix + LocalName).str());
   if (Iter == CheckOptions.end())
     return std::nullopt;
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-fmt.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-fmt.cpp
index 1eaf18ac119966..71c8af190467cf 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-fmt.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format-fmt.cpp
@@ -1,6 +1,6 @@
 // RUN: %check_clang_tidy %s modernize-use-std-format %t -- \
 // RUN:   -config="{CheckOptions: { \
-// RUN:              StrictMode: true, \
+// RUN:              modernize-use-std-format.StrictMode: true, \
 // RUN:              modernize-use-std-format.StrFormatLikeFunctions: 'fmt::sprintf', \
 // RUN:              modernize-use-std-format.ReplacementFormatFunction: 'fmt::format', \
 // RUN:              modernize-use-std-format.FormatHeader: '<fmt/core.h>' \
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/deprecation-global-option.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/deprecation-global-option.cpp
new file mode 100644
index 00000000000000..35adc2511f869b
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/deprecation-global-option.cpp
@@ -0,0 +1,3 @@
+// RUN: clang-tidy %s --config="{CheckOptions:{StrictMode: true}}" -checks="-*,modernize-use-std-format" | FileCheck %s 
+
+// CHECK: warning: deprecation global option 'StrictMode', please use 'modernize-use-std-format.StrictMode'. [clang-tidy-config]

[carlosgalvezp]

I believe this would be clearer:

"global option '%0' is deprecated, please use '%1%0' instead."

[carlosgalvezp]

Looks good in general. I wonder however how it will work for people who are stuck in old clang-tidy files for whatever reason - this warning can be very noisy if running clang-tidy in CI with lots of files. Do we need some CLI option to allow users to silence the warnings?

[HerrCai0907]

I wonder however how it will work for people who are stuck in old clang-tidy files for whatever reason

Good point, maybe we can use more smart way to do warning: only warn the global option is set but local option is not.

Have some redundant options is not the aim for this deprecation things, we only want to avoid user suddenly find the behaviors of lots of check are changed.
So we only need to warn for config which rely on global option and no local option.

[5chmidti]

Good point, maybe we can use more smart way to do warning: only warn the global option is set but local option is not.

Have some redundant options is not the aim for this deprecation things, we only want to avoid user suddenly find the behaviors of lots of check are changed. So we only need to warn for config which rely on global option and no local option.

Your implementation of this looks good.

I wonder however how it will work for people who are stuck in old clang-tidy files for whatever reason - this warning can be very noisy if running clang-tidy in CI with lots of files. Do we need some CLI option to allow users to silence the warnings?

Adding a flag would either entail deprecating it again, or deciding that it is kept for future deprecations (and would therefore need to be general enough). Maybe something like --[quiet/no]-config-warnings? There are 11 StrictMode options, and 21 IgnoreMacros options, so there could be potentially 32 warnings for each checked file of a project, which is a lot. IMO, we don't need a flag, but it would make for a better user experience.
On the other hand, configs could be easily adjusted. Unless someone is in the position that you've described, that they have no control over the .clang-tidy file. While most projects would probably welcome those changes, some users may just want things to work without drowning in config warnings, and so they may prefer having the flag.

[carlosgalvezp]

that it is kept for future deprecations (and would therefore need to be general enough)

Yes, that was my thought, some general-purpose mechanism for deprecation warnings, for example --disable-deprecation-warnings, --no-deprecation-warnings, etc.

The use case I have in mind is when e.g. a 3rd-party .cpp file is pulled in via the compilation database, normally it will be analyzed via its own .clang-tidy file that normally one doesn't have control over. One could however argue that one should probably only analyze own source files, not 3rd-party ones. One probably also doesn't want to disable deprecation warnings globally just because a 3rd-party triggers them - it'd be better to filter out such 3rd-party code from analysis.

Perhaps we can just land this as is for now and introduce the escape hatch if we get feedback indicating it's needed?

[carlosgalvezp]

After this is merged, please create an issue on github to track this removal work for release 22.x

[carlosgalvezp]

I believe we should document this deprecation in the Release Notes.

[5chmidti]

Looks good from my side.

---

Perhaps we can just land this as is for now and introduce the escape hatch if we get feedback indicating it's needed?

I think that is totally fine. If this comes up, we can also immediately backport the flag so that users don't have to wait, e.g., months for their distro (etc.) to update the major version.