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

[Matheus Izvekov via cfe-commits]

=?utf-8?q?Balázs_Kéri?= <balazs.keri at ericsson.com>,
=?utf-8?q?Balázs_Kéri?= <balazs.keri at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/101836 at github.com>


================
@@ -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);
----------------
mizvekov wrote:

Thanks. I think this moved in a better direction.

At least this way, the AST is correct for simple reserialization, whereas as before it was incorrect for that as well.

The new problems are related to merging of definitions. We ought to implement a mechanism to merge default argument definitions here the same way we merge function definitions and such.

For your first example: `template <int I = 1> struct X;` Here is one idea: this should be treated as a definition and merged, and any differences would be an ODR violation.

In particular, merging two modules each containing:
```C++
template <int I, int J = 1> struct X;
```
and
```C++
template <int I = 0, int J = 1> struct X;
```
Because the declaration containing a default argument definition differs, this would be an ODR violation.

I'd be happy to leave solving that problem for a future patch.

For your second example, `X` would already be merged, so I am not sure there is a problem there. So we may end up with getInheritedFrom pointing to a definition which was not chosen as the merged definition, but I believe we keep track of the merged definitions in the ASTContext, so we can make sense of the situation.

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