[analyzer] MisusedMovedObjectChecker: Fix a false positive on state-resetting a base-class sub-object.

[NoQ]

Summary

If a state reset method is defined in a parent class, but called over an object of a child class, then the checker doesn't treat this as a state reset, at least for the sake of "Copying a 'moved-from' object" warning class.

The patch fixes it, but Peter may have a better fix in mind :)

[szepet]

Thank you for working on that, Artem!
The changes look good, just one comment about that suspicious remove.

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

426 ↗

(On Diff #93652)

I am wondering if I made a mistake but I think this should be removeFromState() function call. (We should remove every marked subregions of the object too.)
So I suspect a code like this would result a false positive:

struct A{
B b;
void clear();
};

void test(){
A a;
B b = std::move(a.b);
a.clear();
b = std::move(a); //report a bug
}

I mean it is probably a report we do not want to have.

[szepet]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

426 ↗

(On Diff #93652)

Shame on the test file writer that he does not covered this, though. ^^

[szepet]

lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

426 ↗

(On Diff #93652)

Okay, I have checked it and yes, it produces a bugreport (false positive) on the code snippet above (which contains a misspelled variable name so let me write it again):

struct B{};

struct A{
B b;
void clear();
};

void test(){
A a;
B b = std::move(a.b);
a.clear();
b = std::move(a.b); //report a bug
}

In order to eliminate these type of bugs I suggest using here the removeFromState function and move this "WholeObjectRegion algorithm" (the for loop) to the removeFromState function.
It is probably out of scope from this patch. Anyway, I accept this since it is great to fix this bug and a good step to a follow-up patch. (Well, if you would like to make these changes in this patch. I'm not gonna hold you back. It is highly appreciated and I would certainly review and all the stuff but I would do it gladly in a follow-up too.)
Again, thank you for pointing this out and making the checker more precise!

[szepet]

Hello Artem!

Could you please commit these changes? (And D31541 as well.) Thanks in advance!

[xazax.hun]

I think there was only one comment but that is already addressed in a dependent revision. So I think this one is good as is.

cfe/trunk/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp

if (OperatorEq) {
     return;
   }
 
   // The remaining part is check only for method call on a moved-from object.
   if (isMoveSafeMethod(MethodDecl))
     return;
 
   if (isStateResetMethod(MethodDecl)) {
-    State = State->remove<TrackedRegionMap>(ThisRegion);
+    // 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);
     C.addTransition(State);
     return;
   }
 
   // If it is already reported then we dont report the bug again.
   const RegionState *ThisState = State->get<TrackedRegionMap>(ThisRegion);
   if (!(ThisState && ThisState->isMoved()))
     return;

cfe/trunk/test/Analysis/MisusedMovedObject.cpp

void subRegionMoveTest() {
   // 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
   }
 }
+
+class C: public A {};
+void resetSuperClass() {
+  C c;
+  C c1 = std::move(c);
+  c.clear();
+  C c2 = c; // no-warning
+}