Page MenuHomePhabricator

[clang-tidy] New check: misc-init-local-variables
ClosedPublic

Authored by jpakkane on Jul 12 2019, 3:04 PM.

Details

Summary

This checks finds all primitive type local variables (integers, doubles, pointers) that are declared without an initial value. Includes fixit functionality to initialize said variables with a default value. This is zero for most types and NaN for floating point types. The use of NaNs is copied from the D programming language.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jpakkane marked 2 inline comments as done.Jul 15 2019, 9:50 AM
jpakkane added inline comments.
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp
21 ↗(On Diff #209706)

I tested with the following function:

template<typename T>
void template_test_function() {
  T t;
  int uninitialized;
}

Currently it warns on the "uninitialized" variable regardless of whether the template is instantiated or not. If you call it with an int type, it will warn about variable t being uninitialized. If you call it with a, say, struct type, there is no warnings. Is this a reasonable approach?

32 ↗(On Diff #209706)

I can change that, seems reasonable. Should it still retain this check, though? One would imagine there are other ways of getting variables whose names begin with an underscore.

alexfh added inline comments.Jul 16 2019, 5:15 AM
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp
21 ↗(On Diff #209706)

And what happens, if there are multiple instantiations of the same template, each of them requiring a different fix? Can you try the check with my example above (and maybe also add f("");inside g()). I believe, the check will produce multiple warnings with conflicting fixes (and each of them will be wrong, btw).

32 ↗(On Diff #209706)

I don't know, whether the check for leading underscore will still be valuable. I don't think there's a valid reason why variables with a leading underscore in their names should in general not be initialized.

jpakkane marked an inline comment as done.Jul 16 2019, 2:29 PM
jpakkane added inline comments.
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp
21 ↗(On Diff #209706)

Interestingly it does warn about it, but only once, even if you have two different template specializations.

I tried to suppress this warning when the type being instantiated is a template argument type but no matter what I tried I could not get this to work. Is there a way to get this information from the MatchedDecl object or does one need to do something more complicated like going up the AST until a function definition is found and checking if it is a template specialization (presumably with TemplatedKind)? Any help would be appreciated.

alexfh added inline comments.Jul 17 2019, 6:47 AM
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp
21 ↗(On Diff #209706)

If there are multiple warnings with the same message at the same location (clang-tidy/ClangTidyDiagnosticConsumer.cpp:745), they will be deduplicated. Thus, a random fix will probably be suggested. The proper way to filter out matches in template instantiations is to add unless(isInTemplateInstantiation()) to the matcher.

jpakkane marked an inline comment as done.Jul 17 2019, 8:48 AM
jpakkane added inline comments.
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp
21 ↗(On Diff #209706)

I tried to make this work but I just could not combine statement and declaration matching in a reliable way. Matching a statement that is not in a template declaration can be done, as well as matching a declaration without intial value, but combining those two into one is hard. After trying many, many things the best I could come up with was this:

declStmt(containsDeclaration(0, varDecl(unless(hasInitializer(anything()))).bind("vardecl"))), this)

The problem is that containsDeclaration takes an integer denoting how manyth declaration should be processed. Manually adding matchers for, say, 0, 1, 2, 3 and 4 works and does the right thing but fails if anyone has an uninitialized variable in the sixth location, things will silently fail.

The weird thing is that if you do the matching this way, you don't need to filter out things with unless(isInTemplateInstantiation()). Maybe statements are handled differently from declarations?

alexfh added inline comments.Jul 18 2019, 5:13 AM
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp
21 ↗(On Diff #209706)

I was struggling to understand, why you want to match a statement, but then I figured out that I should have been more precise: while isInTemplateInstantiation only works for Stmts, there's a related matcher that works for Decls: isInstantiated. See clang/include/clang/ASTMatchers/ASTMatchers.h:5187. In general, looking into this header can be useful, if you want to find a matcher that you can vaguely describe (e.g. when looking for something related to instantiations, you can search for the relevant substring and find this and a bunch of other matchers).

Sorry for the confusion. I hope, the suggestion helps.

jpakkane updated this revision to Diff 210868.Jul 19 2019, 11:08 AM

Now properly deals with template instantiations and macros.

jpakkane marked 4 inline comments as done.Jul 19 2019, 11:10 AM
jpakkane added inline comments.
clang-tools-extra/clang-tidy/misc/InitLocalVariablesCheck.cpp
21 ↗(On Diff #209706)

Thanks, got it working now.

32 ↗(On Diff #209706)

Underscore check has been removed and macros are properly handled (description is in code comments).

A general comment: "misc" is a sort of a heap of checks that otherwise don't have a good home. This one would probably better go to bugprone (or maybe there's a relevant CERT or C++ Core Guidelines rule?).

jpakkane marked 2 inline comments as done.Jul 30 2019, 5:33 AM

A general comment: "misc" is a sort of a heap of checks that otherwise don't have a good home. This one would probably better go to bugprone (or maybe there's a relevant CERT or C++ Core Guidelines rule?).

The closest I could find is ES.20 "Always initialize an object": https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es20-always-initialize-an-object

Most other guidelines seem to be formulated as "don't read uninitialized memory" such as: https://cwe.mitre.org/data/definitions/457.html

So a potential name for this could be "cppcoreguidelines-init-variables". However that rule is about initializing all variables, even those in structs and classes. This is about local variables.

Related to this, there seems to be a talk at CppCon about doing something similar to this MR: https://cppcon2019.sched.com/event/SfYc/killing-uninitialized-memory-prot

A general comment: "misc" is a sort of a heap of checks that otherwise don't have a good home. This one would probably better go to bugprone (or maybe there's a relevant CERT or C++ Core Guidelines rule?).

The closest I could find is ES.20 "Always initialize an object": https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es20-always-initialize-an-object

So a potential name for this could be "cppcoreguidelines-init-variables". However that rule is about initializing all variables, even those in structs and classes. This is about local variables.

I am in favor of something like cppcoreguidelines-init-variables that might catch other cases in the future as well. IMHO its fine to start with something partial, especially "just" local variables.

jpakkane updated this revision to Diff 212664.Jul 31 2019, 2:11 PM

Renamed to cppcoreguidelines-init-variables.

Does this still need work? FWICT all issues raised have been fixed.

Eugene.Zelenko added inline comments.Aug 24 2019, 6:52 AM
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp
4

It'll be better to include cstdint.

lebedev.ri added inline comments.Aug 24 2019, 8:04 AM
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp
4

I'll correct that comment: tests normally shouldn't depend on system headers; can you just mock whatever you need from there?

Eugene.Zelenko added inline comments.Aug 24 2019, 8:06 AM
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp
4

If test is C++, why not to include C++ equivalent of system headers? See modernize-deprecated-headers.

aaron.ballman added inline comments.Aug 24 2019, 8:09 AM
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp
4

Because we want our tests to be hermetic and rely on the system running the test as little as possible.

lebedev.ri added inline comments.Aug 24 2019, 8:10 AM
clang-tools-extra/test/clang-tidy/cppcoreguidelines-init-variables.cpp
4

I'm not commenting on c-vs-c++ header, i'm specifically commenting on an <> include; unless i'm misremembering this is advised against.

I used stdint to replicate a real world use case as I'd imagine those types would match this search quite heavily.

The tests already have one test for a typedeffed integer and one that is defined with a macro. If those are deemed sufficient, the stdint type can be removed altogether. If the stdint types are defined via some other mechanism such as compiler intrinsics, there would be no test for that. I do not know if that is the case with current libcs.

I can update the MR as requested once someone with enough stripes makes the policy decision.

I used stdint to replicate a real world use case as I'd imagine those types would match this search quite heavily.

The problem is that this uses the system header search paths and so the testing system has no control over *what* is found from bot to bot. That is why we structure almost all of our tests to not use system includes.

The tests already have one test for a typedeffed integer and one that is defined with a macro. If those are deemed sufficient, the stdint type can be removed altogether. If the stdint types are defined via some other mechanism such as compiler intrinsics, there would be no test for that. I do not know if that is the case with current libcs.

I don't see a need for int32_t specifically in this test.

I can update the MR as requested once someone with enough stripes makes the policy decision.

I'm not certain that we've written this instruction down so much as let new contributors know when they stumble into it. It would probably be a good idea for us to get some testing documentation written down at some point. Despite that, our policy has been to not use #include in test cases unless the included file is relative to the test.

jpakkane updated this revision to Diff 217958.Aug 29 2019, 12:44 PM

Updated patch to remove #include<stdint.h> and the corresponding uint32_t test code snippet.

aaron.ballman added inline comments.Aug 29 2019, 1:12 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
46

Please keep this list sorted alphabetically by string literal.

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

This should not match on variable declarations that are automatically zero-initialized or have a default constructor, no?

30

You can move this closer to where it's being used.

32–33

I think this should be made into a local AST matcher so the check can be hoisted into the matchers.

54

No need for this local; you can lower into its only use.

57

You should elide braces for any single-statement blocks per our coding conventions.

60

I would rather see the correct NaN inserted along with the include. See StringFindStartswithCheck.cpp for an example of how to use the include inserter.

61

Do you expect this check to be run over ObjC code? If so, this should probably be isAnyPointerType().

62

This should be checking for C++11.

jpakkane updated this revision to Diff 218392.Sep 2 2019, 2:23 PM

Updated to fix review comments. NOTE: detecting the include fix is broken because I could not get it to work.

jpakkane marked 9 inline comments as done.Sep 2 2019, 2:33 PM
jpakkane added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
22

That is correct. The test file has checks for global variables and struct members without initialisation.

32–33

As was discussed earlier, the use of isLocalVarDecl is used here because:

  • it has the exact semantics needed in this particular case
  • it is not available as a matcher, I could not make plain matchers replicate its behaviour and even if I could, reimplementing it from scratch seems a bit pointless

If there is a way to use that function directly in the matcher code, let me know how it's done and I will fix this. But if not, then sadly I have no idea how that should be fixed and it is probably a bigger development task than this checker itself.

60

I copied the implementation and could make it add the update. However I could not for the life of me make the test framework accept this. It will always flag an error even though there should be a comment that declares it in the test source. Any help is appreciated.

aaron.ballman added inline comments.Sep 3 2019, 9:48 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
32–33

You can create your own AST matchers that are used only in this translation unit. e.g.,

AST_MATCHER(VarDecl, isLocalVarDecl) {
  return Node.isLocalVarDecl();
}

and then use this in your AST matchers.

60

So the include is inserted as you expect, but the test continues to fail? What does your test look like for it? What failures are you getting?

77

In C++11 mode, I think this should recommend std::numeric_limits<type>::quiet_NaN() if possible, from <limits> rather than <math.h>.

87–102

I think you want this to be:

auto Diagnostic = diag(MatchedDecl->getLocation(), "variable %0 is not initialized")
      << MatchedDecl
      << FixItHint::CreateInsertion(
             MatchedDecl->getLocation().getLocWithOffset(
                 MatchedDecl->getName().size()),
             InitializationString);
  if (AddMathInclude) {
    auto IncludeHint = IncludeInserter->CreateIncludeInsertion(
        Source.getFileID(MatchedDecl->getBeginLoc()), MathHeader, false);
    if (IncludeHint)
      Diagnostic << *IncludeHint;
  }
jpakkane updated this revision to Diff 218511.Sep 3 2019, 12:10 PM

Updated patch as per review comments.

jpakkane marked 6 inline comments as done.Sep 3 2019, 12:19 PM
jpakkane added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
32–33

Fixed, thanks.

60

Just the test in this patch. However after updating the diagnostic code to the one recommended made this problem go away. This now works as intended.

77

As a stylistic point I would disagree with this. In this particular case the "C++ way" looks very verbose and confusing. For a single variable this is not a huge problem, but it gets very ugly when you have something like this (which is common in real world code bases):

double d1, d2, d3, d4, d5, d6, d7;

This just explodes and is very ugly and unpleasant to read when every one of these gets the std::numeric_limits template value. I would recommend using plain NAN for terseness and readability, but can update the PR if the template form is deemed the better choice.

aaron.ballman added inline comments.Sep 4 2019, 3:09 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
67

Elide braces.

77

Let's go with NAN for now. We can always add an option letting the user pick which style to replace with later.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-init-variables.rst
34

This looks incorrect.

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

You can drop this and its uses, we don't care about warnings produced by Clang about the variables not being used.

37

I'd like to see a test using a local static (which should not diagnose, as it's zero-initialized by default) and a local extern (which should not diagnose because it cannot have an initializer).

56

I'd like to see some tests for other initialization forms as well:

int braces{42};
int parens(42);
jpakkane updated this revision to Diff 218970.Sep 5 2019, 1:10 PM
jpakkane marked 2 inline comments as done.

Fixed issues raised in review.

jpakkane marked 6 inline comments as done.Sep 5 2019, 1:11 PM

Aside from the missing documentation bit, I think this LG.

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

You need to document this option.

jpakkane updated this revision to Diff 219162.Sep 6 2019, 1:48 PM

Added documentation. Well, actually just swiped it from abseil-string-find-startswith.

jpakkane marked an inline comment as done.Sep 6 2019, 1:48 PM

It'll be reasonable to get IncludeStyle default from .clang-format.

clang-tools-extra/docs/ReleaseNotes.rst
71

Please sort new checks list alphabetically.

jpakkane marked an inline comment as done.Sep 6 2019, 2:13 PM

It'll be reasonable to get IncludeStyle default from .clang-format.

I looked at existing checks and they all do the same thing as this one. In fact I got the code for this by directly copypasting an existing check.

Grepping for clang format in the check tree produces zero relevant matches, so it's unclear to me how this should be written (assuming that this is not taken care of automatically by utility code).

clang-tools-extra/docs/ReleaseNotes.rst
71

The list is already incorrectly alphabetized. Should I reorganize all the entries or just put mine in a "correct" spot (between "bugprone" and "linuxkernel")?

It'll be reasonable to get IncludeStyle default from .clang-format.

I looked at existing checks and they all do the same thing as this one. In fact I got the code for this by directly copypasting an existing check.

SInse this option may be also set in .clang-format, it's reasonable to re-use it instead of forcing user to duplicate it.

clang-tools-extra/docs/ReleaseNotes.rst
71

It'll not hurt to correct other mistakes too.

jpakkane updated this revision to Diff 219179.Sep 6 2019, 2:27 PM

Ordered doc list alphabetically.

jpakkane marked an inline comment as done.Sep 6 2019, 2:28 PM
aaron.ballman added inline comments.Sep 17 2019, 6:24 AM
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-init-variables.rst
41

This is still missing the documentation for MathHeader and its default value.

jpakkane updated this revision to Diff 221376.Sep 23 2019, 11:34 AM

Added MathHeader documentation.

jpakkane marked an inline comment as done.Sep 23 2019, 11:35 AM
This revision is now accepted and ready to land.Sep 23 2019, 12:40 PM
jpakkane updated this revision to Diff 222335.Sep 29 2019, 12:05 PM

Rebased against master to fix a merge conflict in release notes.

Do you need someone to commit this on your behalf (sorry for not asking that question sooner)?

Do you need someone to commit this on your behalf (sorry for not asking that question sooner)?

Yes, please. I have no rights of any kind, this is in fact my first ever pull request to LLVM.

Do you need someone to commit this on your behalf (sorry for not asking that question sooner)?

Yes, please. I have no rights of any kind, this is in fact my first ever pull request to LLVM.

I'm happy to commit for you, but applying the patch causes merge conflicts. Can you rebase on ToT?

jpakkane updated this revision to Diff 222850.Oct 2 2019, 9:42 AM

Rebased against latest Git master. Didn't see any rebase conflicts, though...

aaron.ballman closed this revision.Oct 2 2019, 10:17 AM

Rebased against latest Git master. Didn't see any rebase conflicts, though...

Oh! I just noticed you made the patch against the monorepo which I'm not using (I'm still on svn), so the paths in your patch file are all wrong for me. After some surgery on the paths, it applied cleanly for me.

I committed on your behalf in r373489. Thank you for the new feature!