[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Ok, let's land this one and see how it goes! I'm looking forward to seeing the follow-up patches.



================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+  // Checking bases.
+  const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD);
+  if (!CXXRD)
+    return ContainsUninitField;
+
+  for (const CXXBaseSpecifier BaseSpec : CXXRD->bases()) {
+    const auto *Base = BaseSpec.getType()->getAsCXXRecordDecl();
----------------
Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Are there many warnings that will be caused by this but won't be caused by conducting the check for the constructor-within-constructor that's currently disabled? Not sure - is it even syntactically possible to not initialize the base class?
> > > > I'm not a 100% sure what you mean. Can you clarify?
> > > > >Not sure - is it even syntactically possible to not initialize the base class?
> > > > If I understand the question correctly, no, as far as I know.
> > > You have a check for `isCalledByConstructor()` in order to skip base class constructors but then you check them manually. That's a lot of code, but it seems that the same result could have been achieved by simply skipping the descent into the base class.
> > > 
> > > Same question for class-type fields, actually.
> > >Same question for class-type fields, actually.
> > 
> > My problem with that approach is that the same uninitialized field could've been emitted multiple times in different warnings. From an earlier conversation (assume that its run in pedantic mode):
> > 
> > >For this code snippet:
> > >
> > > ```
> > >struct A {
> > >   struct B {
> > >      int x, y;
> > >      
> > >      B() {}
> > >   } b;
> > >   int w;
> > >
> > >   A() {
> > >      b = B();
> > >   }
> > >};
> > >```
> > >the warning message after a call to `A::A()` would be "3 uninitialized fields[...]", and for `B::B()` inside `A`s constructor would be "2 uninitialized fields[...]", so it wouldn't be filtered out.
> > 
> > In my opinion, if `A` contains a field of type `B`, it should be the responsibility of `A`'s constructors to initialize those fields properly.
> > >You have a check for isCalledByConstructor() in order to skip base class constructors but then you check them manually. That's a lot of code, but it seems that the same result could have been achieved by simply skipping the descent into the base class.
> > I also believe that a constructor "should be held responsible" for the initialization of the inherited data members. However, in the case of base classes, uniqueing isn't an issue, as in this case:
> > ```
> > struct B {
> >   int a;
> >   B() {}
> > };
> > 
> > struct D : public B {
> >   int b;
> >   D() {}
> > };
> > ```
> > a call to `D::D()` would not check the inherited member (`a`), if I didn't implement it explicitly. That also means that if `isCalledByConstructor` was refactored to not filter out base classes, we would get two warning each with a single note, reporting `b` and `a` respectively. (I haven't actually checked it, but its a reasonable assumption)
> > 
> > Actually, I'm not 100% sure which approach is better: a warning for each base, or one warning for the object, bases included. I personally think one warning per object results in the best user experience.
> > 
> > I like both ideas, but I think if we'd come to the conclusion that a warning per base should be the way to go, it should definitely be implemented in a followup patch, as `isCalledByConstructor` is already in line for some major refactoring.
> > 
> > What do you think? :)
> >[...]but it seems that the same result could have been achieved by simply skipping the descent into the base class.
> I can't edit inline comments sadly, but I'd just like to point out that it wouldn't achieve the same thing, as highlighted above. But, both solution would be okay.
I still don't fully understand the reasoning behind who's responsible for initializing the field, i.e., the exact rule that you're enforcing seems to cause warnings to depend heavily on meta-information such as where does the analysis start, which is confusing. Because we're trying to enforce a coding guideline here, i believe that the guideline itself should be transparent and easy to explain. In particular, it should be obvious to the user which constructor is responsible for initializing a field, and by obvious i mean it should be obvious both from the guideline itself and from the analyzer's warning, and both of them should be the same "obvious".


https://reviews.llvm.org/D45532