[Sema] Fix a -Warc-repeated-use-of-weak false-positive by only calling CheckPlaceholderExpr once

[erik.pilkington]

Summary

Previously, this code discarded the result of CheckPlaceholderExpr for non-matrix subexpressions. Not only is this wasteful, but it was creating a Warc-repeated-use-of-weak false-positive on the attached testcase, since the discarded expression was still registered as a use of the weak property. (This was introduced in D76791)

rdar://66162246

[rjmccall]

LGTM

[fhahn]

LGTM, thanks for addressing this! Please wait a bit with committing, in case there are additional comments.

clang/lib/Sema/SemaExpr.cpp

4598

I think it would be good to split the comment into 2. The case below is base is a MatrixSubscriptExpr, and further down we handle base is a matrix type.

clang/lib/Sema/SemaExpr.cpp

Sema::ActOnArraySubscriptExpr(Scope *S, Expr *base, SourceLocation lbLoc,
   // Separating the index expressions by parenthesis is not allowed.
   if (base->getType()->isSpecificPlaceholderType(
           BuiltinType::IncompleteMatrixIdx) &&
       !isa<MatrixSubscriptExpr>(base)) {
     Diag(base->getExprLoc(), diag::err_matrix_separate_incomplete_index)
         << SourceRange(base->getBeginLoc(), rbLoc);
     return ExprError();
   }
-  // If the base is either a MatrixSubscriptExpr or a matrix type, try to create
-  // a new MatrixSubscriptExpr.
+  // If the base is a MatrixSubscriptExpr, try to create a new
+  // MatrixSubscriptExpr.
   auto *matSubscriptE = dyn_cast<MatrixSubscriptExpr>(base);
   if (matSubscriptE) {
     if (CheckAndReportCommaError(idx))
       return ExprError();
 
     assert(matSubscriptE->isIncomplete() &&
            "base has to be an incomplete matrix subscript");
     return CreateBuiltinMatrixSubscriptExpr(
         matSubscriptE->getBase(), matSubscriptE->getRowIdx(), idx, rbLoc);
   }
-  Expr *matrixBase = base;
-  bool IsMSPropertySubscript = isMSPropertySubscriptExpr(*this, base);
+
+  // Handle any non-overload placeholder types in the base and index
+  // expressions.  We can't handle overloads here because the other
+  // operand might be an overloadable type, in which case the overload
+  // resolution for the operator overload should get the first crack
+  // at the overload.
+  bool IsMSPropertySubscript = false;
+  if (base->getType()->isNonOverloadPlaceholderType()) {
+    IsMSPropertySubscript = isMSPropertySubscriptExpr(*this, base);
     if (!IsMSPropertySubscript) {
       ExprResult result = CheckPlaceholderExpr(base);
-    if (!result.isInvalid())
-      matrixBase = result.get();
+      if (result.isInvalid())
+        return ExprError();
+      base = result.get();
     }
-  if (matrixBase->getType()->isMatrixType()) {
+  }
+
+  // If the base is a matrix type, try to create a new MatrixSubscriptExpr.
+  if (base->getType()->isMatrixType()) {
     if (CheckAndReportCommaError(idx))
       return ExprError();
 
-    return CreateBuiltinMatrixSubscriptExpr(matrixBase, idx, nullptr, rbLoc);
+    return CreateBuiltinMatrixSubscriptExpr(base, idx, nullptr, rbLoc);
   }
 
   // A comma-expression as the index is deprecated in C++2a onwards.
   if (getLangOpts().CPlusPlus20 &&
       ((isa<BinaryOperator>(idx) && cast<BinaryOperator>(idx)->isCommaOp()) ||
        (isa<CXXOperatorCallExpr>(idx) &&
         cast<CXXOperatorCallExpr>(idx)->getOperator() == OO_Comma))) {
     Diag(idx->getExprLoc(), diag::warn_deprecated_comma_subscript)
         << SourceRange(base->getBeginLoc(), rbLoc);
   }
 
-  // Handle any non-overload placeholder types in the base and index
-  // expressions.  We can't handle overloads here because the other
-  // operand might be an overloadable type, in which case the overload
-  // resolution for the operator overload should get the first crack
-  // at the overload.
-  if (base->getType()->isNonOverloadPlaceholderType()) {
-    IsMSPropertySubscript = isMSPropertySubscriptExpr(*this, base);
-    if (!IsMSPropertySubscript) {
-      ExprResult result = CheckPlaceholderExpr(base);
-      if (result.isInvalid())
-        return ExprError();
-      base = result.get();
-    }
-  }
   if (idx->getType()->isNonOverloadPlaceholderType()) {
     ExprResult result = CheckPlaceholderExpr(idx);
     if (result.isInvalid()) return ExprError();
     idx = result.get();
   }
 
   // Build an unanalyzed expression if either operand is type-dependent.
   if (getLangOpts().CPlusPlus &&

clang/test/SemaObjC/arc-repeated-weak.mm

void foo1() {
   INTFPtrTy tmp = (INTFPtrTy)e1;
 #if __has_feature(objc_arc)
 // expected-error@-2{{cast of 'E' to 'INTFPtrTy' (aka 'INTF *') is disallowed with ARC}}
 #endif
 }
 
 @class NSString;
 static NSString* const kGlobal = @"";
+
+@interface NSDictionary
+- (id)objectForKeyedSubscript:(id)key;
+@end
+
+@interface WeakProp
+@property (weak) NSDictionary *nd;
+@end
+
+@implementation WeakProp
+-(void)m {
+  (void)self.nd[@""]; // no warning
+}
+@end