Diagnosing initialization of deprecated data member?

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Diagnosing initialization of deprecated data member?

Manuel Klimek via cfe-dev
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).

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 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.)

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Diagnosing initialization of deprecated data member?

Manuel Klimek via cfe-dev
On Thu, 7 May 2020 at 01:52, Stephan Bergmann via cfe-dev <[hidden email]> 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).

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};"?

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Diagnosing initialization of deprecated data member?

Manuel Klimek via cfe-dev
On 11/05/2020 06:55, Richard Smith wrote:

> On Thu, 7 May 2020 at 01:52, Stephan Bergmann via cfe-dev
> <[hidden email] <mailto:[hidden email]>> 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.

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.)

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.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Diagnosing initialization of deprecated data member?

Manuel Klimek via cfe-dev
On Tue, 12 May 2020 at 00:48, Stephan Bergmann via cfe-dev <[hidden email]> wrote:
On 11/05/2020 06:55, Richard Smith wrote:
> On Thu, 7 May 2020 at 01:52, Stephan Bergmann via cfe-dev
> <[hidden email] <mailto:[hidden email]>> 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.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Diagnosing initialization of deprecated data member?

Manuel Klimek via cfe-dev
On 12/05/2020 20:30, Richard Smith wrote:

> On Tue, 12 May 2020 at 00:48, Stephan Bergmann via cfe-dev
> <[hidden email] <mailto:[hidden email]>> wrote:
>
>     On 11/05/2020 06:55, Richard Smith wrote:
>      > On Thu, 7 May 2020 at 01:52, Stephan Bergmann via cfe-dev
>      > <[hidden email] <mailto:[hidden email]>
>     <mailto:[hidden email] <mailto:[hidden email]>>> 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>
>      >     <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.

Indeed, not emitting -Wmissing-field-initializers for deprecated members
at the end of a struct would look like the best solution in this
particular case.  But would it be a good choice in general, and is
"deprecated members a the end of a struct" a common enough scenario to
even bother?  After all, maybe it is best to just leave things as they
are now...

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev