Change prototype merging error into a warning for builtins

[aaron.ballman]

Summary

As was observed in https://reviews.llvm.org/D123627#3707635, it's confusing that a user can write:

float rintf(void) {}

and get a warning, but writing:

float rintf() {}

gives an error. This patch changes the behavior so that both are warnings, so that users who have functions which conflict with a builtin identifier can still use that identifier as they wish.

Note, if we accept this, I would like to backport it to the Clang 15.x branch because the original change was made in Clang 15.

[erichkeane]

clang/test/Sema/prototype-redecls.c

32

Hmm... this is a definition of a builtin with a completely incompatible prototype. Do we REALLY want this to not be an error?

I guess I could see it being OK with declarations, but it is odd with a definition that is incompatible.

[aaron.ballman]

clang/test/Sema/prototype-redecls.c

32

I think we do want it to not be an error. The user has no choice in whether these are predeclared or not, which steals identifiers from users. By giving the user a warning, they're alerted to the fact they're doing something that may be highly confusing, so I agree we definitely want a diagnostic. If we give the user an error, they're stuck. Also, it's even weirder that:

float rintf(void) {} // Always a warning, is fine
float rintf() {} // Error pre C2x, but fine in C2x

[erichkeane]

Agree with backporting to Clang15, that'll break the fewest users.

clang/test/Sema/prototype-redecls.c

32

Aren't all those identifiers already stolen by the standard? They are reserved already, right?

But that behavior you post there is unfortunate... I guess leaving these as warnings is important so they get suppressed in headers (for cases where a C-library 'implements' it anyway, despite it being a builtin).

I think I can live with this, and I think backporting it is the least breaking way to do this.

[aaron.ballman]

clang/test/Sema/prototype-redecls.c

32

Aren't all those identifiers already stolen by the standard? They are reserved already, right?

Yes, they are. However, the situation we should have sympathy for is a user who is compiling in C17 mode and using an identifier like ckd_add suddenly having that identifier stolen out from under them because C2x added that as a library function and we decided we wanted it to be a builtin for some reason.

clang/lib/Sema/SemaDecl.cpp

if (!getLangOpts().CPlusPlus) {
     // type of the corresponding identifier. ...
     // This cannot be handled by ASTContext::typesAreCompatible() because that
     // doesn't know whether the function type is for a definition or not when
     // eventually calling ASTContext::mergeFunctionTypes(). The only situation
     // we need to cover here is that the number of arguments agree as the
     // default argument promotion rules were already checked by
     // ASTContext::typesAreCompatible().
     if (Old->hasPrototype() && !New->hasWrittenPrototype() && NewDeclIsDefn &&
-        Old->getNumParams() != New->getNumParams()) {
+        Old->getNumParams() != New->getNumParams() && !Old->isImplicit()) {
       if (Old->hasInheritedPrototype())
         Old = Old->getCanonicalDecl();
       Diag(New->getLocation(), diag::err_conflicting_types) << New;
       Diag(Old->getLocation(), PrevDiag) << Old << Old->getType();
       return true;
     }
 
     // If we are merging two functions where only one of them has a prototype,

clang/test/Sema/prototype-redecls.c

 void derp(x) int x; {}
 
 void garp(int);
 void garp();
 void garp(x) int x; {}
 
 // Ensure redeclarations that conflict with a builtin use a note which makes it
 // clear that the previous declaration was a builtin.
-float rintf() { // expected-error {{conflicting types for 'rintf'}} \
+float rintf() { // expected-warning {{incompatible redeclaration of library function 'rintf'}} \
                    expected-note {{'rintf' is a builtin with type 'float (float)'}}
   return 1.0f;
 }