Silence -Wctad-maybe-unsupported stemming from system headers

[aaron.ballman]

Summary

Currently, this diagnostic fires on system header code that the user has no control over, such as std::lock_guard from <mutex> in both libc++ and libstdc++. I believe the diagnostic should be silenced when the class template exists in a system header, for two reasons: 1) the user can't fix that code anyway, so their only recourse is to disable the diagnostic, 2) system headers (especially STL headers) have had time to add the CTAD guides where necessary, and so the warning is more likely to be a false positive in those cases.

This issue was observed by a customer in Intel's downstream and is also related to Issue #44202.

(Adding libc++ reviewers as an FYI for the diagnostic firing in their headers and as a sounding board for whether they agree we should disable this diagnostic in system headers by default.)

[erichkeane]

clang/lib/Sema/SemaInit.cpp

10303

"but do not warn if the type without a deduction guide is in a system..."

[ldionne]

Wouldn't re-applying https://github.com/llvm/llvm-project/commit/fcd549a7d8284a8e7c763fee3da2206acd8cdc4f (which had been reverted IIUC) be a more precise fix for this problem? We'd suppress the warning, but only for classes that we know are OK to use with CTAD.

It is a fact that due to the nature of CTAD (which is enabled by default based on some general rules), several classes in the standard library that predated CTAD were not really designed with CTAD in mind, and CTAD should arguably not be used with them.

Re-applying fcd549a7d8284a8e7c763fee3da2206acd8cdc4f would not require any Clang changes.

[aaron.ballman]

In D133425#3775353, @ldionne wrote:

Wouldn't re-applying https://github.com/llvm/llvm-project/commit/fcd549a7d8284a8e7c763fee3da2206acd8cdc4f (which had been reverted IIUC) be a more precise fix for this problem? We'd suppress the warning, but only for classes that we know are OK to use with CTAD.

Oh that would be a great solution to this! Do you recall why that change was reverted?

It is a fact that due to the nature of CTAD (which is enabled by default based on some general rules), several classes in the standard library that predated CTAD were not really designed with CTAD in mind, and CTAD should arguably not be used with them.

Re-applying fcd549a7d8284a8e7c763fee3da2206acd8cdc4f would not require any Clang changes.

I think that would be a good solution if it's workable, at least for the issue we're seeing internally.

[aaron.ballman]

In D133425#3777598, @aaron.ballman wrote:

In D133425#3775353, @ldionne wrote:

Re-applying fcd549a7d8284a8e7c763fee3da2206acd8cdc4f would not require any Clang changes.

I think that would be a good solution if it's workable, at least for the issue we're seeing internally.

It is worth noting that this won't resolve the issue when using libstdc++ and Clang's behavior with this diagnostic differs from GCC's: https://godbolt.org/z/YMznh4Weq

[dblaikie]

In D133425#3777615, @aaron.ballman wrote:

In D133425#3777598, @aaron.ballman wrote:

In D133425#3775353, @ldionne wrote:

Re-applying fcd549a7d8284a8e7c763fee3da2206acd8cdc4f would not require any Clang changes.

I think that would be a good solution if it's workable, at least for the issue we're seeing internally.

It is worth noting that this won't resolve the issue when using libstdc++ and Clang's behavior with this diagnostic differs from GCC's: https://godbolt.org/z/YMznh4Weq

Yeah, seems worth making this work when using libstdc++ too.

So previously/currently-without-this-patch the diagnostic was suppressed if the use of ctad was in a system header (suppression based on the generic/builtin diagnostic suppression infrastructure) & now it'll suppress if that happens, or if the template is defined in a system header.

Seems fair to me. (but welcome not to take my approval as authoritative if you're looking for more/other feedback)

[ldionne]

In D133425#3777598, @aaron.ballman wrote:

In D133425#3775353, @ldionne wrote:

Wouldn't re-applying https://github.com/llvm/llvm-project/commit/fcd549a7d8284a8e7c763fee3da2206acd8cdc4f (which had been reverted IIUC) be a more precise fix for this problem? We'd suppress the warning, but only for classes that we know are OK to use with CTAD.

Oh that would be a great solution to this! Do you recall why that change was reverted?

I don't, but I can try to re-apply the patch and we'll know :-).

In D133425#3778433, @dblaikie wrote:

So previously/currently-without-this-patch the diagnostic was suppressed if the use of ctad was in a system header (suppression based on the generic/builtin diagnostic suppression infrastructure) & now it'll suppress if that happens, or if the template is defined in a system header.

One thing I don't understand in the current state of things is why the diagnostic fires at all inside system headers. I thought warnings in system headers were discarded? It this whole issue about system headers being added with -I instead of -isystem?

FWIW, my "objection" that we should not silence the warning when users try using CTAD with arbitrary types in std:: remains -- I think it would be making a disservice to users to let them use CTAD with classes that have not been designed with that in mind. At the end of the day, I think I'm advocating for individual classes opting-out of the warning, while the patch as currently formulated forces all classes in system headers to be opted-out of the warning.

[ldionne]

FWIW I just created https://reviews.llvm.org/D133535. IMO that is a safer alternative, in the sense that we only opt-in those types that we know are safe to use with CTAD, but the remaining classes still get a warning.

[dblaikie]

In D133425#3778433, @dblaikie wrote:

So previously/currently-without-this-patch the diagnostic was suppressed if the use of ctad was in a system header (suppression based on the generic/builtin diagnostic suppression infrastructure) & now it'll suppress if that happens, or if the template is defined in a system header.

One thing I don't understand in the current state of things is why the diagnostic fires at all inside system headers. I thought warnings in system headers were discarded?

It doesn't fire if the location of the diagnostic (the place that's using CTAD) is in a system header. But it does fire if that place is outside a system header, but the template that is being used is in a system header (see the SysHeaderObj example above - maybe the naming is confusing? It's a SysHeaderClass but the object (sho) is not in a system header).

It this whole issue about system headers being added with -I instead of -isystem?

Don't think so.

FWIW, my "objection" that we should not silence the warning when users try using CTAD with arbitrary types in std:: remains -- I think it would be making a disservice to users to let them use CTAD with classes that have not been designed with that in mind. At the end of the day, I think I'm advocating for individual classes opting-out of the warning, while the patch as currently formulated forces all classes in system headers to be opted-out of the warning.

I think the problem is that not all system headers match the style required for this constraint - so in the interests of compatibility with the complex world of existing system headers, it's suitable not to enforce this stylistic constraint (requiring explicit deduction guides even when the default is what the template author intended to allow CTAD) on system headers. With the knowledge that this does introduce false negatives, but that we don't have a strong enough signal to avoid them.

[ldionne]

In D133425#3779121, @dblaikie wrote:

One thing I don't understand in the current state of things is why the diagnostic fires at all inside system headers. I thought warnings in system headers were discarded?

It doesn't fire if the location of the diagnostic (the place that's using CTAD) is in a system header. But it does fire if that place is outside a system header, but the template that is being used is in a system header (see the SysHeaderObj example above - maybe the naming is confusing? It's a SysHeaderClass but the object (sho) is not in a system header).

Oh, right, so my confusion was about point-of-definition vs point-of-use.

FWIW, my "objection" that we should not silence the warning when users try using CTAD with arbitrary types in std:: remains -- I think it would be making a disservice to users to let them use CTAD with classes that have not been designed with that in mind. At the end of the day, I think I'm advocating for individual classes opting-out of the warning, while the patch as currently formulated forces all classes in system headers to be opted-out of the warning.

I think the problem is that not all system headers match the style required for this constraint - so in the interests of compatibility with the complex world of existing system headers, it's suitable not to enforce this stylistic constraint (requiring explicit deduction guides even when the default is what the template author intended to allow CTAD) on system headers. With the knowledge that this does introduce false negatives, but that we don't have a strong enough signal to avoid them.

I think this might come down to a difference in opinion here. Personally, I'd rather restrict the use of CTAD only to the small subset of classes that are meant to work with it, and warn everywhere else. And I'm fine if a system library has to jump through a few hoops in order to avoid warning on a class that does work well with CTAD. Allowing users to use CTAD on a class is like a contract given to them, and the fact that this contract is implicitly assumed by the language is often cited as a questionable design decision in C++17. I think this warning is useful and if the special markup had been there on the few classes that need it (like lock_guard), we wouldn't be having that discussion at all because I don't think anybody actually wants to use CTAD with random types in a system library.

In particular, the warning is useful in general user code, and I don't really follow why the fact that a type is authored in a system library would suddenly make it more okay to use with CTAD. Types in system libraries are not different from user types w.r.t. CTAD -- several types are just not designed with CTAD in mind.

Finally, I'd like to note that this warning only triggers if the user explicitly passes -Wctad-maybe-unsupported. It's not enabled by -Wall or -Wextra. I would argue that users that go out of their way to enable this warning probably share my sentiment that CTAD is sometimes harmful, and they probably want to be warned about questionable uses even for classes defined in a system library.

Anyway, I think I've made my opinion clear and I don't think I have much more to bring to the table. I'll ship D133535 regardless, but if this patch ships, I think we should have some way of enabling the warning for types in system headers. For example, folks using -Wctad-maybe-unsupported should really be warned about using CTAD on a type like std::reverse_iterator.

[dblaikie]

In D133425#3780435, @ldionne wrote:

In D133425#3779121, @dblaikie wrote:

One thing I don't understand in the current state of things is why the diagnostic fires at all inside system headers. I thought warnings in system headers were discarded?

It doesn't fire if the location of the diagnostic (the place that's using CTAD) is in a system header. But it does fire if that place is outside a system header, but the template that is being used is in a system header (see the SysHeaderObj example above - maybe the naming is confusing? It's a SysHeaderClass but the object (sho) is not in a system header).

Oh, right, so my confusion was about point-of-definition vs point-of-use.

FWIW, my "objection" that we should not silence the warning when users try using CTAD with arbitrary types in std:: remains -- I think it would be making a disservice to users to let them use CTAD with classes that have not been designed with that in mind. At the end of the day, I think I'm advocating for individual classes opting-out of the warning, while the patch as currently formulated forces all classes in system headers to be opted-out of the warning.

I think the problem is that not all system headers match the style required for this constraint - so in the interests of compatibility with the complex world of existing system headers, it's suitable not to enforce this stylistic constraint (requiring explicit deduction guides even when the default is what the template author intended to allow CTAD) on system headers. With the knowledge that this does introduce false negatives, but that we don't have a strong enough signal to avoid them.

I think this might come down to a difference in opinion here. Personally, I'd rather restrict the use of CTAD only to the small subset of classes that are meant to work with it, and warn everywhere else. And I'm fine if a system library has to jump through a few hoops in order to avoid warning on a class that does work well with CTAD.

But that's the issue, and generally why warnings are suppressed in system headers - as a user, you can't fix the system headers you depend on. So it usually ends up as a "the user can't use this feature with -Werror/the warning enabled, so to write valid code they turn off or ignore the warning".

Allowing users to use CTAD on a class is like a contract given to them, and the fact that this contract is implicitly assumed by the language is often cited as a questionable design decision in C++17. I think this warning is useful and if the special markup had been there on the few classes that need it (like lock_guard), we wouldn't be having that discussion at all because I don't think anybody actually wants to use CTAD with random types in a system library.

In particular, the warning is useful in general user code, and I don't really follow why the fact that a type is authored in a system library would suddenly make it more okay to use with CTAD.

It's more an argument from false positives in the warning because libraries outside your control may not adhere to this coding convention (which it is - like parentheses around assignment in an if condition, it's a somewhat arbitrarily chosen syntax intended to communicate explicit intent rather than the implicit default - but it's something all the authors need to agree on, not something that would be known independently by the author of a library/happen to be just general good practice when writing code in the absence of this warning depending on that spelling/signal)

Types in system libraries are not different from user types w.r.t. CTAD -- several types are just not designed with CTAD in mind.

Finally, I'd like to note that this warning only triggers if the user explicitly passes -Wctad-maybe-unsupported. It's not enabled by -Wall or -Wextra. I would argue that users that go out of their way to enable this warning probably share my sentiment that CTAD is sometimes harmful, and they probably want to be warned about questionable uses even for classes defined in a system library.

Maybe? But if they're stuck with system libraries that don't use this convention, this extra coverage of the warning may prevent them from using the warning at all, even for their non-system-header code.

Anyway, I think I've made my opinion clear and I don't think I have much more to bring to the table. I'll ship D133535 regardless, but if this patch ships, I think we should have some way of enabling the warning for types in system headers. For example, folks using -Wctad-maybe-unsupported should really be warned about using CTAD on a type like std::reverse_iterator.

Yeah, that might be a way forward - splitting the warning in two - have one level that's the current even-in-system-headers behavior, then a subset that's the GCC-behavior. (probably the current flag name should match the GCC behavior, as much as that's a break in compatibility with previous-clang - but I could see the counterargument that we should keep the name/semantics matching previous clang and add a separate GCC-incompatible name for the GCC-compatible behavior... )

[aaron.ballman]

In D133425#3780579, @dblaikie wrote:

Yeah, that might be a way forward - splitting the warning in two - have one level that's the current even-in-system-headers behavior, then a subset that's the GCC-behavior. (probably the current flag name should match the GCC behavior, as much as that's a break in compatibility with previous-clang - but I could see the counterargument that we should keep the name/semantics matching previous clang and add a separate GCC-incompatible name for the GCC-compatible behavior... )

I've been thinking on this a bit, and I think this is the best way forward. I don't have a strong opinion on whether we retain the behavior we had here or not. This diagnostic is currently ignored by default, so it's unlikely to have significant use in the wild. Any preference from the peanut gallery?

[dblaikie]

In D133425#3815118, @aaron.ballman wrote:

In D133425#3780579, @dblaikie wrote:

Yeah, that might be a way forward - splitting the warning in two - have one level that's the current even-in-system-headers behavior, then a subset that's the GCC-behavior. (probably the current flag name should match the GCC behavior, as much as that's a break in compatibility with previous-clang - but I could see the counterargument that we should keep the name/semantics matching previous clang and add a separate GCC-incompatible name for the GCC-compatible behavior... )

I've been thinking on this a bit, and I think this is the best way forward. I don't have a strong opinion on whether we retain the behavior we had here or not. This diagnostic is currently ignored by default, so it's unlikely to have significant use in the wild. Any preference from the peanut gallery?

Google's managed to use the warning with its current behavior for however many years it's been implemented, so we'd probably gain something by continuing to use the more aggressive version (I'm sort of surprised we've been able to use it as long as we have - basically the moment LLVM switched to C++17, the LLVM codebase had violations of the warning, so I'm not sure why we haven't seen that come up in lots of other third party code (which we flag as system headers to reduce the constraints on it generally) - perhaps just less aggressive adoption of newer language feature usage in the other third party code we depend on). But I'm not sure we'd miss it greatly either? Hard to say. The cost is a long term one in terms of maintenance of libraries - code depending on the unintended surface area then, one day, that surface area wanting to change but being stuck (well, higher cost for the change/migration) due to all the accumulated usage.

I was going to say we could keep the full functionality under the current flag name, adding a subflag for the control over the "skipping system headers" part - but that would be a bit inconsistent (I guess usually sub flags add more cases that warn, rather than removing them) and we'd still have different behavior than GCC for the same flag name, which would be unfortunate. So the right thing is probably to change the behavior of this flag, and (potentially) have a subflag with the more aggressive/old behavior.

@EricWF @rsmith as the original author/reviewer of the feature and vested parties on the Google side.

[Mordante]

@aaron.ballman We'd like to clean up the patches from the libc++ review queue. Is this patch is still relevant? Especially after landing https://reviews.llvm.org/D133535.

clang/docs/ReleaseNotes.rst

 - Correctly diagnose a future keyword if it exist as a keyword in the higher
   language version and specifies in which version it will be a keyword. This
   supports both c and c++ language.
 - When diagnosing multi-level pack expansions of mismatched lengths, Clang will
   now, in most cases, be able to point to the relevant outer parameter.
 - no_sanitize("...") on a global variable for known but not relevant sanitizers
   is now just a warning. It now says that this will be ignored instead of
   incorrectly saying no_sanitize only applies to functions and methods.
+- No longer issue ``-Wctad-maybe-unsupported`` warnings when the template which
+  might not support CTAD is defined in a system header. This silences warnings
+  when using ``std::lock_guard`` from ``<mutex>``.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - It's now possible to set the crash diagnostics directory through
   the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
   The ``-fcrash-diagnostics-dir`` flag takes precedence.
 - When using header modules, inclusion of a private header and violations of
   the `use-declaration rules

clang/include/clang/Basic/DiagnosticGroups.td

 def FunctionMultiVersioning
     : DiagGroup<"function-multiversion", [TargetClonesMixedSpecifiers]>;
 
 def NoDeref : DiagGroup<"noderef">;
 
 // A group for cross translation unit static analysis related warnings.
 def CrossTU : DiagGroup<"ctu">;
 
-def CTADMaybeUnsupported : DiagGroup<"ctad-maybe-unsupported">;
-
 def FortifySource : DiagGroup<"fortify-source">;
 
 def MaxTokens : DiagGroup<"max-tokens"> {
   code Documentation = [{
 The warning is issued if the number of pre-processor tokens exceeds
 the token limit, which can be set in three ways:
 
 1. As a limit at a specific point in a file, using the ``clang max_tokens_here``

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def note_deduction_guide_access : Note<
   "deduction guide declared %0 by intervening access specifier">;
 def warn_cxx14_compat_class_template_argument_deduction : Warning<
   "class template argument deduction is incompatible with C++ standards "
   "before C++17%select{|; for compatibility, use explicit type name %1}0">,
   InGroup<CXXPre17Compat>, DefaultIgnore;
 def warn_ctad_maybe_unsupported : Warning<
   "%0 may not intend to support class template argument deduction">,
-  InGroup<CTADMaybeUnsupported>, DefaultIgnore;
+  InGroup<DiagGroup<"ctad-maybe-unsupported">>, DefaultIgnore;
 def note_suppress_ctad_maybe_unsupported : Note<
   "add a deduction guide to suppress this warning">;
 
 
 // C++14 deduced return types
 def err_auto_fn_deduction_failure : Error<
   "cannot deduce return type %0 from returned value of type %1">;
 def err_auto_fn_different_deductions : Error<

clang/lib/Sema/SemaInit.cpp

     break;
   }
 
   // C++ [dcl.type.class.deduct]p1:
   //  The placeholder is replaced by the return type of the function selected
   //  by overload resolution for class template deduction.
   QualType DeducedType =
       SubstAutoType(TSInfo->getType(), Best->Function->getReturnType());
-  Diag(TSInfo->getTypeLoc().getBeginLoc(),
-       diag::warn_cxx14_compat_class_template_argument_deduction)
+  SourceLocation TyLoc = TSInfo->getTypeLoc().getBeginLoc();
+  Diag(TyLoc, diag::warn_cxx14_compat_class_template_argument_deduction)
       << TSInfo->getTypeLoc().getSourceRange() << 1 << DeducedType;
 
   // Warn if CTAD was used on a type that does not have any user-defined
-  // deduction guides.
-  if (!HasAnyDeductionGuide) {
-    Diag(TSInfo->getTypeLoc().getBeginLoc(),
-         diag::warn_ctad_maybe_unsupported)
-        << TemplateName;
+  // deduction guides, but do not warn if the deduction guide is in a system
+  // header on the assumption that the support is intentional there.
+  if (!HasAnyDeductionGuide &&
+      !SourceMgr.isInSystemHeader(Template->getLocation())) {
+    Diag(TyLoc, diag::warn_ctad_maybe_unsupported) << TemplateName;
     Diag(Template->getLocation(), diag::note_suppress_ctad_maybe_unsupported);
   }
 
   return DeducedType;
 }

clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp

 template <class T>
 struct TestSuppression {
   TestSuppression(T) {}
 };
 TestSuppression(allow_ctad_t)->TestSuppression<void>;
 TestSuppression ta("abc");
 static_assert(__is_same(decltype(ta), TestSuppression<const char *>), "");
 }
+
+// Test that the diagnostic is suppressed if within a system header.
+# 2 "test.h" 1 3
+template <class T>
+struct SysHeaderObj {
+  SysHeaderObj(T) {}
+  SysHeaderObj(T, int) {}
+};
+# 6 "test.h" 2
+
+SysHeaderObj sho(42); // Okay because SysHeaderObj is from a system header.
+
 #pragma clang diagnostic pop
 
 namespace PR41549 {
 
 template <class H, class P> struct umm;
 
 template <class H = int, class P = int>
 struct umm {