Skip to content

[Clang][diagnostics] Fix structured binding shadows template param loc #129116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 3, 2025

Conversation

AmrDeveloper
Copy link
Member

Fix structured binding shadows template parameter location

Fixes: #129060

@AmrDeveloper AmrDeveloper added clang Clang issues not falling into any other category clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer labels Feb 27, 2025
@AmrDeveloper AmrDeveloper requested a review from shafik February 27, 2025 20:52
@llvmbot llvmbot added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Feb 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 27, 2025

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

Fix structured binding shadows template parameter location

Fixes: #129060


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+1-2)
  • (modified) clang/test/CXX/temp/temp.res/temp.local/p6.cpp (+5)
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 664d48ccbc382..a3a028b9485d6 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -883,8 +883,7 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator &D,
     // It's not permitted to shadow a template parameter name.
     if (Previous.isSingleResult() &&
         Previous.getFoundDecl()->isTemplateParameter()) {
-      DiagnoseTemplateParameterShadow(D.getIdentifierLoc(),
-                                      Previous.getFoundDecl());
+      DiagnoseTemplateParameterShadow(B.NameLoc, Previous.getFoundDecl());
       Previous.clear();
     }
 
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 00bb35813c39a..e464bb5e7eaef 100644
--- a/clang/test/CXX/temp/temp.res/temp.local/p6.cpp
+++ b/clang/test/CXX/temp/temp.res/temp.local/p6.cpp
@@ -162,3 +162,8 @@ struct A {
 };
 A<0>::B a;
 }
+
+template <typename T> void shadow9() {  // expected-note{{template parameter is declared here}}
+  using arr = int[1]; // expected-warning@+1 {{decomposition declarations are a C++17 extension}}
+  auto [T] = arr{}; // expected-error {{declaration of 'T' shadows template parameter}}
+}

@AmrDeveloper
Copy link
Member Author

@shafik I added the sample to our test cases, not sure if we can add source loc in expected line, I will check that

@shafik
Copy link
Collaborator

shafik commented Feb 27, 2025

@shafik I added the sample to our test cases, not sure if we can add source loc in expected line, I will check that

h/t @erichkeane something like this should help get us the confirmation in the test we want: https://godbolt.org/z/1GfK1K8qf


template <typename T> void shadow9() { // expected-note{{template parameter is declared here}}
using arr = int[1]; // expected-warning@+1 {{decomposition declarations are a C++17 extension}}
auto [T] = arr{}; // expected-error {{declaration of 'T' shadows template parameter}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test case would benefit from the new lines added in the initial issue report - the reported behavior is due to us choosing to report an error based on the wrong part of the declaration, and I don't believe that this test case is sufficient to highlight the problem (or more to the point catch a regression in the behavior)

Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it makes sense to also add a release note.

@@ -214,6 +214,8 @@ Improvements to Clang's diagnostics
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.

- Improve the diagnostics for shadows template parameter.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably mention the bug# as well as slightly better clarify that it is giving a more correct location.

@@ -162,3 +162,10 @@ struct A {
};
A<0>::B a;
}

template <typename T> void shadow() { // expected-note{{template parameter is declared here}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please instantiate this template as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean something like 'template void shadow2() { ... }` or creating a sample with function call

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a call to this function, so: shadow<int>() call. If you change it to return int, you can do: auto Use = shadow<int>() below this to instantiate it.

@@ -162,3 +162,10 @@ struct A {
};
A<0>::B a;
}

template <typename T> void shadow() { // expected-note{{template parameter is declared here}}
using arr = int[1]; // expected-warning@+1 {{decomposition declarations are a C++17 extension}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using arr = int[1]; // expected-warning@+1 {{decomposition declarations are a C++17 extension}}
using arr = int[1];
// expected-warning@+1 {{decomposition declarations are a C++17 extension}}

@AmrDeveloper AmrDeveloper merged commit af464c6 into llvm:main Mar 3, 2025
12 checks passed
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
llvm#129116)

Fix structured binding shadows template parameter location

Fixes: llvm#129060
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang][diagnostics] The location marking that structured binding shadows template parameter is not clear
6 participants