Avoid -Wshadow warnings about constructor parameters named after fields

[rnk]

Summary

Emit a new warning if such parameters are modified in the body of the
constructor.

Fixes PR16088.

[dblaikie]

It's not just modifications of shadowed variables that are a problem - one of the one's I'm concerned we should catch is:

struct foo {
  std::unique_ptr<int> p;
  foo(std::unique_ptr<int> p) : p(std::move(p)) {
    f(*p);
  }
};

[cfe-commits]

+Lang, because he was asking me recently about this improvement & thinking
of chipping in

• msg-16506-48.txt159 BDownload

[rnk]

I'm not sure your example is in scope for -Wshadow, though. Any function call that takes a non-const reference to the parameter could modify it. I guess I'm thinking something like:

void trim_in_place(std::string &s);
struct A {
  std::string s;
  A(std::string s) : s(s) { trim_in_place(s); }
};

I think if we try to match that we'll have too many false positives.

I think your example would be better caught by something like -Wconsumed that looks for uses of objects that have been moved-from. That warning won't be confused by shadowing and will give a better diagnostic anyway.

[alexfh]

As a data point: I ran -Wshadow on our code base with a similar, but simpler patch (http://reviews.llvm.org/D18395, just disables the warning on ctor parameters named after fields). It removes more than half of the hits (from ~100k initially) and a random sample of ~100 removed removed hits didn't contain any issues that we'd like to still be warning about.

Ultimately, I think, this diagnostic should be made much less noisy to be actually useful. So maybe even starting with a removal of a larger subset of warnings (and then gradually adding back the cases that seem to be important to flag) would be better.

lib/Sema/SemaExpr.cpp

9858

Why this change?

[alexfh]

In D18271#381728, @alexfh wrote:

... and then gradually adding back the cases that seem to be important to flag ...

Maybe as separate warnings.

[alexfh]

In D18271#378196, @dblaikie wrote:

It's not just modifications of shadowed variables that are a problem - one of the one's I'm concerned we should catch is:

struct foo {
  std::unique_ptr<int> p;
  foo(std::unique_ptr<int> p) : p(std::move(p)) {
    f(*p);
  }
};

This seems totally out of scope for -Wshadow, and seeing a -Wshadow warning on this wouldn't help (imagine a few tens of lines before the *p is used).

[• thakis]

FWIW we don't currently use this warning on Chromium because it's way to noisy. So something like this looks like a great change to me.

dblaikie, are you aware of any codebases that use this warning in its current form?

[bcraig]

In D18271#381744, @thakis wrote:

FWIW we don't currently use this warning on Chromium because it's way to noisy. So something like this looks like a great change to me.

dblaikie, are you aware of any codebases that use this warning in its current form?

Not dblaikie, but I've used the GCC version of -Wshadow on reasonably large code bases before. The ctor pattern that this is trying to squelch was certainly a significant portion of the warnings, particularly when (old) boost headers got involved.

I believe that newer versions of boost (~1.50 and newer?) attempt to be -Wshadow clean in the headers.

[• thakis]

In D18271#381758, @bcraig wrote:

In D18271#381744, @thakis wrote:

FWIW we don't currently use this warning on Chromium because it's way to noisy. So something like this looks like a great change to me.

dblaikie, are you aware of any codebases that use this warning in its current form?

Not dblaikie, but I've used the GCC version of -Wshadow on reasonably large code bases before. The ctor pattern that this is trying to squelch was certainly a significant portion of the warnings, particularly when (old) boost headers got involved.

I believe that newer versions of boost (~1.50 and newer?) attempt to be -Wshadow clean in the headers.

Did squelching this ctor pattern find any bugs? Do you know if boost would miss the warning firing in this case?

[rsmith]

I think a reasonable approach would be: "do not warn on shadowing if the idiom of naming a constructor parameter after a class member is used, and the class member is initialized from that constructor parameter, and all uses of the parameter in the constructor body would still be valid if it were declared const".

In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).

(I agree that catching A(X x) : x(std::move(x)) { use(x); } is outside the scope of this change.)

lib/Sema/SemaDecl.cpp

6418

Instead of redoing class member lookup every time we check to see if a variable is modifiable, perhaps you could populate a set of shadowed-but-not-diagnosed VarDecl*s here? That way, you can also suppress the duplicate diagnostics if the variable is modified multiple times by removing it from the set.

lib/Sema/SemaExpr.cpp

9663

This query is actually quite expensive (more so than some or all of the other checks below).

[bcraig]

Not dblaikie, but I've used the GCC version of -Wshadow on reasonably large code bases before. The ctor pattern that this is trying to squelch was certainly a significant portion of the warnings, particularly when (old) boost headers got involved.

I believe that newer versions of boost (~1.50 and newer?) attempt to be -Wshadow clean in the headers.

Did squelching this ctor pattern find any bugs? Do you know if boost would miss the warning firing in this case?

I don't recall fixing any bugs with the ctor pattern. I can only recall two different classes of things that I have fixed that were flagged with -Wshadow.

1. Long, horrible, deeply nested method, with a local that was trivially shadowing a different local in a larger scope.
2. Mutex lock guards and similar "sentry" objects. "std::unique_lock<std::mutex>(name_of_a_member);" The user wanted that to lock 'name_of_member' and release it at end of scope, but instead it declared a default constructed unique_lock called 'name_of_a_member' that shadows the member variable. I've had to fix this kind of bug a number of times, and it is my main justification for turning on -Wshadow in the first place. On that note, I'm really looking forward to the day I can say "auto guard = std::unique_lock(name_of_member);" to effectively get rid of this problem.

I doubt boost would miss the warning, and probably wouldn't even notice (barring overlapping list membership). If you're really wondering if boost would care / notice, @mclow.lists would have a better idea.

[rnk]

In D18271#381769, @rsmith wrote:

I think a reasonable approach would be: "do not warn on shadowing if the idiom of naming a constructor parameter after a class member is used, and the class member is initialized from that constructor parameter, and all uses of the parameter in the constructor body would still be valid if it were declared const".

That sounds like the right check, but I don't have a lot of time to work on this and I don't want to let the perfect be the enemy of the good. Do you think is a reasonable half-way point for us to leave it for the next year or so?

In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).

Would the reference binding check be somewhere in SemaInit.cpp? I don't see an obvious spot to do this check at first glance.

(I agree that catching A(X x) : x(std::move(x)) { use(x); } is outside the scope of this change.)

Wouldn't the proposed check for non-const references find this example, though?

lib/Sema/SemaDecl.cpp

6418

That sounds like a good idea. I could even leave it empty if we're not doing -Wshadow.

lib/Sema/SemaExpr.cpp

9663

This check has to be faster than name lookup, right? Maybe do it before that?

[alexfh]

In D18271#382082, @rnk wrote:

In D18271#381769, @rsmith wrote:

I think a reasonable approach would be: "do not warn on shadowing if the idiom of naming a constructor parameter after a class member is used, and the class member is initialized from that constructor parameter,

Depending on how strictly you define "initialized" here, this is either unhelpful or difficult to chpatterneck. Consider following examples:

class C1 {
  T field;
public:
  void set_field(T f) {
    field = f;
    // do something else...
  }
  C1(T field) { set_field(field); }
};

class C2 {
  T1 f1;
  T2 f2;
public:
  C2(T1 f1, T2 f2) {
    // Some form of initialization of `this->field` from `field` that is not just a copy construction/copy/move,
    // e.g. a method call or an assignment of a single member:
    this->f1.a = f1.a;
    this->f2.CopyFrom(f2);
  }
};

I've seen these patterns being used in the real code, and they don't seem to be dangerous or otherwise worth being flagged by this diagnostic.

and all uses of the parameter in the constructor body would still be valid if it were declared const".

That might also be overly restrictive. Consider a variant of set_field above that takes a T& (which is weird, but it doesn't seem like -Wshadow is the right warning in this case).

That sounds like the right check, but I don't have a lot of time to work on this and I don't want to let the perfect be the enemy of the good. Do you think is a reasonable half-way point for us to leave it for the next year or so?

In particular, I think we should probably warn if a non-const reference is bound to the parameter, or if the constructor's member initializer list does not contain field(field).

See above. There are many cases, when at least the latter pattern is used in a reasonable way.

[alexfh]

Depending on how strictly you define "initialized" here, this is either unhelpful or difficult to chpatterneck.

s/chpatterneck/check/

[rnk]

• Address review comments, use a DenseMap instead of redoing lookup

[alexfh]

Richard, is there anything else that blocks this patch?

lib/Sema/SemaDecl.cpp

6372

How about return isa<FieldDecl>(ShadowedDecl) ? SDK_Field : SDK_StaticMember;?

6376

No else after return, please.

[rnk]

We had a conversation about this change around the office the other week, and people were concerned about false negatives like the trim_in_place one. Basically, I don't have time to discover all the ways you can modify your parameters:

struct B {
  A a;
  B(A a) : a(a) { // modifies parameter, not member
    a.a = 42;
    modify_in_place(&a);
    a.setMember(3);
  }
};

These kinds of bugs are uncommon, but the existing -Wshadow protects users against this today. I think it'll be OK if we give these users an extra -Wshadow-constructor preserves the more strict behavior.

[rsmith]

lib/Sema/SemaDecl.cpp

1615

This'd be a good place to produce a -Wshadow-constructor or similar warning if D is still in the map.

6483

Maybe map to the canonical decl before using the Decl* as a key? (This doesn't matter for now, but we'll likely want to extend this to other kinds of shadowing in the future.)

6489–6490

Could be useful to also note the location of the shadowing decl.

[rnk]

• Add -Wshadow-all and -Wshadow-field-in-constructor, also address review comments

[rnk]

• Add a test for -Wshadow-all

lib/Sema/SemaDecl.cpp

6489–6490

Sure, done. I reused and renamed a note from uninit variable diagnostics.

[rnk]

ping

[rsmith]

lib/Sema/SemaDecl.cpp

6428–6436

I would prefer for us to produce the -Wshadow "modifying shadowed" warning where it applies, even when -Wshadow-all is enabled. The usual model is to act as if -W* flags are just a filter over the diagnostic output, and this diverges from that.

Is the goal to ensure that -Wshadow-field-in-constructor diagnoses all constructor parameter / field shadowing, even if -Wshadow is not enabled?

[rnk]

lib/Sema/SemaDecl.cpp

6428–6436

Well, if we do that we'll get two warnings about the shadowed field: one on the site of the parameter declaration and another on the modification. That felt a bit redundant.

I can remove the if/else and we'll get the behavior you're asking for. Most users will not use -Wshadow-all, and even if they do they'll only see the double diagnostic in very rare cases.

[rnk]

• Address review comments
• Add -Wshadow-all and -Wshadow-field-in-constructor, also address review comments
• Warn twice under -Wshadow-all if a shadowing parameter is modified

[rsmith]

lib/Sema/SemaDecl.cpp

6428–6436

Here's what I was thinking:

• The new warning goes in its own group, say -Wshadow-field-in-constructor-modified
• The new warning group is part of both -Wshadow and -Wshadow-field-in-constructor
• We produce the -Wshadow-field-in-constructor warning from ActOnPopScope whenever we remove something from the map.

This means that our behavior is as if -W flags filter our warning output, and we only get one warning for each instance of shadowing, and -Wshadow-field-in-constructor can be used to control all constructor-parameter-shadows-field warnings (and even obscure cases like -Wshadow -Wno-shadow-field-in-constructor do what they look like they should do).

[rnk]

• Implement suggestions to avoid warning twice

[rsmith]

include/clang/Basic/DiagnosticGroups.td

341–342 ↗

(On Diff #55044)

Should ShadowFieldInConstructorModified be a subgroup of ShadowFieldInConstructor? I'd expect -Wshadow-field-in-constructor to enable both.

lib/Sema/SemaDecl.cpp

6427

existance -> existence

[rnk]

• address comments

[rsmith]

LGTM