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

Tue Apr 10 09:59:10 PDT 2018

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.

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/1e0a8461/attachment-0001.html>