Include [[clang::require_explicit_initialization]] warnings in system headers

[higher-performance]

Fixes #141103

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (higher-performance)

Changes

Fixes #141103

---

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

1 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2-1)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 78b36ceb88125..8a3aafeabdad3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2423,7 +2423,8 @@ def note_uninit_reference_member : Note<
   "uninitialized reference member is here">;
 def warn_field_requires_explicit_init : Warning<
   "field %select{%1|in %1}0 requires explicit initialization but is not "
-   "explicitly initialized">, InGroup<UninitializedExplicitInit>;
+   "explicitly initialized">, InGroup<UninitializedExplicitInit>,
+   ShowInSystemHeader;
 def warn_field_is_uninit : Warning<"field %0 is uninitialized when used here">,
   InGroup<Uninitialized>;
 def warn_base_class_is_uninit : Warning<

[efriedma-quic]

Testcase?

This seems like the sort of thing where we might want to cooperate with the C++ library somehow to get better diagnostics, but getting some diagnostic seems like an improvement.

[higher-performance]

@efriedma-quic: I'm not sure what the appropriate way to test this is. The file clang/test/SemaCXX/uninitialized.cpp doesn't include any system headers. Should I just add a system header there and include it?

Re: libc++ cooperation: I thought about something along those lines too, but I couldn't think of a satisfactory solution. Especially because people can have their own templated wrappers too, and also because "system headers" could be anything third-party -- not just the standard library. I think the current approach might actually be best, as inelegant as it feels.

[cor3ntin]

I've been considering similar issues recently.
e.g https://compiler-explorer.com/z/5xcKf8Kj1

A more general solution would be to emit diagnostics in system headers if one of the involved types/expressions names an entity declared outside of a system header.
I haven't had a brilliant idea on how to do that yet. Note that looking at the instantiation stack doesn't help because warnings unrelated to user code can be produced in system headers, caused by an instantiation in user code.

So we need to look at entities.... somehow.

[higher-performance]

@efriedma-quic Ah I figured out how to test this - just added a test.

[efriedma-quic]

Looks fine, but I'd like a second approval; I haven't really looked at this template instantiation stuff recently.

[cor3ntin]

Let me chat with @AaronBallman and @erichkeane on Monday about this.

This feels like a very narrow fix for a widespread issue.

• Presumably, we don't want to warn if library implementers (mis)use std::construct_at internally
• There are tons of other warnings that are silenced.

So I think that either

• We need a better heuristic to hide warnings than "comes from a system header".
• We should add the ShowInSystemHeader flags to a lot of warnings, not just this one.

[higher-performance]

@cor3ntin feel free to chat, but in this case we do actually want to warn (and error, under -Werror) if anybody (mis)uses std::construct_at (i.e. neglects to explicitly initialize a field that declares itself as such). It's the exact analog of using std::invoke to call a function that has a mandatory positional parameter -- it doesn't make sense to allow anyone to bypass it. The entire goal is to make sure that field/parameter has an explicitly provided value in all cases. So I think this really is the right fix for this particular warning, regardless of the more general problem.

[higher-performance]

What do folks think?

[erichkeane]

I don't think this is a good idea, warning on EVERY use of this is incorrect in system headers. IT is going to result in a ton of we-want-to-be-suppressed-positives

I also don't think we can have a heuristic that wouldn't have an absurd amount of false positives.

IMO, the only thing we COULD do here is to have some sort of #pragma clang that wraps the template and says "show this even in system headers", which is an opt-in at the library level.

[higher-performance]

I don't think this is a good idea, warning on EVERY use of this is incorrect in system headers. IT is going to result in a ton of we-want-to-be-suppressed-positives

Thanks @erichkeane. I think I fundamentally disagree with this. The entire intention of this attribute is to flag such a thing -- it's no different from calling std::invoke(foo, x) and expecting to get a diagnostic on every invocation when the function is void foo(int x, int y), especially when you consider that these are basically simulating named-parameters. Do you have a plausible use case in mind that makes you believe people would actually want to suppress the warning in system headers?

[erichkeane]

I don't think this is a good idea, warning on EVERY use of this is incorrect in system headers. IT is going to result in a ton of we-want-to-be-suppressed-positives

Thanks @erichkeane. I think I fundamentally disagree with this. The entire intention of this attribute is to flag such a thing -- it's no different from calling std::invoke(foo, x) and expecting to get a diagnostic on every invocation when the function is void foo(int x, int y), especially when you consider that these are basically simulating named-parameters. Do you have a plausible use case in mind that makes you believe people would actually want to suppress the warning in system headers?

Hmm... I just spent a while looking at the attribute more and changed how I was thinking about this problem, and I'm less confident in my above statement. SINCE this is competely opt-in we could presumably document this better to mean "we mean this even if it is in the nitty-gritty of the standard library". Typically these sorts of attributes don't really mean that/are intended for use in violations by the 'users' code. Of course, construct-at and invoke/etc all make this somewhat more complicated of a differentiation.

My biggest 'plausible' concern is cases where the system header needs to make irrelevant/unused temporaries for the purposes of various operations along the way, which still DO the right thing but are wrong along the way. BUT I also wonder how much sympathy I have for those cases when the author of the type has opted into this.

[higher-performance]

Yeah. I'm not even sure I can think of a case with the irrelevant/unused temporaries you mention, to be honest. The way I would think about this is: this attribute is intended to be equivalent to adding an in-class member initializer that is impossible for anyone to actually invoke. The only reason we don't do that directly is that the error messages are terrible, but otherwise they are solving the same problem. And, similarly, there's no reason to allow that to be bypassed either. (But, just as you said, I don't see reasons to have sympathy for it even if that case was possible to bypass.)

[zygoloid]

There are techniques for determining the number of fields in an aggregate type T by attempting to initialize with T{} then T{arg} then T{arg, arg}, and so on (with an arg that converts to anything), until the initialization fails. Such techniques might in principle show up in a system header (eg, boost probably has this somewhere).

However, any such code should be in an unevaluated operand, so as long as this warning is suppressed in unevaluated operands I think it's fine for it to be enabled in system headers. (For extra assurance, we could make this a DiagRuntimeBehavior to also suppress it in unreachable code, but maybe that's going too far.)

[higher-performance]

Interesting point! A few thoughts:

• Are system headers really any different from non-system headers in that case? It feels like it might be an orthogonal issue we should address in a separate PR.
• Do you mean SFINAE contexts rather than merely unevaluated contexts (?)
• How would I detect if a given Expr (say, an InitListExpr) is unevaluated? Is that easy or would it require a huge rework of the implementation?

[zygoloid]

I don't think this concern is specific to system headers.

I don't think we have precedent for disabling a warning in a SFINAE context. We do suppress warnings produced while substituting into a function template specialization during deduction if we end up not selecting that overload, but I don't think we have something analogous for concepts, and it's not clear that that would help here, given that the warning would be produced in the case where the concept check succeeds.

You can check if you're in an unevaluated context with Sema::isUnevaluatedContext.

[higher-performance]

Got it, thanks. I updated this to ignore unevaluated contexts.

[erichkeane]

I think the unevaluated contexts DOES help a bunch, though I'm still concerned about the ability of the library to create 'temproary' objects/etc via some sort of default construction/aggregate construction, to then fill in the rest.

[higher-performance]

@erichkeane the question of whether S s{}; s.f = ...; should be deemed as a relaxation of S s{.f = ...}; came up during the initial RFC. From memory, the conclusion was that it's something we could relax in a v2 of the RFC, but it is a significant amount of work, and the initial design shouldn't be blocked on it. Furthermore, we haven't really seen significant demand for that on our side. By contrast, the current bug is actually a loophole in the original design that is causing bugs to slip through for current users who only needed the guarantees from the original attribute, so blocking this on that doesn't really feel justified at this stage.

[higher-performance]

Could I move forward with merging this? Or does anyone feel there are blockers here?

[erichkeane]

@erichkeane the question of whether S s{}; s.f = ...; should be deemed as a relaxation of S s{.f = ...}; came up during the initial RFC. From memory, the conclusion was that it's something we could relax in a v2 of the RFC, but it is a significant amount of work, and the initial design shouldn't be blocked on it. Furthermore, we haven't really seen significant demand for that on our side. By contrast, the current bug is actually a loophole in the original design that is causing bugs to slip through for current users who only needed the guarantees from the original attribute, so blocking this on that doesn't really feel justified at this stage.

Its more that we assume/like to assume that the Standard Library always 'does the right thing', even if it does so in ways that look 'wrong'. So we suppress warnings in the standard library headers, since they are assumed to all be false-positives.

This of course is an interesting 'that assumption is not perfectly true' anymore type of situation.

Could I move forward with merging this? Or does anyone feel there are blockers here?

@AaronBallman , @cor3ntin , and need to discuss this still I think and make sure we are sure. Unfortunately Aaron's next 'office hours' (when we'd do this typically) aren't for a few weeks, but I'll see if we can discuss this offline in the near future.

[higher-performance]

I see, thanks for explaining. That would be great if you can discuss it offline in the near future.

In the meantime, let me just emphasize a some points from my side before I forget:

• The question of delayed initialization is (so far as I can tell) entirely orthogonal to the question of what the standard library should be doing here. If we keep the current spec, then obviously the existing behavior is buggy, and this PR is required for the fix. If we do relax the attribute to allow delayed initialization, then it would require dataflow analysis out of the standard library (since it would now be okay to do std::construct_at(...) followed by filling in the fields). Either way, I struggle to see how treating the standard library differently would solve anything.
• Insofar as the current spec goes -- just as std::construct_at cannot possibly detect (let alone try to avoid) an in-class member initializer that goes awry when invoked, I think it also cannot (and should not) try to avoid the same issue w.r.t. this attribute. It simply does not have enough information, and this is true for every function out there that constructs a user-specified type (emplace_back, etc.), not just those in the standard library.

[zygoloid]

Its more that we assume/like to assume that the Standard Library always 'does the right thing', even if it does so in ways that look 'wrong'. So we suppress warnings in the standard library headers, since they are assumed to all be false-positives.

This of course is an interesting 'that assumption is not perfectly true' anymore type of situation.

For me the concern here is minimal: the standard library should never be "inventing" its own way of initializing a user-defined type. If the standard library is performing an initialization, it should either have a reason to believe the initialization works (either because it owns the type being initialized -- never a problem here unless the standard library itself starts using this attribute -- or because the user of the standard library used it in a way that implies that the initialization works -- which is exactly the case in which we want a warning), or it should be testing whether the initialization works (eg, in a SFINAE context).

I wonder if a different approach to this problem would be to make the diagnostic produced by this attribute be an error rather than a warning. That'd make it rather more clear that it should be produced even in system headers :) (And we then shouldn't disable it in unevaluated operands -- instead SFINAE could handle the relevant cases there.)