[c++-concepts] Check function concept definitions

[c++-concepts] Check function concept definitions

[Andrew Sutton]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

This fixes an ICE trying to normalize a function concept with multiple
statements. That error will now be diagnosed at the point of
definition.

Jason, do you want to review this before I commit? This is a pretty small patch.

2014-09-01  Andrew Sutton  <andrew.n.sutton@gmail.com>

        Check requirements on function concept definitions.
        * gcc/cp/decl.c (finish_function): Check properties of a function
        concept definition.
        * gcc/cp/constraint.cc (check_function_concept): New. Check
        for deduced return type and multiple statements.
        (normalize_misc): Don't normalize multiple statements.
        (normalize_stmt_list): Removed.
        * gcc/cp/cp-tree.h (check_function_concept): New.
        * gcc/testsuite/g++.dg/concepts/fn-concept1.C: New.

Andrew

[-- Attachment #2: fn-concept.patch --]
[-- Type: text/x-patch, Size: 4140 bytes --]

Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 214991)
+++ cp/cp-tree.h	(working copy)
@@ -6444,6 +6444,7 @@ extern tree build_concept_check
 extern tree build_constrained_parameter         (tree, tree, tree = NULL_TREE);
 extern bool deduce_constrained_parameter        (tree, tree&, tree&);
 extern tree resolve_constraint_check            (tree);
+extern tree check_function_concept              (tree);
 
 extern tree finish_concept_introduction         (tree, tree);
 extern tree finish_template_constraints         (tree);
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 214268)
+++ cp/decl.c	(working copy)
@@ -14360,6 +14360,10 @@ finish_function (int flags)
       fntype = TREE_TYPE (fndecl);
     }
 
+  // If this is a concept, check that the definition is reasonable.
+  if (DECL_DECLARED_CONCEPT_P (fndecl))
+    check_function_concept (fndecl);
+
   /* Save constexpr function body before it gets munged by
      the NRV transformation.   */
   maybe_save_function_definition (fndecl);
Index: cp/constraint.cc
===================================================================
--- cp/constraint.cc	(revision 214991)
+++ cp/constraint.cc	(working copy)
@@ -269,6 +269,35 @@ deduce_concept_introduction (tree expr)
     gcc_unreachable ();
 }
 
+
+// -------------------------------------------------------------------------- //
+// Declarations
+
+// Check that FN satisfies the structural requirements of a
+// function concept definition.
+tree 
+check_function_concept (tree fn)
+{
+  location_t loc = DECL_SOURCE_LOCATION (fn);
+
+  // If fn was declared with auto, make sure the result type is bool.
+  if (FNDECL_USED_AUTO (fn) && TREE_TYPE (fn) != boolean_type_node) 
+    error_at (loc, "deduced type of concept definition %qD is not %qT", 
+              fn, boolean_type_node);
+
+  // Check that the function is comprised of only a single
+  // return statements.
+  tree body = DECL_SAVED_TREE (fn);
+  if (TREE_CODE (body) == BIND_EXPR)
+    body = BIND_EXPR_BODY (body);
+  if (TREE_CODE (body) != RETURN_EXPR)
+    error_at (loc, "function concept definition %qD has multiple statements", 
+              fn);
+  
+  return NULL_TREE;
+}
+
+
 // -------------------------------------------------------------------------- //
 // Normalization
 //
@@ -425,9 +454,10 @@ normalize_misc (tree t)
     case CONSTRUCTOR:
       return t;
 
-    case STATEMENT_LIST:
-      return normalize_stmt_list (t);
-    
+    // This should have been caught as an error.
+    case STATEMENT_LIST: 
+      return NULL_TREE;
+
     default:
       gcc_unreachable ();
     }
@@ -630,28 +660,6 @@ normalize_requires (tree t)
   return t;
 }
 
-// Reduction rules for the statement list STMTS.
-//
-// Recursively reduce each statement in the list, concatenating each
-// reduced result into a conjunction of requirements. 
-//
-// A constexpr function may include statements other than a return
-// statement. The primary purpose of these rules is to filter those
-// non-return statements from the constraints language.
-tree
-normalize_stmt_list (tree stmts)
-{
-  tree lhs = NULL_TREE;
-  tree_stmt_iterator i = tsi_start (stmts);
-  while (!tsi_end_p (i))
-    {
-      if (tree rhs = normalize_node (tsi_stmt (i)))
-        lhs = conjoin_constraints (lhs, rhs);
-      tsi_next (&i);
-    }
-  return lhs;
-}
-
 // Normalize a cast expression.
 tree
 normalize_cast (tree t) 
@@ -686,6 +694,7 @@ normalize_constraints (tree reqs)
   ++processing_template_decl;
   tree expr = normalize_node (reqs);
   --processing_template_decl;
+
   return expr;
 }
 
Index: testsuite/g++.dg/concepts/fn-concept1.C
===================================================================
--- testsuite/g++.dg/concepts/fn-concept1.C	(revision 0)
+++ testsuite/g++.dg/concepts/fn-concept1.C	(revision 0)
@@ -0,0 +1,9 @@
+// { dg-options "-std=c++1z" }
+
+template<typename T>
+  concept bool Tuple() { // { dg-error "multiple statements" }
+    static_assert(T::value, "");
+    return true;
+  }
+
+  void f(Tuple&);

Re: [c++-concepts] Check function concept definitions

[Jason Merrill]

On 09/29/2014 10:55 AM, Andrew Sutton wrote:
> Jason, do you want to review this before I commit? This is a pretty small patch.

No need to wait for review before committing to the branch.

> +  // If fn was declared with auto, make sure the result type is bool.
> +  if (FNDECL_USED_AUTO (fn) && TREE_TYPE (fn) != boolean_type_node)
> +    error_at (loc, "deduced type of concept definition %qD is not %qT",
> +              fn, boolean_type_node);

This should say what the deduced type is.

> +  // Check that the function is comprised of only a single
> +  // return statements.

*statement

> +template<typename T>
> +  concept bool Tuple() { // { dg-error "multiple statements" }
> +    static_assert(T::value, "");
> +    return true;
> +  }

Hmm, have we actually discussed this in core review?  I'm not seeing it 
on the wiki.  Constexpr started out this way too, and allowing 
static_assert was added pretty fast.  C++11 said

its function-body shall be = delete, = default, or a compound-statement 
that contains only
— null statements,
— static_assert-declarations
— typedef declarations and alias-declarations that do not define classes 
or enumerations,
— using-declarations,
— using-directives,
— and exactly one return statement;

Is there a reason we want to be more strict than this for concept functions?

Jason

Re: [c++-concepts] Check function concept definitions

[Andrew Sutton]

> Hmm, have we actually discussed this in core review?  I'm not seeing it on
> the wiki.  Constexpr started out this way too, and allowing static_assert
> was added pretty fast.  C++11 said
>
> its function-body shall be = delete, = default, or a compound-statement that
> contains only
> — null statements,
> — static_assert-declarations
> — typedef declarations and alias-declarations that do not define classes or
> enumerations,
> — using-declarations,
> — using-directives,
> — and exactly one return statement;
>
> Is there a reason we want to be more strict than this for concept functions?


I don't remember much controversy on that particular limitation in
either Rapperswil or the previous telecon review.

The main reason for the restriction is that concept definitions are
normalized into a single constraint-expression. And it's not obvious
how things like using declarations and static-assertions should be
interpreted within the constraint language.

That said, having a static_assert inside a concept kind of defeats the
purpose since it triggers a diagnostic when its condition isn't
satisfied. That's not very SFINAE friendly :)

Maybe the restriction can relaxed when we consider the TS for adoption in 17.

Andrew

Re: [c++-concepts] Check function concept definitions

[Jason Merrill]

On 09/29/2014 11:46 AM, Andrew Sutton wrote:
> The main reason for the restriction is that concept definitions are
> normalized into a single constraint-expression. And it's not obvious
> how things like using declarations and static-assertions should be
> interpreted within the constraint language.

A using-declaration just affects name lookup.  They and typedefs/aliases 
can help to make the return statement easier to write.

> That said, having a static_assert inside a concept kind of defeats the
> purpose since it triggers a diagnostic when its condition isn't
> satisfied. That's not very SFINAE friendly :)

True. It might still be useful if for some reason testing a concept for 
a certain class of types indicates an error somewhere else.  And people 
are likely to try it, as indicated by the bug report.  :)

> Maybe the restriction can relaxed when we consider the TS for adoption in 17.

I suppose, but I'd prefer not to wait that long.  I guess we can talk 
about it on the call today.

Jason

Re: [c++-concepts] Check function concept definitions

[Roland Bock]

On 2014-09-29 18:32, Jason Merrill wrote:
> On 09/29/2014 11:46 AM, Andrew Sutton wrote:
>> The main reason for the restriction is that concept definitions are
>> normalized into a single constraint-expression. And it's not obvious
>> how things like using declarations and static-assertions should be
>> interpreted within the constraint language.
>
> A using-declaration just affects name lookup.  They and
> typedefs/aliases can help to make the return statement easier to write.
>
>> That said, having a static_assert inside a concept kind of defeats the
>> purpose since it triggers a diagnostic when its condition isn't
>> satisfied. That's not very SFINAE friendly :)
>
> True. It might still be useful if for some reason testing a concept
> for a certain class of types indicates an error somewhere else.  And
> people are likely to try it, as indicated by the bug report.  :)
>
>> Maybe the restriction can relaxed when we consider the TS for
>> adoption in 17.
>
> I suppose, but I'd prefer not to wait that long.  I guess we can talk
> about it on the call today.
>
> Jason
>
Since I sent that sample code with the static_assert inside the concept,
let me add that I wanted to test something completely different and
stumbled over that internal error by accident :-)

That being said, I had expected the same rules for concepts as for
constexpr functions indeed. It seemed quite natural (and would require
less RAM in my brain).

Best,

Roland