Reapply "[Clang][Sema] Diagnose function/variable templates that shadow their own template parameters (#78274)" #79683

sdkrystian · 2024-01-27T09:11:20Z

Reapplies #78274 with the addition of a default-error warning (strict-primary-template-shadow) that is issued for instances of shadowing which were previously accepted prior to this patch.

I couldn't find an established convention for naming diagnostics related to compatibility with previous versions of clang, so I just used the prefix ext_compat_.

llvmbot · 2024-01-27T09:11:53Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Reapplies #78274 with the addition of a default-error warning (strict-primary-template-shadow) that is issued for instances of shadowing which were previously accepted prior to this patch.

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

6 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
(modified) clang/include/clang/Sema/Sema.h (+1-1)
(modified) clang/lib/Sema/SemaDecl.cpp (+22-9)
(modified) clang/lib/Sema/SemaTemplate.cpp (+6-3)
(modified) clang/test/CXX/temp/temp.res/temp.local/p6.cpp (+18-4)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5330cd9caad801..24d3ada5bfd9df 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -97,6 +97,7 @@ Attribute Changes in Clang
 
 Improvements to Clang's diagnostics
 -----------------------------------
+- Clang now diagnoses function/variable templates that shadow their own template parameters, e.g. ``template<class T> void T();``.
 
 Improvements to Clang's time-trace
 ----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 649de9f47c4619..4e3b20548913ab 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4966,6 +4966,9 @@ def err_template_param_shadow : Error<
   "declaration of %0 shadows template parameter">;
 def ext_template_param_shadow : ExtWarn<
   err_template_param_shadow.Summary>, InGroup<MicrosoftTemplateShadow>;
+def ext_compat_template_param_shadow : ExtWarn<
+  err_template_param_shadow.Summary>, InGroup<
+  DiagGroup<"strict-primary-template-shadow">>, DefaultError;
 def note_template_param_here : Note<"template parameter is declared here">;
 def note_template_param_external : Note<
   "template parameter from hidden source: %0">;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 1f1cbd11ff7358..5c148db48140ed 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8256,7 +8256,7 @@ class Sema final {
                                       TemplateSpecializationKind TSK,
                                       bool Complain = true);
 
-  void DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl);
+  void DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl, bool IssueWarning = false);
   TemplateDecl *AdjustDeclIfTemplate(Decl *&Decl);
 
   NamedDecl *ActOnTypeParameter(Scope *S, bool Typename,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f9bf1d14bdc4f6..de65b3c09c28cf 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -6377,12 +6377,6 @@ NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
   } else if (DiagnoseUnexpandedParameterPack(NameInfo, UPPC_DeclarationType))
     return nullptr;
 
-  // The scope passed in may not be a decl scope.  Zip up the scope tree until
-  // we find one that is.
-  while ((S->getFlags() & Scope::DeclScope) == 0 ||
-         (S->getFlags() & Scope::TemplateParamScope) != 0)
-    S = S->getParent();
-
   DeclContext *DC = CurContext;
   if (D.getCXXScopeSpec().isInvalid())
     D.setInvalidType();
@@ -6506,12 +6500,25 @@ NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
     RemoveUsingDecls(Previous);
   }
 
-  if (Previous.isSingleResult() &&
-      Previous.getFoundDecl()->isTemplateParameter()) {
+  // if (Previous.isSingleResult() &&
+  //    Previous.getFoundDecl()->isTemplateParameter()) {
+  if (auto *TPD = Previous.getAsSingle<NamedDecl>(); TPD && TPD->isTemplateParameter()) {
+    // Older versions of clang allowed the names of function/variable templates
+    // to shadow the names of their template parameters. For the compatibility purposes
+    // we detect such cases and issue a default-to-error warning that can be disabled with
+    // -fno-strict-primary-template-shadow.
+    bool IssueShadowingWarning = false;
+    for (Scope *Inner = S; (Inner->getFlags() & Scope::DeclScope) == 0 ||
+         Inner->isTemplateParamScope(); Inner = Inner->getParent()) {
+      if (IssueShadowingWarning = Inner->isDeclScope(TPD))
+        break;
+    }
+
     // Maybe we will complain about the shadowed template parameter.
     if (!D.isInvalidType())
       DiagnoseTemplateParameterShadow(D.getIdentifierLoc(),
-                                      Previous.getFoundDecl());
+                                      TPD,
+                                      IssueShadowingWarning);
 
     // Just pretend that we didn't see the previous declaration.
     Previous.clear();
@@ -6535,6 +6542,12 @@ NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
   if (getLangOpts().CPlusPlus)
     CheckExtraCXXDefaultArguments(D);
 
+  // The scope passed in may not be a decl scope.  Zip up the scope tree until
+  // we find one that is.
+  while ((S->getFlags() & Scope::DeclScope) == 0 ||
+         (S->getFlags() & Scope::TemplateParamScope) != 0)
+    S = S->getParent();
+
   NamedDecl *New;
 
   bool AddToScope = true;
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 9bfa71dc8bcf1d..4ba8dfc19d2f6d 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -885,7 +885,7 @@ bool Sema::DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
 /// that the template parameter 'PrevDecl' is being shadowed by a new
 /// declaration at location Loc. Returns true to indicate that this is
 /// an error, and false otherwise.
-void Sema::DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl) {
+void Sema::DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl, bool IssueWarning) {
   assert(PrevDecl->isTemplateParameter() && "Not a template parameter");
 
   // C++ [temp.local]p4:
@@ -893,8 +893,11 @@ void Sema::DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl) {
   //   scope (including nested scopes).
   //
   // Make this a warning when MSVC compatibility is requested.
-  unsigned DiagId = getLangOpts().MSVCCompat ? diag::ext_template_param_shadow
-                                             : diag::err_template_param_shadow;
+  unsigned DiagId = getLangOpts().MSVCCompat
+      ? diag::ext_template_param_shadow
+      : (IssueWarning
+          ? diag::ext_compat_template_param_shadow
+          : diag::err_template_param_shadow);
   const auto *ND = cast<NamedDecl>(PrevDecl);
   Diag(Loc, DiagId) << ND->getDeclName();
   NoteTemplateParameterLocation(*ND);
diff --git a/clang/test/CXX/temp/temp.res/temp.local/p6.cpp b/clang/test/CXX/temp/temp.res/temp.local/p6.cpp
index e2aa0ff344291d..00bb35813c39af 100644
--- a/clang/test/CXX/temp/temp.res/temp.local/p6.cpp
+++ b/clang/test/CXX/temp/temp.res/temp.local/p6.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify %s -fcxx-exceptions -std=c++1y
+// RUN: %clang_cc1 -verify %s -fcxx-exceptions -std=c++1y -Wno-error=strict-primary-template-shadow
 
 namespace N {}
 
@@ -127,16 +127,30 @@ template<int T> struct Z { // expected-note 16{{declared here}}
 template<typename T> // expected-note {{declared here}}
 void f(int T) {} // expected-error {{declaration of 'T' shadows template parameter}}
 
-// FIXME: These are ill-formed: a template-parameter shall not have the same name as the template name.
 namespace A {
   template<typename T> struct T {};  // expected-error{{declaration of 'T' shadows template parameter}}
                                      // expected-note@-1{{template parameter is declared here}}
+  template<typename T> struct U {
+    template<typename V> struct V {}; // expected-error{{declaration of 'V' shadows template parameter}}
+                                      // expected-note@-1{{template parameter is declared here}}
+  };
 }
 namespace B {
-  template<typename T> void T() {}
+  template<typename T> void T() {} // expected-warning{{declaration of 'T' shadows template parameter}}
+                                   // expected-note@-1{{template parameter is declared here}}
+
+  template<typename T> struct U {
+    template<typename V> void V(); // expected-warning{{declaration of 'V' shadows template parameter}}
+                                   // expected-note@-1{{template parameter is declared here}}
+  };
 }
 namespace C {
-  template<typename T> int T;
+  template<typename T> int T; // expected-warning{{declaration of 'T' shadows template parameter}}
+                              // expected-note@-1{{template parameter is declared here}}
+  template<typename T> struct U {
+    template<typename V> static int V; // expected-warning{{declaration of 'V' shadows template parameter}}
+                                       // expected-note@-1{{template parameter is declared here}}
+  };
 }
 
 namespace PR28023 {

github-actions · 2024-01-27T09:13:40Z

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

sdkrystian · 2024-01-27T09:13:47Z

Need to run clang-format on this and mention the flag in the release notes, but should be otherwise GTG

clang/lib/Sema/SemaDecl.cpp

cor3ntin · 2024-01-27T10:39:21Z

clang/lib/Sema/SemaTemplate.cpp

@@ -885,16 +885,19 @@ bool Sema::DiagnoseUninstantiableTemplate(SourceLocation PointOfInstantiation,
 /// that the template parameter 'PrevDecl' is being shadowed by a new
 /// declaration at location Loc. Returns true to indicate that this is
 /// an error, and false otherwise.
-void Sema::DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl) {
+void Sema::DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl,
+                                           bool IssueWarning) {


Can this be called ShadowsFunctionOrVariableName or something like that ? It might be clearer

I think the more generic IssueWarning is less ambiguous. We only issue the warning for variable/function templates the shadow their own template parameters. For example:

template<typename T> struct A { template<typename U> void U(); // warning template<typename U> void T(); // error void T(); // error };

ShadowsFunctionOrVariableName establishes the expectation that its value will be true for all the above function declarations (its only true for the first one).

I don't care much about the exact name, although i agree with you my suggestion may not be great. It can be ShadowsNameOfFunctionOrVariableItIsAttachedTo or whatever name you think is best describing of what it does. IssueWarning is a bit too generic, or rather it forces us to think about warning in multiple places and especially someone calling DiagnoseTemplateParameterShadow should understand what the bool parameter does.

I can add javadocs for the parameters that explain the semantics of IssueWarning and the motivating use-case for it. Is that a good compromise?

The javadocs don't help too much because they're in the source file rather than attached to the declaration (so they don't show up in all IDEs, like Visual Studio). We should move them to the header file.

I would recommend we change the logic slightly and name the parameter SupportedAsExtension. Then the diagnostic selection becomes SupportedAsExtension ? (MSVC ? msvc_ext_diag : generic_ext_diag) : err_diag and it's easier for the caller to reason about whether this shadowing should be supported as an extension or not in any given case.

WDYT?

@AaronBallman In VS20222 the javadocs show up even when they are attached to the definition... but I'll move them to the declaration to be on the safe side :)

While I mostly agree with changing the name to SupportedAsExtension, I don't think the change to the diagnostic selection logic is quite right. When MSVC compatibility is enabled, template parameter shadowing is always allowed (read: there are no cases where shadowing would be an error with MSVC compatibility enabled), so we should issue the MSVC specific diagnostic regardless of SupportedAsExtension. SupportedAsExtension should be used to select between the error/extension diagnostic only when MSVC compatibility is not enabled. Perhaps we name it SupportedForCompatibility instead?

@AaronBallman In VS20222 the javadocs show up even when they are attached to the definition... but I'll move them to the declaration to be on the safe side :)

For me, that only happens if the definition is in the same TU as the use. When I tried outside of SemaTemplate.cpp (such as the uses in SemaDecl.cpp), no docs appeared in the tooltip.

While I mostly agree with changing the name to SupportedAsExtension, I don't think the change to the diagnostic selection logic is quite right. When MSVC compatibility is enabled, template parameter shadowing is always allowed (read: there are no cases where shadowing would be an error with MSVC compatibility enabled), so we should issue the MSVC specific diagnostic regardless of SupportedAsExtension.

Good point!

Perhaps we name it SupportedForCompatibility instead?

I could be okay with that; @cor3ntin WDYT?

I applied the suggested changes, so this should be good to go once cor3ntin responds

Ping @cor3ntin

@AaronBallman should I still wait for @cor3ntin to reply?

clang/lib/Sema/SemaTemplate.cpp

sdkrystian · 2024-01-30T15:48:29Z

@cor3ntin Addressed review comments... Please approve if it all checks out :)

sdkrystian · 2024-02-05T15:07:09Z

Ping @cor3ntin

sdkrystian · 2024-02-12T16:38:35Z

Ping @cor3ntin

cor3ntin

LGTM

…ow their own template parameters (llvm#78274)"

sdkrystian requested a review from cor3ntin January 27, 2024 09:11

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 27, 2024

sdkrystian mentioned this pull request Jan 27, 2024

[Clang][Sema] Diagnose function/variable templates that shadow their own template parameters #78274

Merged

cor3ntin reviewed Jan 27, 2024

View reviewed changes

clang/lib/Sema/SemaDecl.cpp Outdated Show resolved Hide resolved

sdkrystian force-pushed the reapply-declarator-tparam-shadow branch from abc8f06 to 3912541 Compare January 27, 2024 10:01

cor3ntin reviewed Jan 27, 2024

View reviewed changes

sdkrystian force-pushed the reapply-declarator-tparam-shadow branch 2 times, most recently from 60e72f2 to a9a6b6e Compare January 30, 2024 15:47

sdkrystian force-pushed the reapply-declarator-tparam-shadow branch from a9a6b6e to 0d4a6d1 Compare February 15, 2024 17:50

cor3ntin approved these changes Mar 4, 2024

View reviewed changes

Reapply "[Clang][Sema] Diagnose function/variable templates that shad…

7d05842

…ow their own template parameters (llvm#78274)"

sdkrystian force-pushed the reapply-declarator-tparam-shadow branch from 0d4a6d1 to 7d05842 Compare March 4, 2024 14:03

sdkrystian merged commit c1d8d0a into llvm:main Mar 4, 2024

Reapply "[Clang][Sema] Diagnose function/variable templates that shadow their own template parameters (#78274)" #79683

Reapply "[Clang][Sema] Diagnose function/variable templates that shadow their own template parameters (#78274)" #79683

Uh oh!

Conversation

sdkrystian commented Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 27, 2024

Uh oh!

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

Uh oh!

Uh oh!

sdkrystian commented Jan 27, 2024

Uh oh!

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!

sdkrystian Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

sdkrystian commented Jan 30, 2024

Uh oh!

sdkrystian commented Feb 5, 2024

Uh oh!

sdkrystian commented Feb 12, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sdkrystian commented Jan 27, 2024 •

edited

Loading

github-actions bot commented Jan 27, 2024 •

edited

Loading

sdkrystian Feb 15, 2024 •

edited

Loading