Page MenuHomePhabricator

Silence -Wctad-maybe-unsupported stemming from system headers
Changes PlannedPublic

Authored by aaron.ballman on Sep 7 2022, 7:39 AM.

Details

Reviewers
dblaikie
ldionne
erichkeane
Group Reviewers
Restricted Project
Restricted Project
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.)

Diff Detail

Event Timeline

aaron.ballman created this revision.Sep 7 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 7:39 AM
aaron.ballman requested review of this revision.Sep 7 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 7:39 AM
erichkeane accepted this revision.Sep 7 2022, 7:44 AM
erichkeane added a subscriber: erichkeane.
erichkeane added inline comments.
clang/lib/Sema/SemaInit.cpp
10303

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

This revision is now accepted and ready to land.Sep 7 2022, 7:44 AM

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.

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.

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 accepted this revision.Sep 8 2022, 2:13 PM

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)

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 :-).

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.

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.

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.

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.

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... )

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?

aaron.ballman planned changes to this revision.Sep 26 2022, 7:11 AM

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.