[analyzer] MisusedMovedObjectChecker: Fix false positive on state-resetting, handling method calls on base-class sub-objects

[szepet]

Summary

1. Artem's solution D31538 solves the reset problem, however, the reports should be handled the same way in case of method calls. We should not just report the base class of the object where the method was defined but the whole object.

2. Fixed false positive which came from not removing the subobjects in case of a state-resetting function. (Just replaced the State->remove(...) call to removeFromState(..) which was defined exactly for that purpose.)

3. Some minor typos fixed in this patch as well which did not worth a whole new patch in my opinion, so included them here.

[xazax.hun]

LGTM! I only found a nit.

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

419

I think this loop could be moved after the check isMoveSafeMethod to improve the performance.

[NoQ]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

380–383

I guess we'd have to check this for null, eg. when this-value is Unknown (seeing crashes on dyn_casting this pointer to CXXBaseObjectRegion in the code you just moved).

[xazax.hun]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

380–383

Good catch. I can confirm that this can return a null pointer.

[szepet]

Fixed the commented bug (check if ThisRegion is null).

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

 }
 
 void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call,
                                              CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   const LocationContext *LC = C.getLocationContext();
   ExplodedNode *N = nullptr;
 
-  // Remove the MemRegions from the map on which a ctor/dtor call or assignement
+  // Remove the MemRegions from the map on which a ctor/dtor call or assignment
   // happened.
 
   // Checking constructor calls.
   if (const auto *CC = dyn_cast<CXXConstructorCall>(&Call)) {
     State = removeFromState(State, CC->getCXXThisVal().getAsRegion());
     auto CtorDec = CC->getDecl();
     // Check for copying a moved-from object and report the bug.
     if (CtorDec && CtorDec->isCopyOrMoveConstructor()) {
if (const auto *CC = dyn_cast<CXXConstructorCall>(&Call)) {
     C.addTransition(State, N);
     return;
   }
 
   const auto IC = dyn_cast<CXXInstanceCall>(&Call);
   if (!IC)
     return;
   // In case of destructor call we do not track the object anymore.
   const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
+  if (!ThisRegion)
+    return;
+
   if (dyn_cast_or_null<CXXDestructorDecl>(Call.getDecl())) {
-    State = removeFromState(State, IC->getCXXThisVal().getAsRegion());
+    State = removeFromState(State, ThisRegion);
     C.addTransition(State);
     return;
   }
 
   const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
   if (!MethodDecl)
     return;
   // Checking assignment operators.
if (MethodDecl->isCopyAssignmentOperator() ||
             State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported());
       }
     }
     C.addTransition(State, N);
     return;
   }
 
   // The remaining part is check only for method call on a moved-from object.
+
+  // We want to investigate the whole object, not only sub-object of a parent
+  // class in which the encountered method defined.
+  while (const CXXBaseObjectRegion *BR =
+             dyn_cast<CXXBaseObjectRegion>(ThisRegion))
+    ThisRegion = BR->getSuperRegion();
+
   if (isMoveSafeMethod(MethodDecl))
     return;
 
   if (isStateResetMethod(MethodDecl)) {
-    // A state reset method resets the whole object, not only sub-object
-    // of a parent class in which it is defined.
-    const MemRegion *WholeObjectRegion = ThisRegion;
-    while (const CXXBaseObjectRegion *BR =
-               dyn_cast<CXXBaseObjectRegion>(WholeObjectRegion))
-      WholeObjectRegion = BR->getSuperRegion();
-
-    State = State->remove<TrackedRegionMap>(WholeObjectRegion);
+    State = removeFromState(State, ThisRegion);
     C.addTransition(State);
     return;
   }
 
-  // If it is already reported then we dont report the bug again.
+  // If it is already reported then we don't report the bug again.
   const RegionState *ThisState = State->get<TrackedRegionMap>(ThisRegion);
   if (!(ThisState && ThisState->isMoved()))
     return;
 
-  // Dont report it in case if any base region is already reported
+  // Don't report it in case if any base region is already reported
   if (isAnyBaseRegionReported(State, ThisRegion))
     return;
 
   if (isInMoveSafeContext(LC))
     return;
 
   N = reportBug(ThisRegion, Call, C);
   State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported());

test/Analysis/MisusedMovedObject.cpp

 }
 
 void moveStateResetFunctionsTest() {
   {
     A a;
     A b = std::move(a);
     a.reset(); // no-warning
     a.foo();   // no-warning
+    // Test if resets the state of subregions as well.
+    a.b.foo(); // no-warning
   }
   {
     A a;
     A b = std::move(a);
     a.destroy(); // no-warning
     a.foo();     // no-warning
   }
   {
     A a;
     A b = std::move(a);
     a.clear(); // no-warning
     a.foo();   // no-warning
+    a.b.foo(); // no-warning
   }
 }
 
 // Moves or uses that occur as part of template arguments.
 template <int>
 class ClassTemplate {
 public:
   void foo(A a);
void differentBranchesTest(int i) {
   // A variation on the theme above.
   {
     A a;
     a.foo() > 0 ? a.foo() : A(std::move(a)).foo(); // expected-note {{Assuming the condition is false}} expected-note {{'?' condition is false}}
   }
   // Same thing, but with a switch statement.
   {
     A a, b;
-    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 448}}
+    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 451}}
     case 1:
       b = std::move(a); // no-warning
       break;            // expected-note {{Execution jumps to the end of the function}}
     case 2:
       a.foo(); // no-warning
       break;
     }
   }
   // However, if there's a fallthrough, we do warn.
   {
     A a, b;
-    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 460}}
+    switch (i) { // expected-note {{Control jumps to 'case 1:'  at line 463}}
     case 1:
       b = std::move(a); // expected-note {{'a' became 'moved-from' here}}
     case 2:
       a.foo(); // expected-warning {{Method call on a 'moved-from' object}} expected-note {{Method call on a 'moved-from' object 'a'}}
       break;
     }
   }
 }
void ifStmtSequencesDeclAndConditionTest() {
   for (int i = 0; i < 3; ++i) {
     if (A a = A()) {
       A b;
       b = std::move(a); // no-warning
     }
   }
 }
 
+class C : public A {};
 void subRegionMoveTest() {
   {
     A a;
     B b = std::move(a.b); // expected-note {{'b' became 'moved-from' here}}
     a.b.foo();            // expected-warning {{Method call on a 'moved-from' object 'b'}} expected-note {{Method call on a 'moved-from' object 'b'}}
   }
   {
     A a;
     A a1 = std::move(a); // expected-note {{Calling move constructor for 'A'}} expected-note {{Returning from move constructor for 'A'}}
     a.b.foo();           // expected-warning {{Method call on a 'moved-from' object 'b'}} expected-note {{Method call on a 'moved-from' object 'b'}}
   }
   // Don't report a misuse if any SuperRegion is already reported.
   {
     A a;
     A a1 = std::move(a); // expected-note {{'a' became 'moved-from' here}}
     a.foo();             // expected-warning {{Method call on a 'moved-from' object 'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}
     a.b.foo();           // no-warning
   }
+  {
+    C c;
+    C c1 = std::move(c); // expected-note {{'c' became 'moved-from' here}}
+    c.foo();             // expected-warning {{Method call on a 'moved-from' object 'c'}} expected-note {{Method call on a 'moved-from' object 'c'}}
+    c.b.foo();           // no-warning
+  }
 }
 
-class C: public A {};
 void resetSuperClass() {
   C c;
   C c1 = std::move(c);
   c.clear();
   C c2 = c; // no-warning
 }
+
+void reportSuperClass() {
+  C c;
+  C c1 = std::move(c); // expected-note {{'c' became 'moved-from' here}}
+  c.foo();             // expected-warning {{Method call on a 'moved-from' object 'c'}} expected-note {{Method call on a 'moved-from' object 'c'}}
+  C c2 = c;            // no-warning
+}