[Clang] Avoid misleading 'conflicting types' diagnostic with no-prototype decls.

[cishida]

Summary

Clang has recently started diagnosing prototype redeclaration errors like rG385e7df33046

This flagged legitimate issues in a codebase but was confusing to resolve because it actually conflicted with a function declaration from a system header and not from the one emitted with "note: ".

This patch updates the error handling to use the canonical declaration's source location instead to avoid misleading errors like the one described.

[cishida]

Restrict using the canonical decl's source location to only when the prototype was inferred from previously seen decl.

[aaron.ballman]

LGTM, thank you for catching this!

clang/lib/Sema/SemaDecl.cpp

3914–3918

I don't think it's safe to assign this new value to Old as that object is what we eventually merge the declaration with (so we'll get the decl chain order wrong, I believe).

[cishida]

clang/lib/Sema/SemaDecl.cpp

3914–3918

Ah, I hadn't realized we still can merge decls together in a fatal error case.

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

3914–3918

Oh shoot, good point, we early return here anyway, so the assignment won't matter. So the code LGTM as-is, thank you!

clang/lib/Sema/SemaDecl.cpp

if (!getLangOpts().CPlusPlus) {
     // 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 = 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,
     // we may have enough information to decide to issue a diagnostic that the
     // function without a protoype will change behavior in C2x. This handles
     // cases like:
     //   void i(); void i(int j);
     //   void i(int j); void i();
     //   void i(); void i(int j) {}

clang/test/Sema/prototype-redecls.c

 // RUN: %clang_cc1 -fsyntax-only -Wno-strict-prototypes -verify %s
 
 void blapp(int); // expected-note {{previous}}
 void blapp() { } // expected-error {{conflicting types for 'blapp'}}
 
 void yarp(int, ...); // expected-note {{previous}}
 void yarp();         // expected-error {{conflicting types for 'yarp'}}
 
 void blarg(int, ...); // expected-note {{previous}}
 void blarg() {}       // expected-error {{conflicting types for 'blarg'}}
 
 void blerp(short);      // expected-note {{previous}}
 void blerp(x) int x; {} // expected-error {{conflicting types for 'blerp'}}
 
+void foo(int); // expected-note {{previous}}
+void foo();
+void foo() {} // expected-error {{conflicting types for 'foo'}}
+
 void glerp(int);
 void glerp(x) short x; {} // Okay, promoted type is fine
 
 // All these cases are okay
 void derp(int);
 void derp(x) int x; {}
 
 void garp(int);