[clang-tidy] Change google-explicit-constructor to ignore conversions and operators marked `explicit(false)`

[njames93]

Summary

These explicit(cond) feature in c++20 can be used to specify if a declaration should be implicit or explicit.
For these cases we shouldn't produce any warning as the developer is already enforcing a specific behaviour.

Fixes https://llvm.org/PR53115.

[aaron.ballman]

Hmmm, this is a rule for checking a specific style guide. https://google.github.io/styleguide/cppguide.html#Implicit_Conversions doesn't say that explicit(false) is a reasonable marking. In fact, it says "Implicit conversions can sometimes be necessary and appropriate for types that are designed to be interchangeable, for example when objects of two types are just different representations of the same underlying value. In that case, contact your project leads to request a waiver of this rule."

So I think this behavior needs to be behind a flag so that the default behavior continues to match what the style guide requires (or the style guide should be updated to clarify the behavior of explicit(false)).

[njames93]

In D117593#3273562, @aaron.ballman wrote:

Hmmm, this is a rule for checking a specific style guide. https://google.github.io/styleguide/cppguide.html#Implicit_Conversions doesn't say that explicit(false) is a reasonable marking. In fact, it says "Implicit conversions can sometimes be necessary and appropriate for types that are designed to be interchangeable, for example when objects of two types are just different representations of the same underlying value. In that case, contact your project leads to request a waiver of this rule."

So I think this behavior needs to be behind a flag so that the default behavior continues to match what the style guide requires (or the style guide should be updated to clarify the behavior of explicit(false)).

My understanding of the rule(as a non Googler) was that explicit(false) is an effective way to disable the rule by explicitly declaring the constructor to be implicit. Which is much cleaner than throwing NOLINT markers about.
In any case this c++20 syntax is not supported as the current fixit produces invalid code, as evidenced in the initial bug report.

[aaron.ballman]

In D117593#3274408, @njames93 wrote:

In D117593#3273562, @aaron.ballman wrote:

Hmmm, this is a rule for checking a specific style guide. https://google.github.io/styleguide/cppguide.html#Implicit_Conversions doesn't say that explicit(false) is a reasonable marking. In fact, it says "Implicit conversions can sometimes be necessary and appropriate for types that are designed to be interchangeable, for example when objects of two types are just different representations of the same underlying value. In that case, contact your project leads to request a waiver of this rule."

So I think this behavior needs to be behind a flag so that the default behavior continues to match what the style guide requires (or the style guide should be updated to clarify the behavior of explicit(false)).

My understanding of the rule(as a non Googler) was that explicit(false) is an effective way to disable the rule by explicitly declaring the constructor to be implicit. Which is much cleaner than throwing NOLINT markers about.

FWIW, I agree that this would be a reasonable exception to the rule and makes for cleaner code than NOLINT comments. I'm pointing out that the rule text doesn't say that exceptions are something they want to silence diagnostics for. The way I read the guideline suggests that they don't consider the diagnostic a false positive.

In any case this c++20 syntax is not supported as the current fixit produces invalid code, as evidenced in the initial bug report.

Agreed that issue needs to be fixed.

[LegalizeAdulthood]

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

• fold your changes into the appropriate subdirs, stripping the module prefix from new files.
• make the target check-clang-extra to validate your tests
• make the target docs-clang-tools-html to validate any docs changes

[ProExpertProg]

Is there an update on this? I agree adding a flag to control this behavior would be good but the Google style guide clearly does not take the explicit(false) option into consideration. Currently the only alternatives are a disabling the check completely or adding a bunch of NOLINT statements everywhere, neither of which is great.

Perhaps an alternative would be to add a separate check, either called explicit-constructor or google-explicit-constructor-permissive with this functionality.

clang-tools-extra/clang-tidy/google/ExplicitConstructorCheck.cpp

 void ExplicitConstructorCheck::check(const MatchFinder::MatchResult &Result) {
   constexpr char WarningMessage[] =
       "%0 must be marked explicit to avoid unintentional implicit conversions";
 
   if (const auto *Conversion =
       Result.Nodes.getNodeAs<CXXConversionDecl>("conversion")) {
     if (Conversion->isOutOfLine())
       return;
+    if (Conversion->getExplicitSpecifier().isSpecified())
+      return;
     SourceLocation Loc = Conversion->getLocation();
     // Ignore all macros until we learn to ignore specific ones (e.g. used in
     // gmock to define matchers).
     if (Loc.isMacroID())
       return;
     diag(Loc, WarningMessage)
         << Conversion << FixItHint::CreateInsertion(Loc, "explicit ");
     return;
if (Ctor->isExplicit() &&
     if (ExplicitTokenRange.isValid()) {
       Diag << FixItHint::CreateRemoval(
           CharSourceRange::getCharRange(ExplicitTokenRange));
     }
     return;
   }
 
   if (Ctor->isExplicit() || Ctor->isCopyOrMoveConstructor() ||
-      TakesInitializerList)
+      TakesInitializerList || Ctor->getExplicitSpecifier().isSpecified())
     return;
 
   bool SingleArgument =
       Ctor->getNumParams() == 1 && !Ctor->getParamDecl(0)->isParameterPack();
   SourceLocation Loc = Ctor->getLocation();
   diag(Loc, WarningMessage)
       << (SingleArgument
               ? "single-argument constructors"
               : "constructors that are callable with a single argument")
       << FixItHint::CreateInsertion(Loc, "explicit ");
 }
 
 } // namespace google
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`cert-flp37-c
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Removed default setting ``cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors = "true"``,
   to match the current state of the C++ Core Guidelines.
 
+- Updated :doc:`google-explicit-constructor
+  <clang-tidy/checks/google-explicit-constructor>` check to ignore constructors
+  or conversion operators where they are marked ``explicit(false)``.
+
 - Updated :doc:`google-readability-casting
   <clang-tidy/checks/google-readability-casting>` to diagnose and fix functional
   casts, to achieve feature parity with the corresponding ``cpplint.py`` check.
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   <clang-tidy/checks/fuchsia-trailing-return>` for C++17 deduction guides.
 
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing

clang-tools-extra/test/clang-tidy/checkers/google-explicit-constructor-cxx20.cpp

+// RUN: %check_clang_tidy -std=c++20 %s google-explicit-constructor %t
+
+struct Foo {
+  Foo(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions
+  // CHECK-FIXES: {{^}}  explicit Foo(int) {}
+  explicit Foo(long) {}
+  explicit(false) Foo(float) {}
+  explicit(true) Foo(double) {}
+
+  operator int() const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'operator int' must be marked explicit to avoid unintentional implicit conversions
+  // CHECK-FIXES: {{^}}  explicit operator int() const;
+  explicit operator long() const;
+  explicit(false) operator float() const;
+  explicit(true) operator double() const;
+};