Re: [PATCH] c++: recalculating local specs via build_extra_args [PR114303]

[Patrick Palka <ppalka@redhat.com>]

On Wed, 10 Apr 2024, Jason Merrill wrote:

> On 3/12/24 10:51, Patrick Palka wrote:
> > On Tue, 12 Mar 2024, Patrick Palka wrote:
> > > On Tue, 12 Mar 2024, Jason Merrill wrote:
> > > > On 3/11/24 12:53, Patrick Palka wrote:
> > > > > 
> > > > > r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts
> > > > > first so that we prefer processing a local specialization in an
> > > > > evaluated
> > > > > context even if its first use is in an unevaluated context.  But this
> > > > > means we need to avoid walking a tree that already has extra
> > > > > args/specs
> > > > > saved because the list of saved specs appears to be an evaluated
> > > > > context.  It seems then that we should be calculating the saved specs
> > > > > from scratch each time, rather than potentially walking the saved
> > > > > specs
> > > > > list from an earlier partial instantiation when calling
> > > > > build_extra_args
> > > > > a second time around.
> > > > 
> > > > Makes sense, but I wonder if we want to approach that by avoiding
> > > > walking into
> > > > *_EXTRA_ARGS in extract_locals_r?  Or do we still want to walk into any
> > > > nested
> > > > extra args?  And if so, will we run into this same problem then?
> > > 
> > > I'm not sure totally but I'd expect a nested extra-args tree to always
> > > have empty *_EXTRA_ARGS since the outer extra-args tree should intercept
> > > any substitution before the inner extra-args tree can see it?
> > 
> > ... and so in extract_locals_r I think we can assume *_EXTRA_ARGS is
> > empty, and not have to explicitly avoid walking it.
> 
> It seems more robust to me to handle _EXTRA_ARGS appropriately in
> build_extra_args rather than expect callers to know that they shouldn't pass
> in a tree with _EXTRA_ARGS set.  At least check and abort in that case?

Sounds good.  That IMHO seems simpler than actually avoiding walking
into *_EXTRA_ARGS from extract_locals_r because we'd have to repeat
the walking logic from cp_walk_subtree modulo the *_EXTRA_ARGS walk.

How does the following look? Bootstraped and regtested on
x86_64-pc-linux-gnu.

-- > 8--

Subject: [PATCH] c++: build_extra_args recapturing local specs [PR114303]

	PR c++/114303

gcc/cp/ChangeLog:

	* constraint.cc (tsubst_requires_expr): Clear
	REQUIRES_EXPR_EXTRA_ARGS before calling build_extra_args.
	* pt.cc (extract_locals_r): Assert *_EXTRA_ARGS is empty.
	(tsubst_stmt) <case IF_STMT>: Call build_extra_args
	on the new IF_STMT instead of t which might already have
	IF_STMT_EXTRA_ARGS.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1z/constexpr-if-lambda6.C: New test.
---
 gcc/cp/constraint.cc                             |  1 +
 gcc/cp/pt.cc                                     | 16 +++++++++++++++-
 .../g++.dg/cpp1z/constexpr-if-lambda6.C          | 16 ++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C

diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 49de3211d4c..8a3b5d80ba7 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info info)
 	 matching or dguide constraint rewriting), in which case we need
 	 to partially substitute.  */
       t = copy_node (t);
+      REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE;
       REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, info.complain);
       return t;
     }
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index c38594cd862..6cc9b95fc06 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -13310,6 +13310,19 @@ extract_locals_r (tree *tp, int *walk_subtrees, void *data_)
     /* Remember local typedefs (85214).  */
     tp = &TYPE_NAME (*tp);
 
+  if (has_extra_args_mechanism_p (*tp))
+    {
+      if (PACK_EXPANSION_P (*tp))
+	gcc_checking_assert (!PACK_EXPANSION_EXTRA_ARGS (*tp));
+      else if (TREE_CODE (*tp) == REQUIRES_EXPR)
+	gcc_checking_assert (!REQUIRES_EXPR_EXTRA_ARGS (*tp));
+      else if (TREE_CODE (*tp) == IF_STMT
+	       && IF_STMT_CONSTEXPR_P (*tp))
+	gcc_checking_assert (!IF_STMT_EXTRA_ARGS (*tp));
+      else
+	gcc_unreachable ();
+    }
+
   if (TREE_CODE (*tp) == DECL_EXPR)
     {
       tree decl = DECL_EXPR_DECL (*tp);
@@ -18738,7 +18751,8 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 	  IF_COND (stmt) = IF_COND (t);
 	  THEN_CLAUSE (stmt) = THEN_CLAUSE (t);
 	  ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t);
-	  IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain);
+	  IF_SCOPE (stmt) = NULL_TREE;
+	  IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain);
 	  add_stmt (stmt);
 	  break;
 	}
diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C
new file mode 100644
index 00000000000..038c2a41210
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C
@@ -0,0 +1,16 @@
+// PR c++/114303
+// { dg-do compile { target c++17 } }
+
+struct A { static constexpr bool value = true; };
+
+int main() {
+  [](auto x1) {
+    return [&](auto) {
+      return [&](auto x3) {
+        if constexpr (decltype(x3)::value) {
+          static_assert(decltype(x1)::value);
+        }
+      }(A{});
+    }(0);
+  }(A{});
+}
-- 
2.44.0.548.g91ec36f2cc