[PATCH] D87853: [Sema] Update specialization iterator after template argument deduction.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this is pretty subtle. Here's a small C++20 testcase that invalidates `InsertPos` while matching partial specializations:

  template<typename T, int N> constexpr bool v = true;
  template<int N> requires v<int, N-1> constexpr bool v<int, N> = true;
  template<> constexpr bool v<int, 0> = true;
  int k = v<int, 500>;



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4485
       Template, InstantiationPattern, *InstantiationArgs, TemplateArgs,
       Converted, TemplateNameLoc, InsertPos /*, LateAttrs, StartingScope*/);
   if (!Decl)
----------------
All the handling of `InsertPos` in `BuildVarTemplateInstantiation` and below looks wrong to me, with the same bug that this code had. We end up in `VisitVarTemplateSpecializationDecl` which does this:

```
  // Do substitution on the type of the declaration
  TypeSourceInfo *DI =
      SemaRef.SubstType(D->getTypeSourceInfo(), TemplateArgs,
                        D->getTypeSpecStartLoc(), D->getDeclName());
  if (!DI)
    return nullptr;

  if (DI->getType()->isFunctionType()) {
    SemaRef.Diag(D->getLocation(), diag::err_variable_instantiates_to_function)
        << D->isStaticDataMember() << DI->getType();
    return nullptr;
  }

  // Build the instantiated declaration
  VarTemplateSpecializationDecl *Var = VarTemplateSpecializationDecl::Create(
      SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
      VarTemplate, DI->getType(), DI, D->getStorageClass(), Converted);
  Var->setTemplateArgsInfo(TemplateArgsInfo);
  if (InsertPos)
    VarTemplate->AddSpecialization(Var, InsertPos);
```

But the call to `SubstType` can absolutely invalidate `InsertPos`; here's a test case for a crash caused by that:

```
template<typename T, int N> T v;
template<int N> decltype(v<int, N-1>) v<int, N>;
template<> int v<int, 0>;
int k = v<int, 500>;
```

So I think we should remove the `InsertPos` parameter from this entire cluster of functions. No use of it is correct.

With `InsertPos` removed, we still need to know whether to call `AddSpecialization` or not. We should in principle do that if and only if `!PrevDecl`, but the call to `VisitVarTemplateSpecializationDecl` in `InstantiateVariableDefinition` doesn't pass in a `PrevDecl` even though it has one (`OldVar`). I wonder if we can change `InstantiateVariableDefinition` to pass `OldVar` in as the `PrevDecl` and remove its call to `MergeVarDecl` and probably also its call to `InstantiateVariableInitializer` -- since `BuildVariableInstantiation` should then do all of that for us.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87853/new/

https://reviews.llvm.org/D87853