[clang-tidy] Fix `cppcoreguidelines-init-variables` for invalid vardecl

[Sockke]

Summary

https://godbolt.org/z/n4cK4fo3o
The checker missed a check for invalid vardecl and this could cause a false positive.

[Eugene.Zelenko]

Will be good idea to add test for this situation.

[Sockke]

Added test.

[Eugene.Zelenko]

It'll be better to expand existing init-variables.cpp test.

[Sockke]

Added test in existing init-variables.cpp file.

[Sockke]

Friendly ping.

[Sockke]

Friendly ping.

[carlosgalvezp]

Looks good in general, IMHO it would be good with a couple more comments to make the code and test easier to understand.

clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp

62

Add a comment as to why this is needed? I haven't seen this pattern often in checks.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp

4

Add a comment explaining the purpose of introducing this error? At first sight, it's not obvious to me why this is needed.

132

Perhaps add a comment explaining that clang-tidy should not warn here, and why.

[Sockke]

Added comments

[carlosgalvezp]

Now that I think a bit better about this I wonder - does it really make sense that we increase the complexity of the check to cover for cases where code does not compile? If it fails to include a header, there's many other things that can possibly go wrong - should clang-tidy checks in general really be defensive against that? @njames93 WDYT?

[njames93]

In D138655#4007822, @carlosgalvezp wrote:

Now that I think a bit better about this I wonder - does it really make sense that we increase the complexity of the check to cover for cases where code does not compile? If it fails to include a header, there's many other things that can possibly go wrong - should clang-tidy checks in general really be defensive against that? @njames93 WDYT?

It's not a hard rule that we should be defensive against clang-tidy emitting false positives due to a previous compilation error. However for simple cases, like this, where its just ensuring a declaration is valid before we try to emit diagnostic, It does make sense to handle this.
With this change the main thing would be a little less noise while trying to debug the cause of the compilation error which is always a good thing.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp

4–13

All this is unnecessary there is a much nicer way to test this, see below

129–131

This test line is not testing the behaviour described in this patch and can be removed.

134

Just use this and the declaration will be invalid and you can test the expected behaviour

[Sockke]

Improved the test file.

[njames93]

In D138655#4012557, @Sockke wrote:

Improved the test file.

Just a general workflow comment, can you please mark comments as done when they are addressed, or, if you feel the comment in unjustified, explain why.

[Sockke]

Mark comments.

[Sockke]

Friendly ping.

[Sockke]

Friendly ping.

[Sockke]

Hi, Could anyone please review this diff?

[carlosgalvezp]

LGTM, thanks for the fix!

[carlosgalvezp]

@Sockke do you need help landing the patch? If so, let me know user and email for attribution :)

[carlosgalvezp]

@Sockke ping, can you land this or should we do it for you? If so, please let us know what name and email we should use for attribution.

[Sockke]

Rebased.

[Sockke]

@carlosgalvezp Thanks for your review! I committed it myself.

clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp

void InitVariablesCheck::registerPPCallbacks(const SourceManager &SM,
   IncludeInserter.registerPreprocessor(PP);
 }
 
 void InitVariablesCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("vardecl");
   const ASTContext &Context = *Result.Context;
   const SourceManager &Source = Context.getSourceManager();
 
+  // Clang diagnostic error may cause the variable to be an invalid int vardecl
+  if (MatchedDecl->isInvalidDecl())
+    return;
+
   // We want to warn about cases where the type name
   // comes from a macro like this:
   //
   // TYPENAME_FROM_MACRO var;
   //
   // but not if the entire declaration comes from
   // one:
   //

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/init-variables.cpp

-// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables %t -- -- -fno-delayed-template-parsing -fexceptions
+// RUN: %check_clang_tidy %s cppcoreguidelines-init-variables -fix-errors %t -- -- -fno-delayed-template-parsing -fexceptions
 // CHECK-FIXES: {{^}}#include <math.h>
 
 // Ensure that function declarations are not changed.
 void some_func(int x, double d, bool b, const char *p);
 
 // Ensure that function arguments are not changed
 int identity_function(int x) {
   return x;
 }
 
 int do_not_modify_me;
 
 static int should_not_be_initialized;
 extern int should_not_be_initialized2;
 
 typedef struct {
   int unaltered1;
   int unaltered2;
 } UnusedStruct;
 
void uninitialized_enum() {
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'car' is not initialized [cppcoreguidelines-init-variables]
   Gender gender;
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: variable 'gender' is not initialized [cppcoreguidelines-init-variables]
   Direction direction;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: variable 'direction' is not initialized [cppcoreguidelines-init-variables]
   Fruit fruit;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'fruit' is not initialized [cppcoreguidelines-init-variables]
 }
+
+void test_clang_diagnostic_error() {
+  int a;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'a' is not initialized [cppcoreguidelines-init-variables]
+  // CHECK-FIXES: {{^}}  int a = 0;{{$}}
+
+  UnknownType b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: unknown type name 'UnknownType' [clang-diagnostic-error]
+  // CHECK-FIXES-NOT: {{^}}  UnknownType b = 0;{{$}}
+}