[analyzer][UninitializedObjectChecker] Explicit namespace resolution for inherited data members

[Szelethus]

Summary

For the following example:

struct Base {
  int x;
};

// In a different translation unit

struct Derived : public Base {
  Derived() {}
};

For a call to Derived::Derived(), we'll receive a note that this->x is uninitialized. Since x is not a direct field of Derived, it could be a little confusing. This patch aims to fix this, as well as the case when the derived object has a field that has the name as an inherited uninitialized data member:

struct Base {
  int x; // note: uninitialized field 'this->Base::x'
};

struct Derived : public Base {
  int x = 5;
  Derived() {}
};

This patch is also meant as a conversation starter, as the way base classes were handled has been long debated.

[NoQ]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

114–115

Maybe just BaseClassT->getAsCXXRecordDecl()->getName()?

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h

 namespace ento {
 
 /// Represent a single field. This is only an interface to abstract away special
 /// cases like pointers/references.
 class FieldNode {
 protected:
   const FieldRegion *FR;
 
-  ~FieldNode() = default;
+  /* non-virtual */ ~FieldNode() = default;
 
 public:
-  FieldNode(const FieldRegion *FR) : FR(FR) { assert(FR); }
+  FieldNode(const FieldRegion *FR) : FR(FR) {}
 
   FieldNode() = delete;
   FieldNode(const FieldNode &) = delete;
   FieldNode(FieldNode &&) = delete;
   FieldNode &operator=(const FieldNode &) = delete;
   FieldNode &operator=(const FieldNode &&) = delete;
 
   /// Profile - Used to profile the contents of this object for inclusion in a
   /// FoldingSet.
   void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddPointer(this); }
 
-  bool operator<(const FieldNode &Other) const { return FR < Other.FR; }
-  bool isSameRegion(const FieldRegion *OtherFR) const { return FR == OtherFR; }
+  // Helper method for uniqueing.
+  bool isSameRegion(const FieldRegion *OtherFR) const {
+    // Special FieldNode descendants may wrap nullpointers -- we wouldn't like
+    // to unique these objects.
+    if (FR == nullptr)
+      return false;
+
+    return FR == OtherFR;
+  }
 
   const FieldRegion *getRegion() const { return FR; }
-  const FieldDecl *getDecl() const { return FR->getDecl(); }
+  const FieldDecl *getDecl() const {
+    assert(FR);
+    return FR->getDecl();
+  }
 
   // When a fieldchain is printed (a list of FieldNode objects), it will have
   // the following format:
   // <note message>'<prefix>this-><node><separator><node><separator>...<node>'
 
   /// If this is the last element of the fieldchain, this method will be called.
   /// The note message should state something like "uninitialized field" or
   /// "uninitialized pointee" etc.
   virtual void printNoteMsg(llvm::raw_ostream &Out) const = 0;
 
   /// Print any prefixes before the fieldchain.
   virtual void printPrefix(llvm::raw_ostream &Out) const = 0;
 
   /// Print the node. Should contain the name of the field stored in getRegion.
   virtual void printNode(llvm::raw_ostream &Out) const = 0;
 
   /// Print the separator. For example, fields may be separated with '.' or
   /// "->".
   virtual void printSeparator(llvm::raw_ostream &Out) const = 0;
+
+  virtual bool isBase() const { return false; }
 };
 
 /// 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);
 
 /// Represents a field chain. A field chain is a vector of fields where the
 /// first element of the chain is the object under checking (not stored), and
 /// every other element is a field, and the element that precedes it is the
 /// object that contains it.
 ///
 /// Note that this class is immutable (essentially a wrapper around an
 /// ImmutableList), and new elements can only be added by creating new
 /// FieldChainInfo objects through add().
 class FieldChainInfo {
 public:
   using FieldChainImpl = llvm::ImmutableListImpl<const FieldNode &>;
   using FieldChain = llvm::ImmutableList<const FieldNode &>;
 
 private:
   FieldChain::Factory &ChainFactory;
   FieldChain Chain;
 
+  FieldChainInfo(FieldChain::Factory &F, FieldChain NewChain)
+      : FieldChainInfo(F) {
+    Chain = NewChain;
+  }
+
 public:
   FieldChainInfo() = delete;
   FieldChainInfo(FieldChain::Factory &F) : ChainFactory(F) {}
   FieldChainInfo(const FieldChainInfo &Other) = default;
 
   template <class FieldNodeT> FieldChainInfo add(const FieldNodeT &FN);
+  template <class FieldNodeT> FieldChainInfo replaceHead(const FieldNodeT &FN);
 
   bool contains(const FieldRegion *FR) const;
+  bool isEmpty() const { return Chain.isEmpty(); }
+
   const FieldRegion *getUninitRegion() const;
+  const FieldNode &getHead() { return Chain.getHead(); }
   void printNoteMsg(llvm::raw_ostream &Out) const;
 };
 
 using UninitFieldMap = std::map<const FieldRegion *, llvm::SmallString<50>>;
 
 /// Searches for and stores uninitialized fields in a non-union object.
 class FindUninitializedFields {
   ProgramStateRef State;
assert(!contains(FN.getRegion()) &&
          "Can't add a field that is already a part of the "
          "fieldchain! Is this a cyclic reference?");
 
   FieldChainInfo NewChain = *this;
   NewChain.Chain = ChainFactory.add(FN, Chain);
   return NewChain;
 }
 
+template <class FieldNodeT>
+inline FieldChainInfo FieldChainInfo::replaceHead(const FieldNodeT &FN) {
+  FieldChainInfo NewChain(ChainFactory, Chain.getTail());
+  return NewChain.add(FN);
+}
+
 } // end of namespace ento
 } // end of namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_UNINITIALIZEDOBJECT_H

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

virtual void printNode(llvm::raw_ostream &Out) const override {
     Out << getVariableName(getDecl());
   }
 
   virtual void printSeparator(llvm::raw_ostream &Out) const override {
     Out << '.';
   }
 };
 
+/// Represents that the FieldNode that comes after this is declared in a base
+/// of the previous FieldNode.
+class BaseClass final : public FieldNode {
+  const QualType BaseClassT;
+
+public:
+  BaseClass(const QualType &T) : FieldNode(nullptr), BaseClassT(T) {
+    assert(!T.isNull());
+    assert(T->getAsCXXRecordDecl());
+  }
+
+  virtual void printNoteMsg(llvm::raw_ostream &Out) const override {
+    llvm_unreachable("This node can never be the final node in the "
+                     "fieldchain!");
+  }
+
+  virtual void printPrefix(llvm::raw_ostream &Out) const override {}
+
+  virtual void printNode(llvm::raw_ostream &Out) const override {
+    Out << BaseClassT->getAsCXXRecordDecl()->getName() << "::";
+  }
+
+  virtual void printSeparator(llvm::raw_ostream &Out) const override {}
+
+  virtual bool isBase() const { return true; }
+};
+
 } // end of anonymous namespace
 
 // Utility function declarations.
 
 /// Returns the object that was constructed by CtorDecl, or None if that isn't
 /// possible.
 // TODO: Refactor this function so that it returns the constructed object's
 // region.
bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R,
   if (!CXXRD)
     return ContainsUninitField;
 
   for (const CXXBaseSpecifier &BaseSpec : CXXRD->bases()) {
     const auto *BaseRegion = State->getLValue(BaseSpec, R)
                                  .castAs<loc::MemRegionVal>()
                                  .getRegionAs<TypedValueRegion>();
 
-    if (isNonUnionUninit(BaseRegion, LocalChain))
+    // If the head of the list is also a BaseClass, we'll overwrite it to avoid
+    // note messages like 'this->A::B::x'.
+    if (!LocalChain.isEmpty() && LocalChain.getHead().isBase()) {
+      if (isNonUnionUninit(BaseRegion, LocalChain.replaceHead(
+                                           BaseClass(BaseSpec.getType()))))
         ContainsUninitField = true;
+    } else {
+      if (isNonUnionUninit(BaseRegion,
+                           LocalChain.add(BaseClass(BaseSpec.getType()))))
+        ContainsUninitField = true;
+    }
   }
 
   return ContainsUninitField;
 }
 
 bool FindUninitializedFields::isUnionUninit(const TypedValueRegion *R) {
   assert(R->getValueType()->isUnionType() &&
          "This method only checks union objects!");

test/Analysis/cxx-uninitialized-object-inheritance.cpp

public:
   }
 };
 
 void fNonPolymorphicInheritanceTest1() {
   NonPolymorphicInheritanceTest1();
 }
 
 class NonPolymorphicBaseClass2 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->NonPolymorphicBaseClass2::x'}}
 protected:
   int y;
 
 public:
   NonPolymorphicBaseClass2() = default;
   NonPolymorphicBaseClass2(int) : x(4) {}
 };
 

 void fNonPolymorphicInheritanceTest2() {
   NonPolymorphicInheritanceTest2();
 }
 
 class NonPolymorphicBaseClass3 {
   int x;
 
 protected:
-  int y; // expected-note{{uninitialized field 'this->y'}}
+  int y; // expected-note{{uninitialized field 'this->NonPolymorphicBaseClass3::y'}}
 public:
   NonPolymorphicBaseClass3() = default;
   NonPolymorphicBaseClass3(int) : x(7) {}
 };
 
 class NonPolymorphicInheritanceTest3 : public NonPolymorphicBaseClass3 {
   int z;
 
public:
   }
 };
 
 void fPolymorphicInheritanceTest1() {
   PolymorphicInheritanceTest1();
 }
 
 class PolymorphicRight1 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->PolymorphicRight1::x'}}
 protected:
   int y;
 
 public:
   virtual ~PolymorphicRight1() = default;
   PolymorphicRight1() = default;
   PolymorphicRight1(int) : x(14) {}
 };

 void fPolymorphicInheritanceTest2() {
   PolymorphicInheritanceTest2();
 }
 
 class PolymorphicBaseClass3 {
   int x;
 
 protected:
-  int y; // expected-note{{uninitialized field 'this->y'}}
+  int y; // expected-note{{uninitialized field 'this->PolymorphicBaseClass3::y'}}
 public:
   virtual ~PolymorphicBaseClass3() = default;
   PolymorphicBaseClass3() = default;
   PolymorphicBaseClass3(int) : x(17) {}
 };
 
 class PolymorphicInheritanceTest3 : public PolymorphicBaseClass3 {
   int z;
public:
   }
 };
 
 void fVirtualInheritanceTest1() {
   VirtualInheritanceTest1();
 }
 
 class VirtualPolymorphicRight1 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->VirtualPolymorphicRight1::x'}}
 protected:
   int y;
 
 public:
   virtual ~VirtualPolymorphicRight1() = default;
   VirtualPolymorphicRight1() = default;
   VirtualPolymorphicRight1(int) : x(24) {}
 };

 void fVirtualInheritanceTest2() {
   VirtualInheritanceTest2();
 }
 
 class VirtualPolymorphicBaseClass3 {
   int x;
 
 protected:
-  int y; // expected-note{{uninitialized field 'this->y'}}
+  int y; // expected-note{{uninitialized field 'this->VirtualPolymorphicBaseClass3::y'}}
 public:
   virtual ~VirtualPolymorphicBaseClass3() = default;
   VirtualPolymorphicBaseClass3() = default;
   VirtualPolymorphicBaseClass3(int) : x(27) {}
 };
 
 class VirtualInheritanceTest3 : virtual public VirtualPolymorphicBaseClass3 {
   int z;

 }
 
 struct Left2 {
   int x;
   Left2() = default;
   Left2(int) : x(36) {}
 };
 struct Right2 {
-  int y; // expected-note{{uninitialized field 'this->y'}}
+  int y; // expected-note{{uninitialized field 'this->Right2::y'}}
   Right2() = default;
   Right2(int) : y(37) {}
 };
 
 class MultipleInheritanceTest2 : public Left2, public Right2 {
   int z;
 
 public:
   MultipleInheritanceTest2()
       : Left2(int{}) {
     z = 38; // expected-warning{{1 uninitialized field}}
   }
 };
 
 void fMultipleInheritanceTest2() {
   MultipleInheritanceTest2();
 }
 
 struct Left3 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->Left3::x'}}
   Left3() = default;
   Left3(int) : x(39) {}
 };
 struct Right3 {
   int y;
   Right3() = default;
   Right3(int) : y(40) {}
 };

 }
 
 struct Left5 {
   int x;
   Left5() = default;
   Left5(int) : x(44) {}
 };
 struct Right5 {
-  int y; // expected-note{{uninitialized field 'this->y'}}
+  int y; // expected-note{{uninitialized field 'this->Right5::y'}}
   Right5() = default;
   Right5(int) : y(45) {}
 };
 
 class MultipleInheritanceTest5 : public Left5, public Right5 {
   int z; // expected-note{{uninitialized field 'this->z'}}
 
 public:

 
 void fNonVirtualDiamondInheritanceTest1() {
   NonVirtualDiamondInheritanceTest1();
   NonVirtualDiamondInheritanceTest1(int());
   NonVirtualDiamondInheritanceTest1(int(), int());
 }
 
 struct NonVirtualBase2 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->NonVirtualBase2::x'}}
   NonVirtualBase2() = default;
   NonVirtualBase2(int) : x(52) {}
 };
 struct First2 : public NonVirtualBase2 {
   First2() = default;
   First2(int) : NonVirtualBase2(int{}) {}
 };
 struct Second2 : public NonVirtualBase2 {
public:
   }
 };
 
 void fNonVirtualDiamondInheritanceTest2() {
   NonVirtualDiamondInheritanceTest2();
 }
 
 struct NonVirtualBase3 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->NonVirtualBase3::x'}}
   NonVirtualBase3() = default;
   NonVirtualBase3(int) : x(54) {}
 };
 struct First3 : public NonVirtualBase3 {
   First3() = default;
   First3(int) : NonVirtualBase3(int{}) {}
 };
 struct Second3 : public NonVirtualBase3 {
public:
   }
 };
 
 void fNonVirtualDiamondInheritanceTest3() {
   NonVirtualDiamondInheritanceTest3();
 }
 
 struct NonVirtualBase4 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
-  // expected-note@-1{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->NonVirtualBase4::x'}}
+  // expected-note@-1{{uninitialized field 'this->NonVirtualBase4::x'}}
   NonVirtualBase4() = default;
   NonVirtualBase4(int) : x(56) {}
 };
 struct First4 : public NonVirtualBase4 {
   First4() = default;
   First4(int) : NonVirtualBase4(int{}) {}
 };
 struct Second4 : public NonVirtualBase4 {
public:
   }
 };
 
 void fNonVirtualDiamondInheritanceTest5() {
   NonVirtualDiamondInheritanceTest5();
 }
 
 struct NonVirtualBase6 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->NonVirtualBase6::x'}}
   NonVirtualBase6() = default;
   NonVirtualBase6(int) : x(59) {}
 };
 struct First6 : public NonVirtualBase6 {
   First6() = default;
   First6(int) : NonVirtualBase6(int{}) {}
 };
 struct Second6 : public NonVirtualBase6 {

 
 void fVirtualDiamondInheritanceTest1() {
   VirtualDiamondInheritanceTest1();
   VirtualDiamondInheritanceTest1(int());
   VirtualDiamondInheritanceTest1(int(), int());
 }
 
 struct VirtualBase2 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->VirtualBase2::x'}}
   VirtualBase2() = default;
   VirtualBase2(int) : x(63) {}
 };
 struct VirtualFirst2 : virtual public VirtualBase2 {
   VirtualFirst2() = default;
   VirtualFirst2(int) : VirtualBase2(int{}) {}
   VirtualFirst2(int, int) { x = 64; }
 };
public:
   }
 };
 
 void fVirtualDiamondInheritanceTest2() {
   VirtualDiamondInheritanceTest2();
 }
 
 struct VirtualBase3 {
-  int x; // expected-note{{uninitialized field 'this->x'}}
+  int x; // expected-note{{uninitialized field 'this->VirtualBase3::x'}}
   VirtualBase3() = default;
   VirtualBase3(int) : x(66) {}
 };
 struct VirtualFirst3 : virtual public VirtualBase3 {
   VirtualFirst3() = default;
   VirtualFirst3(int) : VirtualBase3(int{}) {}
   VirtualFirst3(int, int) { x = 67; }
 };