[clang] Don't emit redundant warnings for 'return;'

[• Quuxplusone]

Summary

...when the function declaration's return type is already invalid for
some reason. This is relevant to https://github.com/llvm/llvm-project/issues/49188
because another way that the declaration's return type could become
invalid is that it might be C auto where C<void> is false.

(This doesn't actually fix 49188, but it eliminates a surprising redundant warning in the fix I tried, and also eliminates redundant warnings in the test cases depicted here.)

[sammccall]

Makes sense to me, thanks!

clang/lib/Sema/SemaStmt.cpp

4102

The comment in the test helped me understand this case better. I'd find "(The intended return type may have been void)" a useful hint here.

[• Quuxplusone]

Unfortunately some existing tests fail: https://reviews.llvm.org/harbormaster/unit/view/2282838/
I haven't yet figured out why consteval functions are considered to have FD->isInvalidDecl(). There's also an Objective-C failure that I assume indicates sometimes (when this is a method not a function) we have no FD at all. I'd need to solve both of these problems (the former being the difficult one) before I can make progress here.

[sammccall]

In D119094#3301297, @Quuxplusone wrote:

Unfortunately some existing tests fail: https://reviews.llvm.org/harbormaster/unit/view/2282838/
I haven't yet figured out why consteval functions are considered to have FD->isInvalidDecl(). There's also an Objective-C failure that I assume indicates sometimes (when this is a method not a function) we have no FD at all. I'd need to solve both of these problems (the former being the difficult one) before I can make progress here.

The example seems to be invalid even apart from the missing return value. (I assume on purpose).

There's a constexpr version and a consteval version of the same function there. It looks like the call to MergeCXXFunctionDecl reports that the merge is invalid, which will result in FD ending up invalid.

Note that if you fix those errors (return {}) you hit the error consteval declaration of 'operator+' follows constexpr declaration (err_constexpr_redecl_mismatch) which comes from MergeCXXFunctionDecl.

[• Quuxplusone]

In D119094#3301403, @sammccall wrote:

In D119094#3301297, @Quuxplusone wrote:

Unfortunately some existing tests fail: https://reviews.llvm.org/harbormaster/unit/view/2282838/
I haven't yet figured out why consteval functions are considered to have FD->isInvalidDecl(). There's also an Objective-C failure that I assume indicates sometimes (when this is a method not a function) we have no FD at all. I'd need to solve both of these problems (the former being the difficult one) before I can make progress here.

The example seems to be invalid even apart from the missing return value. (I assume on purpose).

Ohhh, wow, I had missed that. In that case, I'm shocked that Clang isn't giving any more serious error message: shouldn't it be considered invalid, and diagnosed as an error, to have a constexpr definition followed by a consteval redefinition?
https://godbolt.org/z/vKjGvEor8
However, I think the pattern of operators used (++--*/*/) indicates that the original author made a typo there, and it should always have been +-+-*/*/. So I'll feel good about changing that test.

[sammccall]

In D119094#3301481, @Quuxplusone wrote:

In D119094#3301403, @sammccall wrote:

In D119094#3301297, @Quuxplusone wrote:

Unfortunately some existing tests fail: https://reviews.llvm.org/harbormaster/unit/view/2282838/
I haven't yet figured out why consteval functions are considered to have FD->isInvalidDecl(). There's also an Objective-C failure that I assume indicates sometimes (when this is a method not a function) we have no FD at all. I'd need to solve both of these problems (the former being the difficult one) before I can make progress here.

The example seems to be invalid even apart from the missing return value. (I assume on purpose).

Ohhh, wow, I had missed that. In that case, I'm shocked that Clang isn't giving any more serious error message: shouldn't it be considered invalid, and diagnosed as an error, to have a constexpr definition followed by a consteval redefinition?
https://godbolt.org/z/vKjGvEor8
However, I think the pattern of operators used (++--*/*/) indicates that the original author made a typo there, and it should always have been +-+-*/*/. So I'll feel good about changing that test.

It is diagnosed as an error (err_constexpr_redecl_mismatch), but in this case the error for the missing return value ends up suppressing it somehow.

I'm not sure exactly what the intent of this test was :-(

[• Quuxplusone]

Fix the two failing test cases (in one case by fixing what seems to have been a typo in the test).

[sammccall]

In D119094#3302452, @Quuxplusone wrote:

Fix the two failing test cases (in one case by fixing what seems to have been a typo in the test).

I think you forgot to upload these changes.

[• Quuxplusone]

Oops. I'd put [clang] [test] Fix an apparent typo in SemaCXX/consteval-return-void.cpp in a separate commit for hygiene purposes, and forgot to include that commit in this diff. Updated.

clang/lib/Sema/SemaStmt.cpp

if (RetValExp) {
       }
     }
 
     Result = ReturnStmt::Create(Context, ReturnLoc, RetValExp,
                                 /* NRVOCandidate=*/nullptr);
   } else if (!RetValExp && !HasDependentReturnType) {
     FunctionDecl *FD = getCurFunctionDecl();
 
-    if (getLangOpts().CPlusPlus11 && FD && FD->isConstexpr()) {
+    if ((FD && FD->isInvalidDecl()) || FnRetType->containsErrors()) {
+      // The intended return type might have been "void", so don't warn.
+    } else if (getLangOpts().CPlusPlus11 && FD && FD->isConstexpr()) {
       // C++11 [stmt.return]p2
       Diag(ReturnLoc, diag::err_constexpr_return_missing_expr)
           << FD << FD->isConsteval();
       FD->setInvalidDecl();
     } else {
       // C99 6.8.6.4p1 (ext_ since GCC warns)
       // C90 6.6.6.4p4
       unsigned DiagID = getLangOpts().C99 ? diag::ext_return_missing_expr

clang/test/SemaCXX/deduced-return-void.cpp

+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+
+// Check that we don't get any extra warning for "return" without an
+// expression, in a function that might have been intended to return
+// void all along.
+auto f1() {
+  return 1;
+  return; // expected-error {{deduced as 'void' here but deduced as 'int' in earlier return statement}}
+}
+
+decltype(auto) f2() {
+  return 1;
+  return; // expected-error {{deduced as 'void' here but deduced as 'int' in earlier return statement}}
+}
+
+auto *g() {
+  return; // expected-error {{cannot deduce return type 'auto *' from omitted return expression}}
+}
+
+decltype(h1) h1() { // expected-error {{use of undeclared identifier 'h1'}}
+  return;
+}