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

Author: sdkrystian

clang/docs/ReleaseNotes.rst

@@ -42,6 +42,8 @@ C/C++ Language Potentially Breaking Changes
 
 C++ Specific Potentially Breaking Changes
 -----------------------------------------
+- Clang now diagnoses function/variable templates that shadow their own template parameters, e.g. ``template<class T> void T();``.
+  This error can be disabled via `-Wno-strict-primary-template-shadow` for compatibility with previous versions of clang.
 
 ABI Changes in This Version
 ---------------------------

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">;

clang/include/clang/Sema/Scope.h

@@ -200,6 +200,10 @@ class Scope {
   /// other template parameter scopes as parents.
   Scope *TemplateParamParent;
 
+  /// DeclScopeParent - This is a direct link to the immediately containing
+  /// DeclScope, i.e. scope which can contain declarations.
+  Scope *DeclParent;
+
   /// DeclsInScope - This keeps track of all declarations in this scope.  When
   /// the declaration is added to the scope, it is set as the current
   /// declaration for the identifier in the IdentifierTable.  When the scope is
@@ -299,6 +303,9 @@ class Scope {
   Scope *getTemplateParamParent() { return TemplateParamParent; }
   const Scope *getTemplateParamParent() const { return TemplateParamParent; }
 
+  Scope *getDeclParent() { return DeclParent; }
+  const Scope *getDeclParent() const { return DeclParent; }
+
   /// Returns the depth of this scope. The translation-unit has scope depth 0.
   unsigned getDepth() const { return Depth; }
 

clang/include/clang/Sema/Sema.h

@@ -8315,7 +8315,8 @@ 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,

clang/lib/Sema/Scope.cpp

@@ -37,6 +37,7 @@ void Scope::setFlags(Scope *parent, unsigned flags) {
     FnParent       = parent->FnParent;
     BlockParent    = parent->BlockParent;
     TemplateParamParent = parent->TemplateParamParent;
+    DeclParent = parent->DeclParent;
     MSLastManglingParent = parent->MSLastManglingParent;
     MSCurManglingNumber = getMSLastManglingNumber();
     if ((Flags & (FnScope | ClassScope | BlockScope | TemplateParamScope |
@@ -52,6 +53,7 @@ void Scope::setFlags(Scope *parent, unsigned flags) {
     PrototypeIndex = 0;
     MSLastManglingParent = FnParent = BlockParent = nullptr;
     TemplateParamParent = nullptr;
+    DeclParent = nullptr;
     MSLastManglingNumber = 1;
     MSCurManglingNumber = 1;
   }
@@ -76,6 +78,7 @@ void Scope::setFlags(Scope *parent, unsigned flags) {
     PrototypeDepth++;
 
   if (flags & DeclScope) {
+    DeclParent = this;
     if (flags & FunctionPrototypeScope)
       ; // Prototype scopes are uninteresting.
     else if ((flags & ClassScope) && getParent()->isClassScope())

clang/lib/Sema/SemaDecl.cpp

@@ -6359,12 +6359,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();
@@ -6488,12 +6482,22 @@ NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
     RemoveUsingDecls(Previous);
   }
 
-  if (Previous.isSingleResult() &&
-      Previous.getFoundDecl()->isTemplateParameter()) {
-    // Maybe we will complain about the shadowed template parameter.
-    if (!D.isInvalidType())
-      DiagnoseTemplateParameterShadow(D.getIdentifierLoc(),
-                                      Previous.getFoundDecl());
+  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 -Wno-strict-primary-template-shadow.
+    if (!D.isInvalidType()) {
+      bool IssueShadowWarning = false;
+      if (Scope *DeclParent = S->getDeclParent();
+          Scope *TemplateParamParent = S->getTemplateParamParent()) {
+        IssueShadowWarning = DeclParent->Contains(*TemplateParamParent) &&
+                             TemplateParamParent->isDeclScope(TPD);
+      }
+      DiagnoseTemplateParameterShadow(D.getIdentifierLoc(), TPD,
+                                      IssueShadowWarning);
+    }
 
     // Just pretend that we didn't see the previous declaration.
     Previous.clear();
@@ -6517,6 +6521,9 @@ NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
   if (getLangOpts().CPlusPlus)
     CheckExtraCXXDefaultArguments(D);
 
+  /// Get the innermost enclosing declaration scope.
+  S = S->getDeclParent();
+
   NamedDecl *New;
 
   bool AddToScope = true;

clang/lib/Sema/SemaDeclCXX.cpp

@@ -12192,10 +12192,8 @@ Decl *Sema::ActOnUsingDirective(Scope *S, SourceLocation UsingLoc,
   assert(NamespcName && "Invalid NamespcName.");
   assert(IdentLoc.isValid() && "Invalid NamespceName location.");
 
-  // This can only happen along a recovery path.
-  while (S->isTemplateParamScope())
-    S = S->getParent();
-  assert(S->getFlags() & Scope::DeclScope && "Invalid Scope.");
+  // Get the innermost enclosing declaration scope.
+  S = S->getDeclParent();
 
   UsingDirectiveDecl *UDir = nullptr;
   NestedNameSpecifier *Qualifier = nullptr;
@@ -13503,11 +13501,8 @@ Decl *Sema::ActOnAliasDeclaration(Scope *S, AccessSpecifier AS,
                                   SourceLocation UsingLoc, UnqualifiedId &Name,
                                   const ParsedAttributesView &AttrList,
                                   TypeResult Type, Decl *DeclFromDeclSpec) {
-  // Skip up to the relevant declaration scope.
-  while (S->isTemplateParamScope())
-    S = S->getParent();
-  assert((S->getFlags() & Scope::DeclScope) &&
-         "got alias-declaration outside of declaration scope");
+  // Get the innermost enclosing declaration scope.
+  S = S->getDeclParent();
 
   if (Type.isInvalid())
     return nullptr;

clang/lib/Sema/SemaTemplate.cpp

@@ -885,16 +885,31 @@ 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) {
+///
+/// \param Loc The location of the declaration that shadows a template
+///            parameter.
+///
+/// \param PrevDecl The template parameter that the declaration shadows.
+///
+/// \param IssueWarning Whether to issue the diagnostic as a warning for
+///        compatibility with older versions of clang. Ignored when MSVC
+///        compatibility is enabled.
+void Sema::DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl,
+                                           bool IssueWarning) {
   assert(PrevDecl->isTemplateParameter() && "Not a template parameter");
 
-  // C++ [temp.local]p4:
-  //   A template-parameter shall not be redeclared within its
-  //   scope (including nested scopes).
+  // C++23 [temp.local]p6:
+  //   The name of a template-parameter shall not be bound to any following.
+  //   declaration whose locus is contained by the scope to which the
+  //   template-parameter belongs.
   //
-  // Make this a warning when MSVC compatibility is requested.
-  unsigned DiagId = getLangOpts().MSVCCompat ? diag::ext_template_param_shadow
-                                             : diag::err_template_param_shadow;
+  // When MSVC compatibility is enabled, the diagnostic is always a warning
+  // by default. Otherwise, the diagnostic is an error unless IssueWarning
+  // is true, in which case it is a default-to-error warning.
+  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);
@@ -8501,9 +8516,7 @@ Sema::CheckTemplateDeclScope(Scope *S, TemplateParameterList *TemplateParams) {
     return false;
 
   // Find the nearest enclosing declaration scope.
-  while ((S->getFlags() & Scope::DeclScope) == 0 ||
-         (S->getFlags() & Scope::TemplateParamScope) != 0)
-    S = S->getParent();
+  S = S->getDeclParent();
 
   // C++ [temp.pre]p6: [P2096]
   //   A template, explicit specialization, or partial specialization shall not
@@ -10572,11 +10585,8 @@ DeclResult Sema::ActOnExplicitInstantiation(Scope *S,
     return true;
   }
 
-  // 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();
+  // Get the innermost enclosing declaration scope.
+  S = S->getDeclParent();
 
   // Determine the type of the declaration.
   TypeSourceInfo *T = GetTypeForDeclarator(D);

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 {