[PATCH] D119778: [clang] Add a note "deducing return type for 'foo'"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D119778#3327161 <https://reviews.llvm.org/D119778#3327161>, @Quuxplusone wrote:

> Apply @rsmith's suggested approach.

Thanks, the structure of the change looks good to me.

Looking through the test changes, I'm a bit torn by the value proposition of this note. Some of the errors were already pointing at the return type itself, and others were pointing at the return statement. I can see value in making sure that we mention both when diagnosing return type deduction errors, but pointing out the return type twice and never mentioning the return statement seems a bit noisy. I don't think I have any great suggestions here, though; there's not really a great way we can determine which of the two a diagnostic is pointing at so we can point the note at the other one.

The cases that I found in the tests that seem particularly bad today:

  template<typename T> concept Never = false;
  void g();
  // error: cannot form a reference to 'void'
  auto &f() {
      // ...
      return g();
  }
  // error: deduced type 'void' does not satisfy 'Never'
  Never auto h() {
      // ...
      return g();
  }

... would both benefit from a note saying "return type deduced from returned expression of type 'void' here" or something like that, and aren't really helped much by this change. But I think noting *both* the return statement and the return type would be too much (then we guarantee we always have at least one noisy note). :(

What do you think? This seems on balance to be near the edge between a useful note and a noisy note, though in many of the specific cases it's pretty clearly on one side or the other.

>> I don't see any obvious way that this patch could be responsible for that failure, unless it's something like a pre-existing use of uninitialized memory or a use-after-free and this patch is just changing the happenstance behavior.
>
> Well, it might more likely be the fault of D119184 <https://reviews.llvm.org/D119184> rather than this one per se. D119184 <https://reviews.llvm.org/D119184> is doing things with `Context.VoidTy` that weren't there before; could any of that new code be the culprit?

That seems more likely, but nothing in that patch is jumping out at me. Especially given that it only affects the behavior of return type deduction in a function with either no return statements or only `return;`, it's pretty weird that libc++ would even exercise the patch.



================
Comment at: clang/test/SemaCXX/deduced-return-void.cpp:153-154
 };
 auto l5 = []() -> auto& { // expected-error {{cannot form a reference to 'void'}}
+                          // expected-note at -1 {{deducing return type for 'operator()'}}
   return i;
----------------
This seems like an unfortunate case: both the error and the note point at the return type and neither points at the return statement.


================
Comment at: clang/test/SemaTemplate/concepts.cpp:177-178
 
   C auto f1() { return void(); }           // expected-error {{deduced type 'void' does not satisfy 'C'}}
+                                           // expected-note at -1 {{deducing return type for 'f1'}}
   C auto f2() { return; }                  // expected-error {{deduced type 'void' does not satisfy 'C'}}
----------------
This is an unfortunate case: the existing error message already points at the `C auto` constraint, so we now have an error and a note both pointing at the return type, and nothing pointing to the `return` statement itself.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119778/new/

https://reviews.llvm.org/D119778