[Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings

[ahatanak]

Summary

The spurious -Warc-repeated-use-of-weak warnings are issued when an initializer expression uses a weak ObjC pointer.

My first attempt to silence the warnings (r350917) caused clang to reject code that is legal in C++17. The patch is based on the feedback I received from Richard when the patch was reverted.

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190422/268945.html
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190422/268943.html

[rjmccall]

lib/Sema/SemaDecl.cpp

11101 ↗

(On Diff #202115)

if (Init->hasPlaceholderType(BuiltinType::PseudoObject))

Although maybe this should just be isNonOverloadPlaceholderType(), since it's the syntactic-only placeholders that we're trying to cover. (Feel free to add Expr::hasNonOverloadPlaceholderType() for convenience.)

[ahatanak]

Address review comments.

lib/Sema/SemaDecl.cpp

11101 ↗

(On Diff #202115)

I had to move the code in deduceVarTypeFromInitializer that converts UnknownAnyType to id. The conversion has to happen before CheckPlaceholderExpr is called.

[rjmccall]

lib/Sema/SemaDecl.cpp

11101 ↗

(On Diff #202115)

Seems reasonable. Can you do the cleanup I requested?

[ahatanak]

Add function Expr::hasNonOverloadPlaceholderType.

[rjmccall]

Thanks, looks much better.

include/clang/Sema/Sema.h

2066 ↗

(On Diff #207166)

A shame this needs to be propagated down like this. Is there a reasonable way to just mark on the declaration that we defaulted the type, like an unparseable attribute? This might be interesting to other clients ultimately.

[ahatanak]

Add an implicit-only attribute ObjCDefaultedAnyToId to avoid passing the flag down.

[rjmccall]

Thanks, LGTM.

[rjmccall]

include/clang/Basic/Attr.td

1844 ↗

(On Diff #207931)

Oh, please add a comment on this explaining what it means, since it's not based on a language feature.

cfe/trunk/include/clang/AST/Expr.h

public:
   /// Returns whether this expression has a specific placeholder type.
   bool hasPlaceholderType(BuiltinType::Kind K) const {
     assert(BuiltinType::isPlaceholderTypeKind(K));
     if (const BuiltinType *BT = dyn_cast<BuiltinType>(getType()))
       return BT->getKind() == K;
     return false;
   }
 
+  /// Returns whether this expression has a placeholder type that isn't
+  /// Overload.
+  bool hasNonOverloadPlaceholderType() const {
+    if (auto *PlaceholderType = getType()->getAsPlaceholderType())
+      if (PlaceholderType->isNonOverloadPlaceholderType())
+        return true;
+    return false;
+  }
+
   /// isKnownToHaveBooleanValue - Return true if this is an integer expression
   /// that is known to return 0 or 1.  This happens for _Bool/bool expressions
   /// but also int expressions which are produced by things like comparisons in
   /// C.
   bool isKnownToHaveBooleanValue() const;
 
   /// isIntegerConstantExpr - Return true if this expression is a valid integer
   /// constant expression, and, if so, return its value in Result.  If not a

cfe/trunk/include/clang/Basic/Attr.td

 }
 
 def ObjCBoxable : Attr {
   let Spellings = [Clang<"objc_boxable">];
   let Subjects = SubjectList<[Record], ErrorDiag>;
   let Documentation = [ObjCBoxableDocs];
 }
 
+
+// This is used to indicate that the type of the annotated variable's
+// initializer was changed from UnknownAny to 'id'.
+def ObjCDefaultedAnyToId : Attr {
+  // This attribute has no spellings as it is only ever created implicitly.
+  let Spellings = [];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [Undocumented];
+}
+
 def OptimizeNone : InheritableAttr {
   let Spellings = [Clang<"optnone">];
   let Subjects = SubjectList<[Function, ObjCMethod]>;
   let Documentation = [OptnoneDocs];
 }
 
 def Overloadable : Attr {
   let Spellings = [Clang<"overloadable">];

cfe/trunk/lib/Sema/SemaDecl.cpp

QualType Sema::deduceVarTypeFromInitializer(VarDecl *VDecl,
   if (DirectInit && isa<InitListExpr>(DeduceInit)) {
     Diag(Init->getBeginLoc(), IsInitCapture
                                   ? diag::err_init_capture_paren_braces
                                   : diag::err_auto_var_init_paren_braces)
         << isa<InitListExpr>(Init) << VN << Type << Range;
     return QualType();
   }
 
-  // Expressions default to 'id' when we're in a debugger.
-  bool DefaultedAnyToId = false;
-  if (getLangOpts().DebuggerCastResultToId &&
-      Init->getType() == Context.UnknownAnyTy && !IsInitCapture) {
-    ExprResult Result = forceUnknownAnyToType(Init, Context.getObjCIdType());
-    if (Result.isInvalid()) {
-      return QualType();
-    }
-    Init = Result.get();
-    DefaultedAnyToId = true;
-  }
-
   // C++ [dcl.decomp]p1:
   //   If the assignment-expression [...] has array type A and no ref-qualifier
   //   is present, e has type cv A
   if (VDecl && isa<DecompositionDecl>(VDecl) &&
       Context.hasSameUnqualifiedType(Type, Context.getAutoDeductType()) &&
       DeduceInit->getType()->isConstantArrayType())
     return Context.getQualifiedType(DeduceInit->getType(),
                                     Type.getQualifiers());
else
           << DeduceInit->getSourceRange();
   }
 
   // Warn if we deduced 'id'. 'auto' usually implies type-safety, but using
   // 'id' instead of a specific object type prevents most of our usual
   // checks.
   // We only want to warn outside of template instantiations, though:
   // inside a template, the 'id' could have come from a parameter.
+  bool DefaultedAnyToId = VDecl && VDecl->hasAttr<ObjCDefaultedAnyToIdAttr>();
   if (!inTemplateInstantiation() && !DefaultedAnyToId && !IsInitCapture &&
       !DeducedType.isNull() && DeducedType->isObjCIdType()) {
     SourceLocation Loc = TSI->getTypeLoc().getBeginLoc();
     Diag(Loc, diag::warn_auto_var_is_id) << VN << Range;
   }
 
   return DeducedType;
 }
if (VDecl->getType()->isUndeducedType()) {
     // TypoExpr.
     ExprResult Res = CorrectDelayedTyposInExpr(Init, VDecl);
     if (!Res.isUsable()) {
       RealDecl->setInvalidDecl();
       return;
     }
     Init = Res.get();
 
+    // Expressions default to 'id' when we're in a debugger
+    // and we are assigning it to a variable of Objective-C pointer type.
+    if (getLangOpts().DebuggerCastResultToId &&
+        Init->getType() == Context.UnknownAnyTy) {
+      ExprResult Result = forceUnknownAnyToType(Init, Context.getObjCIdType());
+      if (Result.isInvalid()) {
+        VDecl->setInvalidDecl();
+        return;
+      }
+      Init = Result.get();
+      VDecl->addAttr(ObjCDefaultedAnyToIdAttr::CreateImplicit(Context));
+    } else if (!Init->getType().isNull() &&
+               Init->hasNonOverloadPlaceholderType()) {
+      Res = CheckPlaceholderExpr(Init).get();
+      if (!Res.isUsable()) {
+        VDecl->setInvalidDecl();
+        return;
+      }
+      Init = Res.get();
+    }
+
     if (DeduceVariableDeclarationType(VDecl, DirectInit, Init))
       return;
   }
 
   // dllimport cannot be used on variable definitions.
   if (VDecl->hasAttr<DLLImportAttr>() && !VDecl->isStaticDataMember()) {
     Diag(VDecl->getLocation(), diag::err_attribute_dllimport_data_definition);
     VDecl->setInvalidDecl();

cfe/trunk/lib/Sema/SemaExpr.cpp

Sema::MaybeConvertParenListExprToParenExpr(Scope *S, Expr *OrigExpr) {
   if (Result.isInvalid()) return ExprError();
 
   return ActOnParenExpr(E->getLParenLoc(), E->getRParenLoc(), Result.get());
 }
 
 ExprResult Sema::ActOnParenListExpr(SourceLocation L,
                                     SourceLocation R,
                                     MultiExprArg Val) {
+  for (size_t I = 0, E = Val.size(); I != E; ++I)
+    if (Val[I]->hasNonOverloadPlaceholderType()) {
+      ExprResult Result = CheckPlaceholderExpr(Val[I]);
+      if (!Result.isUsable())
+        return ExprError();
+      Val[I] = Result.get();
+    }
+
   return ParenListExpr::Create(Context, L, Val, R);
 }
 
 /// Emit a specialized diagnostic when one expression is a null pointer
 /// constant and the other is not a pointer.  Returns true if a diagnostic is
 /// emitted.
 bool Sema::DiagnoseConditionalForNull(Expr *LHSExpr, Expr *RHSExpr,
                                       SourceLocation QuestionLoc) {

cfe/trunk/lib/Sema/SemaExprCXX.cpp

Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
   Expr **Inits = &Initializer;
   unsigned NumInits = Initializer ? 1 : 0;
   if (ParenListExpr *List = dyn_cast_or_null<ParenListExpr>(Initializer)) {
     assert(initStyle == CXXNewExpr::CallInit && "paren init for non-call init");
     Inits = List->getExprs();
     NumInits = List->getNumExprs();
   }
 
+  for (unsigned I = 0, E = NumInits; I != E; ++I)
+    if (Inits[I]->hasNonOverloadPlaceholderType()) {
+      ExprResult Result = CheckPlaceholderExpr(Inits[I]);
+      if (!Result.isUsable())
+        return ExprError();
+      Inits[I] = Result.get();
+    }
+
   // C++11 [expr.new]p15:
   //   A new-expression that creates an object of type T initializes that
   //   object as follows:
   InitializationKind Kind
       //     - If the new-initializer is omitted, the object is default-
       //       initialized (8.5); if no initialization is performed,
       //       the object has indeterminate value
       = initStyle == CXXNewExpr::NoInit

cfe/trunk/test/SemaObjC/arc-repeated-weak.mm

-// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
-// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++11 -Warc-repeated-use-of-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-arc -fblocks -Wno-objc-root-class -std=c++14 -Warc-repeated-use-of-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++14 -Warc-repeated-use-of-weak -verify %s
 
 @interface Test {
 @public
   Test *ivar;
   __weak id weakIvar;
 }
 @property(weak) Test *weakProp;
 @property(strong) Test *strongProp;
void foo() {
   NSString * t2 = NSBundle.foo2.prop;
   use(NSBundle.foo2.weakProp); // expected-warning{{weak property 'weakProp' may be accessed multiple times}}
   use(NSBundle2.foo2.weakProp); // expected-note{{also accessed here}}
   decltype([NSBundle2.foo2 weakProp]) t3;
   decltype(NSBundle2.foo2.weakProp) t4;
   __typeof__(NSBundle2.foo2.weakProp) t5;
 }
 
+void testAuto() {
+  auto __weak wp = NSBundle2.foo2.weakProp;
+}
+
+void testLambdaCaptureInit() {
+  [capture(NSBundle2.foo2.weakProp)] {} ();
+}
+
+void testAutoNew() {
+  auto p = new auto(NSBundle2.foo2.weakProp);
+}
+
 // This used to crash in the constructor of WeakObjectProfileTy when a
 // DeclRefExpr was passed that didn't reference a VarDecl.
 
 typedef INTF * INTFPtrTy;
 
 enum E {
   e1
 };