[clang] [clang][ASTImporter] New fix for default template parameter values. (PR #101836)

[Balázs Kéri via cfe-commits]

================
@@ -5968,11 +5962,21 @@ ASTNodeImporter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   }
 
   if (D->hasDefaultArgument()) {
+    // Default argument can be "inherited" when it has a reference to the
+    // previous declaration (of the default argument) which is stored only once.
+    // Here we import the default argument in any case, and the inherited state
+    // is updated later after the parent template was created. If the
+    // inherited-from object would be imported here it causes more difficulties
+    // (parent template may not be created yet and import loops can occur).
     Expected<TemplateArgumentLoc> ToDefaultArgOrErr =
         import(D->getDefaultArgument());
     if (!ToDefaultArgOrErr)
       return ToDefaultArgOrErr.takeError();
-    ToD->setDefaultArgument(ToD->getASTContext(), *ToDefaultArgOrErr);
+    // The just called import process can trigger import of the parent template
+    // which can update the default argument value to "inherited". This should
+    // not be changed.
+    if (!ToD->hasDefaultArgument())
+      ToD->setDefaultArgument(ToD->getASTContext(), *ToDefaultArgOrErr);
----------------
balazske wrote:

The case when default argument becomes already set happens when it is changed in function `updateTemplateParametersInheritedFrom` that is called when the template is imported. This change can happen during the import of the default argument object. That function calls the `setInheritedFrom` function. I was experimenting with your idea to just call `getInheritedFrom` and import the result and set it (and not use `updateTemplateParametersInheritedFrom`), but I got another crash. I tried to explain this in the added comment.

The "deferring" is done in the current code from `updateTemplateParametersInheritedFrom` that is called at end of the template import. I wanted to ensure that a default value is always set in the template parameter object, because until `updateTemplateParametersInheritedFrom` is called some other import may happen that uses this value. I think this can cause ODR detection problem. If the default argument is not set (but will be set later from `updateTemplateParametersInheritedFrom`) this causes a gap where the default argument is missing. If during this time something is compared to this object it can cause false structural in-equivalence. To avoid this the default argument is imported and set immediately (even if this object is later discarded). Structural equivalence comparison does not compare if the argument is "inherited" or not, but it does compare the argument value. 

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