[analyzer][UninitializedObjectChecker] Fixed captured lambda variable name

[george.karpenkov]

Looks good overall, but some nits inline.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

683

Can we have a comment explaining the semantics? From my understanding, you find a corresponding capture using a source location, and get a name from there.

686

CXXParent is guaranteed to be non-null at this stage, otherwise dyn_cast fails

688

Could we just use a for-each here? Sorry for being opinionated, I'm pretty sure it would be both shorter and more readable.

690

That's exactly the semantics of the equality operator on source locations, so why not Field->getLocation() == L.getLocation() ?

[Szelethus]

Thanks for the review!

I spend some time thinking about the support for lambda functions, and I start to think that checking for lambda misuse shouldn't be the responsibility of this checker. I created a new revision for that discussion, I wouldn't like to abandon this one until I heard what you think of this! :)
D48318

edit: I was wrong on this one, sorry about that, so please just ignore it. I'll update this diff tomorrow.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

688

Great idea! :) I completely agree, to be honest I scratched my head too while I wrote it down.

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

686

I found this on http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates:

dyn_cast<>:

The dyn_cast<> operator is a “checking cast” operation. It checks to see if the operand is of the specified type, and if so, returns a pointer to it (this operator does not work with references). If the operand is not of the correct type, a null pointer is returned.

So I guess this should be alright.

[george.karpenkov]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

686

Oops sorry my bad.

[Szelethus]

Fixes according to inline comments.

[NoQ]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

691

Uhm, this source location comparison looks really flaky. Ideally we would have looked up by field and get the captured variable directly, but it seems that we don't have a ready-made method for this.

The second ideal thing would probably be a reverse variant of CXXRecordDecl::getCaptureFields(). Looking at how it's implemented, it seems that capture indix is in sync with field index. Because we have FieldDecl::getFieldIndex(), probably we could just lookup the captures array by that index?

[george.karpenkov]

Few more nits.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

683

Is this a member method? Then it should be prefixed with a class name.
Currently it looks like you have a member method without an implementation and a separate C-style function?

684

Doxygen-style comments in LLVM start with a triple slash, and are located above the function. Then the tools for autocompletion (e.g. clangd) can pick them up.

[george.karpenkov]

Looks like @NoQ had some valid comments as well.

[Szelethus]

Finding the correct captured variable now works with FieldDecl::getFieldIndex().

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

683

It is a statically defined method, and you can see its forward declaration in the same diff.

Back when I started writing this checker, the amount of function laying in the anonymous namespace was very few, but this has changed. I'll fix it sometime because it's getting somewhat annoying (and is against the coding standards).

[NoQ]

Looks good!

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

683

Yeah, just make them static, it's easier to read (https://llvm.org/docs/CodingStandards.html#anonymous-namespaces).

[Szelethus]

Thanks! :)

Rebased to rC336994.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp

 /// either a BuiltinType or an EnumeralType.
 bool isPrimitiveType(const QualType &T) {
   return T->isBuiltinType() || T->isEnumeralType();
 }
 
 /// Constructs a note message for a given FieldChainInfo object.
 void printNoteMessage(llvm::raw_ostream &Out, const FieldChainInfo &Chain);
 
+/// Returns with Field's name. This is a helper function to get the correct name
+/// even if Field is a captured lambda variable.
+StringRef getVariableName(const FieldDecl *Field);
+
 } // end of anonymous namespace
 
 //===----------------------------------------------------------------------===//
 //                  Methods for UninitializedObjectChecker.
 //===----------------------------------------------------------------------===//
 
 void UninitializedObjectChecker::checkEndFunction(
     CheckerContext &Context) const {

 //   struct MostDerived : D1, D2 {
 //     MostDerived() {}
 //   }
 //
 // A call to MostDerived::MostDerived() will cause two notes that say
 // "uninitialized field 'this->x'", but we can't refer to 'x' directly,
 // we need an explicit namespace resolution whether the uninit field was
 // 'D1::x' or 'D2::x'.
-//
-// TODO: If a field in the fieldchain is a captured lambda parameter, this
-// function constructs an empty string for it:
-//
-//   template <class Callable> struct A {
-//     Callable c;
-//     A(const Callable &c, int) : c(c) {}
-//   };
-//
-//   int b; // say that this isn't zero initialized
-//   auto alwaysTrue = [&b](int a) { return true; };
-//
-// A call with these parameters: A<decltype(alwaysTrue)>::A(alwaysTrue, int())
-// will emit a note with the message "uninitialized field: 'this->c.'". If
-// possible, the lambda parameter name should be retrieved or be replaced with a
-// "<lambda parameter>" or something similar.
 void FieldChainInfo::print(llvm::raw_ostream &Out) const {
   if (Chain.isEmpty())
     return;
 
   const llvm::ImmutableListImpl<const FieldRegion *> *L =
       Chain.getInternalPointer();
   printTail(Out, L->getTail());
-  Out << L->getHead()->getDecl()->getNameAsString();
+  Out << getVariableName(L->getHead()->getDecl());
 }
 
 void FieldChainInfo::printTail(
     llvm::raw_ostream &Out,
     const llvm::ImmutableListImpl<const FieldRegion *> *L) {
   if (!L)
     return;
 
   printTail(Out, L->getTail());
   const FieldDecl *Field = L->getHead()->getDecl();
-  Out << Field->getNameAsString();
+  Out << getVariableName(Field);
   Out << (Field->getType()->isPointerType() ? "->" : ".");
 }
 
 //===----------------------------------------------------------------------===//
 //                           Utility functions.
 //===----------------------------------------------------------------------===//
 
 namespace {
if (Chain.isPointer()) {
     else
       Out << "uninitialized pointer 'this->";
   } else
     Out << "uninitialized field 'this->";
   Chain.print(Out);
   Out << "'";
 }
 
+StringRef getVariableName(const FieldDecl *Field) {
+  // If Field is a captured lambda variable, Field->getName() will return with
+  // an empty string. We can however acquire it's name from the lambda's
+  // captures.
+  const auto *CXXParent = dyn_cast<CXXRecordDecl>(Field->getParent());
+
+  if (CXXParent && CXXParent->isLambda()) {
+    assert(CXXParent->captures_begin());
+    auto It = CXXParent->captures_begin() + Field->getFieldIndex();
+    return It->getCapturedVar()->getName();
+  }
+
+  return Field->getName();
+}
+
 } // end of anonymous namespace
 
 void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) {
   auto Chk = Mgr.registerChecker<UninitializedObjectChecker>();
   Chk->IsPedantic = Mgr.getAnalyzerOptions().getBooleanOption(
       "Pedantic", /*DefaultVal*/ false, Chk);
   Chk->ShouldConvertNotesToWarnings = Mgr.getAnalyzerOptions().getBooleanOption(
       "NotesAsWarnings", /*DefaultVal*/ false, Chk);
 }

test/Analysis/cxx-uninitialized-object.cpp

void fMemsetTest2() {
   MemsetTest2();
 }
 
 //===----------------------------------------------------------------------===//
 // Lambda tests.
 //===----------------------------------------------------------------------===//
 
 template <class Callable>
+struct LambdaThisTest {
+  Callable functor;
+
+  LambdaThisTest(const Callable &functor, int) : functor(functor) {
+    // All good!
+  }
+};
+
+struct HasCapturableThis {
+  void fLambdaThisTest() {
+    auto isEven = [this](int a) { return a % 2 == 0; }; // no-crash
+    LambdaThisTest<decltype(isEven)>(isEven, int());
+  }
+};
+
+template <class Callable>
 struct LambdaTest1 {
   Callable functor;
 
   LambdaTest1(const Callable &functor, int) : functor(functor) {
     // All good!
   }
 };
 
 void fLambdaTest1() {
   auto isEven = [](int a) { return a % 2 == 0; };
   LambdaTest1<decltype(isEven)>(isEven, int());
 }
 
 #ifdef PEDANTIC
 template <class Callable>
 struct LambdaTest2 {
   Callable functor;
 
   LambdaTest2(const Callable &functor, int) : functor(functor) {} // expected-warning{{1 uninitialized field}}
 };
 
 void fLambdaTest2() {
   int b;
-  auto equals = [&b](int a) { return a == b; }; // expected-note{{uninitialized field 'this->functor.'}}
+  auto equals = [&b](int a) { return a == b; }; // expected-note{{uninitialized field 'this->functor.b'}}
   LambdaTest2<decltype(equals)>(equals, int());
 }
 #else
 template <class Callable>
 struct LambdaTest2 {
   Callable functor;
 
   LambdaTest2(const Callable &functor, int) : functor(functor) {}
 };
 
 void fLambdaTest2() {
   int b;
   auto equals = [&b](int a) { return a == b; };
   LambdaTest2<decltype(equals)>(equals, int());
 }
 #endif //PEDANTIC
 
 #ifdef PEDANTIC
 namespace LT3Detail {
 
 struct RecordType {
-  int x; // expected-note{{uninitialized field 'this->functor..x'}}
-  int y; // expected-note{{uninitialized field 'this->functor..y'}}
+  int x; // expected-note{{uninitialized field 'this->functor.rec1.x'}}
+  int y; // expected-note{{uninitialized field 'this->functor.rec1.y'}}
 };
 
 } // namespace LT3Detail
 template <class Callable>
 struct LambdaTest3 {
   Callable functor;
 
   LambdaTest3(const Callable &functor, int) : functor(functor) {} // expected-warning{{2 uninitialized fields}}
void fLambdaTest3() {
   LT3Detail::RecordType rec1;
   auto equals = [&rec1](LT3Detail::RecordType rec2) {
     return rec1.x == rec2.x;
   };
   LambdaTest3<decltype(equals)>(equals, int());
 }
 #endif //PEDANTIC
 
+template <class Callable>
+struct MultipleLambdaCapturesTest1 {
+  Callable functor;
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  MultipleLambdaCapturesTest1(const Callable &functor, int) : functor(functor) {} // expected-warning{{2 uninitialized field}}
+};
+
+void fMultipleLambdaCapturesTest1() {
+  int b1, b2 = 3, b3;
+  auto equals = [&b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized field 'this->functor.b1'}}
+  // expected-note@-1{{uninitialized field 'this->functor.b3'}}
+  MultipleLambdaCapturesTest1<decltype(equals)>(equals, int());
+}
+
+template <class Callable>
+struct MultipleLambdaCapturesTest2 {
+  Callable functor;
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  MultipleLambdaCapturesTest2(const Callable &functor, int) : functor(functor) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fMultipleLambdaCapturesTest2() {
+  int b1, b2 = 3, b3;
+  auto equals = [b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized field 'this->functor.b3'}}
+  MultipleLambdaCapturesTest2<decltype(equals)>(equals, int());
+}
+
 //===----------------------------------------------------------------------===//
 // System header tests.
 //===----------------------------------------------------------------------===//
 
 #include "Inputs/system-header-simulator-for-cxx-uninitialized-object.h"
 
 struct SystemHeaderTest1 {
   RecordInSystemHeader rec; // defined in the system header simulator