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.

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

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

Merged
merged 12 commits into from
Feb 12, 2024
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ Bug Fixes in This Version
a member class template for an implicit instantiation of a class template.

- Fixed missing warnings when doing bool-like conversions in C23 (`#79435 <https://github.com/llvm/llvm-project/issues/79435>`_).
- Clang's ``-Wshadow`` no longer warns when an init-capture is named the same as
a class field unless the lambda can capture this.
Fixes (`#71976 <https://github.com/llvm/llvm-project/issues/71976>`_)

- Clang now accepts qualified partial/explicit specializations of variable templates that
are not nominable in the lookup context of the specialization.
Expand Down
4 changes: 2 additions & 2 deletions clang/include/clang/Sema/ScopeInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -925,8 +925,8 @@ class LambdaScopeInfo final :
/// that were defined in parent contexts. Used to avoid warnings when the
/// shadowed variables are uncaptured by this lambda.
struct ShadowedOuterDecl {
const VarDecl *VD;
const VarDecl *ShadowedDecl;
const NamedDecl *VD;
const NamedDecl *ShadowedDecl;
};
llvm::SmallVector<ShadowedOuterDecl, 4> ShadowingDecls;

Expand Down
73 changes: 47 additions & 26 deletions clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8357,28 +8357,40 @@ 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 (RD->getLambdaCaptureDefault() == LCD_None) {
// Try to avoid warnings for lambdas with an explicit capture list.
if (const auto *VD = dyn_cast<VarDecl>(ShadowedDecl)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, added.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const auto *LSI = cast<LambdaScopeInfo>(getCurFunction());
// Warn only when the lambda captures the shadowed decl explicitly.
CaptureLoc = getCaptureLocation(LSI, cast<VarDecl>(ShadowedDecl));
if (CaptureLoc.isInvalid())
WarningDiag = diag::warn_decl_shadow_uncaptured_local;
} else {
// Remember that this was shadowed so we can avoid the warning if the
// shadowed decl isn't captured and the warning settings allow it.
if (RD->getLambdaCaptureDefault() == LCD_None) {
// Try to avoid warnings for lambdas with an explicit capture
// list. Warn only when the lambda captures the shadowed decl
// explicitly.
CaptureLoc = getCaptureLocation(LSI, VD);
if (CaptureLoc.isInvalid())
WarningDiag = diag::warn_decl_shadow_uncaptured_local;
} else {
// Remember that this was shadowed so we can avoid the warning if
// the shadowed decl isn't captured and the warning settings allow
// it.
cast<LambdaScopeInfo>(getCurFunction())
->ShadowingDecls.push_back({D, VD});
return;
}
}
if (isa<FieldDecl>(ShadowedDecl)) {
// If lambda can capture this, then emit default shadowing warning,
// Otherwise it is not really a shadowing case since field is not
// available in lambda's body.
// At this point we don't know that lambda can capture this, so
// remember that this was shadowed and delay until we know.
cast<LambdaScopeInfo>(getCurFunction())
->ShadowingDecls.push_back(
{cast<VarDecl>(D), cast<VarDecl>(ShadowedDecl)});
->ShadowingDecls.push_back({D, ShadowedDecl});
return;
}
}

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;
Expand Down Expand Up @@ -8429,19 +8441,28 @@ void Sema::CheckShadow(NamedDecl *D, NamedDecl *ShadowedDecl,
/// when these variables are captured by the lambda.
void Sema::DiagnoseShadowingLambdaDecls(const LambdaScopeInfo *LSI) {
for (const auto &Shadow : LSI->ShadowingDecls) {
const VarDecl *ShadowedDecl = Shadow.ShadowedDecl;
const NamedDecl *ShadowedDecl = Shadow.ShadowedDecl;
// Try to avoid the warning when the shadowed decl isn't captured.
SourceLocation CaptureLoc = getCaptureLocation(LSI, ShadowedDecl);
const DeclContext *OldDC = ShadowedDecl->getDeclContext();
Diag(Shadow.VD->getLocation(), CaptureLoc.isInvalid()
? diag::warn_decl_shadow_uncaptured_local
: diag::warn_decl_shadow)
<< Shadow.VD->getDeclName()
<< computeShadowedDeclKind(ShadowedDecl, OldDC) << OldDC;
if (!CaptureLoc.isInvalid())
Diag(CaptureLoc, diag::note_var_explicitly_captured_here)
<< Shadow.VD->getDeclName() << /*explicitly*/ 0;
Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration);
if (const auto *VD = dyn_cast<VarDecl>(ShadowedDecl)) {
SourceLocation CaptureLoc = getCaptureLocation(LSI, VD);
Diag(Shadow.VD->getLocation(),
CaptureLoc.isInvalid() ? diag::warn_decl_shadow_uncaptured_local
: diag::warn_decl_shadow)
<< Shadow.VD->getDeclName()
<< computeShadowedDeclKind(ShadowedDecl, OldDC) << OldDC;
if (CaptureLoc.isValid())
Diag(CaptureLoc, diag::note_var_explicitly_captured_here)
<< Shadow.VD->getDeclName() << /*explicitly*/ 0;
Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration);
} else if (isa<FieldDecl>(ShadowedDecl)) {
Diag(Shadow.VD->getLocation(),
LSI->isCXXThisCaptured() ? diag::warn_decl_shadow
: diag::warn_decl_shadow_uncaptured_local)
<< Shadow.VD->getDeclName()
<< computeShadowedDeclKind(ShadowedDecl, OldDC) << OldDC;
Diag(ShadowedDecl->getLocation(), diag::note_previous_declaration);
}
}
}

Expand Down
92 changes: 89 additions & 3 deletions clang/test/SemaCXX/warn-shadow-in-lambdas.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow -D AVOID %s
// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow -Wshadow-uncaptured-local %s
// RUN: %clang_cc1 -std=c++14 -verify -fsyntax-only -Wshadow-all %s
// RUN: %clang_cc1 -std=c++14 -verify=expected,cxx14 -fsyntax-only -Wshadow -D AVOID %s
// RUN: %clang_cc1 -std=c++14 -verify=expected,cxx14 -fsyntax-only -Wshadow -Wshadow-uncaptured-local %s
// RUN: %clang_cc1 -std=c++14 -verify=expected,cxx14 -fsyntax-only -Wshadow-all %s
// RUN: %clang_cc1 -std=c++17 -verify -fsyntax-only -Wshadow-all %s
// RUN: %clang_cc1 -std=c++20 -verify -fsyntax-only -Wshadow-all %s

Expand Down Expand Up @@ -179,3 +179,89 @@ void f() {
#endif
}
}

namespace GH71976 {
#ifdef AVOID
struct A {
int b = 5;
int foo() {
return [b = b]() { return b; }(); // no -Wshadow diagnostic, init-capture does not shadow b due to not capturing this
}
};

struct B {
int a;
void foo() {
auto b = [a = this->a] {}; // no -Wshadow diagnostic, init-capture does not shadow a due to not capturing his
}
};

struct C {
int b = 5;
int foo() {
return [a = b]() {
return [=, b = a]() { // no -Wshadow diagnostic, init-capture does not shadow b due to outer lambda
return b;
}();
Fznamznon marked this conversation as resolved.
Show resolved Hide resolved
}();
}
};

#else
struct A {
int b = 5; // expected-note {{previous}}
int foo() {
return [b = b]() { return b; }(); // expected-warning {{declaration shadows a field}}
}
};

struct B {
int a; // expected-note {{previous}}
void foo() {
auto b = [a = this->a] {}; // expected-warning {{declaration shadows a field}}
}
};

struct C {
int b = 5; // expected-note {{previous}}
int foo() {
return [a = b]() {
return [=, b = a]() { // expected-warning {{declaration shadows a field}}
return b;
}();
}();
}
};

struct D {
int b = 5; // expected-note {{previous}}
int foo() {
return [b = b, this]() { return b; }(); // expected-warning {{declaration shadows a field}}
}
};

struct E {
int b = 5;
int foo() {
return [a = b]() { // expected-note {{previous}}
return [=, a = a]() { // expected-warning {{shadows a local}}
return a;
}();
}();
}
};

#endif

struct S {
int a ;
};

int foo() {
auto [a] = S{0}; // expected-note {{previous}} \
// cxx14-warning {{decomposition declarations are a C++17 extension}}
[a = a] () { // expected-warning {{declaration shadows a structured binding}}
}();
}

}
Loading