Skip to content

Conversation

@rniwa
Copy link
Contributor

@rniwa rniwa commented Apr 6, 2025

This PR fixes a bug that when a template specialization is declared with a forward declaration of a template, the checker fails to find its definition in the same translation unit and erroneously emit an unsafe forward declaration warning.

…te specialization

This PR fixes a bug that when a template specialization is declared with a forward declaration
of a template, the checker fails to find its definition in the same translation unit and
erroneously emit an unsafe forward declaration warning.
@rniwa rniwa requested a review from t-rasmud April 6, 2025 16:29
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Apr 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 6, 2025

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

Author: Ryosuke Niwa (rniwa)

Changes

This PR fixes a bug that when a template specialization is declared with a forward declaration of a template, the checker fails to find its definition in the same translation unit and erroneously emit an unsafe forward declaration warning.


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

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp (+12-2)
  • (modified) clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm (+17)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp
index 2c63224df129a..4f63e5ed866ed 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp
@@ -125,8 +125,18 @@ class ForwardDeclChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> {
     if (!R) // Forward declaration of a Objective-C interface is safe.
       return false;
     auto Name = R->getName();
-    return !R->hasDefinition() && !RTC.isUnretained(QT) &&
-           !SystemTypes.contains(CanonicalType) &&
+    if (R->hasDefinition())
+      return false;
+    // Find a definition amongst template declarations.
+    if (auto *Specialization = dyn_cast<ClassTemplateSpecializationDecl>(R)) {
+      if (auto* S = Specialization->getSpecializedTemplate()) {
+        for (S = S->getMostRecentDecl(); S; S = S->getPreviousDecl()) {
+          if (S->isThisDeclarationADefinition())
+            return false;
+        }
+      }
+    }
+    return !RTC.isUnretained(QT) && !SystemTypes.contains(CanonicalType) &&
            !SystemTypes.contains(PointeeType) && !Name.starts_with("Opaque") &&
            Name != "_NSZone";
   }
diff --git a/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm b/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm
index 64100d60c4867..084b47322d7f9 100644
--- a/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm
+++ b/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm
@@ -138,3 +138,20 @@ - (void)doMoreWork:(ObjCObj *)obj {
 }
 
 @end
+
+namespace template_forward_declare {
+
+template<typename> class HashSet;
+
+template<typename T>
+using SingleThreadHashSet = HashSet<T>;
+
+template<typename> class HashSet { };
+
+struct Font { };
+
+struct ComplexTextController {
+    SingleThreadHashSet<const Font>* fallbackFonts { nullptr };
+};
+
+}

@llvmbot
Copy link
Member

llvmbot commented Apr 6, 2025

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

This PR fixes a bug that when a template specialization is declared with a forward declaration of a template, the checker fails to find its definition in the same translation unit and erroneously emit an unsafe forward declaration warning.


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

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp (+12-2)
  • (modified) clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm (+17)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp
index 2c63224df129a..4f63e5ed866ed 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp
@@ -125,8 +125,18 @@ class ForwardDeclChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> {
     if (!R) // Forward declaration of a Objective-C interface is safe.
       return false;
     auto Name = R->getName();
-    return !R->hasDefinition() && !RTC.isUnretained(QT) &&
-           !SystemTypes.contains(CanonicalType) &&
+    if (R->hasDefinition())
+      return false;
+    // Find a definition amongst template declarations.
+    if (auto *Specialization = dyn_cast<ClassTemplateSpecializationDecl>(R)) {
+      if (auto* S = Specialization->getSpecializedTemplate()) {
+        for (S = S->getMostRecentDecl(); S; S = S->getPreviousDecl()) {
+          if (S->isThisDeclarationADefinition())
+            return false;
+        }
+      }
+    }
+    return !RTC.isUnretained(QT) && !SystemTypes.contains(CanonicalType) &&
            !SystemTypes.contains(PointeeType) && !Name.starts_with("Opaque") &&
            Name != "_NSZone";
   }
diff --git a/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm b/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm
index 64100d60c4867..084b47322d7f9 100644
--- a/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm
+++ b/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm
@@ -138,3 +138,20 @@ - (void)doMoreWork:(ObjCObj *)obj {
 }
 
 @end
+
+namespace template_forward_declare {
+
+template<typename> class HashSet;
+
+template<typename T>
+using SingleThreadHashSet = HashSet<T>;
+
+template<typename> class HashSet { };
+
+struct Font { };
+
+struct ComplexTextController {
+    SingleThreadHashSet<const Font>* fallbackFonts { nullptr };
+};
+
+}

@github-actions
Copy link

github-actions bot commented Apr 6, 2025

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

Copy link
Contributor

@t-rasmud t-rasmud left a comment

Choose a reason for hiding this comment

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

LGTM!

@rniwa
Copy link
Contributor Author

rniwa commented Apr 10, 2025

Thanks for the review!

@rniwa rniwa merged commit 6136019 into llvm:main Apr 10, 2025
11 checks passed
@rniwa rniwa deleted the fix-webkit-forward-decl-template-specialization branch April 10, 2025 22:28
rniwa added a commit to rniwa/llvm-project that referenced this pull request Apr 22, 2025
…te specialization (llvm#134545)

This PR fixes a bug that when a template specialization is declared with
a forward declaration of a template, the checker fails to find its
definition in the same translation unit and erroneously emit an unsafe
forward declaration warning.
rniwa added a commit to rniwa/llvm-project that referenced this pull request Aug 21, 2025
…te specialization (llvm#134545)

This PR fixes a bug that when a template specialization is declared with
a forward declaration of a template, the checker fails to find its
definition in the same translation unit and erroneously emit an unsafe
forward declaration warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:static analyzer clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants