Improve the -Wundefined-func-template diagnostic note for invisible template functions #129031

hokein · 2025-02-27T10:16:36Z

See discussion in #125071.

Makes the note clear for unreachable case:

Before:

./hoge.h:5:12: warning: instantiation of function 'x<int>' required here, but no definition is available [-Wundefined-func-template]
    5 | void f() { x<int>(); }
      |            ^
./shared_ptr2.h:4:6: note: forward declaration of template entity is here
    4 | void x() { T t; (void)t; }
      |      ^
./hoge.h:5:12: note: add an explicit instantiation declaration to suppress this warning if 'x<int>' is explicitly instantiated in another translation unit
    5 | void f() { x<int>(); }
      |

After:

./hoge.h:5:12: warning: instantiation of function 'x<int>' required here, but no definition is available [-Wundefined-func-template]
    5 | void f() { x<int>(); }
      |            ^
./shared_ptr2.h:4:6: note: declaration of template entity is unreachable here
    4 | void x() { T t; (void)t; }
      |      ^
1 warning generated.

github-actions · 2025-02-27T10:20:15Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ilya-biryukov · 2025-02-28T12:54:13Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def note_forward_template_decl : Note<
  "forward declaration of template entity is here">;
+def note_unreachable_template_decl
+    : Note<"declaration of template entity is unreachable here">;


I guess "unreachable declaration of template entity is here" is a more natural choice.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

llvmbot · 2025-02-28T14:23:52Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Haojian Wu (hokein)

Changes

See discussion in #125071.

Makes the note clear for unreachable case:

Before:

./hoge.h:5:12: warning: instantiation of function 'x&lt;int&gt;' required here, but no definition is available [-Wundefined-func-template]
    5 | void f() { x&lt;int&gt;(); }
      |            ^
./shared_ptr2.h:4:6: note: forward declaration of template entity is here
    4 | void x() { T t; (void)t; }
      |      ^
./hoge.h:5:12: note: add an explicit instantiation declaration to suppress this warning if 'x&lt;int&gt;' is explicitly instantiated in another translation unit
    5 | void f() { x&lt;int&gt;(); }
      |

After:

./hoge.h:5:12: warning: instantiation of function 'x&lt;int&gt;' required here, but no definition is available [-Wundefined-func-template]
    5 | void f() { x&lt;int&gt;(); }
      |            ^
./shared_ptr2.h:4:6: note: declaration of template entity is unreachable here
    4 | void x() { T t; (void)t; }
      |      ^
1 warning generated.

Full diff: https://github.com/llvm/llvm-project/pull/129031.diff

8 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2)
(modified) clang/include/clang/Sema/Sema.h (+5-7)
(modified) clang/lib/Sema/SemaTemplate.cpp (+7-7)
(modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+19-9)
(added) clang/test/Modules/Inputs/undefined-template/hoge.h (+7)
(added) clang/test/Modules/Inputs/undefined-template/module.modulemap (+7)
(added) clang/test/Modules/Inputs/undefined-template/shared_ptr2.h (+4)
(added) clang/test/Modules/diag-undefined-template.cpp (+10)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f10af8f5bd6b2..80f2d54c8a893 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5654,6 +5654,8 @@ def warn_func_template_missing : Warning<"instantiation of function %q0 "
   InGroup<UndefinedFuncTemplate>, DefaultIgnore;
 def note_forward_template_decl : Note<
   "forward declaration of template entity is here">;
+def note_unreachable_template_decl
+    : Note<"unreachable declaration of template entity is here">;
 def note_inst_declaration_hint : Note<"add an explicit instantiation "
   "declaration to suppress this warning if %q0 is explicitly instantiated in "
   "another translation unit">;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a501b901862b6..dfe1c0342252f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11183,13 +11183,11 @@ class Sema final : public SemaBase {
 
   /// Determine whether we would be unable to instantiate this template (because
   /// it either has no definition, or is in the process of being instantiated).
-  bool DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
-                                      NamedDecl *Instantiation,
-                                      bool InstantiatedFromMember,
-                                      const NamedDecl *Pattern,
-                                      const NamedDecl *PatternDef,
-                                      TemplateSpecializationKind TSK,
-                                      bool Complain = true);
+  bool DiagnoseUninstantiableTemplate(
+      SourceLocation PointOfInstantiation, NamedDecl *Instantiation,
+      bool InstantiatedFromMember, const NamedDecl *Pattern,
+      const NamedDecl *PatternDef, TemplateSpecializationKind TSK,
+      bool Complain = true, bool *Unreachable = nullptr);
 
   /// DiagnoseTemplateParameterShadow - Produce a diagnostic complaining
   /// that the template parameter 'PrevDecl' is being shadowed by a new
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 38fa3ff3ab5b4..9a0e37f344287 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -759,13 +759,11 @@ Sema::BuildDependentDeclRefExpr(const CXXScopeSpec &SS,
       TemplateArgs);
 }
 
-bool Sema::DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
-                                          NamedDecl *Instantiation,
-                                          bool InstantiatedFromMember,
-                                          const NamedDecl *Pattern,
-                                          const NamedDecl *PatternDef,
-                                          TemplateSpecializationKind TSK,
-                                          bool Complain /*= true*/) {
+bool Sema::DiagnoseUninstantiableTemplate(
+    SourceLocation PointOfInstantiation, NamedDecl *Instantiation,
+    bool InstantiatedFromMember, const NamedDecl *Pattern,
+    const NamedDecl *PatternDef, TemplateSpecializationKind TSK,
+    bool Complain /*= true*/, bool *Unreachable) {
   assert(isa<TagDecl>(Instantiation) || isa<FunctionDecl>(Instantiation) ||
          isa<VarDecl>(Instantiation));
 
@@ -778,6 +776,8 @@ bool Sema::DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
     if (!hasReachableDefinition(const_cast<NamedDecl *>(PatternDef),
                                 &SuggestedDef,
                                 /*OnlyNeedComplete*/ false)) {
+      if (Unreachable)
+        *Unreachable = true;
       // If we're allowed to diagnose this and recover, do so.
       bool Recover = Complain && !isSFINAEContext();
       if (Complain)
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 1f42f9500959e..398e41b236ae4 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5052,12 +5052,15 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
       PatternDef = nullptr;
   }
 
+  // True is the template definition is unreachable, otherwise false.
+  bool Unreachable = false;
   // FIXME: We need to track the instantiation stack in order to know which
   // definitions should be visible within this instantiation.
-  if (DiagnoseUninstantiableTemplate(PointOfInstantiation, Function,
-                                Function->getInstantiatedFromMemberFunction(),
-                                     PatternDecl, PatternDef, TSK,
-                                     /*Complain*/DefinitionRequired)) {
+  if (DiagnoseUninstantiableTemplate(
+          PointOfInstantiation, Function,
+          Function->getInstantiatedFromMemberFunction(), PatternDecl,
+          PatternDef, TSK,
+          /*Complain*/ DefinitionRequired, &Unreachable)) {
     if (DefinitionRequired)
       Function->setInvalidDecl();
     else if (TSK == TSK_ExplicitInstantiationDefinition ||
@@ -5082,11 +5085,18 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
       if (AtEndOfTU && !getDiagnostics().hasErrorOccurred() &&
           !getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc())) {
         Diag(PointOfInstantiation, diag::warn_func_template_missing)
-          << Function;
-        Diag(PatternDecl->getLocation(), diag::note_forward_template_decl);
-        if (getLangOpts().CPlusPlus11)
-          Diag(PointOfInstantiation, diag::note_inst_declaration_hint)
-              << Function;
+            << Function;
+        if (Unreachable) {
+          // FIXME: would be nice to mention which module the function template
+          // comes from. 
+          Diag(PatternDecl->getLocation(),
+               diag::note_unreachable_template_decl);
+        } else {
+          Diag(PatternDecl->getLocation(), diag::note_forward_template_decl);
+          if (getLangOpts().CPlusPlus11)
+            Diag(PointOfInstantiation, diag::note_inst_declaration_hint)
+                << Function;
+        }
       }
     }
 
diff --git a/clang/test/Modules/Inputs/undefined-template/hoge.h b/clang/test/Modules/Inputs/undefined-template/hoge.h
new file mode 100644
index 0000000000000..c67e85ed06734
--- /dev/null
+++ b/clang/test/Modules/Inputs/undefined-template/hoge.h
@@ -0,0 +1,7 @@
+#pragma once
+
+#include "shared_ptr2.h"
+
+inline void f() {
+  x<int>();
+}
diff --git a/clang/test/Modules/Inputs/undefined-template/module.modulemap b/clang/test/Modules/Inputs/undefined-template/module.modulemap
new file mode 100644
index 0000000000000..89cc78cd5a2de
--- /dev/null
+++ b/clang/test/Modules/Inputs/undefined-template/module.modulemap
@@ -0,0 +1,7 @@
+module hoge {
+  header "hoge.h"
+}
+
+module shared_ptr2 {
+  header "shared_ptr2.h"
+}
diff --git a/clang/test/Modules/Inputs/undefined-template/shared_ptr2.h b/clang/test/Modules/Inputs/undefined-template/shared_ptr2.h
new file mode 100644
index 0000000000000..495cc6fe539c1
--- /dev/null
+++ b/clang/test/Modules/Inputs/undefined-template/shared_ptr2.h
@@ -0,0 +1,4 @@
+#pragma once
+
+template<class T>
+void x() { }
diff --git a/clang/test/Modules/diag-undefined-template.cpp b/clang/test/Modules/diag-undefined-template.cpp
new file mode 100644
index 0000000000000..027984c3125f3
--- /dev/null
+++ b/clang/test/Modules/diag-undefined-template.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++20 -fmodules -fmodules-cache-path=%t -I%S/Inputs/undefined-template \
+// RUN:   -Wundefined-func-template \
+// RUN:   -fimplicit-module-maps  %s 2>&1 | grep "unreachable declaration of template entity is her"
+
+#include "hoge.h"
+
+int main() {
+  f();
+}

ilya-biryukov

LGTM with a few caveats (see the comments)

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

ilya-biryukov · 2025-03-04T10:06:55Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@hokein I think we could also mention the point of instantiation here too.
There are two kinds of problems we can't distinguish:

it is used intentionally and users want to make it reachable/visible (so imported and declared modules would be relevant),

unreachable declaration is used unintentionally and users want to remove the use (in which case instantiation point is relevant).

So I think we want both.

If we pick that path, I think we could probably simplify the code a little too:

Diag(PatternDecl->getLocation(), Unreachable ? diag::note_unreachable_template_decl : diag::note_forward_template_decl); if (getLangOpts().CPlusPlus11) Diag(PointOfInstantiation, diag::note_inst_declaration_hint) << Function;

The note_inst_declaration_hint is about a fix hint which suggests to add an explicit instantiation declaration to suppress this warning, it is not feasible to the unreachable-template-decl case.

ilya-biryukov · 2025-03-04T10:46:20Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

Should we be talking about "visibility" for Clang header modules and reachability for C++20 modules?
I am a little fuzzy on the terminology, but I think there was some distinction between the two we are making for the users.

@ChuanqiXu9, could you advise on the correct wording here?

Yeah, "unreachable" is a formal definition for C++20 modules. But it looks like the method to judge unreachable decl in this patch is the same for C++20 modules. So I feel it might be fine.

Thanks! If you are okay with this, let's keep as is.

Just a follow-up question for a better understanding: I somehow thought we would only check for reachability (as opposed to visibility) only since C++20.
Or should we expect Clang to sometimes complain about reachability in C++17 and below with -fmodules enabled? (The added test uses C++20, but I believe it can also be C++17; but I'm not 100% sure)

should we expect Clang to sometimes complain about reachability in C++17 and below with -fmodules enabled?

I think yes. Clang already does this — in Sema::DiagnoseUninstantiableTemplate, when a reachable function definition is missing, Clang may emit a "missing-import" diagnostic.

However, for this particular testcase, the issue is only triggered with the C++20 standard module feature

But it looks like the method to judge unreachable decl in this patch is the same for C++20 modules.

Yes, this aligns with my reading of the code. The same code is shared between header modules and C++20 modules.

ilya-biryukov

Even more NITs, but still LGTM

ilya-biryukov · 2025-03-04T13:59:12Z

clang/test/Modules/diag-undefined-template.cpp

NIT: could we get the split-file-based test for this?
It's pretty hard to read those tests that happen to depend on many files otherwise.

ilya-biryukov · 2025-03-04T14:04:21Z

clang/test/Modules/diag-undefined-template.cpp

NIT: Maybe use -verify and expected-note@* {{unreachable declaration}}?

I tried this initially, but it didn't work. The reason is that the diagnostics are triggered when building the hoge module, which is imported from the main file. I suspect the module is built in a separate compiler invocation from the main file, and the -verify flag is not propagated to it.

I added a note in the lit test for clarification.

ilya-biryukov · 2025-03-04T14:07:53Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

Thanks! If you are okay with this, let's keep as is.

Just a follow-up question for a better understanding: I somehow thought we would only check for reachability (as opposed to visibility) only since C++20.
Or should we expect Clang to sometimes complain about reachability in C++17 and below with -fmodules enabled? (The added test uses C++20, but I believe it can also be C++17; but I'm not 100% sure)

shafik · 2025-03-08T23:10:10Z

clang/lib/Sema/SemaTemplate.cpp

@AaronBallman I don't think I have seen annotation before, annotating the default value in a function definition.

Are there tools that verify this like we have for argument comments?

Otherwise I don't think these make as much sense, although I sympathize with the sentiment behind them.

On a tangental note, I think having Clangd show the default argument would help to workaround the lack of these annotations, so I filed #130571.

I don't think we have any tools to check these.

Yeah, might as well drop it here otherwise it is a potential to get out of sync in the future.

Removed the default-argument comment here.

AaronBallman

Should this come with a release note as well?

AaronBallman · 2025-03-11T18:16:05Z

clang/lib/Sema/SemaTemplate.cpp

Yeah, might as well drop it here otherwise it is a potential to get out of sync in the future.

hokein · 2025-03-17T12:58:58Z

Should this come with a release note as well?

Yeah, I think so. Done.

…unction templates

hokein force-pushed the fix-invisible-notes branch from ccd5da1 to 5bf873e Compare February 28, 2025 10:01

ilya-biryukov reviewed Feb 28, 2025

View reviewed changes

hokein marked this pull request as ready for review February 28, 2025 14:23

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Feb 28, 2025

hokein changed the title ~~WIP: Improve the -Wundefined-func-template diagnostic note for invisible template functions~~ Improve the -Wundefined-func-template diagnostic note for invisible template functions Feb 28, 2025

ilya-biryukov approved these changes Mar 4, 2025

View reviewed changes

shafik reviewed Mar 8, 2025

View reviewed changes

AaronBallman reviewed Mar 11, 2025

View reviewed changes

hokein added 4 commits March 17, 2025 14:19

Improve the -Wundefined-func-template diagnostic note for invisible f…

184a449

…unction templates

Add a lit test.

63ba6c9

Address review comments

71c8699

Use split-file for the lit test

7a98e78

hokein force-pushed the fix-invisible-notes branch from aaaffe7 to 7a98e78 Compare March 17, 2025 14:33

clang-format

2b19268

hokein merged commit 434ac46 into llvm:main Mar 19, 2025
12 checks passed

hokein deleted the fix-invisible-notes branch March 19, 2025 09:51

Improve the -Wundefined-func-template diagnostic note for invisible template functions #129031

Improve the -Wundefined-func-template diagnostic note for invisible template functions #129031

Uh oh!

Conversation

hokein commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hokein commented Mar 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hokein commented Feb 27, 2025 •

edited

Loading

github-actions bot commented Feb 27, 2025 •

edited

Loading

llvmbot commented Feb 28, 2025 •

edited

Loading