Re: [PATCH] c++: Implement C++23 P1169R4 - static operator() [PR106651]

[Jakub Jelinek <jakub@redhat.com>]

On Sat, Sep 17, 2022 at 01:23:59AM +0200, Jason Merrill wrote:
> On 9/13/22 12:42, Jakub Jelinek wrote:
> > The following patch attempts to implement C++23 P1169R4 - static operator()
> > paper's compiler side (there is some small library side too not implemented
> > yet).  This allows static members as user operator() declarations and
> > static specifier on lambdas without lambda capture.  As decl specifier
> > parsing doesn't track about the presence and locations of all specifiers,
> > the patch unfortunately replaces the diagnostics about duplicate mutable
> > with diagnostics about conflicting specifiers because the information
> > whether it was mutable mutable, mutable static, static mutable or static
> > static is lost.
> 
> I wonder why we don't give an error when setting the
> conflicting_specifiers_p flag in cp_parser_set_storage_class?  We should be
> able to give a better diagnostic at that point.

I will try that.

> > Beyond this, the synthetized conversion operator changes
> > for static lambdas as it can just return the operator() static method
> > address, doesn't need to create a thunk for it.
> > The change I'm least sure about is the call.cc (joust) change, one thing
> > is that we ICEd because we assumed that len could be different only if
> > both candidates are direct calls but it can be one direct and one indirect
> > call,
> 
> How do you mean?

That is either on the
struct less {
    static constexpr auto operator()(int i, int j) -> bool {
        return i < j;
    }

    using P = bool(*)(int, int);
    operator P() const { return operator(); }
};

static_assert(less{}(1, 2));
testcase from the paper (S in static-operator-call1.C) or any static lambda
which also has static operator() and cast operator.
The paper then talks about
operator()(contrived-parameter, int, int);
call-function(bool(*)(int, int), int, int);
which is exactly what caused the ICE, one of the cand?->fn was the static
operator(), the other cand?->fn isn't a FUNCTION_DECL, but a function
pointer (what the cast operator returns).

> > +	{
> > +	  /* C++23 [over.best.ics.general] says:
> > +	     When the parameter is the implicit object parameter of a static
> > +	     member function, the implicit conversion sequence is a standard
> > +	     conversion sequence that is neither better nor worse than any
> > +	     other standard conversion sequence.
> > +	     Apply this for C++23 or when the static member function is
> > +	     overloaded call operator (C++23 feature we accept as an
> > +	     extension).  */
> > +	  if ((cxx_dialect >= cxx23
> > +	       || (DECL_OVERLOADED_OPERATOR_P (cand1->fn)
> > +		   && DECL_OVERLOADED_OPERATOR_IS (cand1->fn, CALL_EXPR)))
> > +	      && CONVERSION_RANK (cand2->convs[0]) >= cr_user)
> > +	    winner = -1;
> 
> I don't know what you're trying to do here.  The above passage describes how
> we've always compared static to non-static, which should work the same way
> for operator().

It doesn't work for the static operator() case vs. pointer returned by
cast operator, with just the
-      int static_1 = DECL_STATIC_FUNCTION_P (cand1->fn);
-      int static_2 = DECL_STATIC_FUNCTION_P (cand2->fn);
+      int static_1 = (TREE_CODE (cand1->fn) == FUNCTION_DECL
+                     && DECL_STATIC_FUNCTION_P (cand1->fn));
+      int static_2 = (TREE_CODE (cand2->fn) == FUNCTION_DECL
+                     && DECL_STATIC_FUNCTION_P (cand2->fn));

-      if (DECL_CONSTRUCTOR_P (cand1->fn)
+      if (TREE_CODE (cand1->fn) == FUNCTION_DECL
+         && TREE_CODE (cand2->fn) == FUNCTION_DECL
+         && DECL_CONSTRUCTOR_P (cand1->fn)
changes in joust so that it doesn't ICE, the call is ambiguous.
Previously the standard had:
"If F is a static member function, ICS1(F) is defined such that ICS1(F) is neither better
nor worse than ICS1(G) for any function G, and, symmetrically, ICS1(G) is neither better
nor worse than ICS1(F); otherwise,"
but that was removed and instead:
"When the parameter is the implicit object parameter of a static member function, the
implicit conversion sequence is a standard conversion sequence that is neither better
nor worse than any other standard conversion sequence."
was added elsewhere.  The code implements the former.  We don't have any
conversion for the non-existing object parameter to static, so what I want
to do is keep doing what we were before if the other candidate's conversion
on the first parameter is standard (CONVERSION_RANK (cand2->convs[0]) < cr_user)
and otherwise indicate that the static operator() has there a standard
conversion and so it should be better than any user/ellipsis/bad conversion.
Now, it could be done just for cxx_dialect >= cxx23 as that is pedantically
what the standard says, but if we allow with a pedwarn static operator()
in earlier standards, I think we better treat it there the same, because
otherwise one can't call any static lambdas (all those calls would be
ambiguous).

> > @@ -1135,9 +1141,13 @@ maybe_add_lambda_conv_op (tree type)
> >     tree fn_args = NULL_TREE;
> >     {
> >       int ix = 0;
> > -    tree src = DECL_CHAIN (DECL_ARGUMENTS (callop));
> > +    tree src = DECL_ARGUMENTS (callop);
> >       tree tgt = NULL;
> > +    if (thisarg)
> > +      src = DECL_CHAIN (src);
> 
> You could use FUNCTION_FIRST_USER_PARM to skip 'this' if present.

Ok, will try.

> > +    else if (!decltype_call)
> > +      src = NULL_TREE;
> >       while (src)
> >         {
> >   	tree new_node = copy_node (src);
> > @@ -1160,12 +1170,15 @@ maybe_add_lambda_conv_op (tree type)
> >   	if (generic_lambda_p)
> >   	  {
> >   	    tree a = tgt;
> > -	    if (DECL_PACK_P (tgt))
> > +	    if (thisarg)
> 
> This is wrong, pack expansion handling has nothing to do with 'this'. Only
> the assignment to CALL_EXPR_ARG should depend on thisarg.

The point was that make_pack_expansion etc. is only used for what
is later then stored to CALL_EXPR_ARG.  As we don't need to construct
call at all for the static lambda case, that looked like wasted effort.

> > @@ -1203,14 +1216,18 @@ maybe_add_lambda_conv_op (tree type)
> >   			   direct_argvec->address ());
> >       }
> > -  CALL_FROM_THUNK_P (call) = 1;
> > -  SET_EXPR_LOCATION (call, UNKNOWN_LOCATION);
> > +  if (thisarg)
> > +    {
> > +      CALL_FROM_THUNK_P (call) = 1;
> > +      SET_EXPR_LOCATION (call, UNKNOWN_LOCATION);
> > +    }
> > -  tree stattype = build_function_type (fn_result, FUNCTION_ARG_CHAIN (callop));
> > -  stattype = (cp_build_type_attribute_variant
> > -	      (stattype, TYPE_ATTRIBUTES (optype)));
> > -  if (flag_noexcept_type
> > -      && TYPE_NOTHROW_P (TREE_TYPE (callop)))
> > +  tree stattype
> > +    = build_function_type (fn_result, thisarg ? FUNCTION_ARG_CHAIN (callop)
> > +				      : TYPE_ARG_TYPES (optype));
> > +  stattype = cp_build_type_attribute_variant (stattype,
> > +					      TYPE_ATTRIBUTES (optype));
> > +  if (flag_noexcept_type && TYPE_NOTHROW_P (TREE_TYPE (callop)))
> >       stattype = build_exception_variant (stattype, noexcept_true_spec);
> 
> If !thisarg, shouldn't stattype just be optype?  No need to rebuild it in
> that case.

I thought it should be auto-deduced if it isn't yet.  If it doesn't need to
be, sure, we wouldn't need to bother with decltype_call for !thisarg
either and just not do the loop on arguments etc.
I've tried to create a testcase where I could see the generic static lambda
but I failed to construct one, even with the template <...> lambda syntax
I can't use a template id of the lambda to instantiate some particular case
and then use *.  Would passing a generic lambda to a function with
some function pointer argument trigger it?

	Jakub