Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[clang] -Wshadow option produces a warning when explicitly capturing class member using the same name #71976

Closed
flotsamarch opened this issue Nov 10, 2023 · 6 comments · Fixed by #74512
Closed

[clang] -Wshadow option produces a warning when explicitly capturing class member using the same name #71976

flotsamarch opened this issue Nov 10, 2023 · 6 comments · Fixed by #74512
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer

Comments

@flotsamarch
Copy link

Starting from Clang 17.0.0 (built from source, but this also reproduces on Compiler Explorer) using -Wshadow flag now produces diagnostics for this code:

struct A
{
    int b = 5;

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

I couldn't find this change in changelogs and there is no test for this type of capture in warn-shadow-in-lambdas.cpp, so I think this is a bug that was introduced by the commit that fixes #61105.

While technically this might be the case of 'shadowing' (I can't make any references to standard that either support or contradict my opinion), I don't think it has ever been considered a 'shadowing' case which should produce diagnostics and has been a somewhat common C++ pattern.

I don't see much value in that warning, since the name b referring to original class member is not available inside lambda body, and and the name b referring to the lambda member becomes available only when the other b is already not. Should it be considered shadowing?

Expected behavior: No warning generated
Actual behavior: warning: declaration shadows a field of 'A' [-Wshadow] diagnostics produced
@github-actions github-actions bot added the clang Clang issues not falling into any other category label Nov 10, 2023
@EugeneZelenko EugeneZelenko added clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer and removed clang Clang issues not falling into any other category labels Nov 10, 2023
@myrrc
Copy link

myrrc commented Nov 24, 2023 •
edited
Loading

This also reproduces with me on clang 15 and 16:

struct A {
  int a;
  void foo() {
    auto b = [a = this->a] {
 
    };
  }
};

In this scenario, a isn't available in lambda without capturing this so there is no real shadowing. Unfortunately, our project https://github.com/ClickHouse/ClickHouse/ is also built with -Werror so workarounds like [a1 = this->a] have to be written

@shafik
Copy link
Collaborator

shafik commented Nov 28, 2023

CC @Fznamznon

@Rinn
Copy link

Rinn commented Nov 28, 2023

We're also encountering this in the UnrealEngine codebase as many of our compiles are done with -Wshadow and this has caused hundreds of new warnings. At the very least if this could be put on a separate warning flag that would be really appreciated, for the moment this will block us from updating clang as we don't want to disable -Wshadow entirely to work around it.

@Fznamznon
Copy link
Contributor

I don't think it is 0fb84bc that is guilty since it adds an orthogonal error message, but I can take a look anyway.

@Fznamznon
Copy link
Contributor

Fznamznon commented Dec 5, 2023 •
edited
Loading

The new warning appeared due to https://reviews.llvm.org/D124351 . It added a new call to CheckShadow here:

llvm-project/clang/lib/Sema/SemaLambda.cpp

Line 1155 in 410bf5e

CheckShadow(CurrentScope, V);

I don't fully understand why it is required.
If it is, I wonder if we should put this new warning under -Wshadow-uncaptured-local like it happens in case of a function parameter or just a variable (not field decl) - https://godbolt.org/z/GWY9qe79r. Or for example check that the shadowed variable/field can't be referenced from lambda body anyway (due to it or this not being captured explicitly or not).
@cor3ntin I was wondering if you could help to understand the best way to fix, please?

@cor3ntin
Copy link
Contributor

cor3ntin commented Dec 5, 2023

CheckShadow fails to filter out fields for captures.

--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -8351,10 +8351,12 @@ void Sema::CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl,
 
   unsigned WarningDiag = diag::warn_decl_shadow;
   SourceLocation CaptureLoc;
-  if (isa<VarDecl>(D) && isa<VarDecl>(ShadowedDecl) && 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());

I was able to silence the warning with something along the lines of that.
I am not sure I will have time to submit a PR today so feel free to look more into it if you have time

Fznamznon added a commit to Fznamznon/llvm-project that referenced this issue Dec 5, 2023
@Fznamznon
[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 Fznamznon mentioned this issue Dec 5, 2023
Merged
@Wayonb Wayonb mentioned this issue Feb 9, 2024
Open
Fznamznon added a commit that referenced this issue Feb 12, 2024
@Fznamznon
[clang] Avoid -Wshadow warning when init-capture named same as class …
c90114c 
…field (#74512)

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

Fixes #71976
sheredom pushed a commit to sheredom/llvm-project that referenced this issue Mar 12, 2024
@Fznamznon @sheredom
[clang] Avoid -Wshadow warning when init-capture named same as class …
d2740d7 
…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
@sheredom sheredom mentioned this issue Mar 12, 2024
Merged
tstellar pushed a commit to sheredom/llvm-project that referenced this issue Apr 2, 2024
@Fznamznon @tstellar
[clang] Avoid -Wshadow warning when init-capture named same as class …
c13b748 
…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
wanghao75 pushed a commit to openeuler-mirror/llvm-project that referenced this issue Apr 16, 2024
[backport][clang] Avoid -Wshadow warning when init-capture named same
5491555 
 as class field

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

Fixes llvm/llvm-project#71976
xgupta pushed a commit to xgupta/llvm-project that referenced this issue Sep 9, 2024
@Fznamznon
[clang] Avoid -Wshadow warning when init-capture named same as class …
b7db5a0 
…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
@externl externl mentioned this issue Sep 18, 2024
Closed
xgupta pushed a commit to xgupta/llvm-project that referenced this issue Oct 10, 2024
@Fznamznon @xgupta
[clang] Avoid -Wshadow warning when init-capture named same as class …
5a7580e 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[clang] Avoid -Wshadow warning when init-capture named same as class field Fznamznon/llvm-project
7 participants
@Rinn @cor3ntin @shafik @myrrc @EugeneZelenko @Fznamznon @flotsamarch