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

[Ding Fei via cfe-commits]

=?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);
----------------
danix800 wrote:

I digged a little bit about template importing and have a few questions:

1. Templates are redeclarable, but not like some other nodes (`FunctionDecl`/
    `VarDecl`/`TagDecl` currently), no redecls are imported ahead of the current one.
    if redecls are imported, the ordering problem might go away. If this works,
    we could apply the same mechnism to some other redeclarables too (I tested
    locally and templates can be imported with correct order).

    Or, there might be other considerations about not importing redecls for some
    nodes (like performance? but redecls are usually small, right?), for some client
    (StaticAnalyzer), this mis-ordering is still acceptable, but I'm not quite sure how
    many clients depend on the importer and how they depends on the ordering.

2. For template parameters, current implementation supports importing them directly
    but added it into the top-level TU context, which might not be the good for any
    clients, not to say about ODR violations. I'm not clear what's the consideration
    of not importing some of the nodes without importing context firstly.

For the crash fix it's a LGTM.

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