[Sema] If CheckPlaceholderExpr rewrites a placeholder expression when the type of an auto variable is being deduced, use the rewritten expression when performing initialization of the variable.

[ahatanak]

Summary

This prevents CheckPlaceholderExpr from being called twice when the following code is compiled, which prevents clang from incorrectly issuing a -Warc-repeated-use-of-weak warning:

void testAuto() {
  auto __weak wp = NSBundle2.foo2.weakProp;
}

[rjmccall]

I think the right fix here is to call CheckPlaceholderExpr in ActOnDecltypeExpression / HandleExprEvaluationContextForTypeof before the original unevaluated context is exited. You can then change the type-building routines to assert that they aren't given a placeholder. The current ordering probably also causes unwanted behavior with expressions like decltype(&functionTemplate<int>) that should be getting resolved to a single template within the unevaluated context so that the template specialization is not considered ODR-used.

[ahatanak]

Call CheckPlaceholderExpr instead of pushing an unevaluated evaluation context.

[ahatanak]

lib/Sema/SemaType.cpp

8127 ↗

(On Diff #178341)

I couldn't assert here that E isn't a placeholder since there are several call sites that call this function and there are code paths that don't call Sema::ActOnDecltypeExpression before reaching this point.

[rjmccall]

Do you have an example, and is there a reasonable place to call CheckPlaceholderExpr on those paths?

[ahatanak]

Here are a couple of examples I found running the regression tests:

int f0(int);
float f0(float);
decltype(f0) f0_a; // this is not valid.

template <class... Ts>
int var_expr(Ts... ts);

template <class... Ts>
auto a_function(Ts... ts) -> decltype(var_expr(ts...));

template <class T>
using partial = decltype(a_function<int, T>);

int use_partial() { partial<char> n; }

template<class T> void oneT() { }
int main()
{
  decltype(oneT<int>)* fun = 0;
}

If the call to CheckPlaceholderExpr in Sema::BuildDecltypeType is moved to TreeTransform<Derived>::TransformDecltypeType and Parser::ParseDecltypeSpecifier, we can assert at the beginning of Sema::BuildDecltypeType.

[rjmccall]

In D55662#1335146, @ahatanak wrote:

Here are a couple of examples I found running the regression tests:

int f0(int);
float f0(float);
decltype(f0) f0_a; // this is not valid.

template <class... Ts>
int var_expr(Ts... ts);

template <class... Ts>
auto a_function(Ts... ts) -> decltype(var_expr(ts...));

template <class T>
using partial = decltype(a_function<int, T>);

int use_partial() { partial<char> n; }

template<class T> void oneT() { }
int main()
{
  decltype(oneT<int>)* fun = 0;
}

If the call to CheckPlaceholderExpr in Sema::BuildDecltypeType is moved to TreeTransform<Derived>::TransformDecltypeType and Parser::ParseDecltypeSpecifier, we can assert at the beginning of Sema::BuildDecltypeType.

Okay, I'm not really understanding what path goes through those two places but not through ActOnDecltypeExpression, since both of them seem to unconditionally call the latter (at least, in the expression-parsing path).

[ahatanak]

Sorry, please ignore my previous comment. I was looking at the wrong place.

The following code reaches Sema::BuildDecltypeType without going through ActOnDecltypeExpression:

template <typename T>
void overloaded_fn(T);
decltype(auto) v5 = &overloaded_fn<int>;

Sema::BuildDecltypeType is called from Sema::DeduceAutoType, so calling CheckPlaceholderExpr there should fix the assert when the test case above is compiled.

[rjmccall]

In D55662#1335402, @ahatanak wrote:

Sorry, please ignore my previous comment. I was looking at the wrong place.

The following code reaches Sema::BuildDecltypeType without going through ActOnDecltypeExpression:

template <typename T>
void overloaded_fn(T);
decltype(auto) v5 = &overloaded_fn<int>;

Sema::BuildDecltypeType is called from Sema::DeduceAutoType, so calling CheckPlaceholderExpr there should fix the assert when the test case above is compiled.

Okay. You may need to push an unevaluated context when doing that.

[ahatanak]

Remove the call to CheckPlaceholderExpr in Sema::BuildDecltypeType and move it to Sema::DeduceAutoType. Assert that the expression that is passed to Sema::BuildDecltypeType isn't a placeholder.

[ahatanak]

In D55662#1335773, @rjmccall wrote:

Okay. You may need to push an unevaluated context when doing that.

Since I'm just moving the call to CheckPlaceholderExpr to the call site, I don't think I have to push an unevaluated context there?

Also, it looks like I can just change the check Init->getType()->isNonOverloadPlaceholderType() at the beginning of Sema::DeduceAutoType to Init->getType()->getAsPlaceholderType() instead of inserting the call to CheckPlaceholderExpr right before the call to BuildDecltypeType.

[rjmccall]

In D55662#1336835, @ahatanak wrote:

In D55662#1335773, @rjmccall wrote:

Okay. You may need to push an unevaluated context when doing that.

Since I'm just moving the call to CheckPlaceholderExpr to the call site, I don't think I have to push an unevaluated context there?

Hmm. Right, for the auto inference specifically it's fine because the expression is in fact evaluated: we're not just eliminating placeholders in order to resolve decltype, we're eliminating placeholders to actually figure out what's going on with the initialization.

Also, it looks like I can just change the check Init->getType()->isNonOverloadPlaceholderType() at the beginning of Sema::DeduceAutoType to Init->getType()->getAsPlaceholderType() instead of inserting the call to CheckPlaceholderExpr right before the call to BuildDecltypeType.

The reason it's specifically checking for a non-overload placeholder is that normal auto can have additional structure in the declarator that could potentially resolve an overload set without picking a single template specialization. So you can only check for an arbitrary placeholder if this is the decltype(auto) case. But I agree that it still makes sense to only do this once up front, yeah.

lib/Sema/SemaType.cpp

8043 ↗

(On Diff #178957)

There is a isPlaceholderType() method that looks a little cleaner than a getAs call.

[ahatanak]

In D55662#1337141, @rjmccall wrote:

In D55662#1336835, @ahatanak wrote:

In D55662#1335773, @rjmccall wrote:

Okay. You may need to push an unevaluated context when doing that.

Since I'm just moving the call to CheckPlaceholderExpr to the call site, I don't think I have to push an unevaluated context there?

Hmm. Right, for the auto inference specifically it's fine because the expression is in fact evaluated: we're not just eliminating placeholders in order to resolve decltype, we're eliminating placeholders to actually figure out what's going on with the initialization.

clang currently diagnose the repeated use of weak in the following case (with or without this patch):

auto __weak wp = b.weakProp;

I find this counterintuitive, but I guess this is the expected behavior?

[rjmccall]

In D55662#1338766, @ahatanak wrote:

In D55662#1337141, @rjmccall wrote:

In D55662#1336835, @ahatanak wrote:

In D55662#1335773, @rjmccall wrote:

Okay. You may need to push an unevaluated context when doing that.

Since I'm just moving the call to CheckPlaceholderExpr to the call site, I don't think I have to push an unevaluated context there?

Hmm. Right, for the auto inference specifically it's fine because the expression is in fact evaluated: we're not just eliminating placeholders in order to resolve decltype, we're eliminating placeholders to actually figure out what's going on with the initialization.

clang currently diagnose the repeated use of weak in the following case (with or without this patch):

auto __weak wp = b.weakProp;

I find this counterintuitive, but I guess this is the expected behavior?

No, that's not right. it's not a repeated use of the same weak entity.

[ahatanak]

Do not record use of weak variables when the type of an auto variable is being deduced.

I'm not sure creating an RAII class is the best way to silence the warning, but I haven't been able to come up with something better.

[rjmccall]

I think you could just disable the diagnostic in an unevaluated context.

[ahatanak]

In D55662#1346055, @rjmccall wrote:

I think you could just disable the diagnostic in an unevaluated context.

The call to isUnevaluatedContext in ObjCPropertyOpBuilder::complete returns false when the type of auto __weak wp in testAuto is being deduced because the ExpressionEvaluationContextRecord on the stack isn't an unevaluated context. I can silence the warning if I can push an unevaluated context at the entry of Sema::DeduceAutoType, but it's not clear to me that it's safe to do so.

[rjmccall]

In D55662#1346059, @ahatanak wrote:

In D55662#1346055, @rjmccall wrote:

I think you could just disable the diagnostic in an unevaluated context.

The call to isUnevaluatedContext in ObjCPropertyOpBuilder::complete returns false when the type of auto __weak wp in testAuto is being deduced because the ExpressionEvaluationContextRecord on the stack isn't an unevaluated context. I can silence the warning if I can push an unevaluated context at the entry of Sema::DeduceAutoType, but it's not clear to me that it's safe to do so.

Hmm. Are we resolving the placeholder expression twice here? That might be the basic problem.

[ahatanak]

In D55662#1346077, @rjmccall wrote:

Hmm. Are we resolving the placeholder expression twice here? That might be the basic problem.

Yes, CheckPlaceholderExpr is called in Sema::DeduceAutoType to deduce the type of the auto variable and then called again when InitializationSequence::InitializeFrom generates the Expr for initialization.

If we want to avoid calling CheckPlaceholderExpr twice, I think it's possible to cache the results of ObjCPropertyOpBuilder::buildRValueOperation in Sema. Alternatively, we can just pass down a flag that indicates we are deducing auto types and pass it to the constructor of ObjCPropertyOpBuilder.

[rjmccall]

Can we find a way to preserve the rewritten expression out of DeduceAutoType so that the initialization code just happens to never see a placeholder along this path?

[ahatanak]

We can make DeduceVariableDeclarationType return the rewritten expression and replace Init in Sema::AddInitializerToDecl with it.

Alternatively, we can keep a set of ObjCPropertyRefExprs passed to recordUseOfWeak (the original ObjCPropertyRefExpr, if it was rebuilt in SemaPseudoObject.cpp) and avoid recording the use of a weak variable if the expression is passed the second time.

[rjmccall]

If it's at all reasonable to just avoid doing the work multiple times, let's do that. It should also avoid duplicate diagnostics if e.g. an overload is resolved to an unavailable function.

[ahatanak]

Make deduceVarTypeFromInitializer and DeduceVariableDeclarationType return the new initialization expression and use it in Sema::AddInitializerToDecl. Add a test case that tests lambda capture with an initializer.

[rjmccall]

lib/Sema/SemaLambda.cpp

793 ↗

(On Diff #180593)

Please maintain the original order here, even though I suspect it doesn't matter: if this is direct-initialization, use the arguments, otherwise use either DeducedAutoInit or Init. Although really, consider just reassigning Init immediately after the deduceVarType... call.

[rjmccall]

Oh, and please update the commit message to primarily talk about the changes to placeholder checking. You can discuss the impact on the repeated-use-of-weak warning in a follow-up paragraph.

[ahatanak]

Pass Init by reference and use the rewritten expression returned in it to perform variable initialization. Add a test case that tests new expression with auto types.

[ahatanak]

In D55662#1349329, @rjmccall wrote:

Oh, and please update the commit message to primarily talk about the changes to placeholder checking. You can discuss the impact on the repeated-use-of-weak warning in a follow-up paragraph.

This patch has three parts:

1. Use the expression that was rewritten during auto type deduction to perform auto variable initialization.
2. Call CheckPlaceholderExpr while an unevaluated context is still on the stack.
3. Check whether we are in an unevaluated context before calling recordUseOfWeak in Sema::BuildInstanceMessage.

I think I should commit 1 separately from 2 and 3.

lib/Sema/SemaLambda.cpp

793 ↗

(On Diff #180593)

The code that assigns CXXDirectInit's expressions to Arg is no longer needed since Sema::deduceVarTypeFromInitializer removes the ParenListExpr.

If we move the code that declares the InitializedEntity variable up, we can reassign Init immediately after the call to deduceVarTypeFromInitializer.

[rjmccall]

Sounds good.

cfe/trunk/lib/Sema/SemaExpr.cpp

void Sema::DiscardCleanupsInEvaluationContext() {
   ExprCleanupObjects.erase(
          ExprCleanupObjects.begin() + ExprEvalContexts.back().NumCleanupObjects,
          ExprCleanupObjects.end());
   Cleanup.reset();
   MaybeODRUseExprs.clear();
 }
 
 ExprResult Sema::HandleExprEvaluationContextForTypeof(Expr *E) {
+  ExprResult Result = CheckPlaceholderExpr(E);
+  if (Result.isInvalid())
+    return ExprError();
+  E = Result.get();
   if (!E->getType()->isVariablyModifiedType())
     return E;
   return TransformToPotentiallyEvaluated(E);
 }
 
 /// Are we within a context in which some evaluation could be performed (be it
 /// constant evaluation or runtime evaluation)? Sadly, this notion is not quite
 /// captured by C++'s idea of an "unevaluated context".

cfe/trunk/lib/Sema/SemaExprCXX.cpp

 /// certain semantic checks on temporaries are delayed until this point, and
 /// are omitted for the 'topmost' call in the decltype expression. If the
 /// topmost call bound a temporary, strip that temporary off the expression.
 ExprResult Sema::ActOnDecltypeExpression(Expr *E) {
   assert(ExprEvalContexts.back().ExprContext ==
              ExpressionEvaluationContextRecord::EK_Decltype &&
          "not in a decltype expression");
 
+  ExprResult Result = CheckPlaceholderExpr(E);
+  if (Result.isInvalid())
+    return ExprError();
+  E = Result.get();
+
   // C++11 [expr.call]p11:
   //   If a function call is a prvalue of object type,
   // -- if the function call is either
   //   -- the operand of a decltype-specifier, or
   //   -- the right operand of a comma operator that is the operand of a
   //      decltype-specifier,
   //   a temporary object is not introduced for the prvalue.
 

cfe/trunk/lib/Sema/SemaExprObjC.cpp

ExprResult Sema::BuildInstanceMessage(Expr *Receiver,
 
   if (getLangOpts().ObjCWeak) {
     if (!isImplicit && Method) {
       if (const ObjCPropertyDecl *Prop = Method->findPropertyDecl()) {
         bool IsWeak =
           Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak;
         if (!IsWeak && Sel.isUnarySelector())
           IsWeak = ReturnType.getObjCLifetime() & Qualifiers::OCL_Weak;
-        if (IsWeak &&
+        if (IsWeak && !isUnevaluatedContext() &&
             !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, LBracLoc))
           getCurFunction()->recordUseOfWeak(Result, Prop);
       }
     }
   }
 
   CheckObjCCircularContainer(Result);
 

cfe/trunk/lib/Sema/SemaTemplateDeduction.cpp

Sema::DeduceAutoType(TypeLoc Type, Expr *&Init, QualType &Result,
   // don't need to go digging for it.
   if (const AutoType *AT = Type.getType()->getAs<AutoType>()) {
     if (AT->isDecltypeAuto()) {
       if (isa<InitListExpr>(Init)) {
         Diag(Init->getBeginLoc(), diag::err_decltype_auto_initializer_list);
         return DAR_FailedAlreadyDiagnosed;
       }
 
+      ExprResult ER = CheckPlaceholderExpr(Init);
+      if (ER.isInvalid())
+        return DAR_FailedAlreadyDiagnosed;
+      Init = ER.get();
       QualType Deduced = BuildDecltypeType(Init, Init->getBeginLoc(), false);
       if (Deduced.isNull())
         return DAR_FailedAlreadyDiagnosed;
       // FIXME: Support a non-canonical deduced type for 'auto'.
       Deduced = Context.getCanonicalType(Deduced);
       Result = SubstituteDeducedTypeTransform(*this, Deduced).Apply(Type);
       if (Result.isNull())
         return DAR_FailedAlreadyDiagnosed;

cfe/trunk/lib/Sema/SemaType.cpp

else {
     if (Keyword == ETK_None)
       return T;
     NNS = nullptr;
   }
   return Context.getElaboratedType(Keyword, NNS, T, OwnedTagDecl);
 }
 
 QualType Sema::BuildTypeofExprType(Expr *E, SourceLocation Loc) {
-  ExprResult ER = CheckPlaceholderExpr(E);
-  if (ER.isInvalid()) return QualType();
-  E = ER.get();
+  assert(!E->hasPlaceholderType() && "unexpected placeholder");
 
   if (!getLangOpts().CPlusPlus && E->refersToBitField())
     Diag(E->getExprLoc(), diag::err_sizeof_alignof_typeof_bitfield) << 2;
 
   if (!E->isTypeDependent()) {
     QualType T = E->getType();
     if (const TagType *TT = T->getAs<TagType>())
       DiagnoseUseOfDecl(TT->getDecl(), E->getExprLoc());
static QualType getDecltypeForExpr(Sema &S, Expr *E) {
   case VK_RValue: break;
   }
 
   return T;
 }
 
 QualType Sema::BuildDecltypeType(Expr *E, SourceLocation Loc,
                                  bool AsUnevaluated) {
-  ExprResult ER = CheckPlaceholderExpr(E);
-  if (ER.isInvalid()) return QualType();
-  E = ER.get();
+  assert(!E->hasPlaceholderType() && "unexpected placeholder");
 
   if (AsUnevaluated && CodeSynthesisContexts.empty() &&
       E->HasSideEffects(Context, false)) {
     // The expression operand for decltype is in an unevaluated expression
     // context, so side effects could result in unintended consequences.
     Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
   }
 

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

 void foo() {
   NSString * t = NSBundle.foo.prop;
   use(NSBundle.foo.weakProp); // expected-warning{{weak property 'weakProp' may be accessed multiple times}}
   use(NSBundle2.foo.weakProp); // expected-note{{also accessed here}}
 
   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;
 }
 
 // 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 {