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

[• Quuxplusone]

Summary

Fixes https://github.com/llvm/llvm-project/issues/49188

[• Quuxplusone]

Update one additional test where the expected output has changed:

auto& f() { }

now produces "can't make a reference to void" instead of "can't deduce auto& from no return statements." I think this is a mild regression in diagnostic quality, but I don't immediately see what we should do about it so I'm inclined to take the hit. Suggestions welcome.

[• Quuxplusone]

Rebase; clang-format.

[sammccall]

This looks right to me, though I think we should spend a few more lines to preserve the nice diagnostics for simple cases.

clang/lib/Sema/SemaStmt.cpp

3593

nit: /*HasReturnStatement=*/true?

3816

why initialize into an ExprResult instead of an Expr* directly?

3819

(BTW, err_auto_fn_return_void_but_not_auto has no testcases, feel free to add one near deduced-return-type-cxx14.cpp:341, or not)

clang/test/SemaCXX/deduced-return-type-cxx14.cpp

116 ↗

(On Diff #407284)

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)

[• Quuxplusone]

clang/lib/Sema/SemaStmt.cpp

3816

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.

3819

Will do.

clang/test/SemaCXX/deduced-return-type-cxx14.cpp

116 ↗

(On Diff #407284)

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.

[sammccall]

clang/lib/Sema/SemaStmt.cpp

3816

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)

clang/test/SemaCXX/deduced-return-type-cxx14.cpp

116 ↗

(On Diff #407284)

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'

[• Quuxplusone]

Address review comments; add exhaustive tests as a parent revision.

[• Quuxplusone]

Rebase; fix the clang-format nit; reinsert a missing FD->setInvalidDecl() that seemed to be causing crashes in Modules code under libcxx/test/ but otherwise doesn't seem to be tested in clang/test/ :(

[rsmith]

clang/lib/Sema/SemaStmt.cpp

3766

It looks like you ended up not using the new parameter for anything. Did you intend to / do you still need it?

[• Quuxplusone]

clang/lib/Sema/SemaStmt.cpp

3766

Huh, I guess I don't need it anymore.
Btw, does it make sense that I marked AT as const AutoType *? Initially I'd been worried that something on this codepath might be stashing state inside *AT, or otherwise updating it to say "This was written auto&, but now refers to the type <foo>." This week, I'm not sure why I would have ever suspected that.
But still, this codepath does update the state of *FD, so the fact that it doesn't update *AT is still somewhat worthy of note...
Anyway, if the const is uncontroversial, I could just land that const in its own commit to shrink this PR a tiny bit.

[• Quuxplusone]

clang/test/SemaCXX/deduced-return-type-cxx14.cpp

116 ↗

(On Diff #407284)

@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.

[• Quuxplusone]

Rebase and update — this is becoming more and more of a trivial patch, which I guess is good!
Add a test case for https://github.com/llvm/llvm-project/issues/53911 (which I finally thought to test, and was surprised to find it didn't work either before or after my patch).

[• Quuxplusone]

Rebased, poke CI one last time.
If this is green, imma land it.

clang/lib/Sema/SemaDecl.cpp

if (getLangOpts().Coroutines && FSI->isCoroutine())
 
     if (FD) {
       FD->setBody(Body);
       FD->setWillHaveBody(false);
 
       if (getLangOpts().CPlusPlus14) {
         if (!FD->isInvalidDecl() && Body && !FD->isDependentContext() &&
             FD->getReturnType()->isUndeducedType()) {
-          // If the function has a deduced result type but contains no 'return'
-          // statements, the result type as written must be exactly 'auto', and
-          // the deduced result type is 'void'.
+          // For a function with a deduced result type to return void,
+          // the result type as written must be 'auto' or 'decltype(auto)',
+          // possibly cv-qualified or constrained, but not ref-qualified.
           if (!FD->getReturnType()->getAs<AutoType>()) {
             Diag(dcl->getLocation(), diag::err_auto_fn_no_return_but_not_auto)
                 << FD->getReturnType();
             FD->setInvalidDecl();
           } else {
-            // Substitute 'void' for the 'auto' in the type.
-            TypeLoc ResultType = getReturnTypeLoc(FD);
-            Context.adjustDeducedFunctionResultType(
-                FD, SubstAutoType(ResultType.getType(), Context.VoidTy));
+            // Falling off the end of the function is the same as 'return;'.
+            Expr *Dummy = nullptr;
+            if (DeduceFunctionTypeFromReturnExpr(
+                    FD, dcl->getLocation(), Dummy,
+                    FD->getReturnType()->getAs<AutoType>()))
+              FD->setInvalidDecl();
           }
         }
       } else if (getLangOpts().CPlusPlus11 && isLambdaCallOperator(FD)) {
         // In C++11, we don't use 'auto' deduction rules for lambda call
         // operators because we don't support return type deduction.
         auto *LSI = getCurLambda();
         if (LSI->HasImplicitReturnType) {
           deduceClosureReturnType(*LSI);

clang/lib/Sema/SemaStmt.cpp

if (FD->isInvalidDecl())
       return StmtError();
     // In C++1y, the return type may involve 'auto'.
     // FIXME: Blocks might have a return type of 'auto' explicitly specified.
     if (CurCap->ReturnType.isNull())
       CurCap->ReturnType = FD->getReturnType();
 
     AutoType *AT = CurCap->ReturnType->getContainedAutoType();
     assert(AT && "lost auto type from lambda return type");
     if (DeduceFunctionTypeFromReturnExpr(FD, ReturnLoc, RetValExp, AT)) {
       FD->setInvalidDecl();
       // FIXME: preserve the ill-formed return expression.
       return StmtError();
     }
     CurCap->ReturnType = FnRetType = FD->getReturnType();
   } else if (CurCap->HasImplicitReturnType) {
     // For blocks/lambdas with implicit return types, we check each return
     // statement individually, and deduce the common return type when the block

 }
 
 /// Deduce the return type for a function from a returned expression, per
 /// C++1y [dcl.spec.auto]p6.
 bool Sema::DeduceFunctionTypeFromReturnExpr(FunctionDecl *FD,
                                             SourceLocation ReturnLoc,
                                             Expr *&RetExpr,
                                             const AutoType *AT) {
   // If this is the conversion function for a lambda, we choose to deduce its
   // type from the corresponding call operator, not from the synthesized return
   // statement within it. See Sema::DeduceReturnType.
   if (isLambdaConversionOperator(FD))
     return false;
 
   TypeLoc OrigResultType = getReturnTypeLoc(FD);
   QualType Deduced;
 
if (RetExpr) {
     if (DAR != DAR_Succeeded)
       return true;
 
     // If a local type is part of the returned type, mark its fields as
     // referenced.
     LocalTypedefNameReferencer Referencer(*this);
     Referencer.TraverseType(RetExpr->getType());
   } else {
-    //  In the case of a return with no operand, the initializer is considered
-    //  to be void().
-    //
-    // Deduction here can only succeed if the return type is exactly 'cv auto'
-    // or 'decltype(auto)', so just check for that case directly.
+    // For a function with a deduced result type to return void,
+    // the result type as written must be 'auto' or 'decltype(auto)',
+    // possibly cv-qualified or constrained, but not ref-qualified.
     if (!OrigResultType.getType()->getAs<AutoType>()) {
       Diag(ReturnLoc, diag::err_auto_fn_return_void_but_not_auto)
         << OrigResultType.getType();
       return true;
     }
-    // We always deduce U = void in this case.
-    Deduced = SubstAutoType(OrigResultType.getType(), Context.VoidTy);
-    if (Deduced.isNull())
+    // In the case of a return with no operand, the initializer is considered
+    // to be 'void()'.
+    Expr *Dummy = new (Context) CXXScalarValueInitExpr(
+        Context.VoidTy,
+        Context.getTrivialTypeSourceInfo(Context.VoidTy, ReturnLoc), ReturnLoc);
+    DeduceAutoResult DAR = DeduceAutoType(OrigResultType, Dummy, Deduced);
+
+    if (DAR == DAR_Failed && !FD->isInvalidDecl())
+      Diag(ReturnLoc, diag::err_auto_fn_deduction_failure)
+          << OrigResultType.getType() << Dummy->getType();
+
+    if (DAR != DAR_Succeeded)
       return true;
   }
 
   // CUDA: Kernel function must have 'void' return type.
   if (getLangOpts().CUDA)
     if (FD->hasAttr<CUDAGlobalAttr>() && !Deduced->isVoidType()) {
       Diag(FD->getLocation(), diag::err_kern_type_not_void_return)
           << FD->getType() << FD->getSourceRange();

clang/test/SemaTemplate/concepts.cpp

 }
 
 namespace PR50561 {
   template<typename> concept C = false;
   template<typename T, typename U> void f(T, U);
   template<C T, typename U> void f(T, U) = delete;
   void g() { f(0, 0); }
 }
+
+namespace PR49188 {
+  template<class T> concept C = false;     // expected-note 6 {{because 'false' evaluated to false}}
+
+  C auto f1() { // expected-error {{deduced type 'void' does not satisfy 'C'}}
+    return void();
+  }
+  C auto f2() { // expected-error {{deduced type 'void' does not satisfy 'C'}}
+    return;
+  }
+  C auto f3() { // expected-error {{deduced type 'void' does not satisfy 'C'}}
+  }
+  C decltype(auto) f4() { // expected-error {{deduced type 'void' does not satisfy 'C'}}
+    return void();
+  }
+  C decltype(auto) f5() { // expected-error {{deduced type 'void' does not satisfy 'C'}}
+    return;
+  }
+  C decltype(auto) f6() { // expected-error {{deduced type 'void' does not satisfy 'C'}}
+  }
+  C auto& f7() { // expected-error {{cannot form a reference to 'void'}}
+    return void();
+  }
+  C auto& f8() {
+    return; // expected-error {{cannot deduce return type 'C auto &' from omitted return expression}}
+  }
+  C auto& f9() { // expected-error {{cannot deduce return type 'C auto &' for function with no return statements}}
+  }
+}
+namespace PR53911 {
+  template<class T> concept C = false;
+
+  C auto *f1() {
+    return (void*)nullptr; // FIXME: should error
+  }
+  C auto *f2() {
+    return (int*)nullptr; // FIXME: should error
+  }
+}