[clang] return type not correctly deduced for discarded lambdas

[ojhunt]

The early return for lamda expressions with deduced return types in Sema::ActOnCapScopeReturnStmt meant that we were not actually perform the required return type deduction for such lambdas when in a discarded context.

This PR removes that early return allowing the existing return type deduction steps to be performed.

Fixes #153884

Fix developed by and

Co-authored-by: Corentin Jabot corentinjabot@gmail.com

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oliver Hunt (ojhunt)

Changes

The early return for lamda expressions with deduced return types in Sema::ActOnCapScopeReturnStmt meant that we were not actually perform the required return type deduction for such lambdas when in a discarded context.

This PR removes that early return allowing the existing return type deduction steps to be performed.

Fixes #GH153884

---

Full diff: https://github.com/llvm/llvm-project/pull/153921.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaStmt.cpp (-2)
• (modified) clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp (+16)

diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index bc1ddb80961a2..4bde0e54568dd 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -3533,8 +3533,6 @@ StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc,
         return StmtError();
       RetValExp = ER.get();
     }
-    return ReturnStmt::Create(Context, ReturnLoc, RetValExp,
-                              /* NRVOCandidate=*/nullptr);
   }
 
   if (HasDeducedReturnType) {
diff --git a/clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp b/clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp
index abb42447d3e0b..05830de9891fe 100644
--- a/clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp
+++ b/clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp
@@ -239,5 +239,21 @@ void f2() {
 
 }
 
+namespace GH153884 {
+  bool f1() {
+    auto f = [](auto) { return true; };
+    if constexpr (0)
+      return f(1);
+    return false;
+  }
+  bool f2() {
+    auto f = [](auto x) { if (x) return 1.5; else return "wat"; };
+    // expected-error@-1 {{'auto' in return type deduced as 'const char *' here but deduced as 'double' in earlier return statement}}
+    if constexpr (0)
+      return f(1);
+    // expected-note@-1 {{in instantiation of function template specialization 'GH153884::f2()}}
+    return false;
+  }
+}
 
 #endif

[ojhunt]

@cor3ntin This is not actually correct as it fails to handle the not all paths return case. The problem here is that I don't really know just how the full deduction path handles that? (I'll continue to prod around but this area is not familiar to me)

[mizvekov]

Yeah this is not correct, simply removing that return statement will cause us to attempt to use a discarded return statement for return type deduction purposes, but that is not supposed to happen.

That return statement is discarded, so perhaps you should return something which will not cause further errors as well.

I haven't fully reviewed the change that caused the regression, but I'd suspect you could extend the return statement with info about it being discarded.

[ojhunt]

Yeah this is not correct, simply removing that return statement will cause us to attempt to use a discarded return statement for return type deduction purposes, but that is not supposed to happen.

Ah yeah, I had misunderstood (again) the rules for when otherwise discarded statements have relevant semantics, so I thought this was a case where it would interact with deduction (this is why it's a draft :D)

What is weird to me is that the behavior seems to be producing a void type, but given the existence of some incorrect deduction path I can't see where void is coming from: the "obvious" return type of the lambda is a boolean, the undeduced return type is the auto type or other undeduced stand in - so I'd understand an error about a dependent or undeduced return, but the void seems surprising?

For lamdas that have an implicit (rather than deduced, though I would have assumed those were the same from a non-C++ world :D ), there's logic to track the return statements and I think it resolves to void if that list is empty. It seems plausible there's a path that acts similarly for auto returns.

[ojhunt]

Out of curiosity I looked at this, and here we do end up doing the right thing, though I assume in the case of an inferred return we end up simply ignoring the interior branch of the if(0) entirely:

auto f() {
  auto l = [](auto) { return true; };
  if constexpr (0)
    return l(1);
  return false;
}

[mizvekov]

Yes the correct behavior for return type deduction for a function with no non-discarded return statements is for it to be deduced as void: https://godbolt.org/z/qaYebb5q9

[ojhunt]

Yes the correct behavior for return type deduction for a function with no non-discarded return statements is for it to be deduced as void: https://godbolt.org/z/qaYebb5q9

Right, once you pointed it out I remembered that not all of the semantic/erroneous code in discarded statements is considered relevant, and once that was there the behavior of the return type deduction is consistent and reasonable, all things considered.

I'm still confused by how it is the return type is a bool though.

I'm unsure what the distinction between deduced vs inferred return types is because from my FL days those mean the same thing :D

[ojhunt]

@mizvekov @cor3ntin ok, I think the actual issue is in Sema::PushExpressionEvaluationContext. When pushing a new evaluation context down we all set the new context's InDiscardedStatement state to its parent's state.

I think for lambda expressions that is incorrect as it means the body of the lambda is instantiated as if it is an a discarded context, so as @mizvekov explained to me, all the return statements are considered discarded and the instantiation is then deduced as having a void return type.

My very quick testing (just filtering that assignment on the lambda context not being null - which is correct enough to test, but I think is not correct in a number of cases like template args, and similar?) appears to have correct behavior on the new test cases, and doesn't seem to break any existing tests. But I'm concerned that's a lack of coverage as I'm not sure when in the instantiation process we're actually pushing this context down, and I'm similarly not sure what the context for different parts of the overall lambda expression, ie:

auto f = [expressions here might be considered discarded?](auto foo = <another expression> ) requires ( and again ) {
   and now we're in the definitely not discarded bit
}

[cor3ntin]

@ojhunt PushExpressionEvaluationContextForFunction should set everything correctly - maybe we are (incorrectly) using PushExpressionEvaluationContext instead somewhere?

[cor3ntin]

This should work

diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index f02a295220ef..6b423ce06523 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5669,7 +5669,7 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
   };
   Function->setDeclarationNameLoc(NameLocPointsToPattern());
 
-  EnterExpressionEvaluationContext EvalContext(
+  EnterExpressionEvaluationContextForFunction EvalContext(
       *this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
 
   Qualifiers ThisTypeQuals;

[ojhunt]

This should work

diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index f02a295220ef..6b423ce06523 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5669,7 +5669,7 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
   };
   Function->setDeclarationNameLoc(NameLocPointsToPattern());
 
-  EnterExpressionEvaluationContext EvalContext(
+  EnterExpressionEvaluationContextForFunction EvalContext(
       *this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
 
   Qualifiers ThisTypeQuals;

@cor3ntin One thing I'm trying to work out in tests is whether there is any way to get a discarded block that should cause an error in the containing scope but not if it occurred in the lamda instantiation.

e.g a hypothetical

auto lambda = [x=some expression](type/auto foo = some expression, ..) requires { something } {
 ..
 /* I want to be able to construct an equivalent to this, that can live in the above
  * capture, default, or constraint contexts
  */
 if constexpr (0) {
    // some error that won't break the compilation
 }
};

The problem is I can never work out what kind of discarded statements or expressions permit an error to exist without resulting in an actual compilation error.

I am also very sad we no longer allow statement expressions in parameter default expressions. I mean from a language point of view that's obviously the right thing as the old behavior could result in insanity, but from the point of view of trying to make insane test cases it's annoying :D

[cor3ntin]

@cor3ntin One thing I'm trying to work out in tests is whether there is any way to get a discarded block that should cause an error in the containing scope but not if it occurred in the lamda instantiation.

There are never any errors ignored in discarded statements. If the if constexpr appears in a template its content is not instantiated. But it is always parsed.

If you want to write a tests where the if constexpr(false) is in a template, and calls a dependent/generic lambda that can never be instantiated... you can! Maybe something like that:

template <typename T>
void f() {
  auto x = []<typename U>(U, U = U::bar) {};
  if constexpr(false) {
        x(T{});
  }
}
int _ = (f<int>(), 0);

[ojhunt]

  auto x = []<typename U>(U, U = U::bar) {};

I think I technically knew you could have templated lambdas, but I certainly forgot about it entirely :D

What I'm trying to construct is something that would not trigger an error if it was wrapped in an if constexpr(0) inside the lambda body, but that should trigger an error in the outer portions of the lambda. The goal being to test that we do the context change in the right place. The problem of course is that I can't tell if we're doing the right thing, or I'm writing the tests wrong :D

[cor3ntin]

LGTM!