[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

Tue Apr 10 10:52:40 PDT 2018

On Tue, Apr 10, 2018 at 10:20 AM John McCall <rjmccall at gmail.com> wrote:

> Do you think they’re bad precedent?

Somewhat, yes - though -Wparens is perhaps conflating a few cases too. I
think the argument for the -Wparens for precedence is probably pretty good.

But the argument for -Wparens for conditional assignments would probably be
pretty hard to make today, in a codebase that'd never seen the warning
before - the false positive rate would probably be really high & not
something that would be reasonable to expect people to do all the cleanup
for. (that's been the general bar "is the value gained worth the cleanup" -
this precludes a lot of warnings that would probably be acceptable in new
code (where the incremental cost of keeping in compliance with the warning
wouldn't be so bad - but the cleanup cost in an existing codebase would be
quite high/churny (admittedly, as we've gotten better tooling for wide
scale changes/refactoring, this may've changed the balance a bit - but
those tools aren't exactly super widely available/easy to plug in)))

> Do you have a replacement for that approach to warning about those
> problems?

If we were looking at a green field today (code that'd never seen the
warning before), I don't think -Wparens for assignment in conditional would
pass the bar (or at least the bar as Clang seemed to have 6-7 years ago
when I would talk to Doug about warning ideas, as best as I understood the
perspective being espoused back then). I suspect maybe in that world we
would've found other warnings with lower-false-positive that might've been
able to diagnose assignment-in-conditional a bit more precisely than "all
assignments are so suspect you have to manually go and look at them to
decide".

> Because they certainly were precedent for -Wfallthrough, and they
> certainly catch a class of bugs at least as large and important as that
> warning, and they certainly have exactly the same false-positive
> characteristics as it does. I am really struggling to see a difference in
> kind or even degree here.
>
> -Wself-assign is a much less important warning but it’s also far less
> likely to see false positives, except in this pattern of unit tests for
> data structures, which are not commonly-written code. As is, in fact,
> evidenced by Google, a company full of programmers whom I think I can
> fairly but affectionately describe as loving to reinvent data structures,
> only seeing this warning eight times.
>

I think the 8 cases were in Chromium - at Google so far (& I'm not sure if
that's across all of Google's codebase, or some subset) something in the
100-200 cases?

Nico's point that, so far, the only evidence we have is that this warning
added false positives and didn't catch any real world bugs. Yeah, a small
number/easy enough to cleanup, but all we have is the cost and no idea of
the value. (usually when we roll out warnings - even the -Wparens and
-Wfallthrough, we find checked in code that has those problems (& the false
positive cases) & so we evaluate cost/benefit with that data)

>
> John.
> On Tue, Apr 10, 2018 at 19:03 David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Tue, Apr 10, 2018 at 9:59 AM John McCall <rjmccall at gmail.com> wrote:
>>
>>> Also, the standard for the static analyzer is not lower than it is for
>>> the compiler.
>>>
>>> The static analyzer tries to catch a much larger class of bugs, but it
>>> also tries very hard to make all the warnings it emits count. There’s a
>>> really common problem in the static analysis field where users try a static
>>> analyzer, get buried by 50 pages of diagnostics, and immediately throw up
>>> their hands and abandon it forever. Our analyzer is very circumspect about
>>> false positives specifically to try to fight that.
>>>
>>> In contrast, the compiler is *much* more willing to just tell users that
>>> they might have a good reason for doing something but they should write it
>>> another way to shut the compiler up. That is the basic design of -Wparens,
>>> -Wfallthrough, -Wswitch, and a bunch of other major warnings. I think this
>>> is of a piece.
>>>
>>
>> In the past -Wparens and -Wswitch haven't been cited as precedent that
>> was desirable to continue. -Wfallthrough was argued for in favor of the
>> large class of bugs it catches.
>>
>>
>>> We put warnings in the static analyzer if (1) they’re API-specific in a
>>> way that we’re not comfortable with in the compiler or (2) there would be
>>> major false positives that we can only suppress with a much more expensive
>>> analysis than we’re comfortable with in the compiler.
>>>
>>> I do not see a way that we could suppress this warning in the static
>>> analyzer. The static analyzer is unlikely to be able to prove that the
>>> assignment is actually idempotent, and it is not going to be able to infer
>>> that the code is actually in a unit test for the assigned type any better
>>> than the compiler can.
>>>
>>
>> Sure, clang-tidy might be a better place for such things.
>>
>>
>>>
>>> John.
>>> On Tue, Apr 10, 2018 at 18:34 John McCall <rjmccall at gmail.com> wrote:
>>>
>>>> Yeah, -Wself-assign in general is about catching a class of live-coding
>>>> errors which is pretty unlikely to ever make it into committed code, since
>>>> the code would need to be dead/redundant for the mistake to not be noticed.
>>>> So it’s specifically a rather poor match for the static analyzer, and I
>>>> would not expect it to catch much of anything on production code.
>>>>
>>>> John.
>>>> On Tue, Apr 10, 2018 at 18:27 David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>>> On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev <lebedev.ri at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Because *so far* it has been running on the existing code, which i
>>>>>> guess
>>>>>> was already pretty warning free, and was run through the static
>>>>>> analyzer,
>>>>>
>>>>> which obviously should catch such issues.
>>>>>>
>>>>>
>>>>> Existing code this has been run over isn't necessarily run against the
>>>>> static analyzer already. (this has been run against a subset of (maybe all
>>>>> of, I forget) google, which isn't static analyzer clean by any stretch (I'm
>>>>> not even sure if the LLVM codebase itself is static analyzer clean)).
>>>>>
>>>>>
>>>>>> Do you always use [clang] static analyzer, each time you build?
>>>>>> Great! It is indeed very good to do so. But does everyone?
>>>>>>
>>>>>> This particular issue is easily detectable without full static
>>>>>> analysis,
>>>>>> and as it is seen from the differential, was very straight-forward to
>>>>>> implement
>>>>>> as a simple extension of pre-existing -Wself-assign.
>>>>>>
>>>>>> So if this is really truly unacceptable false-positive rate, ok, sure,
>>>>>> file a RC bug,
>>>>>> and i will split it so that it can be simply disabled for *tests*.
>>>>>>
>>>>>> That being said, i understand the reasons behind "keep clang
>>>>>> diagnostics
>>>>>> false-positive free". I don't really want to change that.
>>>>>>
>>>>>> On Tue, Apr 10, 2018 at 7:13 PM, David Blaikie <dblaikie at gmail.com>
>>>>>> wrote:
>>>>>> > It's more the fact that that's /all/ the warning improvement has
>>>>>> found so
>>>>>> > far. If it was 8 false positives & also found 80 actionable
>>>>>> bugs/bad code,
>>>>>> > that'd be one thing.
>>>>>> >
>>>>>> > Now, admittedly, maybe it would help find bugs that people usually
>>>>>> catch
>>>>>> > with a unit test, etc but never make it to checked in code - that's
>>>>>> always
>>>>>> > harder to evaluate though Google has some infrastructure for it at
>>>>>> least.
>>>>>> >
>>>>>> > On Tue, Apr 10, 2018 at 9:07 AM John McCall <rjmccall at gmail.com>
>>>>>> wrote:
>>>>>> >>
>>>>>> >> If you have a concrete suggestion of how to suppress this warning
>>>>>> for
>>>>>> >> user-defined operators just in unit tests, great. I don’t think 8
>>>>>> >> easily-suppressed warnings is an unacceptably large false-positive
>>>>>> problem,
>>>>>> >> though. Most warnings have similar problems, and the standard
>>>>>> cannot
>>>>>> >> possibly be “must never fire on code where the programmer is
>>>>>> actually happy
>>>>>> >> with the behavior”.
>>>>>> >>
>>>>>> >> John.
>>>>>> >> On Tue, Apr 10, 2018 at 17:12 Nico Weber <thakis at chromium.org>
>>>>>> wrote:
>>>>>> >>>
>>>>>> >>> "False positive" means "warning fires but didn't find anything
>>>>>> >>> interesting", not "warning fires while being technically
>>>>>> correct". So all
>>>>>> >>> these instances do count as false positives.
>>>>>> >>>
>>>>>> >>> clang tries super hard to make sure that every time a warning
>>>>>> fires, it
>>>>>> >>> is useful for a dev to fix it. If you build with warnings
>>>>>> enabled, that
>>>>>> >>> should be a rewarding experience. Often, this means dialing back
>>>>>> a warning
>>>>>> >>> to not warn in cases where it would make sense in theory when in
>>>>>> practice
>>>>>> >>> the warning doesn't find much compared to the amount of noise it
>>>>>> generates.
>>>>>> >>> This is why for example clang's -Woverloaded-virtual is usable
>>>>>> while gcc's
>>>>>> >>> isn't (or wasn't last I checked a while ago) – gcc fires always
>>>>>> when it's
>>>>>> >>> technically correct to do so, clang only when it actually matters
>>>>>> in
>>>>>> >>> practice.
>>>>>> >>>
>>>>>> >>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator
>>>>>> via
>>>>>> >>> cfe-commits <cfe-commits at lists.llvm.org> wrote:
>>>>>> >>>>
>>>>>> >>>> lebedev.ri added a comment.
>>>>>> >>>>
>>>>>> >>>> In https://reviews.llvm.org/D44883#1063003, @thakis wrote:
>>>>>> >>>>
>>>>>> >>>> > This landing made our clang trunk bots do an evaluation of this
>>>>>> >>>> > warning :-P It fired 8 times, all false positives, and all
>>>>>> from unit tests
>>>>>> >>>> > testing that operator= works for self-assignment.
>>>>>> >>>> > (
>>>>>> https://chromium-review.googlesource.com/c/chromium/src/+/1000856
>>>>>> has the
>>>>>> >>>> > exact details) It looks like the same issue exists in LLVM
>>>>>> itself too,
>>>>>> >>>> > https://reviews.llvm.org/D45082
>>>>>> >>>>
>>>>>> >>>>
>>>>>> >>>> Right, i guess i only built the chrome binary itself, not the
>>>>>> tests,
>>>>>> >>>> good to know...
>>>>>> >>>>
>>>>>> >>>> > Now tests often need warning suppressions for things like
>>>>>> this, and
>>>>>> >>>> > this in itself doesn't seem horrible. However, this change
>>>>>> takes a warning
>>>>>> >>>> > that was previously 100% noise-free in practice and makes it
>>>>>> pretty noisy –
>>>>>> >>>> > without a big benefit in practice. I get that it's beneficial
>>>>>> in theory, but
>>>>>> >>>> > that's true of many warnings.
>>>>>> >>>> >
>>>>>> >>>> > Based on how this warning does in practice, I think it might
>>>>>> be better
>>>>>> >>>> > for the static analyzer, which has a lower bar for false
>>>>>> positives.
>>>>>> >>>>
>>>>>> >>>> Noisy in the sense that it correctly diagnoses a self-assignment
>>>>>> where
>>>>>> >>>> one **intended** to have self-assignment.
>>>>>> >>>> And unsurprisingly, it happened in the unit-tests, as was
>>>>>> expected ^ in
>>>>>> >>>> previous comments.
>>>>>> >>>> **So far** there are no truly false-positives noise (at least no
>>>>>> reports
>>>>>> >>>> of it).
>>>>>> >>>>
>>>>>> >>>> We could help workaround that the way i initially suggested, by
>>>>>> keeping
>>>>>> >>>> this new part of the diag under it's own sub-flag,
>>>>>> >>>> and suggest to disable it for tests. But yes, that
>>>>>> >>>>
>>>>>> >>>>
>>>>>> >>>>
>>>>>> >>>> Repository:
>>>>>> >>>>   rC Clang
>>>>>> >>>>
>>>>>> >>>> https://reviews.llvm.org/D44883
>>>>>> >>>>
>>>>>> >>>>
>>>>>> >>>>
>>>>>> >>>> _______________________________________________
>>>>>> >>>> cfe-commits mailing list
>>>>>> >>>> cfe-commits at lists.llvm.org
>>>>>> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180410/40fa3f7f/attachment-0001.html>