-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Clang attribute to ensure that fields are initialized explicitly #102040
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: None (higher-performance) ChangesThis is a new Clang-specific attribute to ensure that field initializations are performed explicitly. For example, if we have
|
@AaronBallman I think this is worth of an RFC, WDYT? |
Thank you for this!
Yes, this should definitely get an RFC. Some things worth discussing in the RFC:
(I'm sure there are other questions, but basically, it's good to have a big-picture understanding of why a particular design is the way you think we should go.) |
f09b427
to
5e03c06
Compare
5e03c06
to
9850a8b
Compare
I updated the PR to change the attribute name from @AaronBallman are we good to move forward? |
9850a8b
to
471b41a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope folks are ok with me chiming in as a reviewer for this.
I've left quite a few comments in the RFC and is also supportive of landing this change and happy to invest into supporting it going forward inside our team.
002027b
to
1695451
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
9833f57
to
dc56548
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some good parallel disscussions happening in the RFC, but despite their outcomes, we could probably update the PR to capture current behavior in those interesting cases.
I left a few comments along those lines, PTAL.
4319585
to
9335d80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment about VerifyOnly
and duplicate diagnostics.
The rest are small NITs.
clang/test/SemaCXX/uninitialized.cpp
Outdated
C a; // expected-warning {{not explicitly initialized}} | ||
(void)a; | ||
#endif | ||
D b{.f2 = 1}; // expected-warning {{'x' is not explicitly initialized}} expected-warning {{'q' is not explicitly initialized}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an idea for future improvements: we could also collect all unitialized fields and emit a single diagnostic that lists them all (with notes to the locations of the fields).
However, I think this is good enough for the first version, I don't necessarily feel we should do it right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's more complicated than I can invest in right now, it's something we can do in the future though.
@AaronBallman @cor3ntin I believe we are getting close to finalizing this PR. There was some discussion here and in the RFC, but I don't think there was explicit approval (or objection) to land this, so I wanted to clarify this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the rfc has reached its conclusion yet, and consensus has not been called (for example, i still need to think about whether my questions were addressed) so we should wait for the RFC process before continuing with that PR.
Thanks
Thanks for explicitly calling this out. There were no replies from you on the RFC for some time, so it was unclear whether there is anything left. We will be waiting for your feedback on Discourse. |
Usually the process requires someone like Aaron to call consensus either
way. we are aware that establishing consensus is a bit nebulous at the
moment but it's something we are working on improving.
…On Fri, Sep 13, 2024, 16:03 Ilya Biryukov ***@***.***> wrote:
I don't think the rfc has reached its conclusion yet, and consensus has
not been called (for example, i still need to think about whether my
questions were addressed) so we should wait for the RFC process before
continuing with that PR.
Thanks
Thanks for explicitly calling this out. There were no replies from you on
the RFC for some time, so it was unclear whether there is anything left. We
will be waiting for your feedback on Discourse.
—
Reply to this email directly, view it on GitHub
<#102040 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKX7633PBO3XB4FJRFVRGTZWLWCNAVCNFSM6AAAAABMA3ZTVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBZGA2DCMRWGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
86bde73
to
10ea390
Compare
def warn_cxx20_compat_requires_explicit_init_non_aggregate : Warning< | ||
"explicit initialization of field %0 may not be enforced in C++20 as type %1 " | ||
"will become a non-aggregate due to the presence of user-declared " | ||
"constructors">, DefaultIgnore, InGroup<CXX20Compat>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one I'd like Aaron's opinion here.
10ea390
to
4cd75a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking 'request-changes' not as there are changes needed, but that I have concerns about the implementation that I have to spend some time in a debugger to assuage.
4cd75a8
to
e5a5687
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a way to remove the 'request changes' without approval, but I did some debugging and see what I was assuming was wrong. So I don't have any concerns on the approach.
However, in using it for the last few hours, I REALLY dislike the diagnostic that you get. I think the additional effort (which is, IMO, fairly minor once you have a RecordDecl) of printing the first field is worth it, and necessary for me. The function should be pretty easy though, and you mentioned not really having much time, so here's what it should look like more or less:
FieldDecl *getFirstExplicitInitField(RecordDecl *RD) {
// Current class field seems worth it, and gives the best answer in 99% of uses. Other 1% is when there is just fields in bases.
for (FieldDecl *FD : RD) {
if (FD->hasAttr<ExplicitInitAttr>())
return FD;
}
// DFS for bases, but it doesn't seem worth the effort to work much harder to make sure we do the 'shallowest' field, which, while better diagnostics, is comparatively rare.
if (auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
for (CXXBaseSpecifier BS : CXXRD->bases()) {
CXXRecordDecl *Base = BS.getType()->getAsCXXRecordDecl();
if (Base->hasUninitializedExplicitInitFields())
if (FieldDecl *FD = getFirstExplicitInitField(Base))
return FD;
}
// TODO: Figure out whether `bases` contains virtual bases, and not just non-virtual bases. If not, do the above on `RecordDecl::vbases`
}
}
That should be within a bit of const-correctness/few typos from doing what you need.
THOUGH: that makes me wonder, have you played with virtual bases yet? Are we 'sane' for those?
I have not, but those can't be aggregates, right?
Your comment is exactly hinting at why I said this is neither easy, nor efficient. Notice that the fields might not be directly inside the class at all. They might be embedded in some deep (even anonymous) subitem of array of anonymous union of subobject of some far-away base. Digging them up is nowhere as trivial as in your example, and it requires traversing a potentially very expansive object tree. You could easily traverse 1,000+ fields (possibly inside precompiled modules/headers) to dig up 1 name. That seems... not great? If you're happy with digging up the field name only in a few simple cases (like if it's a direct member of the class or a direct member of one of its direct bases) then sure it's as trivial as your code suggests, and we could do that, but that wouldn't cover everything. |
Right, good, yeah, that shouldn't be a problem then.
I very much think we need to 'do better'. Upon further reflection, I would think that any amount of depth beyond direct fields is perhaps gilding the lily (and would probably require breadcrumbs), so direct fields is perhaps good enough. That said, the diagnostic was really quite opaque when trying to use it/figure out what I was doing. We need to improve the diagnostics in those cases to make it more clear(at least SOME level of hint) to users of this feature what they are looking for. For simple Fields, we can just print the 1st, for types in fields/more complicated fields, perhaps just point at the field (and mention the type name that causes the problem), and do a similar thing for the base classes. |
@AaronBallman this seems to have progressed much further after your review and is currently pending on input from you. Could you please do a round of review here/in the RFC? (Or ask someone else from the community to address your concerns and be a reviewer instead, not sure if that's more useful) @cor3ntin and also the same question. Could you take another look to see if your concerns were addressed? |
IIRC, Aaron is away this week (and was away last week for WG14). So we'll have to wait on him a bit. |
e5a5687
to
963fb67
Compare
@erichkeane I ended up just implementing a full-ish traversal since it didn't. I'm not entirely sure if it might miss any corner cases (like anonymous objects etc.), but it should handle any cases that practically come up. Please take another look. |
963fb67
to
05f0698
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just about there IMO, still have an open on the two diagnostic things, but otherwise only a few nit-like things (but please update the tests, it makes it much easier to review/update in the future)
2df7571
to
0b3d5dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy, pending Aaron's comments on the two diagnostic things.
0b3d5dc
to
b0bb8b8
Compare
… it work in both C and C++
b0bb8b8
to
93bbb04
Compare
Hi all, we've been waiting a few weeks for approval on this (from @AaronBallman I imagine), but I'm guessing there's a lack of bandwidth on the reviewers' sides. Is there a way we can move forward? |
We still want the final OK from Aaron, but he's at 35k feet right now on the way back from LLVM-Dev, so hopefully he'll have time to do a pass next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 more pass thru, just noticed 1 thing, plus highlighting things aaron needs to look at.
def warn_cxx20_compat_requires_explicit_init_non_aggregate : Warning< | ||
"explicit initialization of field %0 may not be enforced in C++20 as type %1 " | ||
"will become a non-aggregate due to the presence of user-declared " | ||
"constructors">, DefaultIgnore, InGroup<CXX20Compat>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need Aaron's comment, re: should this be its own diagnostic group?
@@ -3141,6 +3148,10 @@ def warn_attribute_ignored_no_calls_in_stmt: Warning< | |||
"statement">, | |||
InGroup<IgnoredAttributes>; | |||
|
|||
def warn_attribute_needs_aggregate : Warning< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another place for Aaron, whether warn_field_requires_explicit_init
and warn_attribute_needs_aggregate
should be errors, or can remain warnings.
I'm back in the office now and digging out from under last week. I plan to call consensus (for or against) on the RFC this week as conversation seems to have died down on it, so if anyone has last minute arguments for/against the feature, please leave comments on Discourse! |
Co-authored-by: Erich Keane <[email protected]>
This is a new Clang-specific attribute to ensure that field initializations are performed explicitly.
For example, if we have
then the diagnostic would trigger if we do
B b{};
:This prevents callers from accidentally forgetting to initialize fields, particularly when new fields are added to the class.
Naming:
We are open to alternative names; we would just like their meanings to be clear. For example,
must_init
,requires_init
, etc. are some alternative suggestions that would be fine. However, we would like to avoid a name such asrequired
asmust_specify
, as their meanings might be potentially unclear or confusing (e.g., due to confusion withrequires
).Note:
I'm running into an issue with duplicated diagnostics (see lit tests) that I'm not sure how to properly resolve, but I suspect it revolves around
VerifyOnly
. If you know the proper fix please let me know.