[cfe-dev] Diagnosing initialization of deprecated data member?

Tue May 12 11:30:35 PDT 2020

On Tue, 12 May 2020 at 00:48, Stephan Bergmann via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> On 11/05/2020 06:55, Richard Smith wrote:
> > On Thu, 7 May 2020 at 01:52, Stephan Bergmann via cfe-dev
> > <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
> >
> >     For
> >
> >      > $ cat test.cc
> >      > struct S {
> >      >     int x;
> >      >     [[deprecated]] int y;
> >      > };
> >      > int main() {
> >      >     S s{0, 0};
> >      > }
> >
> >     Clang emits a diagnostic
> >
> >      > $ clang++ test.cc
> >      > test.cc:6:12: warning: 'y' is deprecated
> [-Wdeprecated-declarations]
> >      >     S s{0, 0};
> >      >            ^
> >      > test.cc:3:7: note: 'y' has been explicitly marked deprecated here
> >      >     [[deprecated]] int y;
> >      >       ^
> >      > 1 warning generated.
> >
> >     while e.g. GCC and MSVC do not (as checked with godbolt.org
> >     <http://godbolt.org>).
> >
> >     Is this deliberate behavior on Clang's part, or just a consequence of
> >     how the code happens to work?  Are there opinions on whether or not
> the
> >     behavior is useful?
> >
> >
> > I think it's probably a bug. The C++11 attribute is documented as
> > deprecating the name, not the existence of the entity, so diagnosing a
> > use that doesn't use the name seems somewhat questionable. (It's not
> > obviously a bad choice, though, since the intent is very likely to catch
> > cases that would break if the field is removed, not only if it's
> renamed.)
> >
> > There are a few other warnings channeled through the same path
> > (`DiagnoseUseOfDecl`), such as "availability" warnings, and it's quite
> > likely that some of those *should* still be issued for such cases.
> >
> >     (I came across this issue when seeing a commit in LibreOffice where
> some
> >
> >      > #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> >
> >     were added apparently to silence exactly the above situation in code
> >     initializing a PyTypeObject struct from
> >     /usr/include/python3.8/cpython/object.h, not even realizing that that
> >     pragma would only be needed by Clang and not by GCC.
> >
> >     FWIW, the standard's recommended practice is "to produce a diagnostic
> >     message in case the program refers to a name or entity other than to
> >     declare it", [dcl.attr.deprecated]/4.)
> >
> >
> > What was the intent in the LibreOffice case? Would they want a warning
> > on "S s{.x = 0, .y = 0};" but not on "S s{0, 0};"?
>
> That Python/LibreOffice case is a bit complex, the commit message of
> <
> https://git.libreoffice.org/core/+/23d9966751566028c50ca95ca203d20f36c64f30%5E%21>
>
> "Fix initialization of Python-3.8--only at-end tp_print member" has the
> details.  The intend on the Python side, where the deprecation attribute
> got added, is apparently to warn about "initialization by assignment"
> cases like
>
>    X.tp_print = 0;
>
> It presumably did not intend to cause warnings in list-initialization
> scenarios as used in the LibreOffice code---I guess the re-addition of
> the dummy tp_print member at the end didn't even take into account the
> -Wmissing-field-initializers for client code using list-initialization,
> which causes client code to silence that warning by adding an
> initializer for the dummy member, but which in turn causes
> -Wdeprecated-declarations.
>

The interaction between this flag and -Wmissing-field-initializers seems
unfortunate. I wonder whether it would be reasonable to suppress that
warning for the case where the field is deprecated.

The deprecated field is followed by a #ifdef'd extra field:

>      /* these must be last and never explicitly initialized */
>      Py_ssize_t tp_allocs;

I think that might be OK: assuming you don't provide an initializer for
that field, you'd still get a -Wmissing-field-initializers warning in the
case where the field exists, but this struct is just fundamentally
incompatible with -Wmissing-field-initializers in the case where that field
exists.

> For designated initializer scenarios, the warning is apparently also
> useful (and likely intended by the Python authors) in C code, where it
> would warn about legacy code like
>
>    ..., .tp_dealloc=..., .tp_print=..., .tp_getattr=..., ...
>
> that assumes tp_print at its original position in PyTypeObject.  (For
> C++20, such code would cause an error anyway because tp_print has been
> moved relative to the other members.)
>

I suspect the Python authors do not intend for the tp_print field to be
explicitly initialized, whether named or not. And given the comment on the
tp_allocs field, I suspect they do not intend to support use of that header
with -Wmissing-field-initializers.

> So, in short, for the particular scenario at hand, it looks like not
> emitting -Wdeprecated-declarations in the non-designated
> initializer-list scenario would be the most useful behavior.
>

I don't find this particular case to be strongly persuasive one way or the
other. But I'm slightly leaning towards it being desirable to warn in this
case: providing an explicit initializer (with a /*tp_print*/ comment, no
less) seems fundamentally no different from providing a
designated .tp_print= initializer, which seems fundamentally no different
from providing an x.tp_print= assignment.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200512/7b6c2457/attachment.html>