[clang] Avoid -Wshadow warning when init-capture named same as class field #74512

Fznamznon · 2023-12-05T18:35:39Z

Shadowing warning doesn't make much sense since field is not available in lambda's body without capturing this.

Fixes #71976

…field Shadowing warning doesn't make much sense since field is not available in lambda's body without capturing this. Fixes llvm#71976

llvmbot · 2023-12-05T18:36:10Z

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

Shadowing warning doesn't make much sense since field is not available in lambda's body without capturing this.

Fixes #71976

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/lib/Sema/SemaDecl.cpp (+5-3)
(modified) clang/test/SemaCXX/warn-shadow-in-lambdas.cpp (+18)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 828dd10e3d6db..7ac81e16492d1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -655,6 +655,9 @@ Bug Fixes in This Version
   Fixes (`#64467 <https://github.com/llvm/llvm-project/issues/64467>`_)
 - Clang's ``-Wchar-subscripts`` no longer warns on chars whose values are known non-negative constants.
   Fixes (`#18763 <https://github.com/llvm/llvm-project/issues/18763>`_)
+- Clang's ``-Wshadow`` no longer warns when init-capture named same as class
+  field.
+  Fixes (`#71976 <https://github.com/llvm/llvm-project/issues/71976>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index f12424d33b7da..65d095b2431dd 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -8395,10 +8395,11 @@ void Sema::CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl,
 
   unsigned WarningDiag = diag::warn_decl_shadow;
   SourceLocation CaptureLoc;
-  if (isa<VarDecl>(D) && isa<VarDecl>(ShadowedDecl) && NewDC &&
-      isa<CXXMethodDecl>(NewDC)) {
+  if (isa<VarDecl>(D) && NewDC && isa<CXXMethodDecl>(NewDC)) {
     if (const auto *RD = dyn_cast<CXXRecordDecl>(NewDC->getParent())) {
       if (RD->isLambda() && OldDC->Encloses(NewDC->getLexicalParent())) {
+        if (!isa<VarDecl>(ShadowedDecl))
+          return;
         if (RD->getLambdaCaptureDefault() == LCD_None) {
           // Try to avoid warnings for lambdas with an explicit capture list.
           const auto *LSI = cast<LambdaScopeInfo>(getCurFunction());
@@ -8416,7 +8417,8 @@ void Sema::CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl,
         }
       }
 
-      if (cast<VarDecl>(ShadowedDecl)->hasLocalStorage()) {
+      if (const auto *VD = dyn_cast<VarDecl>(ShadowedDecl);
+          VD && VD->hasLocalStorage()) {
         // A variable can't shadow a local variable in an enclosing scope, if
         // they are separated by a non-capturing declaration context.
         for (DeclContext *ParentDC = NewDC;
diff --git a/clang/test/SemaCXX/warn-shadow-in-lambdas.cpp b/clang/test/SemaCXX/warn-shadow-in-lambdas.cpp
index bda6a65c02168..58af7a2e65c55 100644
--- a/clang/test/SemaCXX/warn-shadow-in-lambdas.cpp
+++ b/clang/test/SemaCXX/warn-shadow-in-lambdas.cpp
@@ -179,3 +179,21 @@ void f() {
 #endif
 }
 }
+
+namespace GH71976 {
+struct A {
+  int b = 5;
+  int foo() {
+    return [b = b]() { return b; }();
+  }
+};
+
+struct B {
+  int a;
+  void foo() {
+    auto b = [a = this->a] {
+
+    };
+  }
+};
+}

cor3ntin

LGTM modulo nitpick.
Thanks a lot for the quick fix

clang/docs/ReleaseNotes.rst

shafik

LGTM w/ minor comments

clang/test/SemaCXX/warn-shadow-in-lambdas.cpp

shafik · 2023-12-05T21:56:40Z

clang/lib/Sema/SemaDecl.cpp

    if (const auto *RD = dyn_cast<CXXRecordDecl>(NewDC->getParent())) {
      if (RD->isLambda() && OldDC->Encloses(NewDC->getLexicalParent())) {
+        if (!isa<VarDecl>(ShadowedDecl))


Maybe a comment here along the lines of "if it is not VarDecl then it can not shadow" I am not sure if that implies it must be a capture but I think it does.

You mean add a comment saying that it is a case when an init-capture references class field?

Yeah, just a comment to explain why ShadownDecl not being a VarDecl matters here.

I changed this all, so probably not relevant anymore.

philnik777 · 2023-12-11T10:07:30Z

I seem to be the only one, but I don't understand how this isn't shadowing. The whole point of the shadowing warning is to avoid confusion about which variable is actually used, which IMO could very well be the case here. I think it would be much better to get a new flag to disable this if it's not wanted than to make it impossible to warn on this shadowing constellation.

Fznamznon · 2023-12-11T10:55:50Z

The whole point of the shadowing warning is to avoid confusion about which variable is actually used, which IMO could very well be the case here.

In the case that is being fixed, a field is not available in lambda's body without capturing this, so there is no such confusion and no shadowing, I think.
I have a concern, that probably in case when this is actually captured, with current implementation there will be no warning as well. Should it be? gcc doesn't give a warning in both cases.

Fznamznon · 2023-12-12T10:01:35Z

@cor3ntin , @shafik WDYT about

I have a concern, that probably in case when this is actually captured, with current implementation of the patch there will be no warning as well.

Should we still emit shadowing warning in case lambda captured this and has an init-capture with the same name as a class field?

cor3ntin · 2024-01-11T16:17:35Z

@philnik777 I agree strongly with @Fznamznon.
If this is not captured there is no actual shadowing in that they do not occupy encompassing scope, and so removing one declaration would not give you access to the other variable.

Should we still emit shadowing warning in case lambda captured this and has an init-capture with the same name as a class field?

Yes, we should probably do that.

(sorry for the very delayed reply, it fell off my radar)

philnik777 · 2024-01-11T16:31:50Z

@philnik777 I agree strongly with @Fznamznon. If this is not captured there is no actual shadowing in that they do not occupy encompassing scope, and so removing one declaration would not give you access to the other variable.

I don't disagree that this is technically not shadowing, but I don't think that's clear from a users perspective. e.g.

void someFunc(int someVal) {
  // assume 100 LoC here

  auto someLambda = [](int someVal) {
    // another 100 LoC here
    return someVal + 40; // IMO it's trivial to confuse this with the `someVal` from `someFunc`
  };
}

It looks like Clang even has a warning for this specific case and GCC considers it shadowing too: https://godbolt.org/z/Y16njoPxh, which makes me think that people want this - at least as a separate flag.

Fznamznon · 2024-01-11T17:41:44Z

It looks like Clang even has a warning for this specific case and GCC considers it shadowing too: https://godbolt.org/z/Y16njoPxh, which makes me think that people want this - at least as a separate flag.

I already came across -Wshadow-uncaptured-local flag earlier - #71976 (comment) . I don't mind putting the warning that I'm avoiding with the current patch under this flag as well.

Fznamznon · 2024-01-18T17:18:40Z

Should we still emit shadowing warning in case lambda captured this and has an init-capture with the same name as a class field?
Yes, we should probably do that.

I'm struggling with detecting the following case:

struct A {
  int b = 5;

  int foo() {
    return [b = b, this]() { return b; }();
  }
};

Since init capture is checked for shadowing before this capture reached, lambda info doesn't yet know about incoming this capture. There is delayed shadowing reporting for lambdas in DiagnoseShadowingLambdaDecls though, looking there.

…local

Fznamznon · 2024-01-26T17:41:17Z

Ok, now it emits -Wshadow-uncaptured-local when lambda doesn't capture this, and -Wshadow when it does.

Fznamznon · 2024-02-02T10:37:57Z

Ping.

Fznamznon · 2024-02-08T12:18:55Z

Ping!

cor3ntin · 2024-02-09T11:00:25Z

clang/lib/Sema/SemaDecl.cpp

    if (const auto *RD = dyn_cast<CXXRecordDecl>(NewDC->getParent())) {
      if (RD->isLambda() && OldDC->Encloses(NewDC->getLexicalParent())) {
-        if (RD->getLambdaCaptureDefault() == LCD_None) {
-          // Try to avoid warnings for lambdas with an explicit capture list.
+        if (const auto *VD = dyn_cast<VarDecl>(ShadowedDecl)) {


Structured bindings aren't always VarDecl (But ValueDecl) so I guess we never diagnosed them !

Well, If we want to, I would prefer doing that in a separate PR.

We currently warn on that actually
https://compiler-explorer.com/z/d4W37n9Mn

Can you test that we still do?

The rest of the changes LGTM

Thanks!
It surprises me that it works, but great!

clang/lib/Sema/SemaDecl.cpp

clang/test/SemaCXX/warn-shadow-in-lambdas.cpp

clang/lib/Sema/SemaDecl.cpp

cor3ntin · 2024-02-09T11:04:31Z

cc @erichkeane @AaronBallman

erichkeane · 2024-02-09T14:22:32Z

cc @erichkeane @AaronBallman

I looked through and don't see anything of concern to me that you haven't covered. I am on the fence as to whether this warning makes sense to have in this situation.

The point of the warning isn't that the language has a problem, it is that it is potentially confusing to readers. And I'm not sure that it STOPS being confusing because of the lack of a capture of 'this'.

Fznamznon · 2024-02-09T17:09:13Z

And I'm not sure that it STOPS being confusing because of the lack of a capture of 'this'.

For that reason, I didn't fully disable the warning, and put it under -Wshadow-uncaptured-local flag instead.

Relaxes incorrect shadowing warning for fields detected as being shadowed in lambdas that don't capture `this`.

…field (llvm#74512) Shadowing warning doesn't make much sense since field is not available in lambda's body without capturing this. Fixes llvm#71976

[clang] Avoid -Wshadow warning when init-capture named same as class …

169d962

…field Shadowing warning doesn't make much sense since field is not available in lambda's body without capturing this. Fixes llvm#71976

Fznamznon requested a review from cor3ntin December 5, 2023 18:35

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 5, 2023

cor3ntin approved these changes Dec 5, 2023

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

shafik approved these changes Dec 5, 2023

View reviewed changes

Fznamznon added 4 commits December 6, 2023 02:21

Merge branch 'main' into 71976

621cb81

Apply comments

3798e1f

Merge branch 'main' into 71976

d1426da

Remove comment

3f01cd3

Fznamznon requested a review from shafik December 7, 2023 10:49

Merge branch 'main' into 71976

81b9def

Fznamznon added 2 commits January 26, 2024 09:04

Merge branch 'main' into 71976

d37e959

Emit -Wshadow when captured this, otherwise emit -Wshadow-uncaptured-…

6b95d07

…local

Fznamznon requested review from cor3ntin and philnik777 January 26, 2024 17:41

Merge branch 'main' into 71976

e32d441

cor3ntin reviewed Feb 9, 2024

View reviewed changes

Merge branch 'main' into 71976

2b49655

Apply comments

ea6bdd4

Fznamznon requested a review from cor3ntin February 9, 2024 17:18

Add structured binding test.

4b25344

cor3ntin approved these changes Feb 9, 2024

View reviewed changes

erichkeane approved these changes Feb 9, 2024

View reviewed changes

Fznamznon merged commit c90114c into llvm:main Feb 12, 2024

jeremy-wildlight added a commit to wildlight-entertainment/llvm-project that referenced this pull request Feb 15, 2024

Apply upstream patch provided by llvm#74512

5abaa4a

Relaxes incorrect shadowing warning for fields detected as being shadowed in lambdas that don't capture `this`.

sheredom mentioned this pull request Mar 12, 2024

Backport shadowing warnings to a LLVM 18 release #84898

Closed

sheredom mentioned this pull request Mar 12, 2024

release/18x: [clang] Avoid -Wshadow warning when init-capture named same as class … #84912

Merged

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

[clang] Avoid -Wshadow warning when init-capture named same as class field #74512

[clang] Avoid -Wshadow warning when init-capture named same as class field #74512

Uh oh!

Conversation

Fznamznon commented Dec 5, 2023

Uh oh!

llvmbot commented Dec 5, 2023

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

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!

philnik777 commented Dec 11, 2023

Uh oh!

Fznamznon commented Dec 11, 2023

Uh oh!

Fznamznon commented Dec 12, 2023

Uh oh!

cor3ntin commented Jan 11, 2024

Uh oh!

philnik777 commented Jan 11, 2024

Uh oh!

Fznamznon commented Jan 11, 2024

Uh oh!

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

Uh oh!

Uh oh!

Fznamznon commented Jan 26, 2024

Uh oh!

Fznamznon commented Feb 2, 2024

Uh oh!

Fznamznon commented Feb 8, 2024

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!

Uh oh!

Uh oh!

cor3ntin commented Feb 9, 2024

Uh oh!

erichkeane commented Feb 9, 2024

Uh oh!

Fznamznon commented Feb 9, 2024

Uh oh!

Uh oh!

Fznamznon commented Jan 18, 2024 •

edited

Loading