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

Tue Apr 10 10:19:56 PDT 2018

Do you think they’re bad precedent? Do you have a replacement for that
approach to warning about those problems? 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.

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/llvm-commits/attachments/20180410/c3af4dc1/attachment.html>