Differential D106431
[clang-tidy] Fix cppcoreguidelines-init-variables by removing the enum FixIt, and add support for initialization check of scoped enum. Authored by Sockke on Jul 21 2021, 4:49 AM.
Details In C++, the enumeration is never Integer, and the enumeration condition judgment is added to avoid compiling errors when it is initialized to an integer. enum Color {Red, Green, Blue};
enum class Gender {Male, Female};
void func() {
Color color; // Color color = 0; <--- fix bug
Gender gender; // <--- no warning
}
Diff Detail
Event TimelineComment Actions Is this the right decision to make, conceptually? It will leave the variable uninitialised still, and reading such an uninit variable is still an issue, even if it is an enum. Could we consider the alternative of warning the user about the uninitialized variable, just not offering an automatic (and potentially bad / incorrect) fix?
Comment Actions Yeah, that's right. However, it's much more difficult to give enum an initial value than an integer.
Make sense, we (ByteDance) are also hesitating whether we should provide automatic fix for uninitialized variables, because automatic fix may change the program semantics.
Comment Actions This is the same as what we did for integer in essential. We are changing an uninitialized value to the default one, which makes sense as uninitialized means it could be anything. Comment Actions At present, Our views on preventing UB are basically the same, so a warning still needs to be reported (BTW, the original version will not report warnings for enum class types). The final question is what is the recommended value for initialization and whether to provide the automatic fix. Hope you can give us a direction, thanks！@aaron.ballman Comment Actions The problem with enums is that translating zero (0, 0.0, nullptr, etc...) to the enum case is not always apparent. A warning should always be given. And if you can find a zero member in the enum, we can report an automated suggestion for that. To give an example enum class BaseColour { BLACK, RED, GREEN, BLUE, WHITE };BLACK is the 0, so we can fix-it. If the 0 is the first member (usually in enums that have a well-defined "default" or "None" case...), then, it is alright to use that as the automated fix. However, if there isn't a "zero case" in the enum, for example: enum class Something {
FOO = -2,
BAR, // This will be -1
BAZ = 84,
QUX // This will be 85
};then simply using the first member might not be the best fitting action, so in such case, I would opt for no FixIt. But, obviously, yes for giving a warning. Comment Actions I think we shouldn't give any fix-it for enumerations. Zero doesn't always mean "the right default value" -- for example, another common idiom is for the *last* member of the enumeration to be a sentinel value.
Comment Actions I agree with you, but we need to consider that the checker works in a way that it gives the "zero" for integers. If we are here, was that the right decision? I mean... I wonder how much consistency we should shoot for. (nullptr for the pointers as default is at least somewhat sensible.) But definitely, the warning must be given, that is true. Comment Actions If we're here, I think the right decision would have been to not support the C++ Core Guidelines in clang-tidy in the first place because of the lack of attention that's been paid to enforcement for the rules by the authors. ;-) But snark aside, I think = 0 is a somewhat defensible naïve default value for integers, but today I would prefer it give its fix-it on a note rather than on the warning. When there's only one fix-it to be applied to a warning, it's possible to apply the fix-it without any per-case user intervention and that default value isn't statically known to be safe. With nullptr for pointers, it's more defensible because that's a well-understood sentinel value that code *should* be checking for (and there's good tooling to catch null pointer dereferences for the other situations). With NAN for floats, it's more defensible because that (usually!) is a "sticky" value that poisons further uses of the type. There's nothing quite like that for integers.
There are at least two ways to chase consistency here -- one is being consistent with other fix-its in the check, the other is being consistent with other fix-its across the project. My feeling is we should aim for consistency with the rest of the toolchain, which is to only issue a fix-it on a warning when the fix is known to be correct. https://clang.llvm.org/docs/InternalsManual.html#fix-it-hints Perhaps one way forward would be to issue the integer fix-it on a note rather than a warning, and for enumerations, we could issue up to two fix-its on a note, one for the first and one for the last enumerator in an enumeration (and if the enum only contains one enumerator, there's only one fix-it to generate which suggests it could be on the warning rather than a note, but that seems like a lot of trouble for an unlikely scenario). However, I don't recall how clang-tidy interacts with fix-its on notes off the top of my head, so I'm making an assumption that clang-tidy's automatic fixit applying mode handles notes the same way as clang and we should double-check that assumption. Another way forward would be to not issue a fix-it for integers or enumerations. Comment Actions I have one information from January that if you're viewing the diagnostic output as a sequence of [warning, note, note, ...] elements (so you "group by" warning), Clang-Tidy will apply the first fix (be it on the warning or the note) in the order of diag() calls. (There was a (never-upstreamed) check in which I had to abuse this fact.) This behaviour could've changed, though... This might be the best course of action, and could be fixed in the same patch (this one)... Comment Actions I think that's a reasonable way forward, though I don't insist on changing the integer behavior if others have strong opinions that it is correct. I do have strong opinions on fixing the enumeration behavior because that fix-it is wrong for C++ code. Comment Actions I may be more supportive of the first view. Based on clang -Wuninitialized, we hope that clang-tidy can do more. In addition, if there are two recommended values for enum initialization, we can put them in one note. Comment Actions First, let's first fix that we should still warn for the uninitialised enum case, without a FixIt. That's the issue at hand, right now, Clang-Tidy generates, as you identified, broken output. We can discuss the later steps after this is fixed. Please implement this logic, and update the patch, so we have a snapshot of how that would look like and the thing working. Afterwards, as Aaron suggested: This might require nontrivial changes to the check's code, and investigating the potential problem with automated fix-it application when multiple conflicting fix-its are given on a note (not a warning line). I think we all would be fine with only doing the first step (reintroduce the warning, without a fixit) in this patch, so it can be merged and hit the the mainline code quickly. The next step can be its own patch. 🙂 In addition, I wager that adding a line about this fix to the release notes at clang-tools-extra/docs/ReleaseNotes.rst is a good option, I can imagine people having turned this check off due to it being broken on enums. Comment Actions +1
I think, the intention of the auto fix for such case is to make the program's behavior well-defined, not with some uninitialize value that cannot be predicted. It is ok as far as we select any enumerator to initialize it from my understanding. We should separate it into anther patch of cause.
Comment Actions Alright, thanks! So it does work that way, I didn't know. I think this is okay from my end. You can mark the inline discussion done, by the way. 🙂 Let's see if @aaron.ballman has any comments. Comment Actions LGTM -- it's not the cleanest solution (we're duplicating the call to diag()), but it'll suffice. If we wanted to clean it up a bit, we could use an llvm::Optional<const char *> Diagnose = llvm::None; where a non-None value means "diagnose" and if the stored pointer value is not null, it additionally adds the fixit information. Comment Actions Thank you for your reply! I generally understand what you mean, but there are three cases, namely, no diagnosis, providing diagnosis, and providing diagnosis & FixIt. Only by judging whether "diagnosis" is null may fail to achieve the goal. I have revised it a little bit, do you think that's alright? @aaron.ballman Comment Actions If we are tinkering with the code post-acceptance, could you please also change const char * to StringRef? They are the same thing essentially, but accessing the length is (AFIAK) O(1) and there isn't a concerning strlen call in the code.
Comment Actions Cheers! Can it be merged? @aaron.ballman
Comment Actions Yes, but I think the patch needs to be rebased first as the pre-merge checks CI system said "patch application failed". Do you have commit access? If not, could you please specify the username and e-mail (the values of Git config variable user.name and user.email) you would like to have associated with the commit? Comment Actions @Sockke Sorry to circle back here, but it seems to me that Clang now has a flag -Wuninitialized. Could you check how it behaves, compared to this check? If there are overlaps, what should we do, @aaron.ballman? Should parts of the check be deprecated? | ||||||||||||||||||||||||||||||||||||||||||||||||
Also, if we're using Optional, this could also be = nullptr instead, and then we have the state when the Optional hasValue() but the value itself is null. In all other cases, it both hasValue() and the value is non-null.
This would also eliminate the need for the strlen call.