Add a warning (off by default) for repeated use of the same weak property.

The motivating example:

if (self.weakProp)
  use(self.weakProp);

As with any non-atomic test-then-use, it is possible a weak property to be
non-nil at the 'if', but be deallocated by the time it is used. The correct
way to write this example is as follows:

id tmp = self.weakProp;
if (tmp)
  use(tmp);

The warning is controlled by -Warc-repeated-use-of-receiver, and uses the
property name and base to determine if the same property on the same object
is being accessed multiple times. In cases where the base is more
complicated than just a single Decl (e.g. 'foo.bar.weakProp'), it picks a
Decl for some degree of uniquing and reports the problem under a subflag,
-Warc-maybe-repeated-use-of-receiver. This gives a way to tune the
aggressiveness of the warning for a particular project.

The warning is not on by default because it is not flow-sensitive and thus
may have a higher-than-acceptable rate of false positives, though it is
less noisy than -Wreceiver-is-weak. On the other hand, it will not warn
about some cases that may be legitimate issues that -Wreceiver-is-weak
will catch, and it does not attempt to reason about methods returning weak
values.

Even though this is not a real "analysis-based" check I've put the bug
emission code in AnalysisBasedWarnings for two reasons: (1) to run on
every kind of code body (function, method, block, or lambda), and (2) to
suggest that it may be enhanced by flow-sensitive analysis in the future.

The second (smaller) half of this work is to extend it to weak locals
and weak ivars. This should use most of the same infrastructure.

Part of <rdar://problem/12280249>

llvm-svn: 164854

5 files changed, 308 insertions, 1 deletions

diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index a3aee9afe08..86e9dc2d648 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -871,6 +871,121 @@ static void DiagnoseSwitchLabelsFallthrough(Sema &S, AnalysisDeclContext &AC,
 }
 
 namespace {
+  typedef std::pair<const Stmt *,
+                    sema::FunctionScopeInfo::WeakObjectUseMap::const_iterator>
+          StmtUsesPair;
+}
+
+template<>
+class BeforeThanCompare<StmtUsesPair> {
+  const SourceManager &SM;
+
+public:
+  explicit BeforeThanCompare(const SourceManager &SM) : SM(SM) { }
+
+  bool operator()(const StmtUsesPair &LHS, const StmtUsesPair &RHS) {
+    return SM.isBeforeInTranslationUnit(LHS.first->getLocStart(),
+                                        RHS.first->getLocStart());
+  }
+};
+
+
+static void diagnoseRepeatedUseOfWeak(Sema &S,
+                                      const sema::FunctionScopeInfo *CurFn,
+                                      const Decl *D) {
+  typedef sema::FunctionScopeInfo::WeakObjectProfileTy WeakObjectProfileTy;
+  typedef sema::FunctionScopeInfo::WeakObjectUseMap WeakObjectUseMap;
+  typedef sema::FunctionScopeInfo::WeakUseVector WeakUseVector;
+
+  const WeakObjectUseMap &WeakMap = CurFn->getWeakObjectUses();
+
+  // Extract all weak objects that are referenced more than once.
+  SmallVector<StmtUsesPair, 8> UsesByStmt;
+  for (WeakObjectUseMap::const_iterator I = WeakMap.begin(), E = WeakMap.end();
+       I != E; ++I) {
+    const WeakUseVector &Uses = I->second;
+    if (Uses.size() <= 1)
+      continue;
+
+    // Find the first read of the weak object.
+    WeakUseVector::const_iterator UI = Uses.begin(), UE = Uses.end();
+    for ( ; UI != UE; ++UI) {
+      if (UI->isUnsafe())
+        break;
+    }
+
+    // If there were only writes to this object, don't warn.
+    if (UI == UE)
+      continue;
+
+    UsesByStmt.push_back(StmtUsesPair(UI->getUseExpr(), I));
+  }
+
+  if (UsesByStmt.empty())
+    return;
+
+  // Sort by first use so that we emit the warnings in a deterministic order.
+  std::sort(UsesByStmt.begin(), UsesByStmt.end(),
+            BeforeThanCompare<StmtUsesPair>(S.getSourceManager()));
+
+  // Classify the current code body for better warning text.
+  // This enum should stay in sync with the cases in
+  // warn_arc_repeated_use_of_weak and warn_arc_possible_repeated_use_of_weak.
+  // FIXME: Should we use a common classification enum and the same set of
+  // possibilities all throughout Sema?
+  enum {
+    Function,
+    Method,
+    Block,
+    Lambda
+  } FunctionKind;
+
+  if (isa<sema::BlockScopeInfo>(CurFn))
+    FunctionKind = Block;
+  else if (isa<sema::LambdaScopeInfo>(CurFn))
+    FunctionKind = Lambda;
+  else if (isa<ObjCMethodDecl>(D))
+    FunctionKind = Method;
+  else
+    FunctionKind = Function;
+
+  // Iterate through the sorted problems and emit warnings for each.
+  for (SmallVectorImpl<StmtUsesPair>::const_iterator I = UsesByStmt.begin(),
+                                                     E = UsesByStmt.end();
+       I != E; ++I) {
+    const Stmt *FirstRead = I->first;
+    const WeakObjectProfileTy &Key = I->second->first;
+    const WeakUseVector &Uses = I->second->second;
+
+    // For complicated expressions like self.foo.bar, it's hard to keep track
+    // of whether 'self.foo' is the same between two cases. We can only be
+    // 100% sure of a repeated use if the "base" part of the key is a variable,
+    // rather than, say, another property.
+    unsigned DiagKind;
+    if (Key.isExactProfile())
+      DiagKind = diag::warn_arc_repeated_use_of_weak;
+    else
+      DiagKind = diag::warn_arc_possible_repeated_use_of_weak;
+
+    // Show the first time the object was read.
+    S.Diag(FirstRead->getLocStart(), DiagKind)
+      << FunctionKind
+      << FirstRead->getSourceRange();
+
+    // Print all the other accesses as notes.
+    for (WeakUseVector::const_iterator UI = Uses.begin(), UE = Uses.end();
+         UI != UE; ++UI) {
+      if (UI->getUseExpr() == FirstRead)
+        continue;
+      S.Diag(UI->getUseExpr()->getLocStart(),
+             diag::note_arc_weak_also_accessed_here)
+        << UI->getUseExpr()->getSourceRange();
+    }
+  }
+}
+
+
+namespace {
 struct SLocSort {
   bool operator()(const UninitUse &a, const UninitUse &b) {
     // Prefer a more confident report over a less confident one.
@@ -1364,6 +1479,11 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
     DiagnoseSwitchLabelsFallthrough(S, AC, !FallThroughDiagFull);
   }
 
+  if (S.getLangOpts().ObjCARCWeak &&
+      Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak,
+                               D->getLocStart()) != DiagnosticsEngine::Ignored)
+    diagnoseRepeatedUseOfWeak(S, fscope, D);
+
   // Collect statistics about the CFG if it was built.
   if (S.CollectStats && AC.isCFGBuilt()) {
     ++NumFunctionsAnalyzed;
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 08ccfa42598..84cdb2b6f04 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -54,6 +54,132 @@ void FunctionScopeInfo::Clear() {
   Returns.clear();
   ErrorTrap.reset();
   PossiblyUnreachableDiags.clear();
+  WeakObjectUses.clear();
+}
+
+static const NamedDecl *getBestPropertyDecl(const ObjCPropertyRefExpr *PropE) {
+  if (PropE->isExplicitProperty())
+    return PropE->getExplicitProperty();
+
+  return PropE->getImplicitPropertyGetter();
+}
+
+static bool isSelfExpr(const Expr *E) {
+  E = E->IgnoreParenImpCasts();
+
+  const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E);
+  if (!DRE)
+    return false;
+
+  const ImplicitParamDecl *Param = dyn_cast<ImplicitParamDecl>(DRE->getDecl());
+  if (!Param)
+    return false;
+
+  const ObjCMethodDecl *M = dyn_cast<ObjCMethodDecl>(Param->getDeclContext());
+  if (!M)
+    return false;
+
+  return M->getSelfDecl() == Param;
+}
+
+FunctionScopeInfo::WeakObjectProfileTy::WeakObjectProfileTy(
+                                          const ObjCPropertyRefExpr *PropE)
+    : Base(0, false), Property(getBestPropertyDecl(PropE)) {
+
+  if (PropE->isObjectReceiver()) {
+    const OpaqueValueExpr *OVE = cast<OpaqueValueExpr>(PropE->getBase());
+    const Expr *E = OVE->getSourceExpr()->IgnoreParenCasts();
+
+    switch (E->getStmtClass()) {
+    case Stmt::DeclRefExprClass:
+      Base.setPointer(cast<DeclRefExpr>(E)->getDecl());
+      Base.setInt(isa<VarDecl>(Base.getPointer()));
+      break;
+    case Stmt::MemberExprClass: {
+      const MemberExpr *ME = cast<MemberExpr>(E);
+      Base.setPointer(ME->getMemberDecl());
+      Base.setInt(isa<CXXThisExpr>(ME->getBase()->IgnoreParenImpCasts()));
+      break;
+    }
+    case Stmt::ObjCIvarRefExprClass: {
+      const ObjCIvarRefExpr *IE = cast<ObjCIvarRefExpr>(E);
+      Base.setPointer(IE->getDecl());
+      if (isSelfExpr(IE->getBase()))
+        Base.setInt(true);
+      break;
+    }
+    case Stmt::PseudoObjectExprClass: {
+      const PseudoObjectExpr *POE = cast<PseudoObjectExpr>(E);
+      const ObjCPropertyRefExpr *BaseProp =
+        dyn_cast<ObjCPropertyRefExpr>(POE->getSyntacticForm());
+      if (BaseProp) {
+        Base.setPointer(getBestPropertyDecl(BaseProp));
+
+        const Expr *DoubleBase = BaseProp->getBase();
+        if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(DoubleBase))
+          DoubleBase = OVE->getSourceExpr();
+
+        if (isSelfExpr(DoubleBase))
+          Base.setInt(true);
+      }
+      break;
+    }
+    default:
+      break;
+    }
+  } else if (PropE->isClassReceiver()) {
+    Base.setPointer(PropE->getClassReceiver());
+    Base.setInt(true);
+  } else {
+    assert(PropE->isSuperReceiver());
+    Base.setInt(true);
+  }
+}
+
+void FunctionScopeInfo::recordUseOfWeak(const ObjCPropertyRefExpr *RefExpr) {
+  assert(RefExpr);
+  WeakUseVector &Uses =
+    WeakObjectUses[FunctionScopeInfo::WeakObjectProfileTy(RefExpr)];
+  Uses.push_back(WeakUseTy(RefExpr, RefExpr->isMessagingGetter()));
+}
+
+void FunctionScopeInfo::markSafeWeakUse(const Expr *E) {
+  E = E->IgnoreParenImpCasts();
+
+  if (const PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(E)) {
+    markSafeWeakUse(POE->getSyntacticForm());
+    return;
+  }
+
+  if (const ConditionalOperator *Cond = dyn_cast<ConditionalOperator>(E)) {
+    markSafeWeakUse(Cond->getTrueExpr());
+    markSafeWeakUse(Cond->getFalseExpr());
+    return;
+  }
+
+  if (const BinaryConditionalOperator *Cond =
+        dyn_cast<BinaryConditionalOperator>(E)) {
+    markSafeWeakUse(Cond->getCommon());
+    markSafeWeakUse(Cond->getFalseExpr());
+    return;
+  }
+
+  if (const ObjCPropertyRefExpr *RefExpr = dyn_cast<ObjCPropertyRefExpr>(E)) {
+    // Has this property been seen as a weak property?
+    FunctionScopeInfo::WeakObjectUseMap::iterator Uses =
+      WeakObjectUses.find(FunctionScopeInfo::WeakObjectProfileTy(RefExpr));
+    if (Uses == WeakObjectUses.end())
+      return;
+
+    // Has there been a read from the property using this Expr?
+    FunctionScopeInfo::WeakUseVector::reverse_iterator ThisUse =
+      std::find(Uses->second.rbegin(), Uses->second.rend(), WeakUseTy(E, true));
+    if (ThisUse == Uses->second.rend())
+      return;
+
+    ThisUse->markSafe();
+    return;
+  }
 }
 
 BlockScopeInfo::~BlockScopeInfo() { }
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 01aaf8be323..592956b9dbc 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -6588,6 +6588,21 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init,
 
     if (VDecl->hasAttr<BlocksAttr>())
       checkRetainCycles(VDecl, Init);
+
+    // It is safe to assign a weak reference into a strong variable.
+    // Although this code can still have problems:
+    //   id x = self.weakProp;
+    //   id y = self.weakProp;
+    // we do not warn to warn spuriously when 'x' and 'y' are on separate
+    // paths through the function. This should be revisited if
+    // -Wrepeated-use-of-weak is made flow-sensitive.
+    if (VDecl->getType().getObjCLifetime() == Qualifiers::OCL_Strong) {
+      DiagnosticsEngine::Level Level =
+        Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak,
+                                 Init->getLocStart());
+      if (Level != DiagnosticsEngine::Ignored)
+        getCurFunction()->markSafeWeakUse(Init);
+    }
   }
 
   Init = MaybeCreateExprWithCleanups(Init);
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 630281a4242..597e4d6f77c 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -7726,6 +7726,19 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
         if (!DRE || DRE->getDecl()->hasAttr<BlocksAttr>())
           checkRetainCycles(LHSExpr, RHS.get());
 
+        // It is safe to assign a weak reference into a strong variable.
+        // Although this code can still have problems:
+        //   id x = self.weakProp;
+        //   id y = self.weakProp;
+        // we do not warn to warn spuriously when 'x' and 'y' are on separate
+        // paths through the function. This should be revisited if
+        // -Wrepeated-use-of-weak is made flow-sensitive.
+        DiagnosticsEngine::Level Level =
+          Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak,
+                                   RHS.get()->getLocStart());
+        if (Level != DiagnosticsEngine::Ignored)
+          getCurFunction()->markSafeWeakUse(RHS.get());
+
       } else if (getLangOpts().ObjCAutoRefCount) {
         checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());
       }
diff --git a/clang/lib/Sema/SemaPseudoObject.cpp b/clang/lib/Sema/SemaPseudoObject.cpp
index 31496b32a07..b07a59b81c2 100644
--- a/clang/lib/Sema/SemaPseudoObject.cpp
+++ b/clang/lib/Sema/SemaPseudoObject.cpp
@@ -31,6 +31,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Sema/SemaInternal.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/Lex/Preprocessor.h"
@@ -186,7 +187,7 @@ namespace {
                                     UnaryOperatorKind opcode,
                                     Expr *op);
 
-    ExprResult complete(Expr *syntacticForm);
+    virtual ExprResult complete(Expr *syntacticForm);
 
     OpaqueValueExpr *capture(Expr *op);
     OpaqueValueExpr *captureValueAsResult(Expr *op);
@@ -238,6 +239,9 @@ namespace {
     Expr *rebuildAndCaptureObject(Expr *syntacticBase);
     ExprResult buildGet();
     ExprResult buildSet(Expr *op, SourceLocation, bool);
+    ExprResult complete(Expr *SyntacticForm);
+
+    bool isWeakProperty() const;
   };
 
  /// A PseudoOpBuilder for Objective-C array/dictionary indexing.
@@ -471,6 +475,23 @@ static ObjCMethodDecl *LookupMethodInReceiverType(Sema &S, Selector sel,
   return S.LookupMethodInObjectType(sel, IT, false);
 }
 
+bool ObjCPropertyOpBuilder::isWeakProperty() const {
+  QualType T;
+  if (RefExpr->isExplicitProperty()) {
+    const ObjCPropertyDecl *Prop = RefExpr->getExplicitProperty();
+    if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak)
+      return true;
+
+    T = Prop->getType();
+  } else if (Getter) {
+    T = Getter->getResultType();
+  } else {
+    return false;
+  }
+
+  return T.getObjCLifetime() == Qualifiers::OCL_Weak;
+}
+
 bool ObjCPropertyOpBuilder::findGetter() {
   if (Getter) return true;
 
@@ -818,6 +839,18 @@ ObjCPropertyOpBuilder::buildIncDecOperation(Scope *Sc, SourceLocation opcLoc,
   return PseudoOpBuilder::buildIncDecOperation(Sc, opcLoc, opcode, op);
 }
 
+ExprResult ObjCPropertyOpBuilder::complete(Expr *SyntacticForm) {
+  if (S.getLangOpts().ObjCAutoRefCount && isWeakProperty()) {
+    DiagnosticsEngine::Level Level =
+      S.Diags.getDiagnosticLevel(diag::warn_arc_repeated_use_of_weak,
+                                 SyntacticForm->getLocStart());
+    if (Level != DiagnosticsEngine::Ignored)
+      S.getCurFunction()->recordUseOfWeak(SyntacticRefExpr);
+  }
+
+  return PseudoOpBuilder::complete(SyntacticForm);
+}
+
 // ObjCSubscript build stuff.
 //