[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;

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}}
+template<int a>
+class ABC {};
+class T {
+  ABC<N> abc;
+};
+static_assert(sizeof(T) == 1, "No valid members");