Page MenuHomePhabricator

[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.
ClosedPublic

Authored by ahatanak on Dec 13 2018, 11:20 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Dec 13 2018, 11:20 AM

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 updated this revision to Diff 178341.Dec 14 2018, 10:49 PM

Call CheckPlaceholderExpr instead of pushing an unevaluated evaluation context.

ahatanak marked an inline comment as done.Dec 14 2018, 10:56 PM
ahatanak added inline comments.
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.

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

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.

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).

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.

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 updated this revision to Diff 178957.Dec 19 2018, 1:48 PM

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.

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.

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.

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?

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 updated this revision to Diff 180186.Jan 3 2019, 6:09 PM
ahatanak marked an inline comment as done.

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.

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

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.

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.

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.

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?

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.

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 updated this revision to Diff 180593.Jan 7 2019, 5:56 PM

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 added inline comments.Jan 7 2019, 11:15 PM
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.

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 updated this revision to Diff 180993.Jan 9 2019, 9:14 PM
ahatanak retitled this revision from [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context. to [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 edited the summary of this revision. (Show Details)

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 marked an inline comment as done.Jan 9 2019, 9:29 PM

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 accepted this revision.Jan 9 2019, 9:31 PM

Sounds good.

This revision is now accepted and ready to land.Jan 9 2019, 9:31 PM
This revision was automatically updated to reflect the committed changes.