Fix -Wshadow false positives with function-local classes.

[alexfh]

Summary

Fixes http://llvm.org/PR33947.

https://godbolt.org/g/54XRMT

void f(int a) {

struct A {
  void g(int a) {}
  A() { int a; }
};

}

3 : <source>:3:16: warning: declaration shadows a local variable [-Wshadow]

void g(int a) {}
           ^

1 : <source>:1:12: note: previous declaration is here
void f(int a) {

^

4 : <source>:4:15: warning: declaration shadows a local variable [-Wshadow]

A() { int a; }
          ^

1 : <source>:1:12: note: previous declaration is here
void f(int a) {

^

2 warnings generated.

The local variable a of the function f can't be accessed from a method of
the function-local class A, thus no shadowing occurs and no diagnostic is
needed.

[rnk]

I'm not really sure this is a bug. The point of -Wshadow is to warn on valid but possibly confusing code. Shadowing a variable is perfectly legal, but people think it's confusing, so we implemented this warning. Are we concerned that people might get confused between the different local variables here?

[alexfh]

In D35941#823020, @rnk wrote:

I'm not really sure this is a bug. The point of -Wshadow is to warn on valid but possibly confusing code. Shadowing a variable is perfectly legal, but people think it's confusing, so we implemented this warning. Are we concerned that people might get confused between the different local variables here?

IIUC, most cases where -Wshadow warnings are issued is when a declaration from an enclosing scope would be accessible if there was no declaration that shadows it. In this case the the local variable of a function would not be accessible inside the local class anyway, so the inner variable with the same name is not confusing (at least, much less confusing than if it would prevent access to the variable of an outer scope). This is close to shadowing of uncaptured locals in lambdas, but it seems to have even less potential for being misleading. We had a few of requests internally to mute -Wshadow for this case. Another data point is that GCC doesn't warn in this case.

But if I'm overseeing reasons to issue a warning in this case, we could add an analogue of -Wshadow-uncaptured-local for this case. WDYT?

[rnk]

Another data point is that GCC doesn't warn in this case.

That seems like a reasonable tie breaker when implementing these kinds of style-enforcement warnings. :)

[• Quuxplusone]

But if I'm overseeing reasons to issue a warning in this case, we could add an analogue of -Wshadow-uncaptured-local for this case. WDYT?

I still think "any warning" is a marginally better UX than "no warning" on the particular code in question; but of course I defer to Reid/Richard/GCC on the practicalities. Adding a new warning option might not be worth the trouble. :)

[rsmith]

In D35941#823524, @alexfh wrote:

IIUC, most cases where -Wshadow warnings are issued is when a declaration from an enclosing scope would be accessible if there was no declaration that shadows it. In this case the the local variable of a function would not be accessible inside the local class anyway

That's not strictly true; the variable can be accessed in unevaluated operands, and in code doing so, a -Wshadow warning might (theoretically) be useful:

void f(SomeComplexType val) {
  struct A {
    decltype(val) &ref;
    void g(int val) {
      decltype(val) *p = &ref;
    }
  } a = {val};
}

That said, suppressing the warning seems like a good thing in the common case. We've discussed the idea of deferring some -Wshadow warnings until we see a use; if someone cares about this case, we could consider warning only if the shadowed variable is actually used in an unevaluated operand.

[alexfh]

In D35941#838633, @rsmith wrote:

In D35941#823524, @alexfh wrote:

IIUC, most cases where -Wshadow warnings are issued is when a declaration from an enclosing scope would be accessible if there was no declaration that shadows it. In this case the the local variable of a function would not be accessible inside the local class anyway

That's not strictly true; the variable can be accessed in unevaluated operands, and in code doing so, a -Wshadow warning might (theoretically) be useful:

void f(SomeComplexType val) {
  struct A {
    decltype(val) &ref;
    void g(int val) {
      decltype(val) *p = &ref;
    }
  } a = {val};
}

Good to know!

That said, suppressing the warning seems like a good thing in the common case. We've discussed the idea of deferring some -Wshadow warnings until we see a use; if someone cares about this case, we could consider warning only if the shadowed variable is actually used in an unevaluated operand.

Sounds reasonable.

cfe/trunk/lib/Sema/SemaDecl.cpp

if (const auto *RD = dyn_cast<CXXRecordDecl>(NewDC->getParent())) {
           // 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(
                   {cast<VarDecl>(D), cast<VarDecl>(ShadowedDecl)});
           return;
         }
       }
+
+      if (cast<VarDecl>(ShadowedDecl)->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;
+             ParentDC && !ParentDC->Equals(OldDC);
+             ParentDC = getLambdaAwareParentOfDeclContext(ParentDC)) {
+          // Only block literals, captured statements, and lambda expressions
+          // can capture; other scopes don't.
+          if (!isa<BlockDecl>(ParentDC) && !isa<CapturedDecl>(ParentDC) &&
+              !isLambdaCallOperator(ParentDC)) {
+            return;
+          }
+        }
+      }
     }
   }
 
   // Only warn about certain kinds of shadowing for class members.
   if (NewDC && NewDC->isRecord()) {
     // In particular, don't warn about shadowing non-class members.
     if (!OldDC->isRecord())
       return;

cfe/trunk/test/SemaCXX/warn-shadow.cpp

 }
 
 extern "C" {
 typedef int externC; // expected-note {{previous declaration is here}}
 }
 void handleLinkageSpec() {
   typedef void externC; // expected-warning {{declaration shadows a typedef in the global namespace}}
 }
+
+namespace PR33947 {
+void f(int a) {
+  struct A {
+    void g(int a) {}
+    A() { int a; }
+  };
+}
+}