[Clang] User-relevant warnings in system headers

[cor3ntin]

We silence warnings in system headers on the basis that such warnings are not user-actionable. In practice, using -Wsystem-headers can be fairly noisy.

https://godbolt.org/z/eaxen4vob

However, we find that this silences... too much.

• Warnings are silenced when an expression involves a user-provided type or expression that is either invoked or converted

  • https://compiler-explorer.com/z/6Ma56fjYf
  • https://godbolt.org/z/GnTsrnYfo ( [clang] [[nodiscard]] ignored in std::function due to warning suppression in system headers #141791 )
  • std::construct_at() seems to bypass [[clang::require_explicit_initialization]] #141103

• Depreciation warnings are not emitted for standard types instantiated by the standard library ([clang] Deprecation warnings in system headers are silenced when the deprecated entity is used indirectly #134425)

This boils down to "happens in system headers" not always being a good heuristic for "is user relevant".

The solutions I thought about are

• Be a lot more aggressive about showing warnings in headers on a case-by-case basis and expect implementers not to trigger any of these warnings.
• Give implementers a pragma/attribute to show warnings in a region of code - this would be used every time library code could involve a user-provided type/expression
• In the front end, provides an API to show warnings in system headers based on multiple locations, rather than the location of the warning itself. We would then inspect types and expressions to determine whether they involve user-defined types.
• Inspect the instantiation stack and show warnings if parts of the stack are in user code
• When emitting warnings, wait until we have collected all the notes and produce warnings if any of the notes are in user code. This seems particularly involved/inefficient.
• A new attribute like [[deprecated]] but never silenced

Instantiation stacks make for a not-so-good heuristic:

• Some warnings in system headers might be relevant even if no instantiation happens
• Some warnings in system headers might be caused by instantiation from user code and still be completely irrelevant.

A simplification of looking at instantiation stacks is that if a warning is produced in a system header after we have completed the initial parse of that header, we display it.

I suspect we might want to land on a mix of some of these ideas.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: cor3ntin (cor3ntin)

We silence warnings in system headers on the basis that such warnings are not user-actionable. In practice, using -Wsystem-headers can be fairly noisy.

https://godbolt.org/z/eaxen4vob

However, we find that this silences... too much.

• Warnings are silenced when an expression involves a user-provided type or expression that is either invoked or converted

  • https://compiler-explorer.com/z/6Ma56fjYf
  • https://godbolt.org/z/GnTsrnYfo ( #141791 )
  • #141103

• Depreciation warnings are not emitted for standard types instantiated by the standard library (#134425)

This boils down to "happens in system headers" not always being a good heuristic for "is user relevant".

The solutions I thought about are

• Be a lot more aggressive about showing warnings in headers on a case-by-case basis and expect implementers not to trigger any of these warnings.
• Give implementers a pragma/attribute to show warnings in a region of code - this would be used every time library code could involve a user-provided type/expression
• In the front end, provides an API to show warnings in system headers based on multiple locations, rather than the location of the warning itself. We would then inspect types and expressions to determine whether they involve user-defined types.
• Inspect the instantiation stack and show warnings if parts of the stack are in user code
• When emitting warnings, wait until we have collected all the notes and produce warnings if any of the notes are in user code. This seems particularly involved/inefficient.
• A new attribute like [[deprecated]] but never silenced

Instantiation stacks make for a not-so-good heuristic:

• Some warnings in system headers might be relevant even if no instantiation happens
• Some warnings in system headers might be caused by instantiation from user code and still be completely irrelevant.

A simplification of looking at instantiation stacks is that if a warning is produced in a system header after we have completed the initial parse of that header, we display it.

I suspect we might want to land on a mix of some of these ideas.

[cor3ntin]

@erichkeane @AaronBallman @ldionne

[cor3ntin]

In case you have opinions @jwakely @StephanTLavavej

[jwakely]

We silence warnings in system headers on the basis that such warnings are not user-actionable. In practice, using -Wsystem-headers can be fairly noisy.

https://godbolt.org/z/eaxen4vob

However, we find that this silences... too much.

• Warnings are silenced when an expression involves a user-provided type or expression that is either invoked or converted

• https://compiler-explorer.com/z/6Ma56fjYf

Another example of suppressed -Wconversion warnings is in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87614

#include <vector>

int main() {
    std::vector<unsigned short> a;
    a.emplace_back(70000);
}

You only get this with -Wsystem-headers:

warning: implicit conversion loses integer precision: 'int' to 'unsigned short' [-Wimplicit-int-conversion]

As an aside, I notice that with -Wsystem-headers clang warns about the conversion when actually initializing the vector element, but also when testing whether the construction is noexcept, in an unevaluated context. So for example Corentin's compiler-explorer link above gives three -Wcharacter-conversion warnings, two(?!) for the noexcept-specifier of __invoke and one for the function body of __invoke where the implicit conversion happens. And the vector example above is similar, with multiple -Wimplicit-int-conversion warnings even though it only happens once. Does it ever make sense to warn for the unevaluated context, such as decltype or the noexcept operator? There can be no loss of precision or change of value in an unevaluated context, because there is no value. And in all realistic cases, the use of decltype or noexcept is going to be to infer some property of an actual expression which will already appear in the code, and so you'll get a warning for the real, evaluated expression. This verbosity makes -Wsystem-headers even more noisy than necessary, making it even more unusable. If you started giving those warnings without -Wsystem-headers I suspect you'd get people asking why they happen three times more than necessary!

[StephanTLavavej]

Wakely's example is a good one. The microsoft/STL philosophy is that we avoid emitting warnings for our own code, but we will emit warnings if the user asks us to perform an action on their behalf that would warn if they did it directly, and nothing about the STL is making it any safer. Truncating an int to unsigned short is a classic example.

[erichkeane]

A simplification of looking at instantiation stacks is that if a warning is produced in a system header after we have completed the initial parse of that header, we display it.

I'm not sure that this would be sufficient. First, of course, deferred template instantiation though we can work around that. Second, warnings in template code fits 1 of 2 patterns. Either they are caused by the user in a way that we don't want to diagnose, OR they are caused by the user in a way we WANT to diagnose.

For example of the 2nd: construct_at or emplace_back that doesn't fully initialize something with the attribute that says 'this field must be initialized'.

For examples of the 1st, we have all of the ones we currently suppress :)

I'm really on the fence here, I don't think instantiation stack helps us at all to intuit which of those we do, so I'm not sure what heuristic there could be. I would love it if @StephanTLavavej could better explain what he means, perhaps the MSVC heuristic is something implementable for us.

[StephanTLavavej]

I don't know if it's reducible to a mechanical heuristic.

Here's an example of a warning that we suppress. aligned_union is deprecated, but aligned_union_t needs to be defined in terms of it. So we suppress the deprecated warning around that usage. It wouldn't make any sense to include <type_traits> and get a warning that deprecated aligned_union is being used, when the user didn't request it.

For another example, our <filesystem> implementation has enumerators for error codes, and we have a switch that mentions a subset of them (with a default). We suppress MSVC's warning C4061 "enumerator not explicitly handled by switch label" because we don't want to have to list them all, and because this has nothing to do with anything the user wrote.

We got to this point by allowing warnings to be emitted in system headers, and fixing (or suppressing) all reported occurrences in our test suites and from customers. We have a system to globally suppress (within STL headers only) warnings that we never ever want to see, like the Future Technology warnings of using if constexpr in C++14, or "unreachable code" warnings, and then we have a few targeted suppressions here and there (with the deprecated-suppressions being the most common). Getting clean is a lot of work but staying clean is fairly easy.