[clang][Diagnostics] Emit fix-it hint separately on overload resolution failure

[hazohelet]

Summary

D153359 addressed the distant source ranges in overload resolution failure, but still clang generates distant source locations when emitting fix-it hint about the addition/deletion of * and & operators.
This patch separates the fix-it emission to another note to resolve this issue.
Code Example:

void f(int n);


void g(int *p) { f(p); }

Before:

source:4:18: error: no matching function for call to 'f'
    4 | void g(int *p) { f(p); }
      |                  ^
source:1:6: note: candidate function not viable: no known conversion from 'int *' to 'int' for 1st argument; dereference the argument with *
    1 | void f(int n);
      |      ^ ~~~~~
    2 |
    3 |
    4 | void g(int *p) { f(p); }
      |
      |                    *

After:

source:4:18: error: no matching function for call to 'f'
    4 | void g(int *p) { f(p); }
      |                  ^
source:1:6: note: candidate function not viable: no known conversion from 'int *' to 'int' for 1st argument
    1 | void f(int n);
      |      ^ ~~~~~
source:4:20: note: dereference the argument with *
    4 | void g(int *p) { f(p); }
      |                    ^
      |                    *

[hazohelet]

This breaks the one-note-for-one-overload-candidate rule of overload resolution failure diagnostics (https://github.com/llvm/llvm-project/blob/ff08c8e57e39d7970b65637595cdc221901f4ed1/clang/lib/Sema/SemaOverload.cpp#L11517-L11526), but in most cases this change would produce less lines of diagnostics.
If we don't like this additional note, we could instead suppress the fix-it diagnostics when the fix-it location and the diagnosed candidate location are not close (for example, more than 2 lines away).

[hazohelet]

Fixed clangd test

[hazohelet]

Ping

[hazohelet]

Ping

[hazohelet]

Ping

[tbaeder]

In D158472#4605228, @hazohelet wrote:

This breaks the one-note-for-one-overload-candidate rule of overload resolution failure diagnostics (https://github.com/llvm/llvm-project/blob/ff08c8e57e39d7970b65637595cdc221901f4ed1/clang/lib/Sema/SemaOverload.cpp#L11517-L11526), but in most cases this change would produce less lines of diagnostics.
If we don't like this additional note, we could instead suppress the fix-it diagnostics when the fix-it location and the diagnosed candidate location are not close (for example, more than 2 lines away).

IIRC @aaron.ballman usually has an opinion on rules like that.

[aaron.ballman]

In D158472#4605228, @hazohelet wrote:

This breaks the one-note-for-one-overload-candidate rule of overload resolution failure diagnostics (https://github.com/llvm/llvm-project/blob/ff08c8e57e39d7970b65637595cdc221901f4ed1/clang/lib/Sema/SemaOverload.cpp#L11517-L11526), but in most cases this change would produce less lines of diagnostics.
If we don't like this additional note, we could instead suppress the fix-it diagnostics when the fix-it location and the diagnosed candidate location are not close (for example, more than 2 lines away).

I'm uncomfortable with starting down the slippery slope of multiple notes per overload resolution failure; that leads to an explosion of extra diagnostics which makes it harder to spot the issue in the first place. My inclination is to try to resolve this by fixing the way we emit fix-it diagnostics so that we remove the extra whitespace from there because this seems like it's a general problem with fix-its. Or am I wrong about this being a general issue? CC @cjdb for opinions as well

clang/test/Sema/overloadable.c

31

Something odd is going on -- why is the note emitted twice?

[hazohelet]

In D158472#4645378, @aaron.ballman wrote:

In D158472#4605228, @hazohelet wrote:

This breaks the one-note-for-one-overload-candidate rule of overload resolution failure diagnostics (https://github.com/llvm/llvm-project/blob/ff08c8e57e39d7970b65637595cdc221901f4ed1/clang/lib/Sema/SemaOverload.cpp#L11517-L11526), but in most cases this change would produce less lines of diagnostics.
If we don't like this additional note, we could instead suppress the fix-it diagnostics when the fix-it location and the diagnosed candidate location are not close (for example, more than 2 lines away).

I'm uncomfortable with starting down the slippery slope of multiple notes per overload resolution failure; that leads to an explosion of extra diagnostics which makes it harder to spot the issue in the first place. My inclination is to try to resolve this by fixing the way we emit fix-it diagnostics so that we remove the extra whitespace from there because this seems like it's a general problem with fix-its. Or am I wrong about this being a general issue? CC @cjdb for opinions as well

I agree that the ideal fix would be a systematic change in how we display fix-it hints, or for that matter, distant source ranges in general.
We probably want output that looks like this:

<source>:1:6: note: candidate function not viable: no known conversion from 'int' to 'int *' for 1st argument; take the address of the argument with &
    1 | void ptr(int *p);
      |      ^   ~~~~~~
    ---
    5 |   ptr(n);
      |       
      |       &

In any case, suppressing fix-it hints would be disastrous for LSP things like clangd that use clang diagnostic output. So, this is a problem of how clang diagnostic engine display fix-it hints.

clang/test/Sema/overloadable.c

31

There are two candidates, both of which would become viable if we take the address of them. (https://godbolt.org/z/6onn9x89a)

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

TEST(DiagnosticsTest, FixItFromHeader) {
   TestTU TU;
   TU.Code = Source.code().str();
   TU.HeaderCode = Header.str();
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       UnorderedElementsAre(AllOf(
           Diag(Source.range("diag"), "no matching function for call to 'foo'"),
           withFix(Fix(Source.range("fix"), "&",
-                      "candidate function not viable: no known conversion from "
-                      "'int' to 'int *' for 1st argument; take the address of "
-                      "the argument with &")))));
+                      "take the address of the argument with &")))));
 }
 
 TEST(DiagnosticsTest, UnusedInHeader) {
   // Clang diagnoses unused static inline functions outside headers.
   auto TU = TestTU::withCode("static inline void foo(void) {}");
   TU.ExtraArgs.push_back("-Wunused-function");
   TU.Filename = "test.c";
   EXPECT_THAT(TU.build().getDiagnostics(),

clang/docs/ReleaseNotes.rst

 - Clang contexpr evaluator now displays notes as well as an error when a constructor
   of a base class is not called in the constructor of its derived class.
 - Clang no longer emits ``-Wmissing-variable-declarations`` for variables declared
   with the ``register`` storage class.
 - Clang's ``-Wtautological-negation-compare`` flag now diagnoses logical
   tautologies like ``x && !x`` and ``!x || x`` in expressions. This also
   makes ``-Winfinite-recursion`` diagnose more cases.
   (`#56035: <https://github.com/llvm/llvm-project/issues/56035>`_).
+- Clang now emits a separate note for fix-it hint about the use of ``*`` and ``&``
+  unary operators in overload resolution failure.
 
 Bug Fixes in This Version
 -------------------------
 - Fixed an issue where a class template specialization whose declaration is
   instantiated in one module and whose definition is instantiated in another
   module may end up with members associated with the wrong declaration of the
   class, which can result in miscompiles in some cases.
 - Fix crash on use of a variadic overloaded operator.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_ovl_candidate_bad_list_argument : Note<
     "|too many initializers in list}7 argument to %4">;
 def note_ovl_candidate_bad_overload : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "no overload of %4 matching %3 for %ordinal5 argument">;
 def note_ovl_candidate_bad_conv : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "no known conversion "
     "%diff{from $ to $|from argument type to parameter type}3,4 for "
-    "%select{%ordinal6 argument|object argument}5"
-    "%select{|; dereference the argument with *|"
-    "; take the address of the argument with &|"
-    "; remove *|"
-    "; remove &}7">;
+    "%select{%ordinal6 argument|object argument}5">;
+def note_ovl_candidate_fixit : Note<
+    "%select{|dereference the argument with *|"
+    "take the address of the argument with &|"
+    "remove *|"
+    "remove &}0">;
 def note_ovl_candidate_bad_arc_conv : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "cannot implicitly convert argument "
     "%diff{of type $ to $|type to parameter type}3,4 for "
     "%select{%ordinal6 argument|object argument}5 under ARC">;
 def note_ovl_candidate_bad_value_category : Note<
     "candidate %sub{select_ovl_candidate_kind}0,1,2 not viable: "
     "expects an %select{lvalue|rvalue}5 for "

clang/lib/Sema/SemaOverload.cpp

if (isa<ObjCObjectPointerType>(CFromTy) &&
     }
   }
 
   if (TakingCandidateAddress &&
       !checkAddressOfCandidateIsAvailable(S, Cand->Function))
     return;
 
   // Emit the generic diagnostic and, optionally, add the hints to it.
-  PartialDiagnostic FDiag = S.PDiag(diag::note_ovl_candidate_bad_conv);
-  FDiag << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-        << ToParamRange << FromTy << ToTy << (unsigned)isObjectArgument << I + 1
-        << (unsigned)(Cand->Fix.Kind);
+  S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_conv)
+      << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
+      << ToParamRange << FromTy << ToTy << (unsigned)isObjectArgument << I + 1;
 
   // Check that location of Fn is not in system header.
-  if (!S.SourceMgr.isInSystemHeader(Fn->getLocation())) {
+  if (!S.SourceMgr.isInSystemHeader(Fn->getLocation()) &&
+      Cand->Fix.Hints.size() > 0) {
     // If we can fix the conversion, suggest the FixIts.
+    PartialDiagnostic HintDiag = S.PDiag(diag::note_ovl_candidate_fixit)
+                                 << (unsigned)(Cand->Fix.Kind);
     for (const FixItHint &HI : Cand->Fix.Hints)
-        FDiag << HI;
+      HintDiag << HI;
+    const FixItHint &FirstHint = Cand->Fix.Hints[0];
+    SourceLocation FirstHintLoc = FirstHint.RemoveRange.getBegin();
+    FirstHintLoc = FirstHintLoc.isInvalid()
+                       ? FirstHint.InsertFromRange.getBegin()
+                       : FirstHintLoc;
+    S.Diag(FirstHintLoc, HintDiag);
   }
 
-  S.Diag(Fn->getLocation(), FDiag);
-
   MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
 }
 
 /// Additional arity mismatch diagnosis specific to a function overload
 /// candidates. This is not covered by the more general DiagnoseArityMismatch()
 /// over a candidate in any candidate set.
 static bool CheckArityMismatch(Sema &S, OverloadCandidate *Cand,
                                unsigned NumArgs) {

clang/test/FixIt/fixit-function-call.cpp

 
 void f0(int *a);
 void f1(double *a);
 void f1(intTy &a);
 
 void f2(intTy2 *a) {
 // CHECK: error: no matching function for call to 'f1
 // CHECK: dereference the argument with *
-// CHECK: void f1(intTy &a);
+// CHECK-NEXT: f1(a + 1);
 // CHECK: fix-it{{.*}}*(
 // CHECK-NEXT: fix-it{{.*}})
 // CHECK: void f1(double *a);
   f1(a + 1);
 
 // This call cannot be fixed since without resulting in null pointer dereference.
 // CHECK: error: no matching function for call to 'f1
 // CHECK-NOT: dereference the argument with *

 
 // TODO: Test that we do not provide a fixit when inheritance is private.
 // CHECK: error: no matching function for call to 'bp
 // There should not be a fixit here:
 // CHECK: fix-it
   bp(c);
 
 // CHECK: no matching function for call to 'u'
-// CHECK: candidate function not viable: no known conversion from 'C' to 'const C *' for 1st argument; take the address of the argument with &
+// CHECK: candidate function not viable: no known conversion from 'C' to 'const C *' for 1st argument
+// CHECK: take the address of the argument with &
 // CHECK: candidate function not viable
 // CHECK: candidate function not viable
   u(c);
 }
 
 void accept_void(void*);
 
 void issue58958(const char* a, volatile char * v, const volatile char * cv) {

clang/test/Index/fixit-sysheader-test.cpp

 // RUN: c-index-test -test-load-source all %s 2>&1 | FileCheck %s
 
 #include "fixit-sys-header.h"
 #include "fixit-user-header.h"
 
 int main(int argc, char const *argv[])
 {
     char* str;{};
     
     func_in_sys_header(str, str + 10);
     // CHECK: Number FIX-ITs = 0
-    // CHECK-NEXT: candidate function not viable: no known conversion from 'char *' to 'unsigned long' for 2nd argument; dereference the argument with *
+    // CHECK-NEXT: candidate function not viable: no known conversion from 'char *' to 'unsigned long' for 2nd argument
     // CHECK-NEXT: Number FIX-ITs = 0
     
     func_in_user_header(str, str + 10);
     // CHECK: Number FIX-ITs = 0
-    // CHECK-NEXT: candidate function not viable: no known conversion from 'char *' to 'unsigned long' for 2nd argument; dereference the argument with *
+    // CHECK-NEXT: candidate function not viable: no known conversion from 'char *' to 'unsigned long' for 2nd argument
+    // CHECK-NEXT: Number FIX-ITs = 0
+    // CHECK-NEXT: dereference the argument with *
     // CHECK-NEXT: Number FIX-ITs = 2
 
     return 0;
 }

clang/test/Sema/overloadable.c

 int *accept_funcptr(int (*)()) __attribute__((overloadable)); //         \
   // expected-note{{candidate function}}
 float *accept_funcptr(int (*)(int, double)) __attribute__((overloadable)); //  \
   // expected-note{{candidate function}}
 
 void test_funcptr(int (*f1)(int, double),
                   int (*f2)(int, float)) {
   float *fp = accept_funcptr(f1);
-  accept_funcptr(f2); // expected-error{{no matching function for call to 'accept_funcptr'}}
+  accept_funcptr(f2); // expected-error{{no matching function for call to 'accept_funcptr'}} \
+                      // expected-note 2{{take the address of the argument with &}}
 }
 
 struct X { int x; float y; };
 struct Y { int x; float y; };
 int* accept_struct(struct X x) __attribute__((__overloadable__));
 float* accept_struct(struct Y y) __attribute__((overloadable));
 
 void test_struct(struct X x, struct Y y) {

clang/test/SemaCXX/MicrosoftExtensions.cpp

 int foo_unaligned(__unaligned A_unaligned *arg) { return 0; } // expected-note {{not viable: no known conversion from 'const __unaligned int *' to '__unaligned A_unaligned *'}}
 void *foo_unaligned(B_unaligned *arg) { return 0; } // expected-note {{not viable: no known conversion from 'const __unaligned int *' to 'B_unaligned *'}}
 
 void test_unaligned() {
   int *p1 = 0;
   foo_unaligned(p1);
 
   const __unaligned int *const_p1 = 0;
-  foo_unaligned(const_p1); // expected-error {{no matching function for call to 'foo_unaligned'}}
+  foo_unaligned(const_p1); // expected-error {{no matching function for call to 'foo_unaligned'}} \
+                           // expected-note {{dereference the argument with *}}
 
   __unaligned int *p2 = 0;
   foo_unaligned(p2);
 
   __unaligned B_unaligned *p3 = 0;
   int p4 = foo_unaligned(p3); // expected-error {{cannot initialize a variable of type 'int' with an rvalue of type 'void *'}}
   // expected-warning@-1 {{implicit cast from type '__unaligned B_unaligned *' to type 'B_unaligned *' drops __unaligned qualifier}}
 

clang/test/SemaCXX/builtin-operator-new-delete.cpp

 
 void test_no_args() {
   __builtin_operator_new();    // expected-error {{no matching function for call to 'operator new'}}
   __builtin_operator_delete(); // expected-error {{no matching function for call to 'operator delete'}}
 }
 
 void test_no_matching_fn() {
   Tag<1> tag;
-  __builtin_operator_new(42, tag);    // expected-error {{no matching function for call to 'operator new'}}
-  __builtin_operator_delete(NP, tag); // expected-error {{no matching function for call to 'operator delete'}}
+  __builtin_operator_new(42, tag);    // expected-error {{no matching function for call to 'operator new'}} \
+                                      // expected-note {{take the address of the argument with &}}
+  __builtin_operator_delete(NP, tag); // expected-error {{no matching function for call to 'operator delete'}} \
+                                      // expected-note {{take the address of the argument with &}}
 }
 
 template <class Tp, class Up, class RetT>
 void test_dependent_call(Tp new_arg, Up delete_arg, RetT) {
   RetT ret = __builtin_operator_new(new_arg);
   __builtin_operator_delete(delete_arg);
 }
 template void test_dependent_call(int, int*, void*);
 
 void test_const_attribute() {
   __builtin_operator_new(42); // expected-warning {{ignoring return value of function declared with const attribute}}
 #ifdef __cpp_aligned_new
   __builtin_operator_new(42, std::align_val_t(8)); // expected-warning {{ignoring return value of function declared with const attribute}}
 #endif
 }

clang/test/SemaCXX/crashes.cpp

 #endif
 
   Y();
 };
 
 struct {
   Y obj;
 } objs[] = {
-  new Y // expected-error{{no viable conversion}}
+  new Y // expected-error{{no viable conversion}} \
+        // expected-note{{dereference the argument with *}}
 };
 }
 
 // http://llvm.org/PR8234
 namespace PR8234 {
 template<typename Signature>
 class callback
 {

clang/test/SemaCXX/for-range-dereference.cpp

void f() {
   NoBegin **ppNB;
   for (auto i : ppNB) { }// expected-error{{invalid range expression of type 'NoBegin **'; no viable 'begin' function available}}
   NoBegin *****pppppNB;
   for (auto i : pppppNB) { }// expected-error{{invalid range expression of type 'NoBegin *****'; no viable 'begin' function available}}
 
   ADLNoEnd ANE;
   for (auto i : ANE) { } // expected-error{{invalid range expression of type 'ADLNoEnd'; no viable 'end' function available}}
   ADLNoEnd *pANE;
-  for (auto i : pANE) { } // expected-error{{invalid range expression of type 'ADLNoEnd *'; no viable 'begin' function available}}
+  for (auto i : pANE) { } // expected-error{{invalid range expression of type 'ADLNoEnd *'; no viable 'begin' function available}} \
+                          // expected-note {{dereference the argument with *}}
 
   DeletedEnd DE;
   for (auto i : DE) { } // expected-error{{attempt to use a deleted function}} \
 expected-note {{when looking up 'end' function for range expression of type 'DeletedEnd'}}
   DeletedEnd *pDE;
 
   for (auto i : pDE) { } // expected-error {{invalid range expression of type 'DeletedEnd *'; no viable 'begin' function available}}
 

clang/test/SemaCXX/overload-member-call.cpp

namespace b7398190 {
   };
   const S *p;
   int k = p->f(); // expected-error {{no matching member function for call to 'f'}}
 }
 
 void member_call_op_template(int *p) {
   // Ensure that we don't get confused about relative parameter / argument
   // indexing here.
-  [](int, int, auto...){}(p, p); // expected-error {{no matching function}} expected-note {{no known conversion}}
+  [](int, int, auto...){}(p, p); // expected-error {{no matching function}} expected-note {{no known conversion}} \
+                                 // expected-note {{dereference the argument with *}}
 }
 

clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl

 // Test that overloads that use atomic_double are not available when the fp64
 // extension is disabled.  Test this by counting the number of notes about
 // candidate functions.
 void test_atomic_double_reporting(volatile __generic atomic_int *a) {
   atomic_init(a, a);
   // expected-error@-1{{no matching function for call to 'atomic_init'}}
 #if defined(NO_FP64)
   // Expecting 5 candidates: int, uint, long, ulong, float
-  // expected-note@-4 5 {{candidate function not viable: no known conversion}}
+  // expected-note@-4 5 {{candidate function not viable: no known conversion}} \
+  // expected-note@-4 5{{dereference the argument with *}}
 #else
   // Expecting 6 candidates: int, uint, long, ulong, float, double
-  // expected-note@-7 6 {{candidate function not viable: no known conversion}}
+  // expected-note@-8 6 {{candidate function not viable: no known conversion}} \
+  // expected-note@-8 6{{dereference the argument with *}}
 #endif
 }
 
 #endif
 
 #if defined(NO_ATOMSCOPE) && __OPENCL_C_VERSION__ >= 300
 // Disable the feature by undefining the feature macro.
 #undef __opencl_c_atomic_scope_device

clang/test/SemaTemplate/instantiate-expr-4.cpp

 
 template struct New1<int, float>;
 template struct New1<Y, Y>;
 template struct New1<X, Y>; // expected-note{{instantiation}}
 
 template<typename T, typename Arg1, typename Arg2>
 struct New2 {
   T* f(bool x, Arg1 a1, Arg2 a2) {
-    return new T(a1, a2); // expected-error{{no matching}}
+    return new T(a1, a2); // expected-error{{no matching}} \
+                          // expected-note{{dereference the argument with *}}
   }
 };
 
 template struct New2<X, int, float>;
 template struct New2<X, int, int*>; // expected-note{{instantiation}}
 // FIXME: template struct New2<int, int, float>;
 
 // PR5833