[clang][Diagnostics] Fix distant source ranges in bad-conversion notes

[hazohelet]

Summary

Now that clang supports printing of multiple lines of code snippet in diagnostics, source ranges in diagnostics that are located in different lines from the diagnosed source location get to be printed if the gap happened to be less than the maximum number of lines clang is allowed to print in.
Many of the bad-conversion notes in overload resolution failures have their source location in the function declaration and source range in the argument of function call. This can cause an unnecessarily many lines of snippet printing.
This patch fixes it by changing the source range from function callsite to the problematic parameter in function declaration.

e.g.

void func(int aa, int bb);


void test() { func(1, "two"); }

BEFORE this patch:

source:4:15: error: no matching function for call to 'func'
    4 | void test() { func(1, "two"); }
      |               ^~~~
source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
    1 | void func(int aa, int bb);
      |      ^
    2 |
    3 |
    4 | void test() { func(1, "two"); }
      |                       ~~~~~
1 error generated.

AFTER this patch:

source:4:15: error: no matching function for call to 'func'
    4 | void test() { func(1, "two"); }
      |               ^~~~
source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
    1 | void func(int aa, int bb);
      |      ^            ~~~~~~

[tbaeder]

Can you post a before/after comparison of the exact output for that example?

clang/lib/Sema/SemaOverload.cpp

10756

Can this be const?

10756

Actually, ToPVD is only used to access its source range later, isn't it? If so, can we just declare the a ToPVDRange or something here and use that later for diagnostics?

[hazohelet]

Address comments from @tbaeder

• Bind parameter source range rather than its declaration to variable.

[hazohelet]

Consider the following code. (I added another parameter to the original code so that the covered range becomes clearer)

void func(int aa, int bb);


void test() { func(1, "two"); }

BEFORE this patch:

source:4:15: error: no matching function for call to 'func'
    4 | void test() { func(1, "two"); }
      |               ^~~~
source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
    1 | void func(int aa, int bb);
      |      ^
    2 |
    3 |
    4 | void test() { func(1, "two"); }
      |                       ~~~~~

AFTER this patch:

source:4:15: error: no matching function for call to 'func'
    4 | void test() { func(1, "two"); }
      |               ^~~~
source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
    1 | void func(int aa, int bb);
      |      ^            ~~~~~~

clang/lib/Sema/SemaOverload.cpp

10756

Right. I fixed it.

[cjdb]

I like this patch, thanks for working on it 😄

In D153359#4436873, @hazohelet wrote:

Consider the following code. (I added another parameter to the original code so that the covered range becomes clearer)

void func(int aa, int bb);


void test() { func(1, "two"); }

BEFORE this patch:

source:4:15: error: no matching function for call to 'func'
    4 | void test() { func(1, "two"); }
      |               ^~~~
source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
    1 | void func(int aa, int bb);
      |      ^
    2 |
    3 |
    4 | void test() { func(1, "two"); }
      |                       ~~~~~

AFTER this patch:

source:4:15: error: no matching function for call to 'func'
    4 | void test() { func(1, "two"); }
      |               ^~~~
source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
    1 | void func(int aa, int bb);
      |      ^            ~~~~~~

Having this in the commit message would be great, thanks!

clang/lib/Sema/SemaOverload.cpp

10825

Please don't commit until this is resolved (either tests added or it's removed).

[hazohelet]

clang/lib/Sema/SemaOverload.cpp

10825

I opened another diff to remove this block: D153690

[hazohelet]

Removed FIXME comment that is already addressed in D153690

[cjdb]

Per this Discourse post, I think it'd be best for the example I asked to be in your commit message to actually be in your release notes. Sorry for the churn!

[hazohelet]

Address comment from cjdb

• Added example code & before/after diagnostic display to release note

Thanks for reminding!

[tbaeder]

@cjdb I know this is kind of silly after @hazohelet has already added it to the release notes... but it seems like showing the difference is useless since the difference was only introduced during the clang 17 development.(?) For a 16 -> 17 transition, it probably causes almost no change.

[cjdb]

In D153359#4463009, @tbaeder wrote:

@cjdb I know this is kind of silly after @hazohelet has already added it to the release notes... but it seems like showing the difference is useless since the difference was only introduced during the clang 17 development.(?) For a 16 -> 17 transition, it probably causes almost no change.

Ah, fair enough then.

[hazohelet]

Edited release note so that it only mentions the clang 16 -> 17 changes, and not development internal ones

[tbaeder]

@cjdb LGTY?

[cjdb]

Yep! Thanks for working on this :-)

clang/docs/ReleaseNotes.rst

- Clang now diagnoses unexpected tokens after a
   (`#13920: <https://github.com/llvm/llvm-project/issues/13920>`_)
 - Clang now does not try to analyze cast validity on variables with dependent alignment (`#63007: <https://github.com/llvm/llvm-project/issues/63007>`_).
 - Clang constexpr evaluator now displays member function calls more precisely
   by making use of the syntactical structure of function calls. This avoids display
   of syntactically invalid codes in diagnostics.
   (`#57081: <https://github.com/llvm/llvm-project/issues/57081>`_)
 - Clang no longer emits inappropriate notes about the loss of ``__unaligned`` qualifier
   on overload resolution, when the actual reason for the failure is loss of other qualifiers.
+- Clang's notes about unconvertible types in overload resolution failure now covers
+  the source range of parameter declaration of the candidate function declaration.
+  
+  *Example Code*:
+
+  .. code-block:: c++
+
+     void func(int aa, int bb);
+     void test() { func(1, "two"); }
+
+  *BEFORE*:
+
+  .. code-block:: text
+
+    source:2:15: error: no matching function for call to 'func'
+    void test() { func(1, "two");  }
+                  ^~~~
+    source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
+    void func(int aa, int bb);
+         ^
+
+  *AFTER*:
+
+  .. code-block:: text
+
+    source:2:15: error: no matching function for call to 'func'
+    void test() { func(1, "two");  }
+                  ^~~~
+    source:1:6: note: candidate function not viable: no known conversion from 'const char[4]' to 'int' for 2nd argument
+    void func(int aa, int bb);
+         ^            ~~~~~~
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix segfault while running clang-rename on a non existing file.
   (`#36471 <https://github.com/llvm/llvm-project/issues/36471>`_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.

clang/lib/Sema/SemaOverload.cpp

static void DiagnoseBadConversion(Sema &S, OverloadCandidate *Cand,
   std::string FnDesc;
   std::pair<OverloadCandidateKind, OverloadCandidateSelect> FnKindPair =
       ClassifyOverloadCandidate(S, Cand->FoundDecl, Fn, Cand->getRewriteKind(),
                                 FnDesc);
 
   Expr *FromExpr = Conv.Bad.FromExpr;
   QualType FromTy = Conv.Bad.getFromType();
   QualType ToTy = Conv.Bad.getToType();
+  SourceRange ToParamRange =
+      !isObjectArgument ? Fn->getParamDecl(I)->getSourceRange() : SourceRange();
 
   if (FromTy == S.Context.OverloadTy) {
     assert(FromExpr && "overload set argument came from implicit argument?");
     Expr *E = FromExpr->IgnoreParens();
     if (isa<UnaryOperator>(E))
       E = cast<UnaryOperator>(E)->getSubExpr()->IgnoreParens();
     DeclarationName Name = cast<OverloadExpr>(E)->getName();
 
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_overload)
         << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-        << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << ToTy
-        << Name << I + 1;
+        << ToParamRange << ToTy << Name << I + 1;
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
   // Do some hand-waving analysis to see if the non-viability is due
   // to a qualifier mismatch.
   CanQualType CFromTy = S.Context.getCanonicalType(FromTy);
   CanQualType CToTy = S.Context.getCanonicalType(ToTy);
if (CToTy.getUnqualifiedType() == CFromTy.getUnqualifiedType() &&
       !CToTy.isAtLeastAsQualifiedAs(CFromTy)) {
     Qualifiers FromQs = CFromTy.getQualifiers();
     Qualifiers ToQs = CToTy.getQualifiers();
 
     if (FromQs.getAddressSpace() != ToQs.getAddressSpace()) {
       if (isObjectArgument)
         S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_addrspace_this)
             << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second
-            << FnDesc << (FromExpr ? FromExpr->getSourceRange() : SourceRange())
-            << FromQs.getAddressSpace() << ToQs.getAddressSpace();
+            << FnDesc << FromQs.getAddressSpace() << ToQs.getAddressSpace();
       else
         S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_addrspace)
             << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second
-            << FnDesc << (FromExpr ? FromExpr->getSourceRange() : SourceRange())
-            << FromQs.getAddressSpace() << ToQs.getAddressSpace()
-            << ToTy->isReferenceType() << I + 1;
+            << FnDesc << ToParamRange << FromQs.getAddressSpace()
+            << ToQs.getAddressSpace() << ToTy->isReferenceType() << I + 1;
       MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
       return;
     }
 
     if (FromQs.getObjCLifetime() != ToQs.getObjCLifetime()) {
       S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_ownership)
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-          << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
-          << FromQs.getObjCLifetime() << ToQs.getObjCLifetime()
-          << (unsigned)isObjectArgument << I + 1;
+          << ToParamRange << FromTy << FromQs.getObjCLifetime()
+          << ToQs.getObjCLifetime() << (unsigned)isObjectArgument << I + 1;
       MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
       return;
     }
 
     if (FromQs.getObjCGCAttr() != ToQs.getObjCGCAttr()) {
       S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_gc)
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-          << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
-          << FromQs.getObjCGCAttr() << ToQs.getObjCGCAttr()
-          << (unsigned)isObjectArgument << I + 1;
+          << ToParamRange << FromTy << FromQs.getObjCGCAttr()
+          << ToQs.getObjCGCAttr() << (unsigned)isObjectArgument << I + 1;
       MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
       return;
     }
 
     unsigned CVR = FromQs.getCVRQualifiers() & ~ToQs.getCVRQualifiers();
     assert(CVR && "expected qualifiers mismatch");
 
     if (isObjectArgument) {
       S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_cvr_this)
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-          << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
-          << (CVR - 1);
+          << FromTy << (CVR - 1);
     } else {
       S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_cvr)
           << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-          << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
-          << (CVR - 1) << I + 1;
+          << ToParamRange << FromTy << (CVR - 1) << I + 1;
     }
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
   if (Conv.Bad.Kind == BadConversionSequence::lvalue_ref_to_rvalue ||
       Conv.Bad.Kind == BadConversionSequence::rvalue_ref_to_lvalue) {
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_value_category)
         << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
         << (unsigned)isObjectArgument << I + 1
         << (Conv.Bad.Kind == BadConversionSequence::rvalue_ref_to_lvalue)
-        << (FromExpr ? FromExpr->getSourceRange() : SourceRange());
+        << ToParamRange;
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
   // Special diagnostic for failure to convert an initializer list, since
   // telling the user that it has type void is not useful.
   if (FromExpr && isa<InitListExpr>(FromExpr)) {
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_list_argument)
         << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-        << FromExpr->getSourceRange() << FromTy << ToTy
-        << (unsigned)isObjectArgument << I + 1
+        << ToParamRange << FromTy << ToTy << (unsigned)isObjectArgument << I + 1
         << (Conv.Bad.Kind == BadConversionSequence::too_few_initializers ? 1
             : Conv.Bad.Kind == BadConversionSequence::too_many_initializers
                 ? 2
                 : 0);
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
   // Diagnose references or pointers to incomplete types differently,
   // since it's far from impossible that the incompleteness triggered
   // the failure.
   QualType TempFromTy = FromTy.getNonReferenceType();
   if (const PointerType *PTy = TempFromTy->getAs<PointerType>())
     TempFromTy = PTy->getPointeeType();
   if (TempFromTy->isIncompleteType()) {
     // Emit the generic diagnostic and, optionally, add the hints to it.
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_conv_incomplete)
         << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-        << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
-        << ToTy << (unsigned)isObjectArgument << I + 1
+        << ToParamRange << FromTy << ToTy << (unsigned)isObjectArgument << I + 1
         << (unsigned)(Cand->Fix.Kind);
 
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
   // Diagnose base -> derived pointer conversions.
   unsigned BaseToDerivedConversion = 0;
if (ToRefTy->getPointeeType().isAtLeastAsQualifiedAs(FromTy) &&
         S.IsDerivedFrom(SourceLocation(), ToRefTy->getPointeeType(), FromTy)) {
       BaseToDerivedConversion = 3;
     }
   }
 
   if (BaseToDerivedConversion) {
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_base_to_derived_conv)
         << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
-        << (FromExpr ? FromExpr->getSourceRange() : SourceRange())
-        << (BaseToDerivedConversion - 1) << FromTy << ToTy << I + 1;
+        << ToParamRange << (BaseToDerivedConversion - 1) << FromTy << ToTy
+        << I + 1;
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
   if (isa<ObjCObjectPointerType>(CFromTy) &&
       isa<PointerType>(CToTy)) {
-      Qualifiers FromQs = CFromTy.getQualifiers();
-      Qualifiers ToQs = CToTy.getQualifiers();
-      if (FromQs.getObjCLifetime() != ToQs.getObjCLifetime()) {
-        S.Diag(Fn->getLocation(), diag::note_ovl_candidate_bad_arc_conv)
-            << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second
-            << FnDesc << (FromExpr ? FromExpr->getSourceRange() : SourceRange())
-            << FromTy << ToTy << (unsigned)isObjectArgument << I + 1;
-        MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
-        return;
-      }
+          << (unsigned)FnKindPair.first << (unsigned)FnKindPair.second << FnDesc
+          << ToParamRange << FromTy << ToTy << (unsigned)isObjectArgument
+          << I + 1;
   }
 
   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
-        << (FromExpr ? FromExpr->getSourceRange() : SourceRange()) << FromTy
-        << ToTy << (unsigned)isObjectArgument << I + 1
+        << ToParamRange << FromTy << ToTy << (unsigned)isObjectArgument << I + 1
         << (unsigned)(Cand->Fix.Kind);
 
   // Check that location of Fn is not in system header.
   if (!S.SourceMgr.isInSystemHeader(Fn->getLocation())) {
     // If we can fix the conversion, suggest the FixIts.
     for (const FixItHint &HI : Cand->Fix.Hints)
         FDiag << HI;
   }

clang/test/Misc/diag-overload-cand-ranges.cpp

+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-print-source-range-info %s 2>&1 | FileCheck %s --strict-whitespace
+// CHECK:      error: no matching function
+template <typename T> struct mcdata {
+  typedef int result_type;
+};
+template <class T> typename mcdata<T>::result_type wrap_mean(mcdata<T> const &);
+// CHECK:      :{[[@LINE+1]]:19-[[@LINE+1]]:53}: note: {{.*}}: no overload of 'wrap_mean'
+void add_property(double (*)(mcdata<double> const &));
+void f() { add_property(&wrap_mean); }
+
+// CHECK:      error: no matching function
+// CHECK:      :{[[@LINE+1]]:10-[[@LINE+1]]:51}: note: {{.*}}: cannot pass pointer to generic address space
+void baz(__attribute__((opencl_private)) int *Data) {}
+void fizz() {
+  int *Nop;
+  baz(Nop);
+  // CHECK:    error: no matching function
+  // CHECK:    :[[@LINE+1]]:53: note: {{.*}}: 'this' object is in address space '__private'
+  __attribute__((opencl_private)) static auto err = [&]() {};
+  err();
+}
+
+// CHECK:      error: no matching function
+struct Bar {
+// CHECK:      :{[[@LINE+1]]:26-[[@LINE+1]]:32}: note: {{.*}} would lose const qualifier
+static void foo(int num, int *X) {}
+// CHECK:      :{[[@LINE+1]]:17-[[@LINE+1]]:25}: note: {{.*}} no known conversion
+static void foo(int *err, int *x) {}
+};
+void bar(const int *Y) {
+  Bar::foo(5, Y);
+}
+
+struct InComp;
+
+struct A {};
+struct B : public A{};
+// CHECK:      error: no matching function
+// CHECK:      :{[[@LINE+5]]:36-[[@LINE+5]]:50}: note: {{.*}}: cannot convert initializer
+// CHECK:      error: no matching function
+// CHECK:      :{[[@LINE+3]]:36-[[@LINE+3]]:50}: note: {{.*}}: cannot convert argument
+// CHECK:      error: no matching function
+// CHECK:      :{[[@LINE+1]]:11-[[@LINE+1]]:18}: note: {{.*}}: no known conversion
+void hoge(char aa, const char *bb, const A& third);
+
+// CHECK:      error: no matching function
+// CHECK:      :{[[@LINE+1]]:14-[[@LINE+1]]:16}: note: {{.*}}: cannot convert from base class
+void derived(B*);
+
+void func(const A &arg) {
+  hoge(1, "pass", {{{arg}}});
+  InComp *a;
+  hoge(1, "pass", a);
+  hoge("first", 5, 6);
+  A *b;
+  derived(b);
+}
+
+struct Q {
+  // CHECK:    error: invalid operands
+  // CHECK:    :[[@LINE+1]]:6: note: {{.*}}: 'this' argument has type 'const Q'
+  Q &operator+(void*);
+};
+
+void fuga(const Q q) { q + 3; }
+
+template <short T> class Type1 {};
+// CHECK:      error: no matching function
+// CHECK:      :{[[@LINE+1]]:43-[[@LINE+1]]:54}: note: {{.*}}: expects an lvalue
+template <short T> void Function1(int zz, Type1<T> &x, int ww) {}
+
+void Function() { Function1(33, Type1<-42>(), 66); }

clang/test/Misc/diag-overload-cand-ranges.mm

+// RUN: not %clang_cc1 -fobjc-runtime-has-weak -fobjc-arc -fsyntax-only -fdiagnostics-print-source-range-info %s 2>&1 | FileCheck %s --strict-whitespace -check-prefixes=CHECK,ARC
+// RUN: not %clang_cc1 -fobjc-runtime-has-weak -fobjc-gc -fsyntax-only -fdiagnostics-print-source-range-info %s 2>&1 | FileCheck %s --strict-whitespace -check-prefixes=CHECK,GC
+
+// CHECK:      error: no matching function
+// CHECK:      :{[[@LINE+1]]:15-[[@LINE+1]]:28}: note: {{.*}}: 1st argument
+void powerful(__strong id &);
+void lifetime_gcattr_mismatch() {
+  static __weak id weak_id;
+  powerful(weak_id);
+}
+
+// CHECK:      error: no matching function
+// ARC:        :{[[@LINE+2]]:11-[[@LINE+2]]:21}: note: {{.*}}: cannot implicitly convert
+// GC:         :{[[@LINE+1]]:11-[[@LINE+1]]:21}: note: {{.*}}: no known conversion
+void func(char *uiui);
+
+__attribute__((objc_root_class))
+@interface Interface
+- (void)something;
+@end
+
+@implementation Interface
+- (void)something{
+    func(self);
+}
+@end