[clang] [analyzer] WebKit checkers: support ref and deref defined on different classes. (PR #68170)

[Artem Dergachev via cfe-commits]

================
@@ -70,29 +88,45 @@ std::optional<bool> isRefCountable(const CXXRecordDecl* R)
   if (!R)
     return std::nullopt;
 
-  if (hasPublicRefAndDeref(R))
+  bool hasRef = hasPublicRefMethod(R);
+  bool hasDeref = hasPublicDerefMethod(R);
+  if (hasRef && hasDeref)
     return true;
 
   CXXBasePaths Paths;
   Paths.setOrigin(const_cast<CXXRecordDecl *>(R));
 
   bool AnyInconclusiveBase = false;
-  const auto isRefCountableBase =
-      [&AnyInconclusiveBase](const CXXBaseSpecifier* Base, CXXBasePath&) {
-          std::optional<const clang::CXXRecordDecl*> IsRefCountable = clang::isRefCountable(Base);
-          if (!IsRefCountable) {
-              AnyInconclusiveBase = true;
-              return false;
-          }
-          return (*IsRefCountable) != nullptr;
+  const auto hasPublicRefInBase =
+      [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+        auto hasRefInBase = clang::hasPublicRefInBase(Base);
+        if (!hasRefInBase) {
+          AnyInconclusiveBase = true;
+          return false;
+        }
+        return (*hasRefInBase) != nullptr;
       };
 
-  bool BasesResult = R->lookupInBases(isRefCountableBase, Paths,
-                                      /*LookupInDependent =*/true);
+  hasRef =
+      R->lookupInBases(hasPublicRefInBase, Paths, /*LookupInDependent =*/true);
+  if (AnyInconclusiveBase)
+    return std::nullopt;
+
+  const auto hasPublicDerefInBase =
+      [&AnyInconclusiveBase](const CXXBaseSpecifier *Base, CXXBasePath &) {
+        auto hasDerefInBase = clang::hasPublicDerefInBase(Base);
+        if (!hasDerefInBase) {
+          AnyInconclusiveBase = true;
+          return false;
+        }
+        return (*hasDerefInBase) != nullptr;
+      };
+  hasDeref = R->lookupInBases(hasPublicDerefInBase, Paths,
+                              /*LookupInDependent =*/true);
----------------
haoNoQ wrote:

This overwrites the old `hasDeref` value, so it may give an incorrect answer if eg. `Deref` is in the class itself but `Ref` is in a base class.

In the other copy of similar code we see `hasDeref = hasDeref || lookupInBases(...)` which is probably more correct.

(Which makes me wish there weren't two copies of this code in the first place.)

https://github.com/llvm/llvm-project/pull/68170