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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3816
+        Context.getTrivialTypeSourceInfo(Context.VoidTy, ReturnLoc), ReturnLoc);
+    Expr *Dummy = R.get();
+    DeduceAutoResult DAR = DeduceAutoType(OrigResultType, Dummy, Deduced);
----------------
Quuxplusone wrote:
> sammccall wrote:
> > why initialize into an ExprResult instead of an Expr* directly?
> Because I didn't know the latter was possible. :) Btw, I assume there's no worry here about how the Expr will ultimately get garbage-collected, right? All the code I looked at seems pretty cavalier about it, so I assume `new (Context)` magically takes care of the memory management part.
Allocating a node here as if the user wrote `return void();` seems reasonable to me, and you don't have to worry about cleaning it up.

AST nodes are allocated on an arena owned by the ASTContext and never individually destroyed, the arena (BumpPtrAllocator) is just destroyed at the end along with the ASTContext.

(In fact the IIRC the default `clang -cc1` behavior is not even to destroy ASTContext, Sema etc and just exit, see `FrontendOpts::DisableFree`)


================
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'
----------------
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'
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119184