[clang] [C++20][Modules] Do not update the declaration generation number if the redeclaration chain completion was delayed. (PR #129982)

[Michael Park via cfe-commits]

================
@@ -9180,6 +9180,12 @@ bool Sema::hasAcceptableDefinition(NamedDecl *D, NamedDecl **Suggested,
   if (!getLangOpts().Modules && !getLangOpts().ModulesLocalVisibility)
     return true;
 
+  // The external source may have additional definitions of this entity that are
+  // visible, so complete the redeclaration chain now.
+  if (auto *Source = Context.getExternalSource()) {
+    Source->CompleteRedeclChain(D);
+  }
----------------
mpark wrote:

Thanks @ChuanqiXu9 for following up!

> why it was not a problem but it is after we delay pending the complete decl chain?

I'll look into this again in more detail, but previous investigations what I know is that the [`RD->addedMember(MD);`](https://github.com/llvm/llvm-project/blob/release/20.x/clang/lib/Serialization/ASTReader.cpp#L10478) adds more entries to `PendingIncompleteDeclChains`. Somewhat surprisingly, the entries that get added are the `RD` portion of the call: `Empty<char>`. When `markIncompleteDeclChain` happens prior to this (the current state, after the revert), we leave `finishPendingActions` with `Empty<char>` still in `PendingIncompleteDeclChains`, which is not actually marked as incomplete until the next time `finishPendingActions` is invoked. With #121245, the `markIncompleteDeclChain` happens after this, which marks `Empty<char>` incomplete and clears out `PendingIncompleteDeclChain` fully before leaving `finishPendingActions`.

That tends to change the order of how things get processed quite a bit. One place this effects for sure is this call to [getMostRecentDecl()](https://github.com/llvm/llvm-project/blob/release/20.x/clang/lib/AST/DeclBase.cpp#L1889). In the current state (where `Empty<char>` is left in `PendingIncompleteDeclChains`), when we come across that `getMostRecentDecl()` call it does nothing. With #121245, `Empty<char>` has been marked incomplete, the `getMostRecentDecl()` call there actually does work.

I know these are rather specific, narrow-scoped observations. I'll need to dig a bit deeper to understand the bigger picture of what really changes.

https://github.com/llvm/llvm-project/pull/129982