[alpha.webkit.UncountedCallArgsChecker] Allow protector functions in Objective-C++

[rniwa]

This PR fixes the bug that WebKit checkers didn't recognize the return value of an Objective-C++ selector which returns Ref or RefPtr to be safe.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

This PR fixes the bug that WebKit checkers didn't recognize the return value of an Objective-C++ selector which returns Ref or RefPtr to be safe.

---

Full diff: https://github.com/llvm/llvm-project/pull/108184.diff

4 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+11)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+5)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp (+1-2)
• (added) clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm (+26)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 49bbff1942167b..5a484d0546e95f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -143,6 +143,17 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) {
   return false;
 }
 
+std::optional<bool> isUncounted(const clang::QualType T)
+{
+  if (auto *Subst = dyn_cast<SubstTemplateTypeParmType>(T)) {
+    if (auto *Decl = Subst->getAssociatedDecl()) {
+      if (isRefType(safeGetName(Decl)))
+        return false;
+    }
+  }
+  return isUncounted(T->getAsCXXRecordDecl());
+}
+
 std::optional<bool> isUncounted(const CXXRecordDecl* Class)
 {
   // Keep isRefCounted first as it's cheaper.
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index ec1db1cc335807..2932e62ad06e4b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -20,6 +20,7 @@ class CXXMethodDecl;
 class CXXRecordDecl;
 class Decl;
 class FunctionDecl;
+class QualType;
 class Stmt;
 class Type;
 
@@ -42,6 +43,10 @@ std::optional<bool> isRefCountable(const clang::CXXRecordDecl* Class);
 /// \returns true if \p Class is ref-counted, false if not.
 bool isRefCounted(const clang::CXXRecordDecl *Class);
 
+/// \returns true if \p Class is ref-countable AND not ref-counted, false if
+/// not, std::nullopt if inconclusive.
+std::optional<bool> isUncounted(const clang::QualType T);
+
 /// \returns true if \p Class is ref-countable AND not ref-counted, false if
 /// not, std::nullopt if inconclusive.
 std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index 704c082a4d1d63..81c2434ce64775 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -87,8 +87,7 @@ class UncountedCallArgsChecker
         }
         auto *E = MemberCallExpr->getImplicitObjectArgument();
         QualType ArgType = MemberCallExpr->getObjectType();
-        std::optional<bool> IsUncounted =
-            isUncounted(ArgType->getAsCXXRecordDecl());
+        std::optional<bool> IsUncounted = isUncounted(ArgType);
         if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E))
           reportBugOnThis(E);
       }
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm
new file mode 100644
index 00000000000000..db0c5b19eec5bb
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+// expected-no-diagnostics
+
+#import "mock-types.h"
+#import "mock-system-header.h"
+#import "../../Inputs/system-header-simulator-for-objc-dealloc.h"
+
+@interface Foo : NSObject
+
+@property (nonatomic, readonly) RefPtr<RefCountable> countable;
+
+- (void)execute;
+- (RefPtr<RefCountable>)_protectedRefCountable;
+@end
+
+@implementation Foo
+
+- (void)execute {
+  self._protectedRefCountable->method();
+}
+
+- (RefPtr<RefCountable>)_protectedRefCountable {
+  return _countable;
+}
+
+@end

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[haoNoQ]

Aha ok LGTM!

[haoNoQ]

Should we force every checker to use the new function now that we know about this cornercase?

[rniwa]

So the only other place where this function is called is in isUncountedPtr but that function needs to evaluate whether the pointee is ref counted or not so I don't think we can use the new function.

[rniwa]

Thanks for the review!