[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/test/SemaCXX/deduced-return-type-cxx14.cpp:116
 
-auto &void_ret_2() {} // expected-error {{cannot deduce return type 'auto &' for function with no return statements}}
+auto &void_ret_2() {} // expected-error {{cannot form a reference to 'void'}}
 const auto void_ret_3() {} // ok, return type 'const void' is adjusted to 'void'
----------------
sammccall wrote:
> Quuxplusone wrote:
> > sammccall wrote:
> > > This feels like a regression, this diagnostic is just attached to the end of the function and there's no longer any explicit indication that the return type or deduction is involved.
> > > 
> > > It may be worth keeping the explicit check in `ActOnFinishFunctionBody` before the deduction happens to improve the diagnostic in this case.
> > > 
> > > (I don't think it's important to do this in the return-with-no-argument case, since the error will point at the return statement which provides enough context I think)
> > I agree about the (mild) regression but am not sure what to do about it. I think it might help if Clang emitted a note `note: during return type deduction here` (pointer to the offending `return` statement or end-of-function); do you think I should do that? and if so, how?
> > 
> > > It may be worth keeping the explicit check in `ActOnFinishFunctionBody` before the deduction happens to improve the diagnostic in this case.
> > 
> > I can't picture what you mean; can you suggest a specific patch?
> > 
> > In case it matters, I'm a little leery of hard-coding a special case like we had in the old `DeduceFunctionTypeFromReturnExpr` lines 3814–3816,
> > ```
> >     // Deduction here can only succeed if the return type is exactly 'cv auto'
> >     // or 'decltype(auto)', so just check for that case directly.
> >     if (!OrigResultType.getType()->getAs<AutoType>()) {
> > ```
> > because special-casing `void` is kind of how we got into this mess in the first place.
> > I can't picture what you mean; can you suggest a specific patch?
> > In case it matters, I'm a little leery of hard-coding a special case like we had in the old
> > DeduceFunctionTypeFromReturnExpr
> 
> Yeah, so that was pretty much my suggestion: keep the `getAs<AutoType>()` check in `ActOnFinishFunctionBody` to reject the most common cases with a good diagnostic, and then if it passes do the actual deduction.
> I agree this is ugly, I just didn't really have a better idea.
> 
> > I think it might help if Clang emitted a note note: during return type deduction here 
> 
> This is a nice idea, maybe better than the existing diagnostic.
> 
> I haven't added notes myself, but I think all you have to do is `add a note_*` entry to DiagnosticSemaKinds.td, and emit it after the primary diagnostic as you'd emit any other diagnostic.
> Here if you see `DAR_Failed` then I think you know the call emitted a diagnostic and you can add the note. I'm not sure about `DAR_FailedAlreadyDiagnosed` - if it means no new diagnostic was emitted then you also don't want to add your note.
> 
> (You'd want to do this in the `if (RetExpr)` case too of course)
> 
> > pointer to the offending return statement or end-of-function
> 
> The main diagnostics is going to point at that location already. So it might be more helpful to point the "during return type deduction" note at the return type written in the function declaration:
> 
> ```
>   return;
>   ^~~~~~
> error: cannot form reference to 'void'
> 
> auto& foo() {
>       ^~~~
> note: deducing return type for 'foo'
> ```
@rsmith: This is the discussion that led to D119778 adding the note. I suppose that by keeping all the special-case error messages in `ActOnFinishFunctionBody`, we've reduced the benefit of the note.
In particular, `void_ret_2` no longer has this "regression," so there's no longer any need for the note here — in fact, as you mentioned, the note is a bit noisy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119184