Fix PR25627: Certain constant local variables must be usable as template arguments (without being odr-used)

[faisalv]

Summary

This patch ensures that clang processes the expression-nodes that are generated when disambiguating between types and expressions within template arguments, as if they were truly constant-expressions. Currently, trunk correctly disambiguates, and identifies the expression as an expression - and while it annotates the token with the expression - it fails to complete the odr-use processing (specifically, failing to trigger Sema::UpdateMarkingForLValueToRValue as is done so for all Constant Expressions, which removes it from being considered odr-used).

For e.g:
template<int> struct X { };
void f() {

const int N = 10;
X<N> x; // should be OK.
[] { return X<N>{}; }; // also OK - no capture.

}
See a related bug: https://bugs.llvm.org//show_bug.cgi?id=25627

The fix is as follows:

• Remove the EnteredConstantEvaluatedContext action from ParseTemplateArgumentList (relying that ParseTemplateArgument will get it right)
• Add the EnteredConstantEvaluatedContext action just prior to undergoing the disambiguating parse, and if the parse succeeds for an expression, make sure it doesn't linger within MaybeODRUsedExprs by clearing it (while asserting that it only contains the offending expression)

I need to add some tests... and fix one regression.

Does the approach look sound?
Thanks!

[rsmith]

lib/Parse/ParseTemplate.cpp

1219–1220

Please add a comment here, something like:

isCXXTypeId might look up and annotate an identifier as an id-expression during disambiguation, so enter the appropriate context for a constant expression template argument before trying to disambiguate.

1230–1259

... we shouldn't need to do any of this: instead, keep your ExprEvaluationContext alive through the call to ParseConstantExpression, and tell it to not create its own context in this case.

1250–1253

This doesn't seem right. If the template parameter is of reference type, the named entity should be considered to be odr-used. And likewise just because we stopped disambiguation after finding an id-expression that names a non-type, that does not imply that the overall template argument is the entity named by that id-expression. (Eg, consider X<F()>, where F is a functor whose operator() returns this -- that template argument should be considered to odr-use F.) But...

[faisalv]

Updated the patch following Richard's feedback:

• teach ParseConstantExpression to create its own ExpressionEvaluationContext only if asked to.

[faisalv]

Fixed a regression test that should have passed without emitting error diagnostics - and now does.

[faisalv]

*ping*

[faisalv]

*ping*
*ping*

[rsmith]

lib/Parse/ParseExpr.cpp

203–222

This seems more complexity than we need. How about factoring out a ParseConstantExpressionInExprEvalContext function that doesn't create a context, and then calling it from this function after creating the context?

lib/Parse/ParseTemplate.cpp

1208–1233

There's a bunch of whitespace changes here. I have no objection to them but they should be handled separately rather than mixed into this change.

[faisalv]

Modified and committed as: http://llvm.org/viewvc/llvm-project?view=revision&revision=303492

[faisalv]

Committed here : https://reviews.llvm.org/rL303492

include/clang/Parse/Parser.h

   };
 
   ExprResult ParseExpression(TypeCastState isTypeCast = NotTypeCast);
-  ExprResult ParseConstantExpression(TypeCastState isTypeCast = NotTypeCast);
+  ExprResult
+  ParseConstantExpression(TypeCastState isTypeCast = NotTypeCast,
+                          bool CreateOwnExpressionEvaluationContext = true);
   ExprResult ParseConstraintExpression();
   // Expr that doesn't include commas.
   ExprResult ParseAssignmentExpression(TypeCastState isTypeCast = NotTypeCast);

lib/Parse/ParseExpr.cpp

   return ParseRHSOfBinaryExpression(R, prec::Assignment);
 }
 
-
-ExprResult Parser::ParseConstantExpression(TypeCastState isTypeCast) {
+ExprResult
+Parser::ParseConstantExpression(TypeCastState isTypeCast,
+                                bool CreateOwnExpressionEvaluationContext) {
   // C++03 [basic.def.odr]p2:
   //   An expression is potentially evaluated unless it appears where an
   //   integral constant expression is required (see 5.19) [...].
   // C++98 and C++11 have no such rule, but this is only a defect in C++98.
-  EnterExpressionEvaluationContext ConstantEvaluated(
-      Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
 
+  // Create the ExpressionEvaluationContext on the stack - but only if asked to.
+  struct EnterExpressionEvaluationContextConditionalRAII {
+    llvm::AlignedCharArray<alignof(EnterExpressionEvaluationContext),
+                           sizeof(EnterExpressionEvaluationContext)>
+        MyStackStorage;
+    const EnterExpressionEvaluationContext *const ConstantEvaluatedContext;
+    EnterExpressionEvaluationContextConditionalRAII(bool CreateIt,
+                                                    Sema &Actions)
+        : ConstantEvaluatedContext(
+              CreateIt
+                  ? new (&MyStackStorage) EnterExpressionEvaluationContext(
+                        Actions,
+                        Sema::ExpressionEvaluationContext::ConstantEvaluated)
+                  : nullptr) {}
+    ~EnterExpressionEvaluationContextConditionalRAII() {
+      if (ConstantEvaluatedContext)
+        ConstantEvaluatedContext->~EnterExpressionEvaluationContext();
+    }
+  } EnterExpressionEvaluationContextConditionalRAII(
+      CreateOwnExpressionEvaluationContext, Actions);
+
   ExprResult LHS(ParseCastExpression(false, false, isTypeCast));
   ExprResult Res(ParseRHSOfBinaryExpression(LHS, prec::Conditional));
+
   return Actions.ActOnConstantExpression(Res);
 }
 

lib/Parse/ParseTemplate.cpp

   //   expression is resolved to a type-id, regardless of the form of
   //   the corresponding template-parameter.
   //
-  // Therefore, we initially try to parse a type-id.  
+  // Therefore, we initially try to parse a type-id - and isCXXTypeId might look
+  // up and annotate an identifier as an id-expression during disambiguation,
+  // so enter the appropriate context for a constant expression template
+  // argument before trying to disambiguate.
+
+  EnterExpressionEvaluationContext EnterConstantEvaluated(
+      Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
   if (isCXXTypeId(TypeIdAsTemplateArgument)) {
     SourceLocation Loc = Tok.getLocation();
-    TypeResult TypeArg = ParseTypeName(/*Range=*/nullptr,
-                                       Declarator::TemplateTypeArgContext);
+    TypeResult TypeArg =
+        ParseTypeName(/*Range=*/nullptr, Declarator::TemplateTypeArgContext);
     if (TypeArg.isInvalid())
       return ParsedTemplateArgument();
-    
     return ParsedTemplateArgument(ParsedTemplateArgument::Type,
-                                  TypeArg.get().getAsOpaquePtr(), 
-                                  Loc);
+                                  TypeArg.get().getAsOpaquePtr(), Loc);
   }
-  
   // Try to parse a template template argument.
   {
     TentativeParsingAction TPA(*this);
 
-    ParsedTemplateArgument TemplateTemplateArgument
-      = ParseTemplateTemplateArgument();
+    ParsedTemplateArgument TemplateTemplateArgument =
+        ParseTemplateTemplateArgument();
     if (!TemplateTemplateArgument.isInvalid()) {
       TPA.Commit();
       return TemplateTemplateArgument;
     }
-    
     // Revert this tentative parse to parse a non-type template argument.
     TPA.Revert();
   }
-  
-  // Parse a non-type template argument. 
+  // Parse a non-type template argument.
   SourceLocation Loc = Tok.getLocation();
-  ExprResult ExprArg = ParseConstantExpression(MaybeTypeCast);
+  ExprResult ExprArg = ParseConstantExpression(
+      MaybeTypeCast,
+      false /*don't create another ConstantExpressionEvaluationContext, 
+            use ours instead*/);
   if (ExprArg.isInvalid() || !ExprArg.get())
     return ParsedTemplateArgument();
 
-  return ParsedTemplateArgument(ParsedTemplateArgument::NonType, 
-                                ExprArg.get(), Loc);
+  return ParsedTemplateArgument(ParsedTemplateArgument::NonType, ExprArg.get(),
+                                Loc);
 }
 
 /// \brief Determine whether the current tokens can only be parsed as a 

 ///         template-argument-list ',' template-argument
 bool
 Parser::ParseTemplateArgumentList(TemplateArgList &TemplateArgs) {
-  // Template argument lists are constant-evaluation contexts.
-  EnterExpressionEvaluationContext EvalContext(
-      Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+  
   ColonProtectionRAIIObject ColonProtection(*this, false);
 
   do {
     ParsedTemplateArgument Arg = ParseTemplateArgument();
     SourceLocation EllipsisLoc;
-    if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
+    if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
+      // FIXME: Do we need need to enter a constant evaluation context here?
       Arg = Actions.ActOnPackExpansion(Arg, EllipsisLoc);
+    }
 
     if (Arg.isInvalid()) {
       SkipUntil(tok::comma, tok::greater, StopAtSemi | StopBeforeMatch);

test/SemaCXX/lambda-expressions.cpp

 class S {};
 
 void foo() {
-  const int num = 18;  // expected-note {{'num' declared here}}
+  const int num = 18; 
   auto outer = []() {
-    auto inner = [](S<num> &X) {};  // expected-error {{variable 'num' cannot be implicitly captured in a lambda with no capture-default specified}}
+    auto inner = [](S<num> &X) {};  
   };
 }
 }

   auto s1 = S1{[name=name]() {}}; // expected-error {{use of undeclared identifier 'name'; did you mean 'name1'?}}
 }
 }
+
+namespace PR25627_dont_odr_use_local_consts {
+  
+  template<int> struct X {};
+  
+  void foo() {
+    const int N = 10;
+    (void) [] { X<N> x; };
+  }
+}

test/SemaCXX/local-classes.cpp

     };
   }
 }
+
+namespace PR25627_dont_odr_use_local_consts {
+  template<int> struct X {};
+  
+  void foo() {
+    const int N = 10;
+  
+    struct Local {
+      void f(X<N>) {} // OK
+    }; 
+  }
+}