[Sema] Populate declarations inside TypeLocs for some invalid types

[ilya-biryukov]

Summary

This also reverts 282cae0b9a602267ad7ef622f770066491332a11 as the
particular crash is now handled by the new code.

Before this change Clang would always leave declarations inside the
type-locs as null if the declarator had an invalid type. This patch
populates declarations even for invalid types if the structure of the
type and the type-locs match.

There are certain cases that may still cause crashes. These happen when
Clang recovers the type in a way that is not reflected in the
declarator's structure, e.g. adding a pointer when it was not present in
the code for ObjC interfaces or ignoring pointers written in the code
in C++ with auto return type (auto* foo() -> int). Those cases look
fixable with a better recovery strategy and I plan to follow up with
more patches to address those.

The first attempt caused 31 tests from check-clang to crash due to
different structure of the types and type-locs after certain errors. The
good news is that the failure is localized and mismatch in structures is
discovered by assertions inside DeclaratorLocFiller. Some notable
cases caught by existing tests:

• Invalid chunks when type is fully ignored and replace with int or now. Crashed in C/C2x/n2838.c.
• Invalid return types in lambdas. Crashed in CXX/drs/dr6xx.cpp.
• Invalid member pointers. Crashed in CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p3-generic-lambda-1y.cpp
• ObjC recovery that adds pointers. Crashed in SemaObjC/blocks.m

This change also updates the output of Index/complete-blocks.m.
Not entirely sure what causes the change, but the new function signature
is closer to the source code, so this seems like an improvement.

[erichkeane]

One other thing we probably should do is have an assert when creating a function type that none of its params are null. WDYT?

clang/lib/Sema/SemaType.cpp

5962

Shouldn't the D.setInvalidType(true) in each of the branches work here? So this variable is unnecessary? Else this is a good change IMO.

[ilya-biryukov]

In D146971#4224465, @erichkeane wrote:

One other thing we probably should do is have an assert when creating a function type that none of its params are null. WDYT?

This would definitely be great, however I don't think this is possible without some large redesign.
Currently, FunctionType does not store parameter decls. Instead, they are stored in the FunctionTypeLoc.
The latter gets created by allocating the necessary number of bytes and filling the empty state.
In principle, it would be much easier if FuncionTypeLoc and FunctionType were created in parallel, ensuring consistency there would be much easier. I have also though about this, but this looks like a large change and I didn't explore further.

clang/lib/Sema/SemaType.cpp

5962

(Let me know if I'm misreading the suggestion, I was not sure if my understanding is correct).
If we call setInvalidType more, we would actually get more crashes as the problematic branch is the one that calls getTrivialTypeSourceInfo.
To avoid the extra variable, we could instead ensure that the type always gets replaced with something trivial T = Context.IntTy. But I didn't want to go this path because this causes worse error recovery (going from something like void(<recovered-to:int>) to <recovered-to:int>) and possibly correctness issues (e.g. will we start getting function-decls or lambdas that do not have function types and other assertions may fire).

[erichkeane]

In D146971#4224519, @ilya-biryukov wrote:

In D146971#4224465, @erichkeane wrote:

One other thing we probably should do is have an assert when creating a function type that none of its params are null. WDYT?

This would definitely be great, however I don't think this is possible without some large redesign.
Currently, FunctionType does not store parameter decls. Instead, they are stored in the FunctionTypeLoc.
The latter gets created by allocating the necessary number of bytes and filling the empty state.
In principle, it would be much easier if FuncionTypeLoc and FunctionType were created in parallel, ensuring consistency there would be much easier. I have also though about this, but this looks like a large change and I didn't explore further.

Ah, hrmph. I guess I was just hoping for some assert (perhaps even in GetFullTypeForDeclarator?) to assert in order to give future folks a hint as to why their change crashes? Basically, whenever the FunctionType/FunctionTypeLoc are 'finalized' as a check perhaps? Not really looking for a refactor, so much as an assert to prevent us from getting here again.

clang/lib/Sema/SemaType.cpp

5962

My suggestion was that setInvalidType is called everywhere that AreDeclatorChunksValid above (sans 1 perhaps?). So my question was whether this introduced variable was necessary.

[kadircet]

i've added a reproducer for https://reviews.llvm.org/D146634, can you patch that into here? unfortunately i couldn't get it to crash in any place other than through clangd.

[ilya-biryukov]

In D146971#4224540, @erichkeane wrote:

Ah, hrmph. I guess I was just hoping for some assert (perhaps even in GetFullTypeForDeclarator?) to assert in order to give future folks a hint as to why their change crashes? Basically, whenever the FunctionType/FunctionTypeLoc are 'finalized' as a check perhaps? Not really looking for a refactor, so much as an assert to prevent us from getting here again.

Yeah, that makes a lot of sense. We need to fix the FIXMEs for that to work, I'm eager to try this in follow-ups. But probably not in this particular change as it seems like a lot of work and this is already moving in the right direction.

We probably need asserts in two places: GetFullTypeForDeclarator and getTrivialTypeSourceInfo.
I'm less worried about the former, but fixing getTrivialTypeSourceInfo might be harder as it has a lot of call sites and I suspect some of them actually pass FunctionType as inputs. The assert would basically prohibit creating FunctionTypeLoc without parameter declarations.
I don't think it's unreasonable, the code that produces FunctionTypeLoc without having parameters should either find the corresponding parameters or be rewritten to use QualType instead of TypeLoc, which does not need parameters.
But I expect this to be a lot of work to actually update the callsites, and this will probably take quite some time.

clang/lib/Sema/SemaType.cpp

5962

Ah, thanks for clarifying the question.
setInvalidType is called in much more places and the point of this patch is to capture a subcategory of invalid types that still follow the declarator structure.

[erichkeane]

clang/lib/Sema/SemaType.cpp

5962

Got it, thanks for the clarification. I'm OK with this as-is, then.

[aaron.ballman]

LGTM, though the change should come with a release note. Suggestion you can take or leave as you see fit: should we turn the places where we removed the null pointer check into assertions that the pointer is nonnull? Or are we hoping the crash will be sufficient to tell us when we've missed a case?

[ilya-biryukov]

• Add assertions for removed null checks

[ilya-biryukov]

• Another forgotten non-null assertion

[ilya-biryukov]

Sorry for taking so long to land this, it fell off my radar.

In D146971#4227482, @aaron.ballman wrote:

LGTM, though the change should come with a release note. Suggestion you can take or leave as you see fit: should we turn the places where we removed the null pointer check into assertions that the pointer is nonnull? Or are we hoping the crash will be sufficient to tell us when we've missed a case?

Added the check. I think they're useful, at the very least if folks hit them they have a better chance of finding this change and reaching out to the right people.

clang/lib/Sema/SemaChecking.cpp

 /// function are appropriate for the definition of a function. This
 /// takes care of any checks that cannot be performed on the
 /// declaration itself, e.g., that the types of each of the function
 /// parameters are complete.
 bool Sema::CheckParmsForFunctionDef(ArrayRef<ParmVarDecl *> Parameters,
                                     bool CheckParameterNames) {
   bool HasInvalidParm = false;
   for (ParmVarDecl *Param : Parameters) {
-    if (!Param) {
-      HasInvalidParm = true;
-      continue;
-    }
+    assert(Param && "null in a parameter list");
     // C99 6.7.5.3p4: the parameters in a parameter type list in a
     // function declarator that is part of a function definition of
     // that function shall not have incomplete type.
     //
     // This is also C++ [dcl.fct]p6.
     if (!Param->isInvalidDecl() &&
         RequireCompleteType(Param->getLocation(), Param->getType(),
                             diag::err_typecheck_decl_incomplete_type)) {

clang/lib/Sema/SemaDeclCXX.cpp

static bool CheckConstexprDestructorSubobjects(Sema &SemaRef,
   for (const FieldDecl *FD : RD->fields())
     if (!Check(FD->getLocation(), FD->getType(), FD))
       return false;
   return true;
 }
 
 /// Check whether a function's parameter types are all literal types. If so,
 /// return true. If not, produce a suitable diagnostic and return false.
-/// If any ParamDecl is null, return false without producing a diagnostic.
-/// The code creating null parameters is responsible for producing a diagnostic.
 static bool CheckConstexprParameterTypes(Sema &SemaRef,
                                          const FunctionDecl *FD,
                                          Sema::CheckConstexprKind Kind) {
   unsigned ArgIndex = 0;
   const auto *FT = FD->getType()->castAs<FunctionProtoType>();
   for (FunctionProtoType::param_type_iterator i = FT->param_type_begin(),
                                               e = FT->param_type_end();
        i != e; ++i, ++ArgIndex) {
     const ParmVarDecl *PD = FD->getParamDecl(ArgIndex);
-    if (!PD)
-      return false;
+    assert(PD && "null in a parameter list");
     SourceLocation ParamLoc = PD->getLocation();
     if (CheckLiteralType(SemaRef, Kind, ParamLoc, *i,
                          diag::err_constexpr_non_literal_param, ArgIndex + 1,
                          PD->getSourceRange(), isa<CXXConstructorDecl>(FD),
                          FD->isConsteval()))
       return false;
   }
   return true;

clang/lib/Sema/SemaLambda.cpp

void Sema::CompleteLambdaCallOperator(
   Method->setType(buildTypeForLambdaCallOperator(*this, LSI->Lambda,
                                                  TemplateParams, MethodTyInfo));
   Method->setConstexprKind(ConstexprKind);
   Method->setStorageClass(SC);
   if (!Params.empty()) {
     CheckParmsForFunctionDef(Params, /*CheckParameterNames=*/false);
     Method->setParams(Params);
     for (auto P : Method->parameters()) {
-      if (!P)
-        continue;
+      assert(P && "null in a parameter list");
       P->setOwningFunction(Method);
     }
   }
 
   buildLambdaScopeReturnType(*this, LSI, Method, HasExplicitResultType);
 }
 
 void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,

clang/lib/Sema/SemaTemplateInstantiate.cpp

ExprResult TransformLambdaExpr(LambdaExpr *E) {
       LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
       Sema::ConstraintEvalRAII<TemplateInstantiator> RAII(*this);
       ExprResult Result = inherited::TransformLambdaExpr(E);
       if (Result.isInvalid())
         return Result;
 
       CXXMethodDecl *MD = Result.getAs<LambdaExpr>()->getCallOperator();
       for (ParmVarDecl *PVD : MD->parameters()) {
-        if (!PVD || !PVD->hasDefaultArg())
+        assert(PVD && "null in a parameter list");
+        if (!PVD->hasDefaultArg())
           continue;
         Expr *UninstExpr = PVD->getUninstantiatedDefaultArg();
         // FIXME: Obtain the source location for the '=' token.
         SourceLocation EqualLoc = UninstExpr->getBeginLoc();
         if (SemaRef.SubstDefaultArgument(EqualLoc, PVD, TemplateArgs)) {
           // If substitution fails, the default argument is set to a
           // RecoveryExpr that wraps the uninstantiated default argument so
           // that downstream diagnostics are omitted.

clang/lib/Sema/SemaType.cpp

static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
   }
 
   bool ExpectNoDerefChunk =
       state.getCurrentAttributes().hasAttribute(ParsedAttr::AT_NoDeref);
 
   // Walk the DeclTypeInfo, building the recursive type as we go.
   // DeclTypeInfos are ordered from the identifier out, which is
   // opposite of what we want :).
+
+  // Track if the produced type matches the structure of the declarator.
+  // This is used later to decide if we can fill `TypeLoc` from
+  // `DeclaratorChunk`s. E.g. it must be false if Clang recovers from
+  // an error by replacing the type with `int`.
+  bool AreDeclaratorChunksValid = true;
   for (unsigned i = 0, e = D.getNumTypeObjects(); i != e; ++i) {
     unsigned chunkIndex = e - i - 1;
     state.setCurrentChunkIndex(chunkIndex);
     DeclaratorChunk &DeclType = D.getTypeObject(chunkIndex);
     IsQualifiedFunction &= DeclType.Kind == DeclaratorChunk::Paren;
     switch (DeclType.Kind) {
     case DeclaratorChunk::Paren:
       if (i == 0)
case DeclaratorChunk::Function: {
             !FTI.hasTrailingReturnType() && chunkIndex == 0) {
           if (!S.getLangOpts().CPlusPlus14) {
             S.Diag(D.getDeclSpec().getTypeSpecTypeLoc(),
                    D.getDeclSpec().getTypeSpecType() == DeclSpec::TST_auto
                        ? diag::err_auto_missing_trailing_return
                        : diag::err_deduced_return_type);
             T = Context.IntTy;
             D.setInvalidType(true);
+            AreDeclaratorChunksValid = false;
           } else {
             S.Diag(D.getDeclSpec().getTypeSpecTypeLoc(),
                    diag::warn_cxx11_compat_deduced_return_type);
           }
         } else if (FTI.hasTrailingReturnType()) {
           // T must be exactly 'auto' at this point. See CWG issue 681.
           if (isa<ParenType>(T)) {
             S.Diag(D.getBeginLoc(), diag::err_trailing_return_in_parens)
                 << T << D.getSourceRange();
             D.setInvalidType(true);
+            // FIXME: recover and fill decls in `TypeLoc`s.
+            AreDeclaratorChunksValid = false;
           } else if (D.getName().getKind() ==
                      UnqualifiedIdKind::IK_DeductionGuideName) {
             if (T != Context.DependentTy) {
               S.Diag(D.getDeclSpec().getBeginLoc(),
                      diag::err_deduction_guide_with_complex_decl)
                   << D.getSourceRange();
               D.setInvalidType(true);
+              // FIXME: recover and fill decls in `TypeLoc`s.
+              AreDeclaratorChunksValid = false;
             }
           } else if (D.getContext() != DeclaratorContext::LambdaExpr &&
                      (T.hasQualifiers() || !isa<AutoType>(T) ||
                       cast<AutoType>(T)->getKeyword() !=
                           AutoTypeKeyword::Auto ||
                       cast<AutoType>(T)->isConstrained())) {
             S.Diag(D.getDeclSpec().getTypeSpecTypeLoc(),
                    diag::err_trailing_return_without_auto)
                 << T << D.getDeclSpec().getSourceRange();
             D.setInvalidType(true);
+            // FIXME: recover and fill decls in `TypeLoc`s.
+            AreDeclaratorChunksValid = false;
           }
           T = S.GetTypeFromParser(FTI.getTrailingReturnType(), &TInfo);
           if (T.isNull()) {
             // An error occurred parsing the trailing return type.
             T = Context.IntTy;
             D.setInvalidType(true);
           } else if (AutoType *Auto = T->getContainedAutoType()) {
             // If the trailing return type contains an `auto`, we may need to
case DeclaratorChunk::Function: {
         // Last processing chunk in block context means this function chunk
         // represents the block.
         if (chunkIndex == 0 &&
             D.getContext() == DeclaratorContext::BlockLiteral)
           diagID = diag::err_block_returning_array_function;
         S.Diag(DeclType.Loc, diagID) << T->isFunctionType() << T;
         T = Context.IntTy;
         D.setInvalidType(true);
+        AreDeclaratorChunksValid = false;
       }
 
       // Do not allow returning half FP value.
       // FIXME: This really should be in BuildFunctionType.
       if (T->isHalfType()) {
         if (S.getLangOpts().OpenCL) {
           if (!S.getOpenCLOptions().isAvailableOption("cl_khr_fp16",
                                                       S.getLangOpts())) {
case DeclaratorChunk::Function: {
 
         T = Context.getObjCObjectPointerType(T);
         if (TInfo) {
           TypeLocBuilder TLB;
           TLB.pushFullCopy(TInfo->getTypeLoc());
           ObjCObjectPointerTypeLoc TLoc = TLB.push<ObjCObjectPointerTypeLoc>(T);
           TLoc.setStarLoc(FixitLoc);
           TInfo = TLB.getTypeSourceInfo(Context, T);
+        } else {
+          AreDeclaratorChunksValid = false;
         }
 
         D.setInvalidType(true);
       }
 
       // cv-qualifiers on return types are pointless except when the type is a
       // class type in C++.
       if ((T.getCVRQualifiers() || T->isAtomicType()) &&
case DeclaratorChunk::Function: {
           // definition.
           S.Diag(FTI.Params[0].IdentLoc,
                  diag::err_ident_list_in_fn_declaration);
           D.setInvalidType(true);
           // Recover by creating a K&R-style function type, if possible.
           T = (!LangOpts.requiresStrictPrototypes() && !LangOpts.OpenCL)
                   ? Context.getFunctionNoProtoType(T, EI)
                   : Context.IntTy;
+          AreDeclaratorChunksValid = false;
           break;
         }
 
         FunctionProtoType::ExtProtoInfo EPI;
         EPI.ExtInfo = EI;
         EPI.Variadic = FTI.isVariadic;
         EPI.EllipsisLoc = FTI.getEllipsisLoc();
         EPI.HasTrailingReturn = FTI.hasTrailingReturnType();
case DeclaratorChunk::MemberPointer: {
           << (D.getIdentifier() ? D.getIdentifier()->getName() : "type name")
           << DeclType.Mem.Scope().getRange();
         D.setInvalidType(true);
       }
 
       if (!ClsType.isNull())
         T = S.BuildMemberPointerType(T, ClsType, DeclType.Loc,
                                      D.getIdentifier());
+      else
+        AreDeclaratorChunksValid = false;
+
       if (T.isNull()) {
         T = Context.IntTy;
         D.setInvalidType(true);
+        AreDeclaratorChunksValid = false;
       } else if (DeclType.Mem.TypeQuals) {
         T = S.BuildQualifiedType(T, DeclType.Loc, DeclType.Mem.TypeQuals);
       }
       break;
     }
 
     case DeclaratorChunk::Pipe: {
       T = S.BuildReadPipeType(T, DeclType.Loc);
       processTypeAttrs(state, T, TAL_DeclSpec,
                        D.getMutableDeclSpec().getAttributes());
       break;
     }
     }
 
     if (T.isNull()) {
       D.setInvalidType(true);
       T = Context.IntTy;
+      AreDeclaratorChunksValid = false;
     }
 
     // See if there are any attributes on this declarator chunk.
     processTypeAttrs(state, T, TAL_DeclChunk, DeclType.getAttrs());
 
     if (DeclType.Kind != DeclaratorChunk::Paren) {
       if (ExpectNoDerefChunk && !IsNoDerefableChunk(DeclType))
         S.Diag(DeclType.Loc, diag::warn_noderef_on_non_pointer_or_array);
case DeclaratorContext::Association:
       S.Diag(D.getEllipsisLoc(),
              diag::err_ellipsis_in_declarator_not_parameter);
       D.setEllipsisLoc(SourceLocation());
       break;
     }
   }
 
   assert(!T.isNull() && "T must not be null at the end of this function");
-  if (D.isInvalidType())
+  if (!AreDeclaratorChunksValid)
     return Context.getTrivialTypeSourceInfo(T);
-
   return GetTypeSourceInfoForDeclarator(state, T, TInfo);
 }
 
 /// GetTypeForDeclarator - Convert the type for the specified
 /// declarator to Type instances.
 ///
 /// The result of this call will never be null, but the associated
 /// type may be a null type if there's an unrecoverable error.

clang/test/Index/complete-blocks.m

 // CHECK-CC6: ObjCInstanceMethodDecl:{ResultType id}{TypedText method6:}{Placeholder ^(block_t block)arg} (35)
 
 // RUN: c-index-test -code-completion-at=%s:50:1 %s | FileCheck -check-prefix=CHECK-CC7 %s
 // CHECK-CC7: FunctionDecl:{ResultType void}{TypedText f2}{LeftParen (}{Placeholder ^int(int x, int y)block}{RightParen )} (50)
 // RUN: c-index-test -code-completion-at=%s:50:6 %s | FileCheck -check-prefix=CHECK-CC8 %s
 // CHECK-CC8: ObjCInstanceMethodDecl:{ResultType id}{TypedText method7:}{Placeholder ^int(int x, int y)b} (35)
 
 // RUN: c-index-test -code-completion-at=%s:59:6 %s | FileCheck -check-prefix=CHECK-CC9 %s
-// CHECK-CC9: ObjCInstanceMethodDecl:{ResultType void}{TypedText foo:}{Placeholder ^int *(int)arg} (35)
+// CHECK-CC9: ObjCInstanceMethodDecl:{ResultType void}{TypedText foo:}{Placeholder ^int *(float)arg} (35)