[AST] Fix crashes on decltype(recovery-expr).

[hokein]

Summary

We mark these decls as invalid.

[sammccall]

clang/include/clang/AST/Type.h

2144

Why is this called isErrorType when the expr version is containsErrors?

clang/lib/Parse/ParseExprCXX.cpp

3109

Add a comment for what the null case means? (When do we actually hit this?)

clang/lib/Sema/SemaType.cpp

1591

we've added the ability to represent types containing errors, but now we're dropping the type.
Is marking the declarator as invalid sufficient?

[hokein]

address review comments.

clang/lib/Parse/ParseExprCXX.cpp

3109

yeah, the [CompleteTest](https://github.com/llvm/llvm-project/blob/master/clang/unittests/Sema/CodeCompleteTest.cpp#L490) hits the assertion after this patch.

the assertion seems incorrect -- IIUC, the assertion is for the isInvalidType() sanity check on Line 3090, however
In ActOnTypeName, DeclaratorInfo could be modified (by GetTypeForDeclarator) before calling isInvalidType.

btw, I think for the CodeCompleteTest, would be nicer to make ActOnTypeName return decltype(<recovery-expr>(bar)), rather than the null type, but I'm not sure changing the ActOnTypeName behavior has any side effect.

[sammccall]

clang/lib/Parse/ParseExprCXX.cpp

3109

the assertion seems incorrect -- IIUC, the assertion is for the isInvalidType() sanity check on Line 3090, however

What you say makes sense, but I think it's worth probing why it's not currently hit (e.g. by int x(auto);, where GetDeclSpecTypeForDeclarator marks the decl as invalid because auto isn't allowed in a prototype).

btw, I think for the CodeCompleteTest, would be nicer to make ActOnTypeName return decltype(<recovery-expr>(bar)), rather than the null type

Definitely. I think "invalid" on a type-concept is stronger than what we're looking for - since we're not tracking errors in decls, we'd want to use "haserrors" on type-concepts and then promote to "invalid" on decl-concepts.

Ugh, the design of "Declarator" makes this difficult, because there's no distinction between "type of this declarator is invalid" and "type of this declarator makes the declarator invalid".

I'd suggest leaving a FIXME on the changes in SemaType, saying something like "we want resulting declarations to be marked invalid, but claiming the type is invalid is too strong - e.g. it causes ActOnTypeName to return a null type."

clang/lib/Sema/SemaType.cpp

1594

are you sure you want this in the individual cases, rather than once at the end of this function?

[hokein]

address comments.

clang/lib/Parse/ParseExprCXX.cpp

3109

What you say makes sense, but I think it's worth probing why it's not currently hit (e.g. by int x(auto);, where GetDeclSpecTypeForDeclarator marks the decl as invalid because auto isn't allowed in a prototype).

I believe the issue exists even before this patch, it was just not caught by tests. Added one.

clang/lib/Sema/SemaType.cpp

1594

ah, good point, moved the end of the switch statement.

clang/include/clang/AST/DependenceFlags.h

enum TypeDependence : uint8_t {
     /// Whether this type somehow involves a template parameter, even
     /// if the resolution of the type does not depend on a template parameter.
     Instantiation = 2,
     /// Whether this type is a dependent type (C++ [temp.dep.type]).
     Dependent = 4,
     /// Whether this type is a variably-modified type (C99 6.7.5).
     VariablyModified = 8,
 
-    // FIXME: add Error bit.
+    /// Whether this type references an error, e.g. decltype(err-expression)
+    /// yields an error type.
+    Error = 16,
 
     None = 0,
-    All = 15,
+    All = 31,
 
     DependentInstantiation = Dependent | Instantiation,
 
-    LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/VariablyModified)
+    LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Error)
   };
 };
 using TypeDependence = TypeDependenceScope::TypeDependence;
 
 #define LLVM_COMMON_DEPENDENCE(NAME)                                           \
   struct NAME##Scope {                                                         \
     enum NAME : uint8_t {                                                      \
       UnexpandedPack = 1,                                                      \
Dependence(TemplateNameDependence D)
       : V(translate(D, TNDependence::UnexpandedPack, UnexpandedPack) |
              translate(D, TNDependence::Instantiation, Instantiation) |
              translate(D, TNDependence::Dependent, Dependent)) {}
 
   TypeDependence type() const {
     return translate(V, UnexpandedPack, TypeDependence::UnexpandedPack) |
            translate(V, Instantiation, TypeDependence::Instantiation) |
            translate(V, Dependent, TypeDependence::Dependent) |
+           translate(V, Error, TypeDependence::Error) |
            translate(V, VariablyModified, TypeDependence::VariablyModified);
   }
 
   ExprDependence expr() const {
     return translate(V, UnexpandedPack, ExprDependence::UnexpandedPack) |
            translate(V, Instantiation, ExprDependence::Instantiation) |
            translate(V, Type, ExprDependence::Type) |
            translate(V, Value, ExprDependence::Value) |

clang/include/clang/AST/Type.h

#include "clang/Basic/OpenCLExtensionTypes.def"
 
   /// Given that this is a scalar type, classify it.
   ScalarTypeKind getScalarTypeKind() const;
 
   TypeDependence getDependence() const {
     return static_cast<TypeDependence>(TypeBits.Dependence);
   }
 
+  /// Whether this type is an error type.
+  bool containsErrors() const {
+    return getDependence() & TypeDependence::Error;
+  }
+
   /// Whether this type is a dependent type, meaning that its definition
   /// somehow depends on a template parameter (C++ [temp.dep.type]).
   bool isDependentType() const {
     return getDependence() & TypeDependence::Dependent;
   }
 
   /// Determine whether this type is an instantiation-dependent type,
   /// meaning that the type involves a template parameter (even if the

clang/lib/Parse/ParseExprCXX.cpp

if (Tok.is(tok::l_paren)) {
     BalancedDelimiterTracker T(*this, tok::l_paren);
     T.consumeOpen();
     ConstructorLParen = T.getOpenLocation();
     if (Tok.isNot(tok::r_paren)) {
       CommaLocsTy CommaLocs;
       auto RunSignatureHelp = [&]() {
         ParsedType TypeRep =
             Actions.ActOnTypeName(getCurScope(), DeclaratorInfo).get();
-        assert(TypeRep && "invalid types should be handled before");
-        QualType PreferredType = Actions.ProduceConstructorSignatureHelp(
+        QualType PreferredType;
+        // ActOnTypeName might adjust DeclaratorInfo and return a null type even
+        // the passing DeclaratorInfo is valid, e.g. running SignatureHelp on
+        // `new decltype(invalid) (^)`.
+        if (TypeRep)
+          PreferredType = Actions.ProduceConstructorSignatureHelp(
               getCurScope(), TypeRep.get()->getCanonicalTypeInternal(),
               DeclaratorInfo.getEndLoc(), ConstructorArgs, ConstructorLParen);
         CalledSignatureHelp = true;
         return PreferredType;
       };
       if (ParseExpressionList(ConstructorArgs, CommaLocs, [&] {
             PreferredType.enterFunctionArgument(Tok.getLocation(),
                                                 RunSignatureHelp);
           })) {
         if (PP.isCodeCompletionReached() && !CalledSignatureHelp)

clang/lib/Sema/SemaType.cpp

case DeclSpec::TST_typeofType:
     Result = Context.getTypeOfType(Result);
     break;
   case DeclSpec::TST_typeofExpr: {
     Expr *E = DS.getRepAsExpr();
     assert(E && "Didn't get an expression for typeof?");
     // TypeQuals handled by caller.
     Result = S.BuildTypeofExprType(E, DS.getTypeSpecTypeLoc());
     if (Result.isNull()) {
       Result = Context.IntTy;
       declarator.setInvalidType(true);
     }
     break;
   }
   case DeclSpec::TST_decltype: {
     Expr *E = DS.getRepAsExpr();
     assert(E && "Didn't get an expression for decltype?");
     // TypeQuals handled by caller.
     Result = S.BuildDecltypeType(E, DS.getTypeSpecTypeLoc());
     if (Result.isNull()) {
       Result = Context.IntTy;

 #include "clang/Basic/OpenCLImageTypes.def"
 
   case DeclSpec::TST_error:
     Result = Context.IntTy;
     declarator.setInvalidType(true);
     break;
   }
 
+  // FIXME: we want resulting declarations to be marked invalid, but claiming
+  // the type is invalid is too strong - e.g. it causes ActOnTypeName to return
+  // a null type.
+  if (Result->containsErrors())
+    declarator.setInvalidType();
+
   if (S.getLangOpts().OpenCL &&
       S.checkOpenCLDisabledTypeDeclSpec(DS, Result))
     declarator.setInvalidType(true);
 
   bool IsFixedPointType = DS.getTypeSpecType() == DeclSpec::TST_accum ||
                           DS.getTypeSpecType() == DeclSpec::TST_fract;
 
   // Only fixed point types can be saturated

clang/test/AST/ast-dump-expr-errors.cpp

 // CHECK-NEXT:|   `-BinaryOperator {{.*}} '<dependent type>' contains-errors '+'
 // CHECK-NEXT:|     |-RecoveryExpr {{.*}} '<dependent type>' contains-errors
 // CHECK-NEXT:|     | `-UnresolvedLookupExpr {{.*}} 'bar'
 // CHECK-NEXT:|     `-IntegerLiteral {{.*}} 1
 int d = static_cast<int>(bar() + 1);
 
 // FIXME: store initializer even when 'auto' could not be deduced.
 // Expressions with errors currently do not keep initializers around.
-// CHECK:     `-VarDecl {{.*}} invalid e 'auto'
+// CHECK: -VarDecl {{.*}} invalid e 'auto'
 auto e = bar();
+
+// Error type should result in an invalid decl.
+// CHECK: -VarDecl {{.*}} invalid f 'decltype(<recovery-expr>(bar))'
+decltype(bar()) f;

clang/test/Sema/invalid-member.cpp

-// RUN: %clang_cc1 -verify -fsyntax-only %s
-void foo(); // expected-note {{requires 0 arguments}}
+// RUN: %clang_cc1 -verify -fsyntax-only -fno-recovery-ast %s
+// RUN: %clang_cc1 -verify -fsyntax-only -frecovery-ast %s
+
+void foo(); // expected-note 2{{requires 0 arguments}}
 class X {
   decltype(foo(42)) invalid; // expected-error {{no matching function}}
 };
 // Should be able to evaluate sizeof without crashing.
 static_assert(sizeof(X) == 1, "No valid members");
+
+class Y {
+  typeof(foo(42)) invalid; // expected-error {{no matching function}}
+};
+// Should be able to evaluate sizeof without crashing.
+static_assert(sizeof(Y) == 1, "No valid members");

clang/unittests/Sema/CodeCompleteTest.cpp

TEST(PreferredTypeTest, FunctionArguments) {
   )cpp";
   EXPECT_THAT(collectPreferredTypes(Code), Each("vector<int>"));
 }
 
 TEST(PreferredTypeTest, NoCrashOnInvalidTypes) {
   StringRef Code = R"cpp(
     auto x = decltype(&1)(^);
     auto y = new decltype(&1)(^);
+    // GNU decimal type extension is not supported in clang.
+    auto z = new _Decimal128(^);
   )cpp";
   EXPECT_THAT(collectPreferredTypes(Code), Each("NULL TYPE"));
 }
+
 } // namespace