clang's -Wdeprecated-non-prototype warning points at the wrong line

[bhaible]

Clang's -Wdeprecated-non-prototype warning can warn about a conflict between a function declaration and the definition of the same function. That's good.

When warning about a conflict, using the current style with "warning: ..." and "note: ...", one of the two lines must be mentioned in the "warning", and the other one in the "note".

Unfortunately, Clang's choice to put the function declaration in the "warning" line and the function definition in the "note" line has a silly effect: The same header file, included by two different compilation units, shows a warning in one compilation unit but not in the other compilation unit.

How to reproduce (with Clang 15.0.6):
Put this in foo.h:

void func3 ();

Put this in foo1.c:

#include "foo.h"
void func3 (int x, int y) {}

Put this in foo2.c:

#include "foo.h"

Compile foo1.c and foo2.c:

$ clang -S -Wdeprecated-non-prototype foo1.c
In file included from foo1.c:1:
./foo.h:1:6: warning: a function declaration without a prototype is deprecated in all versions of C and is treated as a zero-parameter prototype in C2x, conflicting with a subsequent definition [-Wdeprecated-non-prototype]
void func3 ();
     ^
foo1.c:2:6: note: conflicting prototype is here
void func3 (int x, int y) {}
     ^
1 warning generated.
$ clang -S -Wdeprecated-non-prototype foo2.c

Suggested fix:

$ clang -S -Wdeprecated-non-prototype foo1.c
foo1.c:2:6: warning: function definition conflicts with previous non-prototype declaration that will be treated like a zero-parameter prototype in C2x [-Wdeprecated-non-prototype]
void func3 (int x, int y) {}
     ^
In file included from foo1.c:1:
./foo.h:1:6: note: conflicting function declaration is here
void func3 ();
     ^
1 warning generated.

[AaronBallman]

The situation you point out is a false negative that we miss (where foo1.c expects arguments but it's possible to call the function from foo2.c with no arguments and no warning).

The reasoning behind the current behavior is that the function with a prototype is the preferred approach and the function without the prototype is what should be warned on because that's the functionality that's changing (in addition to already being deprecated). The suggestion here is to reverse that logic and warn about the preferred approach. (NB -- I have no data one way or the other that users are more likely to change code labeled with a warning vs code labeled with a note. It'd be interesting if we could get any data about this. CC @cjdb as this sort of relates to the diagnostic improvement work he's been looking into.)

The way the diagnostic works in this case is that we have to see the conflicting declaration and definition to know there's a problem. (Otherwise, the user could be doing void func(); void func() {} which is not a breaking change in C2x and thus not something we want to diagnose by default.) In the foo1.c TU, we see both declarations and we can diagnose. In the foo2.c TU, we will only see the declaration without a prototype and we won't diagnose until we see a call expression. If the call expression passes arguments, then we'll diagnose (because this code will definitely break in C2x), and if the call expression doesn't pass arguments, we won't diagnose (because the code will be fine in C2x). Moving the warning from the definition to the declaration won't change this.

So:

Unfortunately, clang's choice to put the function declaration in the "warning" line and the function definition in the "note" line has a silly effect: The same header file, included by two different compilation units, shows a warning in one compilation unit but not in the other compilation unit.

is by careful design intended to reduce the false positive rate of the diagnostic. If the goal is "warn me about any declaration of a function without a prototype", then -Wstrict-prototypes does that for you in Clang and that would get you the diagnostic you're after when compiling foo2.c, but at the expense of potential chattiness about nonissues. Given that it's common for a TU to include a header file and not use all of the functions from the header, I think the current behavior for -Wdeprecated-non-prototype in Clang is what we'd want.

[llvmbot]

@llvm/issue-subscribers-c

[bhaible]

The reasoning behind the current behavior is that the function with a prototype is the preferred approach

You are arguing with concepts of the past. If the goal of the warning was to provide safe code for C17, I would agree with you. But the warning option -Wdeprecated-non-prototype was introduced precisely for the migration to C23. In C23, there is no concept of prototyped vs. unprototyped function declarations/types/definitions any more; all function declarations/types/definitions are prototyped. So both void func3 (); and void func3 (int x, int y) {} are equally valid in C23. Picking the declaration as the place to stick the "warning:" on is arbitrary.

and the function without the prototype is what should be warned on because that's the functionality that's changing

Here you are assuming that the programmer has not done the migration from K&R C code to code with prototypes in 33 years. That is one possibility; but the other possibility is that they have done this migration already, and they have left-over unused arguments in the void func3 (int x, int y) {} line.

In other words, you are trying to predict which of two elements of the conflict is the one the programmer will want to change. 1. It's not clear that your prediction is correct more than 50% of the time. 2. Programmers usually consider both the 'warning:' line and the 'note:' lines before deciding which one to edit/correct.

[AaronBallman]

But the warning option -Wdeprecated-non-prototype was introduced precisely for the migration to C23.

Exactly. In C17 and before, that code was valid. In C23, it's no longer valid. The definition provides strictly more information than the declaration does, so the most likely fix for a user who needs to migrate from C17 to C23 is to fix the declaration, not the definition. Hence we warn on the declaration.

Here you are assuming that the programmer has not done the migration from K&R C code to code with prototypes in 33 years.

Sadly, longer than that. It was deprecated in 1978 with K&R C 2nd edition. But that assumption is not unfounded, as a rather public example shows: madler/zlib#633 (comment) Further, there's a lot of unmaintained C code in various distro packages that needs to keep working, etc.

At the end of the day, I'm still not certain there's a path forward to warn about the declaration in foo2.c. The compiler only sees one TU at a time, so warning about the declaration in that case is effectively the same as -Wstrict-prototypes. There's no reason to believe void f(); in isolation (no definition, no other declarations, no calls) is an issue worth diagnosing due to the false positive rate.

[AaronBallman]

Closing this as not a bug because it's behaving by design, and I don't think Clang's behavior is going to change in this area. However, if you disagree, feel free to reopen the issue with more information so we can continue the discussion.