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

Tue Apr 10 10:19:19 PDT 2018

On Tue, Apr 10, 2018 at 12:59 PM, 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 my opinion, the reasoning for -Wparens is that adding the parens is
arguably a good thing to do anyhow, since not everybody has the precedence
rules memorized and they help human readers. The same isn't true for `a =
*&a`.

Wfallthrough finds oodles of bugs in real life (I turned on that warning in
Chromium not too long ago and it found 13 real bugs, two of them
user-visible).

I should make clear that I don't have a huge problem with this change here.
I think it makes the warning strictly less useful, but the warning is still
usable. So if general consensus is that this is a good change, I can live
with it.

>
> 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.
>
> 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/137734c2/attachment-0001.html>