structurally compare type_arg_packs [93933]

Message ID 9304c353-cf03-cadd-f9b3-e40f208c1b94@acm.org
State New
Headers show
Series
  • structurally compare type_arg_packs [93933]
Related show

Commit Message

Nathan Sidwell Feb. 25, 2020, 9:09 p.m.
We consider all TYPE_ARGUMENT_PACKS distinct types, leading to problems 
with redeclarations.

This patch fixes things by:
a) marking all such types for structural comparison
b) teaching structural_comptypes how to compare them.

1) It appears that NONTYPE_ARGUMENT_PACKS just work, I think because we 
just compare TREE_OPERANDs, which dtrt.

2) I don't think the ARGUMENT_PACK_INCOMPLETE_P and 
ARGUMENT_PACK_EXPLICIT_ARGS affect the comparison, (icbw?)

I'll apply to trunk shortly, unless someone indicates I'm confused.

As the PR says, this comes from the modules branch, where I need to 
merge global module entities.

nathan
-- 
Nathan Sidwell

Comments

Jason Merrill Feb. 26, 2020, 10 p.m. | #1
On 2/25/20 4:09 PM, Nathan Sidwell wrote:
> We consider all TYPE_ARGUMENT_PACKS distinct types, leading to problems 

> with redeclarations.


> This patch fixes things by:

> a) marking all such types for structural comparison

> b) teaching structural_comptypes how to compare them.

> 

> 1) It appears that NONTYPE_ARGUMENT_PACKS just work, I think because we 

> just compare TREE_OPERANDs, which dtrt.

> 

> 2) I don't think the ARGUMENT_PACK_INCOMPLETE_P and 

> ARGUMENT_PACK_EXPLICIT_ARGS affect the comparison, (icbw?)

> 

> I'll apply to trunk shortly, unless someone indicates I'm confused.


I'd think that the bug is that we're treating them as types in the first 
place; they aren't types, so they shouldn't reach comptypes.  I'd lean 
toward adding an assert to that effect and fixing the caller to use e.g. 
template_args_equal.

Jason
Nathan Sidwell Feb. 27, 2020, 3:33 p.m. | #2
On 2/26/20 5:00 PM, Jason Merrill wrote:
> On 2/25/20 4:09 PM, Nathan Sidwell wrote:

>> We consider all TYPE_ARGUMENT_PACKS distinct types, leading to 

>> problems with redeclarations.


> I'd think that the bug is that we're treating them as types in the first 

> place; they aren't types, so they shouldn't reach comptypes.  I'd lean 

> toward adding an assert to that effect and fixing the caller to use e.g. 

> template_args_equal.


Thanks, this patch implements that approach.

That TYPE_ARGUMENT_PACKS are not types, suggests to me that 
NONTYPE_ARGUMENT_PACKS are not expressions.  Perhaps their 
representation should be unified -- I keep encountering code handling 
them essentially doing the same thing for both kinds.  But that's a 
GCC-11 thing at least.

nathan

-- 
Nathan Sidwell
2020-02-27  Nathan Sidwell  <nathan@acm.org>

	PR c++/93933
	* pt.c (template_args_equal): Pass ARGUMENT_PACKS through to
	cp_tree_equal.
	* tree.c (cp_tree_equal): Compare ARGUMENT_PACKS here,
	* typeck.c (comptypes): Assert we don't get any argument packs.

diff --git c/gcc/cp/pt.c w/gcc/cp/pt.c
index 6c9abb8f3d3..622c70b352f 100644
--- c/gcc/cp/pt.c
+++ w/gcc/cp/pt.c
@@ -8999,25 +8999,8 @@ template_args_equal (tree ot, tree nt, bool partial_order /* = false */)
 				    PACK_EXPANSION_PATTERN (nt))
 	    && template_args_equal (PACK_EXPANSION_EXTRA_ARGS (ot),
 				    PACK_EXPANSION_EXTRA_ARGS (nt)));
-  else if (ARGUMENT_PACK_P (ot))
-    {
-      int i, len;
-      tree opack, npack;
-
-      if (!ARGUMENT_PACK_P (nt))
-	return 0;
-
-      opack = ARGUMENT_PACK_ARGS (ot);
-      npack = ARGUMENT_PACK_ARGS (nt);
-      len = TREE_VEC_LENGTH (opack);
-      if (TREE_VEC_LENGTH (npack) != len)
-	return 0;
-      for (i = 0; i < len; ++i)
-	if (!template_args_equal (TREE_VEC_ELT (opack, i),
-				  TREE_VEC_ELT (npack, i)))
-	  return 0;
-      return 1;
-    }
+  else if (ARGUMENT_PACK_P (ot) || ARGUMENT_PACK_P (nt))
+    return cp_tree_equal (ot, nt);
   else if (ot && TREE_CODE (ot) == ARGUMENT_PACK_SELECT)
     gcc_unreachable ();
   else if (TYPE_P (nt))
diff --git c/gcc/cp/tree.c w/gcc/cp/tree.c
index 72b3a720ee8..3fc6287d566 100644
--- c/gcc/cp/tree.c
+++ w/gcc/cp/tree.c
@@ -3857,12 +3857,27 @@ cp_tree_equal (tree t1, tree t2)
 			     DEFERRED_NOEXCEPT_PATTERN (t2))
 	      && comp_template_args (DEFERRED_NOEXCEPT_ARGS (t1),
 				     DEFERRED_NOEXCEPT_ARGS (t2)));
-      break;
 
     case LAMBDA_EXPR:
       /* Two lambda-expressions are never considered equivalent.  */
       return false;
 
+    case TYPE_ARGUMENT_PACK:
+    case NONTYPE_ARGUMENT_PACK:
+      {
+	tree p1 = ARGUMENT_PACK_ARGS (t1);
+	tree p2 = ARGUMENT_PACK_ARGS (t2);
+	int len = TREE_VEC_LENGTH (p1);
+	if (TREE_VEC_LENGTH (p2) != len)
+	  return false;
+
+	for (int ix = 0; ix != len; ix++)
+	  if (!template_args_equal (TREE_VEC_ELT (p1, ix),
+				    TREE_VEC_ELT (p2, ix)))
+	    return false;
+	return true;
+      }
+
     default:
       break;
     }
diff --git c/gcc/cp/typeck.c w/gcc/cp/typeck.c
index 42d0b47cf1b..2a3243f3e81 100644
--- c/gcc/cp/typeck.c
+++ w/gcc/cp/typeck.c
@@ -1485,6 +1485,10 @@ comptypes (tree t1, tree t2, int strict)
 {
   gcc_checking_assert (t1 && t2);
 
+  /* TYPE_ARGUMENT_PACKS are not really types.  */
+  gcc_checking_assert (TREE_CODE (t1) != TYPE_ARGUMENT_PACK
+		       && TREE_CODE (t2) != TYPE_ARGUMENT_PACK);
+
   if (strict == COMPARE_STRICT && comparing_specializations
       && (t1 != TYPE_CANONICAL (t1) || t2 != TYPE_CANONICAL (t2)))
     /* If comparing_specializations, treat dependent aliases as distinct.  */
diff --git c/gcc/testsuite/g++.dg/concepts/pr93933.C w/gcc/testsuite/g++.dg/concepts/pr93933.C
new file mode 100644
index 00000000000..b4f2c36374d
--- /dev/null
+++ w/gcc/testsuite/g++.dg/concepts/pr93933.C
@@ -0,0 +1,31 @@
+// { dg-do compile { target c++17 } }
+// { dg-options "-fconcepts" }
+
+// distilled from <concepts>, via header units
+
+template<typename _ArgTypes>
+struct is_invocable;
+
+template<typename... _Args>
+concept invocable = is_invocable<_Args...>::value;
+
+template<typename _Is>
+requires invocable<_Is>
+class BUG;
+
+template<typename _Is>
+requires invocable<_Is>
+class BUG {}; // { dg-bogus "different constraints" }
+
+template<int> struct is_invocable_NT;
+
+template<int... Ints>
+concept invocable_NT = is_invocable_NT<Ints...>::value;
+
+template<int _Is>
+requires invocable_NT<_Is>
+class BUG_NT;
+
+template<int _Is>
+requires invocable_NT<_Is>
+class BUG_NT {};
Jason Merrill Feb. 27, 2020, 3:47 p.m. | #3
On 2/27/20 10:33 AM, Nathan Sidwell wrote:
> On 2/26/20 5:00 PM, Jason Merrill wrote:

>> On 2/25/20 4:09 PM, Nathan Sidwell wrote:

>>> We consider all TYPE_ARGUMENT_PACKS distinct types, leading to 

>>> problems with redeclarations.

> 

>> I'd think that the bug is that we're treating them as types in the 

>> first place; they aren't types, so they shouldn't reach comptypes.  

>> I'd lean toward adding an assert to that effect and fixing the caller 

>> to use e.g. template_args_equal.

> 

> Thanks, this patch implements that approach.


> That TYPE_ARGUMENT_PACKS are not types, suggests to me that 

> NONTYPE_ARGUMENT_PACKS are not expressions.  Perhaps their 

> representation should be unified -- I keep encountering code handling 

> them essentially doing the same thing for both kinds.  But that's a 

> GCC-11 thing at least.


Agreed.  I took a step in that direction when I removed TREE_TYPE from 
NONTYPE_ARGUMENT_PACK, but going on to unify the packs makes sense to me.

Jason

Patch

2020-02-25  Nathan Sidwell  <nathan@acm.org>

	PR c++/93933
	* pt.c (template_parm_to_arg): TYPE_ARGUMENT_PACKS are structural.
	(coerce_template_parameter_pack, make_argument_pack)
	(tsubst_argument_pack, tsubst, type_unification_real)
	(unify_pack_expansion): Likewise.
	* typeck.c (structural_comptypes): Compare TYPE_ARGUMENT_PACK args.

diff --git c/gcc/cp/pt.c w/gcc/cp/pt.c
index 6c9abb8f3d3..8716f979027 100644
--- c/gcc/cp/pt.c
+++ w/gcc/cp/pt.c
@@ -4670,6 +4670,7 @@  template_parm_to_arg (tree t)
 	  TREE_VEC_ELT (vec, 0) = make_pack_expansion (t);
 
 	  t = cxx_make_type (TYPE_ARGUMENT_PACK);
+	  SET_TYPE_STRUCTURAL_EQUALITY (t);
 	  SET_ARGUMENT_PACK_ARGS (t, vec);
 	}
     }
@@ -8500,7 +8501,10 @@  coerce_template_parameter_pack (tree parms,
 
   if (TREE_CODE (TREE_VALUE (parm)) == TYPE_DECL
       || TREE_CODE (TREE_VALUE (parm)) == TEMPLATE_DECL)
-    argument_pack = cxx_make_type (TYPE_ARGUMENT_PACK);
+    {
+      argument_pack = cxx_make_type (TYPE_ARGUMENT_PACK);
+      SET_TYPE_STRUCTURAL_EQUALITY (argument_pack);
+    }
   else
     {
       argument_pack = make_node (NONTYPE_ARGUMENT_PACK);
@@ -13007,7 +13011,10 @@  make_argument_pack (tree vec)
   tree pack;
   tree elt = TREE_VEC_ELT (vec, 0);
   if (TYPE_P (elt))
-    pack = cxx_make_type (TYPE_ARGUMENT_PACK);
+    {
+      pack = cxx_make_type (TYPE_ARGUMENT_PACK);
+      SET_TYPE_STRUCTURAL_EQUALITY (pack);
+    }
   else
     {
       pack = make_node (NONTYPE_ARGUMENT_PACK);
@@ -13050,19 +13057,24 @@  tsubst_argument_pack (tree orig_arg, tree args, tsubst_flags_t complain,
 		      tree in_decl)
 {
   /* Substitute into each of the arguments.  */
-  tree new_arg = TYPE_P (orig_arg)
-    ? cxx_make_type (TREE_CODE (orig_arg))
-    : make_node (TREE_CODE (orig_arg));
-
   tree pack_args = tsubst_template_args (ARGUMENT_PACK_ARGS (orig_arg),
 					 args, complain, in_decl);
-  if (pack_args == error_mark_node)
-    new_arg = error_mark_node;
-  else
-    SET_ARGUMENT_PACK_ARGS (new_arg, pack_args);
+  tree new_arg = error_mark_node;
+  if (pack_args != error_mark_node)
+    {
+      if (TYPE_P (orig_arg))
+	{
+	  new_arg = cxx_make_type (TREE_CODE (orig_arg));
+	  SET_TYPE_STRUCTURAL_EQUALITY (new_arg);
+	}
+      else
+	{
+	  new_arg = make_node (TREE_CODE (orig_arg));
+	  TREE_CONSTANT (new_arg) = TREE_CONSTANT (orig_arg);
+	}
 
-  if (TREE_CODE (new_arg) == NONTYPE_ARGUMENT_PACK)
-    TREE_CONSTANT (new_arg) = TREE_CONSTANT (orig_arg);
+      SET_ARGUMENT_PACK_ARGS (new_arg, pack_args);
+    }
 
   return new_arg;
 }
@@ -15804,8 +15816,11 @@  tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 	if (code == NONTYPE_ARGUMENT_PACK)
 	  r = make_node (code);
 	else
-	  r = cxx_make_type (code);
-
+	  {
+	    r = cxx_make_type (code);
+	    SET_TYPE_STRUCTURAL_EQUALITY (r);
+	  }
+	
 	tree pack_args = ARGUMENT_PACK_ARGS (t);
 	pack_args = tsubst_template_args (pack_args, args, complain, in_decl);
 	SET_ARGUMENT_PACK_ARGS (r, pack_args);
@@ -21786,7 +21801,10 @@  type_unification_real (tree tparms,
 		  TREE_CONSTANT (arg) = 1;
 		}
 	      else
-		arg = cxx_make_type (TYPE_ARGUMENT_PACK);
+		{
+		  arg = cxx_make_type (TYPE_ARGUMENT_PACK);
+		  SET_TYPE_STRUCTURAL_EQUALITY (arg);
+		}
 
 	      SET_ARGUMENT_PACK_ARGS (arg, make_tree_vec (0));
 
@@ -22618,7 +22636,10 @@  unify_pack_expansion (tree tparms, tree targs, tree packed_parms,
               TREE_CONSTANT (result) = 1;
             }
           else
-            result = cxx_make_type (TYPE_ARGUMENT_PACK);
+	    {
+	      result = cxx_make_type (TYPE_ARGUMENT_PACK);
+	      SET_TYPE_STRUCTURAL_EQUALITY (result);
+	    }
 
           SET_ARGUMENT_PACK_ARGS (result, new_args);
 
diff --git c/gcc/cp/typeck.c w/gcc/cp/typeck.c
index c0c98dad980..0954c37444e 100644
--- c/gcc/cp/typeck.c
+++ w/gcc/cp/typeck.c
@@ -1453,6 +1453,11 @@  structural_comptypes (tree t1, tree t2, int strict)
       return same_type_p (UNDERLYING_TYPE_TYPE (t1), 
 			  UNDERLYING_TYPE_TYPE (t2));
 
+    case TYPE_ARGUMENT_PACK:
+      if (!cp_tree_equal (ARGUMENT_PACK_ARGS (t1), ARGUMENT_PACK_ARGS (t2)))
+	return false;
+      break;
+
     default:
       return false;
     }
diff --git c/gcc/testsuite/g++.dg/concepts/pr93933.C w/gcc/testsuite/g++.dg/concepts/pr93933.C
new file mode 100644
index 00000000000..b4f2c36374d
--- /dev/null
+++ w/gcc/testsuite/g++.dg/concepts/pr93933.C
@@ -0,0 +1,31 @@ 
+// { dg-do compile { target c++17 } }
+// { dg-options "-fconcepts" }
+
+// distilled from <concepts>, via header units
+
+template<typename _ArgTypes>
+struct is_invocable;
+
+template<typename... _Args>
+concept invocable = is_invocable<_Args...>::value;
+
+template<typename _Is>
+requires invocable<_Is>
+class BUG;
+
+template<typename _Is>
+requires invocable<_Is>
+class BUG {}; // { dg-bogus "different constraints" }
+
+template<int> struct is_invocable_NT;
+
+template<int... Ints>
+concept invocable_NT = is_invocable_NT<Ints...>::value;
+
+template<int _Is>
+requires invocable_NT<_Is>
+class BUG_NT;
+
+template<int _Is>
+requires invocable_NT<_Is>
+class BUG_NT {};