[AST][RecoveryExpr] Popagate the error-bit from a VarDecl's initializer to DeclRefExpr.

[hokein]

Summary

The error-bit was missing, if a DeclRefExpr (which refers to a VarDecl
with a contains-errors initializer).

It could cause different violations in clang -- the DeclRefExpr is value-dependent,
but not contains-errors, decltype(DeclRefExpr) could produce a non-error
dependent type in non-template context.

[hokein]

update.

[sammccall]

Neither of the testcases look like the right behavior to me, and I think the bug is elsewhere in clang.

The crux is we're forcing decltype(N) to be a (unique) dependent type, which feels wrong.
This isn't specific to error-dependence, the same is true in

template <int K> void foo() {
  constexpr int N = K;
  decltype(K) // dependent?
}

ASTContext::getDecltypeType() has to determine whether http://eel.is/c++draft/temp.type#4 applies.
There are three behaviors:

• standard up to C++14: traversing the expr looking for a template param to be lexically included (this is my reading of the standard)
• what clang actually does: check instantiation dependence, which I think pulls in too many cases
• standard after http://wg21.link/cwg2064: check type dependence

I think it's probably OK to adopt the C++17 behavior for all versions (if I'm right that the current behavior is a bug).
@rsmith It's your DR, what do you think :-)

(Per offline discussion, this was reduced from a related bug that didn't involve decltype, but I think we should treat that one as distinct)

[rsmith]

In D86048#2254607, @sammccall wrote:

The crux is we're forcing decltype(N) to be a (unique) dependent type, which feels wrong.
[...]
There are three behaviors:

• standard up to C++14: traversing the expr looking for a template param to be lexically included (this is my reading of the standard)

FWIW, "involves a template parameter" is exactly the same phrasing that [temp.over.link] uses to refer to instantiation-dependence; that's why Clang uses instantiation-dependence in this case at the moment.

• what clang actually does: check instantiation dependence, which I think pulls in too many cases
• standard after http://wg21.link/cwg2064: check type dependence

I think it's probably OK to adopt the C++17 behavior for all versions (if I'm right that the current behavior is a bug).
@rsmith It's your DR, what do you think :-)

Let's try it and see what happens. I think this will reveal a collection of cases where we don't properly handle instantiation-dependent-but-not-type-dependent constructs (such as, if I remember correctly, the types of non-type template parameters), but we should be fixing those bugs anyway :)

[hokein]

In D86048#2270400, @rsmith wrote:

In D86048#2254607, @sammccall wrote:

The crux is we're forcing decltype(N) to be a (unique) dependent type, which feels wrong.
[...]
There are three behaviors:

• standard up to C++14: traversing the expr looking for a template param to be lexically included (this is my reading of the standard)

FWIW, "involves a template parameter" is exactly the same phrasing that [temp.over.link] uses to refer to instantiation-dependence; that's why Clang uses instantiation-dependence in this case at the moment.

• what clang actually does: check instantiation dependence, which I think pulls in too many cases
• standard after http://wg21.link/cwg2064: check type dependence

I think it's probably OK to adopt the C++17 behavior for all versions (if I'm right that the current behavior is a bug).
@rsmith It's your DR, what do you think :-)

Let's try it and see what happens. I think this will reveal a collection of cases where we don't properly handle instantiation-dependent-but-not-type-dependent constructs (such as, if I remember correctly, the types of non-type template parameters), but we should be fixing those bugs anyway :)

https://reviews.llvm.org/D87349 is an attempt.

[hokein]

update the patch to fix another crash ABC<N> abc where N's initailizer is contains-errors.

[hokein]

clang/test/Sema/invalid-member.cpp

29

The current fix (by invalidating the member decl) is probably suboptimal.

The crash is caused by the fact that we escape the dependent-type guard in HandleSizeOf -- the recordType T is not type-dependent, because the implementation TagDecl::isDependentType just checks isDependentContext().

This is incorrect after recovery-expr is introduced -- in this example, ABC<N> abc is type-dependent because the template argument DeclRefExpr is value-dependent,
so the recordType T should be type-dependent.

I think right fix is to make the recordType T type-dependent, but there are a few tricky things in clang making it harder:

• The TagType is cached and uniqued, and the dependence-bits are set in the constructor;
• The TagType can be created before we see the RecordDecl body;

To set the type-dependent, we need to see class body, by that time, the TagType is created, and cached, I'm not sure it is reasonable to rewrite the dependence-bits when we complete a class definition. Any suggestions?

[sammccall]

I agree with the concerns about type T not being dependent, but don't have a good suggestion of where to draw the line (introducing dependence bits for Decl is obviously a big can of worms).

However the concrete change here seems sensible to me: if value-dependence propagates from initializer to reference, then maybe so should errors? Particularly in cases of constants...
And it fixes the bug in a reasonably intuitive way: ABC<N> isn't a valid type, so any variable declared with it is invalid.

So LGTM unless @rsmith has any objections.

[hokein]

@rsmith I'm landing this patch now. Happy to follow-up if you have other comments.

clang/lib/AST/ComputeDependence.cpp

ExprDependence clang::computeDependence(DeclRefExpr *E, const ASTContext &Ctx) {
   //       -  an entity with reference type and is initialized with an
   //          expression that is value-dependent [C++11]
   if (VarDecl *Var = dyn_cast<VarDecl>(Decl)) {
     if ((Ctx.getLangOpts().CPlusPlus11
              ? Var->getType()->isLiteralType(Ctx)
              : Var->getType()->isIntegralOrEnumerationType()) &&
         (Var->getType().isConstQualified() ||
          Var->getType()->isReferenceType())) {
-      if (const Expr *Init = Var->getAnyInitializer())
-        if (Init->isValueDependent()) {
+      if (const Expr *Init = Var->getAnyInitializer()) {
+        if (Init->isValueDependent())
           Deps |= ExprDependence::ValueInstantiation;
+        if (Init->containsErrors()) {
+          Deps |= ExprDependence::Error;
         }
       }
 
     // (VD) - FIXME: Missing from the standard:
     //      -  a member function or a static data member of the current
     //         instantiation
     if (Var->isStaticDataMember() &&
         Var->getDeclContext()->isDependentContext()) {
       Deps |= ExprDependence::ValueInstantiation;
       TypeSourceInfo *TInfo = Var->getFirstDecl()->getTypeSourceInfo();
       if (TInfo->getType()->isIncompleteArrayType())
         Deps |= ExprDependence::Type;
     }
 
     return Deps;
   }
 
   // (VD) - FIXME: Missing from the standard:
   //      -  a member function or a static data member of the current
   //         instantiation
   if (isa<CXXMethodDecl>(Decl) && Decl->getDeclContext()->isDependentContext())
     Deps |= ExprDependence::ValueInstantiation;
   return Deps;
 }
 
 ExprDependence clang::computeDependence(RecoveryExpr *E) {
   // RecoveryExpr is
   //   - always value-dependent, and therefore instantiation dependent
   //   - contains errors (ExprDependence::Error), by definition
   //   - type-dependent if we don't know the type (fallback to an opaque
   //     dependent type), or the type is known and dependent, or it has
   //     type-dependent subexpressions.
   auto D = toExprDependence(E->getType()->getDependence()) |
            ExprDependence::ErrorDependent;
   // FIXME: remove the type-dependent bit from subexpressions, if the
   // RecoveryExpr has a non-dependent type.
   for (auto *S : E->subExpressions())
     D |= S->getDependence();
   return D;
 }
 
 ExprDependence clang::computeDependence(PredefinedExpr *E) {
   return toExprDependence(E->getType()->getDependence()) &
          ~ExprDependence::UnexpandedPack;
 }
 
 ExprDependence clang::computeDependence(CallExpr *E,
                                         llvm::ArrayRef<Expr *> PreArgs) {
   auto D = E->getCallee()->getDependence();
   for (auto *A : llvm::makeArrayRef(E->getArgs(), E->getNumArgs())) {
     if (A)
       D |= A->getDependence();
   }
   for (auto *A : PreArgs)
     D |= A->getDependence();
   return D;
 }
 
 ExprDependence clang::computeDependence(OffsetOfExpr *E) {
   auto D = turnTypeToValueDependence(
       toExprDependence(E->getTypeSourceInfo()->getType()->getDependence()));
   for (unsigned I = 0, N = E->getNumExpressions(); I < N; ++I)
     D |= turnTypeToValueDependence(E->getIndexExpr(I)->getDependence());
   return D;
 }
 
 ExprDependence clang::computeDependence(MemberExpr *E) {
   auto *MemberDecl = E->getMemberDecl();
   auto D = E->getBase()->getDependence();
   if (FieldDecl *FD = dyn_cast<FieldDecl>(MemberDecl)) {
     DeclContext *DC = MemberDecl->getDeclContext();
     // dyn_cast_or_null is used to handle objC variables which do not
     // have a declaration context.
     CXXRecordDecl *RD = dyn_cast_or_null<CXXRecordDecl>(DC);
     if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC)) {
       if (!E->getType()->isDependentType())
         D &= ~ExprDependence::Type;
     }
 
     // Bitfield with value-dependent width is type-dependent.
     if (FD && FD->isBitField() && FD->getBitWidth()->isValueDependent()) {
       D |= ExprDependence::Type;
     }
   }
   // FIXME: move remaining dependence computation from MemberExpr::Create()
   return D;
 }
 
 ExprDependence clang::computeDependence(InitListExpr *E) {
   auto D = ExprDependence::None;
   for (auto *A : E->inits())
     D |= A->getDependence();
   return D;
 }
 
 ExprDependence clang::computeDependence(ShuffleVectorExpr *E) {
   auto D = toExprDependence(E->getType()->getDependence());
   for (auto *C : llvm::makeArrayRef(E->getSubExprs(), E->getNumSubExprs()))
     D |= C->getDependence();
   return D;
 }
 
 ExprDependence clang::computeDependence(GenericSelectionExpr *E,
                                         bool ContainsUnexpandedPack) {
   auto D = ContainsUnexpandedPack ? ExprDependence::UnexpandedPack
                                   : ExprDependence::None;
   for (auto *AE : E->getAssocExprs())
     D |= AE->getDependence() & ExprDependence::Error;
   D |= E->getControllingExpr()->getDependence() & ExprDependence::Error;
 
   if (E->isResultDependent())
     return D | ExprDependence::TypeValueInstantiation;
   return D | (E->getResultExpr()->getDependence() &
               ~ExprDependence::UnexpandedPack);
 }
 
 ExprDependence clang::computeDependence(DesignatedInitExpr *E) {
   auto Deps = E->getInit()->getDependence();
   for (auto D : E->designators()) {
     auto DesignatorDeps = ExprDependence::None;
     if (D.isArrayDesignator())
       DesignatorDeps |= E->getArrayIndex(D)->getDependence();
     else if (D.isArrayRangeDesignator())
       DesignatorDeps |= E->getArrayRangeStart(D)->getDependence() |
                         E->getArrayRangeEnd(D)->getDependence();
     Deps |= DesignatorDeps;
     if (DesignatorDeps & ExprDependence::TypeValue)
       Deps |= ExprDependence::TypeValueInstantiation;
   }
   return Deps;
 }
 
 ExprDependence clang::computeDependence(PseudoObjectExpr *O) {
   auto D = O->getSyntacticForm()->getDependence();
   for (auto *E : O->semantics())
     D |= E->getDependence();
   return D;
 }
 
 ExprDependence clang::computeDependence(AtomicExpr *A) {
   auto D = ExprDependence::None;
   for (auto *E : llvm::makeArrayRef(A->getSubExprs(), A->getNumSubExprs()))
     D |= E->getDependence();
   return D;
 }
 
 ExprDependence clang::computeDependence(CXXNewExpr *E) {
   auto D = toExprDependence(E->getType()->getDependence());
   auto Size = E->getArraySize();
   if (Size.hasValue() && *Size)
     D |= turnTypeToValueDependence((*Size)->getDependence());
   if (auto *I = E->getInitializer())
     D |= turnTypeToValueDependence(I->getDependence());
   for (auto *A : E->placement_arguments())
     D |= turnTypeToValueDependence(A->getDependence());
   return D;
 }
 
 ExprDependence clang::computeDependence(CXXPseudoDestructorExpr *E) {
   auto D = E->getBase()->getDependence();
   if (!E->getDestroyedType().isNull())
     D |= toExprDependence(E->getDestroyedType()->getDependence());
   if (auto *ST = E->getScopeTypeInfo())
     D |= turnTypeToValueDependence(
         toExprDependence(ST->getType()->getDependence()));
   if (auto *Q = E->getQualifier())
     D |= toExprDependence(Q->getDependence() &
                           ~NestedNameSpecifierDependence::Dependent);
   return D;
 }
 
 static inline ExprDependence getDependenceInExpr(DeclarationNameInfo Name) {
   auto D = ExprDependence::None;
   if (Name.isInstantiationDependent())
     D |= ExprDependence::Instantiation;
   if (Name.containsUnexpandedParameterPack())
     D |= ExprDependence::UnexpandedPack;
   return D;
 }
 
 ExprDependence
 clang::computeDependence(OverloadExpr *E, bool KnownDependent,
                          bool KnownInstantiationDependent,
                          bool KnownContainsUnexpandedParameterPack) {
   auto Deps = ExprDependence::None;
   if (KnownDependent)
     Deps |= ExprDependence::TypeValue;
   if (KnownInstantiationDependent)
     Deps |= ExprDependence::Instantiation;
   if (KnownContainsUnexpandedParameterPack)
     Deps |= ExprDependence::UnexpandedPack;
   Deps |= getDependenceInExpr(E->getNameInfo());
   if (auto *Q = E->getQualifier())
     Deps |= toExprDependence(Q->getDependence() &
                              ~NestedNameSpecifierDependence::Dependent);
   for (auto *D : E->decls()) {
     if (D->getDeclContext()->isDependentContext() ||
         isa<UnresolvedUsingValueDecl>(D))
       Deps |= ExprDependence::TypeValueInstantiation;
   }
   // If we have explicit template arguments, check for dependent
   // template arguments and whether they contain any unexpanded pack
   // expansions.
   for (auto A : E->template_arguments())
     Deps |= toExprDependence(A.getArgument().getDependence());
   return Deps;
 }
 
 ExprDependence clang::computeDependence(DependentScopeDeclRefExpr *E) {
   auto D = ExprDependence::TypeValue;
   D |= getDependenceInExpr(E->getNameInfo());
   if (auto *Q = E->getQualifier())
     D |= toExprDependence(Q->getDependence());
   for (auto A : E->template_arguments())
     D |= toExprDependence(A.getArgument().getDependence());
   return D;
 }
 
 ExprDependence clang::computeDependence(CXXConstructExpr *E) {
   auto D = toExprDependence(E->getType()->getDependence());
   for (auto *A : E->arguments())
     D |= A->getDependence() & ~ExprDependence::Type;
   return D;
 }
 
 ExprDependence clang::computeDependence(LambdaExpr *E,
                                         bool ContainsUnexpandedParameterPack) {
   auto D = toExprDependence(E->getType()->getDependence());
   if (ContainsUnexpandedParameterPack)
     D |= ExprDependence::UnexpandedPack;
   return D;
 }
 
 ExprDependence clang::computeDependence(CXXUnresolvedConstructExpr *E) {
   auto D = ExprDependence::ValueInstantiation;
   D |= toExprDependence(E->getType()->getDependence());
   if (E->getType()->getContainedDeducedType())
     D |= ExprDependence::Type;
   for (auto *A : E->arguments())
     D |= A->getDependence() &
          (ExprDependence::UnexpandedPack | ExprDependence::Error);
   return D;

clang/test/Sema/invalid-member.cpp

 // Should be able to evaluate sizeof without crashing.
 static_assert(sizeof(Y) == 1, "No valid members");
 
 class Z {
   int array[sizeof(invalid())]; // expected-error {{use of undeclared identifier}}
 };
 // Should be able to evaluate sizeof without crashing.
 static_assert(sizeof(Z) == 1, "No valid members");
+
+constexpr int N = undef; // expected-error {{use of undeclared identifier}}
+
+class T {
+  decltype(N) member;
+};
+static_assert(sizeof(T) == 1, "No valid members");

clang/test/SemaCXX/invalid-template-base-specifier.cpp

 template <typename T>
 class Base {};
 template <typename T>
 struct Crash3 : Base<decltype(Foo(T()))> { // expected-error {{no matching function for call to 'Foo'}}
   Crash3(){};
 };
 
 void test3() { Crash3<int>(); } // expected-note {{in instantiation of template class}}
+
+constexpr int N = undef; // expected-error {{use of undeclared identifier}}
+
+class Crash4 : decltype(N) {
+};
+
+static_assert(sizeof(Crash4) == 1, "");