[clang] [alpha.webkit.UncountedLocalVarsChecker] Don't skip the else branch of an if statement with a condition variable and a trivial then branch (PR #227262)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Fady Farag (iidmsa)

<details>
<summary>Changes</summary>

Previously, when an `if` statement had a condition variable and a trivial then branch, `TraverseIfStmt` skipped the entire statement. That is correct for the condition variable, which is null in the else branch, but it also skipped the else branch, which caused a missing warning for any raw pointer/reference local variable declared there. This still exempts the condition variable but traverses the else branch when it is not trivial.

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


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp (+8-5) 
- (modified) clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp (+31) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index d0191bd0ccc62..232cca1c7a2c4 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -339,12 +339,15 @@ class RawPtrRefLocalVarsChecker
 
       bool TraverseIfStmt(IfStmt *IS) override {
         if (IS->getConditionVariable()) {
-          // This code currently does not explicitly check the "else" statement
-          // since getConditionVariable returns nullptr when there is a
-          // condition defined after ";" as in "if (auto foo = ~; !foo)". If
-          // this semantics change, we should add an explicit check for "else".
-          if (auto *Then = IS->getThen(); !Then || TFA.isTrivial(Then))
+          // This code does not check the condition variable in the "else"
+          // statement since getConditionVariable returns nullptr when there
+          // is a condition defined after ";" as in "if (auto foo = ~; !foo)".
+          // If this semantics change, we should check it in "else" as well.
+          if (auto *Then = IS->getThen(); !Then || TFA.isTrivial(Then)) {
+            if (auto *Else = IS->getElse(); Else && !TFA.isTrivial(Else))
+              return TraverseStmt(Else);
             return true;
+          }
         }
         if (!TFA.isTrivial(IS))
           return DynamicRecursiveASTVisitor::TraverseIfStmt(IS);
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 96ff48b9605b3..7f086f13f68e4 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -704,6 +704,37 @@ namespace vardecl_in_if_condition {
       return obj->next();
   }
 
+  RefCountable* trivialProvide() { return nullptr; }
+
+  void local_in_non_trivial_else() {
+    if (auto* obj = provide())
+      obj->trivial();
+    else {
+      auto* other = provide(); // expected-warning{{Local variable 'other' is a raw pointer to RefPtr-capable type 'RefCountable' [alpha.webkit.UncountedLocalVarsChecker]}}
+      someFunction();
+      other->method();
+    }
+  }
+
+  void local_in_non_trivial_else_if(bool flag) {
+    if (auto* obj = provide())
+      obj->trivial();
+    else if (flag) {
+      auto* other = provide(); // expected-warning{{Local variable 'other' is a raw pointer to RefPtr-capable type 'RefCountable' [alpha.webkit.UncountedLocalVarsChecker]}}
+      someFunction();
+      other->method();
+    }
+  }
+
+  void local_in_trivial_else() {
+    if (auto* obj = provide())
+      obj->trivial();
+    else {
+      auto* other = trivialProvide(); // no warning
+      other->trivial();
+    }
+  }
+
 }
 
 namespace delete_unresolved_type {

``````````

</details>


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