[cfe-dev] Warning when calling virtual functions from constructor/desctructor?

[John McCall via cfe-dev]

On 19 Dec 2018, at 9:22, Aaron Ballman via cfe-dev wrote:

> On Tue, Dec 18, 2018 at 6:59 PM Artem Dergachev via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
>>
>> Static Analyzer's checker already has a mode in which it warns on 
>> case B, but it's off by default because it is surprisingly noisy. In 
>> particular, i vaguely recall that i've seen ~90 warnings on the 
>> OpenOffice codebase - none of them were pure virtual calls and most 
>> of them looked like false positives in the sense that the virtual 
>> function is called intentionally. Which means that i would be 
>> generally worried about this warning. That said, i don't know how 
>> warnings are accepted into Clang and whether there is a threshold on 
>> intentional violations. I suspect that it depends on whether the 
>> warning is expected to be on by default or be part of -Wall or only 
>> -Weverything.
>
> In general, warnings in Clang are expected to be on-by-default and
> have an extremely low false positive rate (certainly lower than for
> the static analyzer). I suspect this warning would not meet the usual
> bar for inclusion as a Clang warning given the number of false
> positives it's shown in the past. I think the static analyzer is a
> better place for the functionality to live, especially given that it
> requires actual static analysis of runtime control flows to reduce
> those false positives.
>
>> Another good thing to do with a warning that may be intentionally 
>> triggered by the user is to add a hint on how to suppress it. Eg., 
>> how -Wparentheses does it:
>>
>>   test.c:2:9: note: place parentheses around the assignment to 
>> silence this warning
>>     if (x = 1) {
>>           ^
>>         (    )
>>
>> I guess you could add a note that suggests to qualify the call with a 
>> class name, which is a good thing to do anyway. With that, in my 
>> opinion it'd be a good warning.
>
> Agreed that this would be a sensible way for users to suppress the
> diagnostic -- it clarifies that the programmer understands the
> behavior of the call.

Agreed.

In general, whether or not to add a new default-on warning comes down to 
two
somewhat orthogonal considerations:

- Are we willing to say that the code pattern is Wrong?

   This involves weighing the harm of unknowing uses of the code pattern
   against the harm of avoiding the diagnostic.

- Are knowing, correct uses of the code pattern too common in practice
   to justify annoying a large fraction of the community?

   This involves weighing the diagnostic's impact on all code, which is
   impossible, so we have to muddle through with incomplete testing.

For this particular diagnostic, I'm not sure about either of those 
points:

- The code pattern is definitely not always harmful.  Most importantly, 
the
   behavior is only confusing if the method is actually overridden in a 
subclass.

- There's probably a lot of code which does this innocuously.  The usual
   admonition that running code is usually correct is stronger than 
usual here
   because most code in a constructor or destructor will actually be run 
on the
   common path.

- Some programmers will reasonably object that they know the semantics 
here.

- The workaround can be a bit verbose.

- The best workaround is to name the current class, but programmers 
might find
   that weird if it's not where the method is actually defined. If they 
instead
   try to name the defining class, the workaround is making the code 
more brittle.

So I'm not sure if this is a good warning to add.  I agree that it may 
be better
suited for the static analyzer, which can try to ameliorate at least 
some of
these concerns.

**That said**, I'm concerned about the growing pressure to not add new
warnings.  That's not an intended goal of the pressure, but it is the 
basic
outcome.  There is a tightly-woven web of different factors that have 
the
effect of discouraging the addition of any new diagnostics:

- We discourage adding new default-off warnings on the theories that
   (1) warnings should have maximal impact and (2) we don't want unused
   complexity in the compiler.

- New default-on warnings are immediately exercised on a massive amount 
of
   code and will be reverted if they cause any problems for existing 
code because
   we try to keep the codebase as close as possible to an acceptable 
release
   state at all times.

- Project maintainers quite reasonably feel that (1) they did not ask 
for this
   new warning and (2) they don't really want to deal with it right now 
but (3)
   it's being forced on them anyway and (4) they are rather upset about 
that.

The basic problem here is that we have no way for projects to evolve 
with
default-on warnings, and so we necessarily conflate two very different 
ideas:

- We want the warning to eventually be useful to as many people as 
possible.

- We want the warning to be useful to everyone right now.

But the first doesn't require the second.  It would make a lot more 
sense
if we curated an ever-expanding but *versioned* set of default-on 
warnings
and allowed users to ratchet their version forward at their own pace.
More precisely:

1. We would add a flag to control the current default diagnostic 
settings,
    something like `-Wversion=clang-7` to say to use Clang 7's default 
warning
    behavior.

2. The first version of that would turn on a whole bunch of warnings 
that we've
    always thought ought to be on by default but aren't because of GCC 
compatibility.

3. In the absence of an explicit `-Wversion` flag, the compiler would 
continue
    using the supposedly-GCC-compatible defaults.

4. It would be understood that `-Wversion=X+1` will generally be defined 
as
    `-Wversion=X -Wfoo -Wbar -Wbaz`.  (Not literally in terms of the 
set-algebra
    of warnings flags, just in terms of each version adding new 
warnings.)

5. Project maintainers can roll forward their per-compiler warning 
versions
    when they're ready to deal with the consequences.

6. The compiler would not complain if it's passed a future version; it
    would just use its most recent rules.  This allows projects to 
easily
    roll their warnings version forward while still working on older 
compilers.
    We would clearly document that projects which intentionally 
post-date
    their warnings version do so at their own risk.

7. Platform maintainers and distributors like Google and Apple (and 
other
    motivated projects like e.g. Mozilla) would periodically qualify the 
current
    `-Wversion` on their codebases in order to gather feedback on the 
current
    set of newly-added warnings.

8. Tools like Xcode can make their own decisions about rolling forward 
warning
    versions.  For example, a new project might be configured to use the 
latest
    warning settings but then stay with that version until manually 
updated.

This flag isn't meant to be a guarantee that we'll emit exactly the same
diagnostics as the target version.  For one, of course, we might change
diagnostic text or emit the diagnostic somewhere else or add a note or 
whatever.
More importantly, I think we'd still reserve the right to add new 
warnings
that are unconditionally on by default if we think they're important or
unproblematic enough.

This wouldn't be a blank check to add new default-on warnings: the two 
basic
considerations above would still apply.  It's just that the second
consideration would be far more focused:

- It wouldn't need to account for the impact on legacy projects that 
might
   lack an active maintainer.  Someone who wants to audit the new 
warnings on
   that project can easily do so, but otherwise the build is unaffected.

- It wouldn't need to account for the inter-project politics of 
compilers
   forcing unwanted new warnings on other programmers.  A project 
maintainer
   who intentionally rolls their version forward but doesn't like one of 
the
   new warnings can easily deal with that problem themselves.

- Instead, it would just evaluate the impact from the perspective of a 
project
   maintainer who has voluntarily requested a larger set of warnings.  
Again,
   that wouldn't be a blank check: the warning still needs to be 
worthwhile.
   But it's necessarily a less fraught prospect.

There would also be a very clear path for warnings under active 
development:
our expectation would be that most new warnings would eventually be 
added to
the set of default-on warnings.  In my opinion, that leaves room for 
warnings
to remain default-off as long as we expect that they will eventually 
become
acceptable to include.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190108/8f0c08b3/attachment.html>