Proposal: -Wshadow-field flag

Hi everyone,

I would like to propose adding a -Wshadow-field to clang. This flag would emit a warning if a derived class declares a member with the same name as one in one of the base classes, regardless of visibility.

Here is the rationale: In the Mozilla code base, there are fields that are often declared with the same name in thousands of classes, either by convention or through macros. One concrete example is mRefCnt, which is the variable holding the object’s reference count. Every once in a while, we find bugs that are caused by the derived class inadvertently shadowing the base member, and therefore code in the derived class and its descendants in the hierarchy will access the wrong variable. In the case of mRefCnt, for example, these bugs are security sensitive since this typically causes the reference count of the object to get out of balance, causing use-after-free issues. One can conceive of other similar problems with other fields as well.

I was going to implement this warning in the clang plugin that we use for static analysis of our code base, but realized that this may probably be useful for other projects as well. Is there any interest in this feature? If yes, I would be happy to submit a patch.

Cheers,

I think this would be a great addition! In general, -Wshadow fires in a lot of situations, and I think having more granularity here is helpful for users.

I think this would be a great addition! In general, -Wshadow fires in a
lot of situations, and I think having more granularity here is helpful for
users.

Note that -Wshadow doesn't diagnose this specific case, so my proposal
isn't really a subset of -Wshadow.

Right, this sounds like a new and useful warning by itself.

I was mostly saying this seems like the right direction. The existing
checks under -Wshadow are both too little and too much. Having more checks
and more granular flags will help us explore the space.

FWIW I’d be a bit concerned that this warning, while it might have a good false positive rate on a codebase with the particular idioms you’ve described, would have a false positive rate a bit too high for a normal diagnostic bar.

Only one way to find out, though.

I think this would be a great addition! In general, -Wshadow fires in a
lot of situations, and I think having more granularity here is helpful for
users.

Note that -Wshadow doesn't diagnose this specific case, so my proposal
isn't really a subset of -Wshadow.

Right, this sounds like a new and useful warning by itself.

Great to hear that. I'll give it a shot then. :slight_smile:

I was mostly saying this seems like the right direction. The existing
checks under -Wshadow are both too little and too much. Having more checks
and more granular flags will help us explore the space.

Yeah, agreed. For those interested, this is a good read on why we decided
to not turn on -Wshadow whole-sale for Mozilla: <
563195 - Turn -Wshadow build option back on for layout/style.

Cheers,

FWIW I'd be a bit concerned that this warning, while it might have a good
false positive rate on a codebase with the particular idioms you've
described, would have a false positive rate a bit too high for a normal
diagnostic bar.

Only one way to find out, though.

That is a good point.

FWIW, I don't think we should turn this warning on as part of -Wall, maybe
as part of -Wextra? I'm not at all familiar with what the usual process
for turning on warnings by default looks like, so I would appreciate if
someone can point me in the right direction there.

Cheers,
Ehsan

Confusingly, "on by default" and "enabled by -Wall" are distinct in Clang,
but I don't find the distinction is not very useful.

I would suggest adding the warning under -Wshadow-field, and make it a
subgroup of -Wshadow. This way, -Wshadow users will get new warnings that
they probably wanted anyway. -Wshadow is not part of -Wall, so -Wall won't
enable it. Mark each new diagnostic as DefaultIgnore in
DiagnosticSemaKinds.td, so that it will not be on by default.

Once it's in, we can try it out, and if it has a really low false-positive
rate, then we can discuss enabling it by default and/or putting it in -Wall.

FWIW I'd be a bit concerned that this warning, while it might have a good
false positive rate on a codebase with the particular idioms you've
described, would have a false positive rate a bit too high for a normal
diagnostic bar.

Only one way to find out, though.

That is a good point.

FWIW, I don't think we should turn this warning on as part of -Wall, maybe
as part of -Wextra? I'm not at all familiar with what the usual process for
turning on warnings by default looks like, so I would appreciate if someone
can point me in the right direction there.

Confusingly, "on by default" and "enabled by -Wall" are distinct in Clang,
but I don't find the distinction is not very useful.

I would suggest adding the warning under -Wshadow-field, and make it a
subgroup of -Wshadow. This way, -Wshadow users will get new warnings that
they probably wanted anyway.

The part of this warning that I'm not sure the 'normal' user of
-Wshadow need is where it warns regardless of access control - for
most users I wouldn't imagine it's important that my base class has a
private field called 'foo' when my derived class has a field called
'foo'.

-Wshadow is not part of -Wall, so -Wall won't
enable it. Mark each new diagnostic as DefaultIgnore in
DiagnosticSemaKinds.td, so that it will not be on by default.

Once it's in, we can try it out, and if it has a really low false-positive
rate, then we can discuss enabling it by default and/or putting it in -Wall.

Historically there's been a fair bit of push back on off-by-default
warnings as they tend to add weight to the compiler without much in
the way of usage or quality checks (because of a lack of usage), but
perhaps this attitude has changed.

- David