Skip to content

[alpha.webkit.UncountedCallArgsChecker] Ignore methods of WTF String classes. #90704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 2, 2024
Merged

[alpha.webkit.UncountedCallArgsChecker] Ignore methods of WTF String classes. #90704

merged 1 commit into from
May 2, 2024

Conversation

rniwa
Copy link
Contributor

@rniwa rniwa commented May 1, 2024

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels May 1, 2024
@llvmbot
Copy link
Member

llvmbot commented May 1, 2024

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

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

3 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp (+10-3)
  • (modified) clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp (+118)
  • (modified) clang/test/Analysis/Checkers/WebKit/mock-types.h (+6-3)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index ae494de58da3da..0f40ecc7ba3000 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -227,10 +227,17 @@ class UncountedCallArgsChecker
     return NamespaceName == "WTF" &&
            (MethodName == "find" || MethodName == "findIf" ||
             MethodName == "reverseFind" || MethodName == "reverseFindIf" ||
-            MethodName == "get" || MethodName == "inlineGet" ||
-            MethodName == "contains" || MethodName == "containsIf") &&
+            MethodName == "findIgnoringASCIICase" || MethodName == "get" ||
+            MethodName == "inlineGet" || MethodName == "contains" ||
+            MethodName == "containsIf" ||
+            MethodName == "containsIgnoringASCIICase" ||
+            MethodName == "startsWith" || MethodName == "endsWith" ||
+            MethodName == "startsWithIgnoringASCIICase" ||
+            MethodName == "endsWithIgnoringASCIICase" ||
+            MethodName == "substring") &&
            (ClsName.ends_with("Vector") || ClsName.ends_with("Set") ||
-            ClsName.ends_with("Map"));
+            ClsName.ends_with("Map") || ClsName == "StringImpl" ||
+            ClsName.ends_with("String"));
   }
 
   void reportBug(const Expr *CallArg, const ParmVarDecl *Param) const {
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp
index 0a63a789856127..17e25d9a627039 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp
@@ -4,6 +4,92 @@
 
 namespace WTF {
 
+  constexpr unsigned long notFound = static_cast<unsigned long>(-1);
+
+  class String;
+  class StringImpl;
+
+  class StringView {
+  public:
+    StringView(const String&);
+  private:
+    RefPtr<StringImpl> m_impl;
+  };
+
+  class StringImpl {
+  public:
+    void ref() const { ++m_refCount; }
+    void deref() const {
+      if (!--m_refCount)
+        delete this;
+    }
+
+    static constexpr unsigned s_flagIs8Bit = 1u << 0;
+    bool is8Bit() const { return m_hashAndFlags & s_flagIs8Bit; }
+    const char* characters8() const { return m_char8; }
+    const short* characters16() const { return m_char16; }
+    unsigned length() const { return m_length; }
+    Ref<StringImpl> substring(unsigned position, unsigned length) const;
+
+    unsigned long find(char) const;
+    unsigned long find(StringView) const;
+    unsigned long contains(StringView) const;
+    unsigned long findIgnoringASCIICase(StringView) const;
+
+    bool startsWith(StringView) const;
+    bool startsWithIgnoringASCIICase(StringView) const;
+    bool endsWith(StringView) const;
+    bool endsWithIgnoringASCIICase(StringView) const;
+
+  private:
+    mutable unsigned m_refCount { 0 };
+    unsigned m_length { 0 };
+    union {
+      const char* m_char8;
+      const short* m_char16;
+    };
+    unsigned m_hashAndFlags { 0 };
+  };
+
+  class String {
+  public:
+    String() = default;
+    String(StringImpl& impl) : m_impl(&impl) { }
+    String(StringImpl* impl) : m_impl(impl) { }
+    String(Ref<StringImpl>&& impl) : m_impl(impl.get()) { }
+    StringImpl* impl() { return m_impl.get(); }
+    unsigned length() const { return m_impl ? m_impl->length() : 0; }
+    const char* characters8() const { return m_impl ? m_impl->characters8() : nullptr; }
+    const short* characters16() const { return m_impl ? m_impl->characters16() : nullptr; }
+
+    bool is8Bit() const { return !m_impl || m_impl->is8Bit(); }
+
+    unsigned long find(char character) const { return m_impl ? m_impl->find(character) : notFound; }
+    unsigned long find(StringView str) const { return m_impl ? m_impl->find(str) : notFound; }
+    unsigned long findIgnoringASCIICase(StringView) const;
+
+    bool contains(char character) const { return find(character) != notFound; }
+    bool contains(StringView) const;
+    bool containsIgnoringASCIICase(StringView) const;
+
+    bool startsWith(StringView) const;
+    bool startsWithIgnoringASCIICase(StringView) const;
+    bool endsWith(StringView) const;
+    bool endsWithIgnoringASCIICase(StringView) const;
+
+    String substring(unsigned position, unsigned length) const
+    {
+      if (!m_impl)
+        return { };
+      if (!position && length >= m_impl->length())
+        return *this;
+      return m_impl->substring(position, length);
+    }
+
+  private:
+    RefPtr<StringImpl> m_impl;
+  };
+
   template <typename T>
   class HashSet {
   public:
@@ -89,6 +175,9 @@ namespace WTF {
 
 }
 
+using WTF::StringView;
+using WTF::StringImpl;
+using WTF::String;
 using WTF::HashSet;
 using WTF::HashMap;
 using WTF::WeakHashSet;
@@ -101,8 +190,37 @@ class RefCounted {
 };
 
 RefCounted* object();
+StringImpl* strImpl();
+String* str();
+StringView strView();
 
 void test() {
+  strImpl()->is8Bit();
+  strImpl()->characters8();
+  strImpl()->characters16();
+  strImpl()->length();
+  strImpl()->substring(2, 4);
+  strImpl()->find(strView());
+  strImpl()->contains(strView());
+  strImpl()->findIgnoringASCIICase(strView());
+  strImpl()->startsWith(strView());
+  strImpl()->startsWithIgnoringASCIICase(strView());
+  strImpl()->endsWith(strView());
+  strImpl()->endsWithIgnoringASCIICase(strView());
+
+  str()->is8Bit();
+  str()->characters8();
+  str()->characters16();
+  str()->length();
+  str()->substring(2, 4);
+  str()->find(strView());
+  str()->contains(strView());
+  str()->findIgnoringASCIICase(strView());
+  str()->startsWith(strView());
+  str()->startsWithIgnoringASCIICase(strView());
+  str()->endsWith(strView());
+  str()->endsWithIgnoringASCIICase(strView());
+
   HashSet<RefPtr<RefCounted>> set;
   set.find(*object());
   set.contains(*object());
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index c27ea9baaf3bf5..c427b22fd683e5 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -47,7 +47,7 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
   typename PtrTraits::StorageType t;
 
   Ref() : t{} {};
-  Ref(T &t) : t(RefDerefTraits::refIfNotNull(t)) { }
+  Ref(T &t) : t(&RefDerefTraits::ref(t)) { }
   Ref(const Ref& o) : t(RefDerefTraits::refIfNotNull(PtrTraits::unwrap(o.t))) { }
   ~Ref() { RefDerefTraits::derefIfNotNull(PtrTraits::exchange(t, nullptr)); }
   T &get() { return *PtrTraits::unwrap(t); }
@@ -55,7 +55,7 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
   T *operator->() { return PtrTraits::unwrap(t); }
   operator const T &() const { return *PtrTraits::unwrap(t); }
   operator T &() { return *PtrTraits::unwrap(t); }
-  T* leakRef() { PtrTraits::exchange(t, nullptr); }
+  T* leakRef() { return PtrTraits::exchange(t, nullptr); }
 };
 
 template <typename T> struct RefPtr {
@@ -67,6 +67,9 @@ template <typename T> struct RefPtr {
     if (t)
       t->ref();
   }
+  RefPtr(Ref<T>&& o)
+    : t(o.leakRef())
+  { }
   ~RefPtr() {
     if (t)
       t->deref();
@@ -76,7 +79,7 @@ template <typename T> struct RefPtr {
   const T *operator->() const { return t; }
   T &operator*() { return *t; }
   RefPtr &operator=(T *) { return *this; }
-  operator bool() { return t; }
+  operator bool() const { return t; }
 };
 
 template <typename T> bool operator==(const RefPtr<T> &, const RefPtr<T> &) {

@llvmbot
Copy link
Member

llvmbot commented May 1, 2024

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

Author: Ryosuke Niwa (rniwa)

Changes

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

3 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp (+10-3)
  • (modified) clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp (+118)
  • (modified) clang/test/Analysis/Checkers/WebKit/mock-types.h (+6-3)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index ae494de58da3da..0f40ecc7ba3000 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -227,10 +227,17 @@ class UncountedCallArgsChecker
     return NamespaceName == "WTF" &&
            (MethodName == "find" || MethodName == "findIf" ||
             MethodName == "reverseFind" || MethodName == "reverseFindIf" ||
-            MethodName == "get" || MethodName == "inlineGet" ||
-            MethodName == "contains" || MethodName == "containsIf") &&
+            MethodName == "findIgnoringASCIICase" || MethodName == "get" ||
+            MethodName == "inlineGet" || MethodName == "contains" ||
+            MethodName == "containsIf" ||
+            MethodName == "containsIgnoringASCIICase" ||
+            MethodName == "startsWith" || MethodName == "endsWith" ||
+            MethodName == "startsWithIgnoringASCIICase" ||
+            MethodName == "endsWithIgnoringASCIICase" ||
+            MethodName == "substring") &&
            (ClsName.ends_with("Vector") || ClsName.ends_with("Set") ||
-            ClsName.ends_with("Map"));
+            ClsName.ends_with("Map") || ClsName == "StringImpl" ||
+            ClsName.ends_with("String"));
   }
 
   void reportBug(const Expr *CallArg, const ParmVarDecl *Param) const {
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp
index 0a63a789856127..17e25d9a627039 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-wtf-containers.cpp
@@ -4,6 +4,92 @@
 
 namespace WTF {
 
+  constexpr unsigned long notFound = static_cast<unsigned long>(-1);
+
+  class String;
+  class StringImpl;
+
+  class StringView {
+  public:
+    StringView(const String&);
+  private:
+    RefPtr<StringImpl> m_impl;
+  };
+
+  class StringImpl {
+  public:
+    void ref() const { ++m_refCount; }
+    void deref() const {
+      if (!--m_refCount)
+        delete this;
+    }
+
+    static constexpr unsigned s_flagIs8Bit = 1u << 0;
+    bool is8Bit() const { return m_hashAndFlags & s_flagIs8Bit; }
+    const char* characters8() const { return m_char8; }
+    const short* characters16() const { return m_char16; }
+    unsigned length() const { return m_length; }
+    Ref<StringImpl> substring(unsigned position, unsigned length) const;
+
+    unsigned long find(char) const;
+    unsigned long find(StringView) const;
+    unsigned long contains(StringView) const;
+    unsigned long findIgnoringASCIICase(StringView) const;
+
+    bool startsWith(StringView) const;
+    bool startsWithIgnoringASCIICase(StringView) const;
+    bool endsWith(StringView) const;
+    bool endsWithIgnoringASCIICase(StringView) const;
+
+  private:
+    mutable unsigned m_refCount { 0 };
+    unsigned m_length { 0 };
+    union {
+      const char* m_char8;
+      const short* m_char16;
+    };
+    unsigned m_hashAndFlags { 0 };
+  };
+
+  class String {
+  public:
+    String() = default;
+    String(StringImpl& impl) : m_impl(&impl) { }
+    String(StringImpl* impl) : m_impl(impl) { }
+    String(Ref<StringImpl>&& impl) : m_impl(impl.get()) { }
+    StringImpl* impl() { return m_impl.get(); }
+    unsigned length() const { return m_impl ? m_impl->length() : 0; }
+    const char* characters8() const { return m_impl ? m_impl->characters8() : nullptr; }
+    const short* characters16() const { return m_impl ? m_impl->characters16() : nullptr; }
+
+    bool is8Bit() const { return !m_impl || m_impl->is8Bit(); }
+
+    unsigned long find(char character) const { return m_impl ? m_impl->find(character) : notFound; }
+    unsigned long find(StringView str) const { return m_impl ? m_impl->find(str) : notFound; }
+    unsigned long findIgnoringASCIICase(StringView) const;
+
+    bool contains(char character) const { return find(character) != notFound; }
+    bool contains(StringView) const;
+    bool containsIgnoringASCIICase(StringView) const;
+
+    bool startsWith(StringView) const;
+    bool startsWithIgnoringASCIICase(StringView) const;
+    bool endsWith(StringView) const;
+    bool endsWithIgnoringASCIICase(StringView) const;
+
+    String substring(unsigned position, unsigned length) const
+    {
+      if (!m_impl)
+        return { };
+      if (!position && length >= m_impl->length())
+        return *this;
+      return m_impl->substring(position, length);
+    }
+
+  private:
+    RefPtr<StringImpl> m_impl;
+  };
+
   template <typename T>
   class HashSet {
   public:
@@ -89,6 +175,9 @@ namespace WTF {
 
 }
 
+using WTF::StringView;
+using WTF::StringImpl;
+using WTF::String;
 using WTF::HashSet;
 using WTF::HashMap;
 using WTF::WeakHashSet;
@@ -101,8 +190,37 @@ class RefCounted {
 };
 
 RefCounted* object();
+StringImpl* strImpl();
+String* str();
+StringView strView();
 
 void test() {
+  strImpl()->is8Bit();
+  strImpl()->characters8();
+  strImpl()->characters16();
+  strImpl()->length();
+  strImpl()->substring(2, 4);
+  strImpl()->find(strView());
+  strImpl()->contains(strView());
+  strImpl()->findIgnoringASCIICase(strView());
+  strImpl()->startsWith(strView());
+  strImpl()->startsWithIgnoringASCIICase(strView());
+  strImpl()->endsWith(strView());
+  strImpl()->endsWithIgnoringASCIICase(strView());
+
+  str()->is8Bit();
+  str()->characters8();
+  str()->characters16();
+  str()->length();
+  str()->substring(2, 4);
+  str()->find(strView());
+  str()->contains(strView());
+  str()->findIgnoringASCIICase(strView());
+  str()->startsWith(strView());
+  str()->startsWithIgnoringASCIICase(strView());
+  str()->endsWith(strView());
+  str()->endsWithIgnoringASCIICase(strView());
+
   HashSet<RefPtr<RefCounted>> set;
   set.find(*object());
   set.contains(*object());
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index c27ea9baaf3bf5..c427b22fd683e5 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -47,7 +47,7 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
   typename PtrTraits::StorageType t;
 
   Ref() : t{} {};
-  Ref(T &t) : t(RefDerefTraits::refIfNotNull(t)) { }
+  Ref(T &t) : t(&RefDerefTraits::ref(t)) { }
   Ref(const Ref& o) : t(RefDerefTraits::refIfNotNull(PtrTraits::unwrap(o.t))) { }
   ~Ref() { RefDerefTraits::derefIfNotNull(PtrTraits::exchange(t, nullptr)); }
   T &get() { return *PtrTraits::unwrap(t); }
@@ -55,7 +55,7 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
   T *operator->() { return PtrTraits::unwrap(t); }
   operator const T &() const { return *PtrTraits::unwrap(t); }
   operator T &() { return *PtrTraits::unwrap(t); }
-  T* leakRef() { PtrTraits::exchange(t, nullptr); }
+  T* leakRef() { return PtrTraits::exchange(t, nullptr); }
 };
 
 template <typename T> struct RefPtr {
@@ -67,6 +67,9 @@ template <typename T> struct RefPtr {
     if (t)
       t->ref();
   }
+  RefPtr(Ref<T>&& o)
+    : t(o.leakRef())
+  { }
   ~RefPtr() {
     if (t)
       t->deref();
@@ -76,7 +79,7 @@ template <typename T> struct RefPtr {
   const T *operator->() const { return t; }
   T &operator*() { return *t; }
   RefPtr &operator=(T *) { return *this; }
-  operator bool() { return t; }
+  operator bool() const { return t; }
 };
 
 template <typename T> bool operator==(const RefPtr<T> &, const RefPtr<T> &) {

haoNoQ
haoNoQ approved these changes May 1, 2024
View reviewed changes
Copy link
Collaborator

@haoNoQ haoNoQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The revert was caused entirely by something in the mock headers in tests right?

@rniwa
Copy link
Contributor Author

rniwa commented May 1, 2024

LGTM! The revert was caused entirely by something in the mock headers in tests right?

Yeah, there were some typos in mock-types. Not sure why checks didn't catch it.

@rniwa rniwa merged commit df91cde into llvm:main May 2, 2024
@rniwa rniwa deleted the ignore-wtf-string-containers branch May 2, 2024 03:29
rniwa added a commit to rniwa/llvm-project that referenced this pull request Feb 3, 2025
@rniwa
[alpha.webkit.UncountedCallArgsChecker] Ignore methods of WTF String …
6de0667 
…classes. (llvm#90704)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haoNoQ haoNoQ haoNoQ approved these changes

Assignees
No one assigned
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rniwa @llvmbot @haoNoQ