[Analyzer][WebKit] UncountedLocalVarsChecker

[jkorous]

• added docs

[NoQ]

clang/docs/analyzer/checkers.rst

2541–2543

Why exactly is this bad but strictly nested scopes are good? You can't lexically use uncounted after the destructor is invoked. Even if you make a separate ref-counted object in this scope and start counting uncounted with it, it'll still be dead *before* the destructor of counted because destructors are guaranteed to be invoked in the opposite order.

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

52

*tries out the new fancy phabricator suggestions*

[jkorous]

clang/docs/analyzer/checkers.rst

2541–2543

I agree that this is not sound in its current shape. IIRC the idea for this rule came from WebKit folks and we decided to give it a try. I only got this far but since it's somewhat self-contained and somewhat coherent I was thinking landing it as alpha checker might be ok. What do you think?

The thinking was that ultimately we'd be able to make it pretty much sound. The rule about the scope was more about being conservative and making implementation of the checker easier.

For the full picture of the glorious hypothetical future please:

1. Imagine that uncounted variables could be initialized only from ref-counted variables whose names start with guard_ and are outside of the declaration scope of the uncounted variable.
2. Imagine that we'd prohibit any modifications (assignments, resets, destructions, etc) for ref-counted variables named guard_ in scopes embedded in their declaration scope.
3. Imagine that we might even prohibit access to the raw pointer from guard_ prefixed ref-counted variables directly in the scope they are declared. To avoid zombie-fication between their definition and the embedded scope they'd guard.
4. Consider that we might allow definitions of uninitialized uncounted local variables only under certain conditions.
5. Consider that we might prohibit and check for delete-ing values obtained from ref-counted variables named guard_* (and potentially uncounted local variables).

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

52

Thanks!

That's a sweet phab feature!

[jkorous]

fix typo

[jkorous]

Friendly ping

[NoQ]

clang/docs/analyzer/checkers.rst

2541–2543

Ok! I mean, i still don't understand why same-level variables are bad, but i guess a nested scope is a pretty good indication that the user has thought deeply about ownership.

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

33

It's possible to do this linearly, in top-down manner, right? I.e., instead of looking up parents in the parent map, remember them as a context on the stack while visiting stuff. I guess that's what i would have done if i had a normal non-recursive StmtVisitor from the start; in this case i'd simply skip the nested DeclStmts when visiting if-statements and such (possibly diving into their initializers directly, not sure if that's necessary but i guess those could contain GNU StmtExprs).

But i guess it's still more or less linear given that we're looking up at most two parents every time.

85–90

Mmm, ok, this one's a lot less linear than the other one. I still feel like you should consider doing a plain StmtVisitor (which should work just fine given that you're already analyzing only function bodies), and then maintain a stack of scopes as part of your state during traversal. It might end up being a lot more straightforward and possibly faster.

233

I think there should be more explanation behind the word "unsafe". I.e., "not backed by a ref-counted variable in a larger scope" or something like that.

[jkorous]

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

33

I could probably use a recursive StmtVisitor, filter out everything I am not interested in (seems like a lot https://clang.llvm.org/doxygen/classclang_1_1CompoundStmt.html) and visit VarDecl child nodes.

But won't the complexity be actually worse?

O(count(Stmt)) "could be >" O(count(local VarDecl))

85–90

I see what you don't like and agree that I'd be happier to have a clearer and more performant approach but I don't understand your suggestion.

Let me start by explaining how I understand the task we have here and how I implemented it. Maybe you'll see some completely different approach than I did.

We get an arbitrary pair (Guarded, MaybeGuardian) of VarDecl-s where Guarded is newly defined local variable and MaybeGuardian is used to initialize it (potentially "indirectly").

FooClass guarded = maybeguardian.get();

Informally speaking we are trying to verify that MaybeGuardian VarDecl is defined in a block that is an ancestor (== direct or indirect parent, a block is NOT ancestor to itself) to the block where Guarded VarDecl is defined.

In terms of nodes we are trying to verify that MaybeGuardian VarDecl is part of CompoundStmt which also contains the CompoundStmt which contains Guarded VarDecl. The "CompoundStmt which contains the CompoundStmt" is important as we don't allow Guarded and MaybeGuardian to be declared in the same scope/as part of the same CompoundStmt.

The way I implemented is:

1. Find the closest CompoundStmt ancestor of MaybeGuardian VarDecl.
2. Iterate CompoundStmt ancestors of Guarded VarDecl (from Guarded to TranslationUnitDecl).
   1. Skip the first CompoundStmt ancestors of Guarded as we don't allow Guarded and MaybeGuardian to be defined in the same scope
   2. If any further CompoundStmt ancestors of Guarded == the closest CompoundStmt ancestor of MaybeGuardian VarDecl from step 1. we've verified that the requirement about blocks is satisfied => return true;.
3. Otherwise return false;.

Now, I guess we definitely could trade some memory for CPU cycles but I don't see any trivial solution due to these:

• It probably doesn't make sense to run the checker from any other node than uncounted local variable being initialized - this is the Guarded VarDecl in this context. I feel that if we rewrite the checker to start by visiting() any other node that plays a role in this pattern the performance might be even worse.
• We shouldn't expect that the MaybeGuardian has to be in the direct parent CompoundStmt of Guarded.

I can imagine that we could keep a stack of CompoundStmt and manage it in dataTraverseStmtPre() and dataTraverseStmtPost() but don't see how to use it so it'd be a clear win in terms of readability or performance.

[jkorous]

Also, another thought - this idea is really alpha for us too in the sense that we still haven't gotten very good feel for how invasive it is. That means that it's pretty likely that it'll change and I would not be surprised if we for example decide to drop the requirement for embedded block.
I am wondering whether it makes sense to invest into refactoring now. Do you think landing it as it is would still somehow fit under the alpha label or is it expected such checkers to live out of tree until they mature enough?

[jkorous]

Sorry to bother you @NoQ - it'd be awesome if you could take a look!

[jkorous]

Friendly ping

[NoQ]

Sry! I'll try my best to keep up from now on.

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

33

The code is already O(Stmt) because that's what RecursiveASTVisitor does anyway, but it's more like O(count(Stmt)²) in your worst case when for every statement you ascend through all of its parents. In this case it probably doesn't matter because there are just two parents. In the other case it probably matters, but only a little bit - of course such tall ASTs don't exist in practice.

85–90

(i seem to be replying in the other comment)

and manage it in dataTraverseStmtPre() and dataTraverseStmtPost()

In case of non-recursive visitor it's just

void VisitCompoundStmt(const CompoundStmt *S) {
  // Put stuff on stack.
  VisitChildren(S);
  // Remove stuff from stack.
}

So it's very idiomatic and intuitive. Furthermore, instead of a stack you can do something fancier like ImmutableSet of all active variables that'll allow you to not iterate at all over previous layers while still keeping you O(Stmt).

But then, again, it's not a strong opinion. The code is understandable as-is and i doubt performance benefits are actually going to be noticeable.

131

I don't understand what is this bugtype trying to tell me. I'm like, "Type parameter? Is it about template arguments?"

How about "Raw pointer or reference not backed by ref-counted variable"?

clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp

9

You mean like a rule, "always initialize your raw pointers"? I'm not sure there's much benefit over the existing checker that finds use of uninitialized variables path-sensitively... are we that desparate(?)

101

FileCheck isn't actually running in this test file.

[NoQ]

(I.e., i don't have strong opinions about high-level design and will happily push the green button as soon as minor nits are addressed)

[jkorous]

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

131

How about "Uncounted raw pointer or reference not provably backed by ref-counted variable"?

• We don't warn about types not related to ref-counting - for example int* would be ignored.
• Since as of now we have the rule about scopes there are trivial examples where we'd print the warning even for pointers very obviously backed by ref-counted variable.

void foo() {
   RefPtr<Foo> rc_foo;
   Foo * raw_foo = rc_foo.get(); // warning
}

clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp

9

I probably wrote much less than was on my mind which is roughly that the current implementation is a set of individual rules that prevent certain classes of bugs but definitely still have many "holes" if we try using it as a system which would guarantee absence of certain classes of bugs. In the future I'd like to keep individual rules simple but close the loopholes. One such glaring loophole in particular is that we don't check for assignments to uncounted local pointer variables. The uninitialized-ness here is just a signal that we don't check that particular variable at all (as of now).

101

Ahh, thanks! I originally used FileCheck for tests of the prototype.

[jkorous]

Nits were addressed (hopefully).

[NoQ]

Thank you, i think this is good to land!

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

131

That sounds pretty awesome, i like it.

[jkorous]

Thank you Artem!

[jkorous]

I made a typo in the commit message reference so this revision didn't get closed automatically.

clang/docs/analyzer/checkers.rst

 We also define a set of safe transformations which if passed a safe value as an input provide (usually it's the return value) a safe value (or an object that provides safe values). This is also a heuristic.
 
 - constructors of ref-counted types (including factory methods)
 - getters of ref-counted types
 - member overloaded operators
 - casts
 - unary operators like ``&`` or ``*``
 
+alpha.webkit.UncountedLocalVarsChecker
+""""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that any uncounted local variable is backed by a ref-counted object with lifetime that is strictly larger than the scope of the uncounted local variable. To be on the safe side we require the scope of an uncounted variable to be embedded in the scope of ref-counted object that backs it.
+
+These are examples of cases that we consider safe:
+
+  .. code-block:: cpp
+    void foo1() {
+      RefPtr<RefCountable> counted;
+      // The scope of uncounted is EMBEDDED in the scope of counted.
+      {
+        RefCountable* uncounted = counted.get(); // ok
+      }
+    }
+
+    void foo2(RefPtr<RefCountable> counted_param) {
+      RefCountable* uncounted = counted_param.get(); // ok
+    }
+
+    void FooClass::foo_method() {
+      RefCountable* uncounted = this; // ok
+    }
+
+Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that an argument is safe or it's considered if not a bug then bug-prone.
+
+  .. code-block:: cpp
+
+    void foo1() {
+      RefCountable* uncounted = new RefCountable; // warn
+    }
+
+    RefCountable* global_uncounted;
+    void foo2() {
+      RefCountable* uncounted = global_uncounted; // warn
+    }
+
+    void foo3() {
+      RefPtr<RefCountable> counted;
+      // The scope of uncounted is not EMBEDDED in the scope of counted.
+      RefCountable* uncounted = counted.get(); // warn
+    }
+
+We don't warn about these cases - we don't consider them necessarily safe but since they are very common and usually safe we'd introduce a lot of false positives otherwise:
+- variable defined in condition part of an ```if``` statement
+- variable defined in init statement condition of a ```for``` statement
+
+For the time being we also don't warn about uninitialized uncounted local variables.
 
 Debug Checkers
 ---------------
 
 .. _debug-checkers:
 
 
 debug

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 } // end webkit
 
 let ParentPackage = WebKitAlpha in {
 
 def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
   HelpText<"Check uncounted call arguments.">,
   Documentation<HasDocumentation>;
 
+def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">,
+  HelpText<"Check uncounted local variables.">,
+  Documentation<HasDocumentation>;
+
 } // end alpha.webkit

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   ValistChecker.cpp
   VirtualCallChecker.cpp
   WebKit/NoUncountedMembersChecker.cpp
   WebKit/ASTUtils.cpp
   WebKit/PtrTypesSemantics.cpp
   WebKit/RefCntblBaseVirtualDtorChecker.cpp
   WebKit/UncountedCallArgsChecker.cpp
   WebKit/UncountedLambdaCapturesChecker.cpp
+  WebKit/UncountedLocalVarsChecker.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangAnalysis
   clangBasic
   clangLex
   clangStaticAnalyzerCore
 
   DEPENDS
   omp_gen
   )

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

+//=======- UncountedLocalVarsChecker.cpp -------------------------*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ASTUtils.h"
+#include "DiagOutputUtils.h"
+#include "PtrTypesSemantics.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/ParentMapContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "llvm/ADT/DenseSet.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+// for ( int a = ...) ... true
+// for ( int a : ...) ... true
+// if ( int* a = ) ... true
+// anything else ... false
+bool isDeclaredInForOrIf(const VarDecl *Var) {
+  assert(Var);
+  auto &ASTCtx = Var->getASTContext();
+  auto parent = ASTCtx.getParents(*Var);
+
+  if (parent.size() == 1) {
+    if (auto *DS = parent.begin()->get<DeclStmt>()) {
+      DynTypedNodeList grandParent = ASTCtx.getParents(*DS);
+      if (grandParent.size() == 1) {
+        return grandParent.begin()->get<ForStmt>() ||
+               grandParent.begin()->get<IfStmt>() ||
+               grandParent.begin()->get<CXXForRangeStmt>();
+      }
+    }
+  }
+  return false;
+}
+
+// FIXME: should be defined by annotations in the future
+bool isRefcountedStringsHack(const VarDecl *V) {
+  assert(V);
+  auto safeClass = [](const std::string &className) {
+    return className == "String" || className == "AtomString" ||
+           className == "UniquedString" || className == "Identifier";
+  };
+  QualType QT = V->getType();
+  auto *T = QT.getTypePtr();
+  if (auto *CXXRD = T->getAsCXXRecordDecl()) {
+    if (safeClass(safeGetName(CXXRD)))
+      return true;
+  }
+  if (T->isPointerType() || T->isReferenceType()) {
+    if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
+      if (safeClass(safeGetName(CXXRD)))
+        return true;
+    }
+  }
+  return false;
+}
+
+bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
+                                           const VarDecl *MaybeGuardian) {
+  assert(Guarded);
+  assert(MaybeGuardian);
+
+  if (!MaybeGuardian->isLocalVarDecl())
+    return false;
+
+  const CompoundStmt *guardiansClosestCompStmtAncestor = nullptr;
+
+  ASTContext &ctx = MaybeGuardian->getASTContext();
+
+  for (DynTypedNodeList guardianAncestors = ctx.getParents(*MaybeGuardian);
+       !guardianAncestors.empty();
+       guardianAncestors = ctx.getParents(
+           *guardianAncestors
+                .begin()) // FIXME - should we handle all of the parents?
+  ) {
+    for (auto &guardianAncestor : guardianAncestors) {
+      if (auto *CStmtParentAncestor = guardianAncestor.get<CompoundStmt>()) {
+        guardiansClosestCompStmtAncestor = CStmtParentAncestor;
+        break;
+      }
+    }
+    if (guardiansClosestCompStmtAncestor)
+      break;
+  }
+
+  if (!guardiansClosestCompStmtAncestor)
+    return false;
+
+  // We need to skip the first CompoundStmt to avoid situation when guardian is
+  // defined in the same scope as guarded variable.
+  bool HaveSkippedFirstCompoundStmt = false;
+  for (DynTypedNodeList guardedVarAncestors = ctx.getParents(*Guarded);
+       !guardedVarAncestors.empty();
+       guardedVarAncestors = ctx.getParents(
+           *guardedVarAncestors
+                .begin()) // FIXME - should we handle all of the parents?
+  ) {
+    for (auto &guardedVarAncestor : guardedVarAncestors) {
+      if (auto *CStmtAncestor = guardedVarAncestor.get<CompoundStmt>()) {
+        if (!HaveSkippedFirstCompoundStmt) {
+          HaveSkippedFirstCompoundStmt = true;
+          continue;
+        }
+        if (CStmtAncestor == guardiansClosestCompStmtAncestor)
+          return true;
+      }
+    }
+  }
+
+  return false;
+}
+
+class UncountedLocalVarsChecker
+    : public Checker<check::ASTDecl<TranslationUnitDecl>> {
+  BugType Bug{
+      this, "Uncounted local variable of type raw pointer/reference parameter",
+      "WebKit coding guidelines"};
+  mutable BugReporter *BR;
+
+public:
+  void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
+                    BugReporter &BRArg) const {
+    BR = &BRArg;
+
+    // The calls to checkAST* from AnalysisConsumer don't
+    // visit template instantiations or lambda classes. We
+    // want to visit those, so we make our own RecursiveASTVisitor.
+    struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
+      const UncountedLocalVarsChecker *Checker;
+      explicit LocalVisitor(const UncountedLocalVarsChecker *Checker)
+          : Checker(Checker) {
+        assert(Checker);
+      }
+
+      bool shouldVisitTemplateInstantiations() const { return true; }
+      bool shouldVisitImplicitCode() const { return false; }
+
+      bool VisitVarDecl(VarDecl *V) {
+        Checker->visitVarDecl(V);
+        return true;
+      }
+    };
+
+    LocalVisitor visitor(this);
+    visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+  }
+
+  void visitVarDecl(const VarDecl *V) const {
+    if (shouldSkipVarDecl(V))
+      return;
+
+    const auto *ArgType = V->getType().getTypePtr();
+    if (!ArgType)
+      return;
+
+    if (isUncountedPtr(ArgType)) {
+      const Expr *const InitExpr = V->getInit();
+      if (!InitExpr)
+        return; // FIXME: later on we might warn on uninitialized vars too
+
+      const clang::Expr *const InitArgOrigin =
+          tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false)
+              .first;
+      if (!InitArgOrigin)
+        return;
+
+      if (isa<CXXThisExpr>(InitArgOrigin))
+        return;
+
+      if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
+        if (auto *MaybeGuardian =
+                dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+          const auto *MaybeGuardianArgType =
+              MaybeGuardian->getType().getTypePtr();
+          if (!MaybeGuardianArgType)
+            return;
+          const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
+              MaybeGuardianArgType->getAsCXXRecordDecl();
+          if (!MaybeGuardianArgCXXRecord)
+            return;
+
+          if (MaybeGuardian->isLocalVarDecl() &&
+              (isRefCounted(MaybeGuardianArgCXXRecord) ||
+               isRefcountedStringsHack(MaybeGuardian)) &&
+              isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) {
+            return;
+          }
+
+          // Parameters are guaranteed to be safe for the duration of the call
+          // by another checker.
+          if (isa<ParmVarDecl>(MaybeGuardian))
+            return;
+        }
+      }
+
+      reportBug(V);
+    }
+  }
+
+  bool shouldSkipVarDecl(const VarDecl *V) const {
+    assert(V);
+    if (!V->isLocalVarDecl())
+      return true;
+
+    if (isDeclaredInForOrIf(V))
+      return true;
+
+    return false;
+  }
+
+  void reportBug(const VarDecl *V) const {
+    assert(V);
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    Os << "Local variable ";
+    printQuotedQualifiedName(Os, V);
+    Os << " is uncounted and unsafe.";
+
+    PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    Report->addRange(V->getSourceRange());
+    BR->emitReport(std::move(Report));
+  }
+};
+} // namespace
+
+void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UncountedLocalVarsChecker>();
+}
+
+bool ento::shouldRegisterUncountedLocalVarsChecker(const CheckerManager &) {
+  return true;
+}

clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp

+// RUN: %clang_analyze_cc1
+// -analyzer-checker=alpha.webkit.UncountedLocalVarsChecker -verify %s
+
+#include "mock-types.h"
+
+namespace raw_ptr {
+void foo() {
+  RefCountable *bar;
+  // FIXME: later on we might warn on uninitialized vars too
+}
+
+void bar(RefCountable *) {}
+} // namespace raw_ptr
+
+namespace reference {
+void foo_ref() {
+  RefCountable automatic;
+  RefCountable &bar = automatic;
+  // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe
+  // [alpha.webkit.UncountedLocalVarsChecker]}}
+}
+
+void bar_ref(RefCountable &) {}
+} // namespace reference
+
+namespace guardian_scopes {
+void foo1() {
+  RefPtr<RefCountable> foo;
+  { RefCountable *bar = foo.get(); }
+}
+
+void foo2() {
+  RefPtr<RefCountable> foo;
+  // missing embedded scope here
+  RefCountable *bar = foo.get();
+  // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe
+  // [alpha.webkit.UncountedLocalVarsChecker]}}
+}
+
+void foo3() {
+  RefPtr<RefCountable> foo;
+  {
+    { RefCountable *bar = foo.get(); }
+  }
+}
+
+void foo4() {
+  {
+    RefPtr<RefCountable> foo;
+    { RefCountable *bar = foo.get(); }
+  }
+}
+} // namespace guardian_scopes
+
+namespace auto_keyword {
+class Foo {
+  RefCountable *provide_ref_ctnbl() { return nullptr; }
+
+  void evil_func() {
+    RefCountable *bar = provide_ref_ctnbl();
+    // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe
+    // [alpha.webkit.UncountedLocalVarsChecker]}}
+    auto *baz = provide_ref_ctnbl();
+    // expected-warning@-1{{Local variable 'baz' is uncounted and unsafe
+    // [alpha.webkit.UncountedLocalVarsChecker]}}
+    auto *baz2 = this->provide_ref_ctnbl();
+    // expected-warning@-1{{Local variable 'baz2' is uncounted and unsafe
+    // [alpha.webkit.UncountedLocalVarsChecker]}}
+  }
+};
+} // namespace auto_keyword
+
+namespace guardian_casts {
+void foo1() {
+  RefPtr<RefCountable> foo;
+  { RefCountable *bar = downcast<RefCountable>(foo.get()); }
+}
+
+void foo2() {
+  RefPtr<RefCountable> foo;
+  {
+    RefCountable *bar =
+        static_cast<RefCountable *>(downcast<RefCountable>(foo.get()));
+  }
+}
+} // namespace guardian_casts
+
+namespace guardian_ref_conversion_operator {
+void foo() {
+  Ref<RefCountable> rc;
+  { RefCountable &rr = rc; }
+}
+} // namespace guardian_ref_conversion_operator
+
+namespace ignore_for_if {
+RefCountable *provide_ref_ctnbl() { return nullptr; }
+
+void foo() {
+  if (RefCountable *a = provide_ref_ctnbl()) {
+  }
+  // CHECK-NOT: warning: [WebKit]
+  for (RefCountable *a = provide_ref_ctnbl(); a != nullptr;) {
+  }
+  // CHECK-NOT: warning: [WebKit]
+  RefCountable *array[1];
+  for (RefCountable *a : array) {
+  }
+  // CHECK-NOT: warning: [WebKit]
+}
+} // namespace ignore_for_if