Re: [PATCH v3 1/2] c++: Initial support for P0847R7 (Deducing This) [PR102609]

[waffl3x <waffl3x@protonmail.com>]

On Monday, November 13th, 2023 at 8:48 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/11/23 05:43, waffl3x wrote:
> 
> > > [combined reply to all three threads]
> > > 
> > > On 11/9/23 23:24, waffl3x wrote:
> > > 
> > > > > > There are a few known issues still present in this patch. Most importantly,
> > > > > > the implicit object argument fails to convert when passed to by-value xobj
> > > > > > parameters. This occurs both for xobj parameters that match the argument type
> > > > > > and xobj parameters that are unrelated to the object type, but have valid
> > > > > > conversions available. This behavior can be observed in the
> > > > > > explicit-obj-by-value[1-3].C tests. The implicit object argument appears to be
> > > > > > simply reinterpreted instead of any conversion applied. This is elaborated on
> > > > > > in the test cases.
> > > > > 
> > > > > Yes, that's because of:
> > > > > 
> > > > > > @@ -9949,7 +9951,8 @@ build_over_call (struct z_candidate cand, int flags, tsubst_flags_t complain)
> > > > > > }
> > > > > > }
> > > > > > / Bypass access control for 'this' parameter. */
> > > > > > - else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
> > > > > > + else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE
> > > > > > + || DECL_XOBJ_MEMBER_FUNC_P (fn))
> > > > > 
> > > > > We don't want to take this path for xob fns. Instead I think we need to
> > > > > change the existing:
> > > > > 
> > > > > > gcc_assert (first_arg == NULL_TREE);
> > > > > 
> > > > > to assert that if first_arg is non-null, we're dealing with an xob fn,
> > > > > and then go ahead and do the same conversion as the loop body on first_arg.
> > > > > 
> > > > > > Despite this, calls where there is no valid conversion
> > > > > > available are correctly rejected, which I find surprising. The
> > > > > > explicit-obj-by-value4.C testcase demonstrates this odd but correct behavior.
> > > > > 
> > > > > Yes, because checking for conversions is handled elsewhere.
> > > > 
> > > > Yeah, as I noted above I realized that just handling it the same way as
> > > > iobj member functions is fundamentally broken. I was staring at it last
> > > > night and eventually realized that I could just copy the loop body. I
> > > > ended up asserting in the body handling the implicit object argument
> > > > for xobj member functions that first_arg != NULL_TREE, which I wasn't
> > > > sure of, but it seems to work.
> > > 
> > > That sounds like it might cause trouble with
> > > 
> > > struct A {
> > > void f(this A);
> > > };
> > > 
> > > int main()
> > > {
> > > (&A::f) (A());
> > > }
> > 
> > I will check to see what the behavior with this is. This sounds related
> > to the next question I asked as well.
> > 
> > > > I tried asking in IRC if there are any circumstances where first_arg
> > > > would be null for a non-static member function and I didn't get an
> > > > answer. The code above seemed to indicate that it could be. It just
> > > > looks like old code that is no longer valid and never got removed.
> > > > Consequently this function has made it on my list of things to refactor
> > > > :^).
> > > 
> > > Right, first_arg is only actually used for the implicit object argument,
> > > it's just easier to store it separately from the arguments in (). I'm
> > > not sure which code you mean is no longer valid?
> > 
> > Yeah I agree that it's easier to store it separately.
> > 
> > -- call.cc:build_over_call
> > `else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE) { tree arg = build_this (first_arg != NULL_TREE ? first_arg : (*args)[arg_index]);`
> > 
> > The trouble is, the code (shown above) does not assume that this holds
> > true. It handles the case where the implicit object argument was passed
> > in with the rest of the arguments. As far as I've observed, it seems
> > like it's always passed in through the first_arg member of cand, which
> > is what I was referring to here.
> > 
> > > > ended up asserting in the body handling the implicit object argument
> > > > for xobj member functions that first_arg != NULL_TREE, which I wasn't
> > > > sure of, but it seems to work.
> > 
> > Since it wasn't clear what I was referring to, here is the code that I
> > wrote (copied from the loop really) handling the case. In case it isn't
> > obvious, I didn't snip the code in the METHOD_TYPE block, it's just
> > snipped here as it's not code I've modified. I'm hopeful that the case
> > you mentioned above is not problematic, but like I said I will be sure
> > to test it.
> > 
> > -- call.cc:build_over_call
> > ```
> > else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
> > {
> > /* SNIP */
> > if (first_arg != NULL_TREE)
> > first_arg = NULL_TREE;
> > else
> > ++arg_index;
> > ++i;
> > is_method = 1;
> > }
> > else if (DECL_XOBJ_MEMBER_FUNC_P (fn))
> > {
> > gcc_assert (cand->first_arg);
> > gcc_assert (cand->num_convs > 0);
> > tree type = TREE_VALUE (parm);
> > tree arg = cand->first_arg;
> > bool conversion_warning = true;
> > 
> > conv = convs[0];
> > 
> > /* Set user_conv_p on the argument conversions, so rvalue/base handling
> > knows not to allow any more UDCs. This needs to happen after we
> > process cand->warnings. */
> > if (flags & LOOKUP_NO_CONVERSION)
> > conv->user_conv_p = true;
> > 
> > tsubst_flags_t arg_complain = complain;
> > if (!conversion_warning)
> > arg_complain &= ~tf_warning;
> > 
> > if (arg_complain & tf_warning)
> > maybe_warn_pessimizing_move (arg, type, /return_p/false);
> > 
> > val = convert_like_with_context (conv, arg, fn, 0,
> > arg_complain);
> > val = convert_for_arg_passing (type, val, arg_complain);
> > 
> > if (val == error_mark_node)
> > return error_mark_node;
> > else
> > argarray[j++] = val;
> > /* Advance parameter chain, don't advance arg index */
> > parm = TREE_CHAIN (parm);
> > // ++arg_index;
> > ++i;
> > is_method = 1;
> > first_arg = NULL_TREE;
> > }
> > ```
> > 
> > Anyway, I hope we can assume that first_argument is populated for all
> > non-static member function calls, and if we can't perhaps that is
> > something we can change. The assumptions we can make are unclear with
> > how the code handling METHOD_TYPE is right now. If we make it a
> > guarantee that first_argument is populated for non-static member
> > function calls that code can be vastly simplified.
> 
> 
> Somewhat, but the current METHOD_TYPE code works fine, so let's not mess
> with it. There might be corner cases that end up with the object
> argument in the args vec.

Yeah I haven't messed with it, I fully agree that it's not something
that should be a part of this patch. I do want to thoroughly
investigate whether these corner cases exist in the future though.

> [other reply]
> 
> > I already showed my code here but I didn't think to include the
> > previous conditional block to demonstrate how the first_arg variable
> > gets handled in it. Although, maybe you understood they set it to
> > NULL_TREE by the end of the block and I just poorly communicated that I
> > would be doing the same.
> 
> 
> I meant that I expect the
> 
> > gcc_assert (cand->first_arg);
> 
> 
> to fail on my example.
> 
> > Perhaps you think it better to handle the implicit object argument
> > within the loop, but given that it is stored in first_arg I don't think
> > that would be correct. Granted, as I previously said, maybe there are
> > some situations where it isn't that I haven't encountered yet. The
> > other thing is there is some handling in the loop that isn't relevant
> > to the implicit object argument. Well, I would also just argue this
> > function needs a fair degree of refactoring, but I've already talked
> > about that.
> 
> 
> Factoring out the loop body (into a lambda?) rather than copying it
> would make sense to me.

Gladly, the duplication made me uneasy for sure.

> > > > > > Other than this, lambdas are not yet supported,
> > > > > 
> > > > > The error I'm seeing with the lambda testcase is "explicit object member
> > > > > function cannot have cv-qualifier". To avoid that, in
> > > > > cp_parser_lambda_declarator_opt you need to set quals to
> > > > > TYPE_UNQUALIFIED around where we do that for mutable lambdas.
> > > > 
> > > > Yeah, this ended up being the case as suspected, and it was exactly in
> > > > cp_parser_lambda_declarator_opt that it needed to be done. There's an
> > > > additional quirk in start_preparsed_function, which I already rambled
> > > > about a little in a previous reply on this chain, that suddenly
> > > > restored setting up the 'this' pointer. Previously, it was being
> > > > skipped because ctype was not being set for xobj member functions.
> > > > However, ctype not being set was causing the scope to not be set up
> > > > correctly for lambdas. In truth I don't remember exactly how the
> > > > problem was presenting but I do know how I fixed it.
> > > > 
> > > > `tree fntype = TREE_TYPE (decl1); if (TREE_CODE (fntype) == METHOD_TYPE) ctype = TYPE_METHOD_BASETYPE (fntype); else if (DECL_XOBJ_MEMBER_FUNC_P (decl1)) ctype = DECL_CONTEXT (decl1);`
> > > > 
> > > > In hindsight though, I have to question if this is the correct way of
> > > > dealing with it. As I mentioned previously, there's an additional quirk
> > > > in start_preparsed_function where it sets up the 'this' param, or at
> > > > least, it kind of looks like it? This is where my rambling was about.
> > > > 
> > > > `/* We don't need deal with 'this' or vtable for xobj member functions. */ if (ctype && !doing_friend && !(DECL_STATIC_FUNCTION_P (decl1) || DECL_XOBJ_MEMBER_FUNC_P (decl1)))`
> > > > 
> > > > My solution was to just not enter that block for xobj member functions,
> > > > but I'm realizing now that ctype doesn't seem to be set for static
> > > > member functions so setting ctype for xobj member functions might be
> > > > incorrect. I'll need to investigate it closer, I recall seeing the
> > > > second block above and thinking "it's checking to make sure decl1 is
> > > > not a static member function before entering this block, so that means
> > > > it must be set." It seems that was incorrect.
> > > > 
> > > > At bare minimum, I'll have to look at this again.
> > > 
> > > Yeah, that could use some refactoring. IMO ctype should be set iff we
> > > want to push_nested_class, and the code for setting up 'this' should
> > > check for METHOD_TYPE instead of referring to ctype.
> > 
> > I'll put it on the todo list for after this patch is done. I'll note
> > that it seemed like not doing push_nested_class seemed to work just
> > fine for xobj member functions, and only caused problems once I was
> > working on lambdas.
> 
> 
> Hmm, we want push_nested_class for any member function so that name
> lookup within the function finds class members.

Okay, thanks for noting this explicitly, I probably would have run into
problems otherwise. Come to think of it, I might have to be careful to
make sure that I'm handling calling a function of base from a derived
class correctly. I definitely need tests for this case.

> > For this patch, I think I will change the condition
> > for setting up 'this' instead. On the other hand, are we sure that
> > lambdas don't assume 'this' was setup when accessing their captures? I
> > will investigate this briefly but now that I've thought of that, maybe
> > I won't mess around with this after all. On the other other hand, I'm
> > pretty sure captures still work for lambdas even though that block
> > isn't being handled now... or maybe I haven't tested them.
> 
> 
> Captures don't rely on being able to name the object parameter, they use
> it directly without name lookup. And indeed 'this' doesn't name the
> object parameter for the lambda op(), finish_this_expr looks past it.

Wonderful, that should be helpful.

> > Actually, I think it doesn't matter if lambdas do use that or not for
> > their captures, because handling lambdas with an xobj parameter
> > correctly will still involve changing how lambdas access their
> > captures. PR102609 has a reply providing a test case when calling a
> > lambdas call operator where the first argument is a type unrelated to
> > the lambda's closure type. This might actually be relevant because I
> > can think of a case where one could conditionally (if constexpr) access
> > captures based on the type of the xobj parameter. THEN AGAIN, I don't
> > think it's possible to pass an unrelated implicit object parameter to a
> > lambda's call operator. (Not without explicitly calling it by function
> > pointer of course.) There are types that are derived from the lambda,
> > but those are still related so I assume that captures should be able to
> > be accessed in such a case.
> > 
> > Man, that raises even more questions for myself. If we are supposed to
> > implicitly use the explicit object parameter for accessing captures...
> > 
> > ([PR102609] Gašper Ažman from comment #17)
> > 
> > > When a lambda has captures, the explicit object parameter is used to get at
> > > them silently, if they are related, otherwise the program is ill-formed:
> > 
> > ...then what happens when the type of the explicit object parameter is
> > a class derived from the lambda, and the derived class has a member
> > variable with the same name as a capture we are trying to access?
> 
> 
> That ought to work fine; capture fields don't have the name of the
> variables they capture, they are only accessible through the capture
> proxy (from build_capture_proxy), which in turn refers directly to the
> member by its _DECL, not by name lookup.

Okay, so just adding error checking for an unrelated object parameter
type should do the trick for us then.

> > > > Somewhat related is some warnings I wanted to implement for impossible
> > > > to call by-value xobj member functions. Ones that involve an unrelated
> > > > type get dicey because people could in theory have legitimate use cases
> > > > for that, P0847R7 includes an example of this combining recursive
> > > > lambdas and the overload pattern to create a visitor. However, I think
> > > > it would be reasonable to warn when a by-value xobj member function can
> > > > not be called due to the presence of overloads that take references.
> > > > Off the top of my head I don't recall how many overloads it would take
> > > > to prevent a by-value version from being called, nor have I looked into
> > > > how to implement this.
> > > 
> > > Hmm, is it possible to make it un-callable? A function taking A and a
> > > function taking A&& are ambiguous when called with an rvalue argument,
> > > the by-value overload isn't hidden. Similarly, A and A& are ambiguous
> > > when called with a cv-unqualified lvalue.
> > 
> > I believe so, consider the following:
> > 
> > struct S {
> > void f(this S) {}
> > void f(this S const&) {}
> > void f(this S&&) {}
> > };
> > 
> > I'm not 100% what the rules on this are, if I were to hazard a guess I
> > bet that the reference overloads are taken because they have one less
> > conversion?
> 
> 
> Initializing a by-value parameter isn't an extra conversion, it's an
> identity conversion, so ambiguous with the reference overloads, just as with

I will have to check again, but I recall seeing cand had conversion to
rvalue for arguments that initialize by-value parameters. I assume I'm
either mistaken or misunderstanding what I was looking at though.

> struct A { };
> void f (A);
> void f (const A&);
> void f (A&&);
> 
> int main()
> {
> A a;
> f (a); // ambiguous
> f (A()); // ambiguous
> }

Ambiguous is good though, we might not need to issue a warning at all.

> > To be clear, I recognize that you could still select the by-value
> > candidate by assigning it to a pointer to function type. I just don't
> > really think this is a useful or typical case, while accidentally
> > preventing a by-value xobj member function from being callable seems
> > like something that could easily happen, especially if we had the
> > following.
> > 
> > struct S {
> > template<typename Self>
> > void f(this Self&&) {}
> > /* Our class is small enough, optimize const calls here. */
> > void f(this S) {}
> > }
> > 
> > I could see this being a typical enough refactor that I would want a
> > warning to alert me that my function that I'm writing for optimization
> > purposes won't ever be called in the cases I intend it to be called in.
> > 
> > Granted, I fully admit that I don't fully understand the semantics with
> > overload resolution here so my concerns might be entirely unfounded.
> > This is especially true in the second case since I'm not sure if the
> > deduced case actually will eat up all the calls of the member function
> > or not.
> 
> 
> Rather, in the deduced case the by-value overload always wins because
> overload resolution prefers a non-template if the conversions are
> equally good.
> 
> struct A { };
> void f (A);
> template <class T> void f (T&&);
> 
> 
> int main()
> {
> A a;
> f (a); // calls non-template
> f (A()); // calls non-template
> }

That's less good, maybe we want a warning in the other direction now
then? I had expected it to prioritize the template for this. I can
definitely imagine someone getting confused why their object isn't
mutating in a case like this if they were delegating to other functions
in the function call.

Well, I tried to write an example and I'm realizing that maybe my
concerns are imagined. So I'm not going to worry about it for now. I'm
thinking that the cases where someone wants to deduce Self and also
have a by-value overload are not that common. On the other hand, maybe
we still want to warn for it in case people try to do it.

I'll put it in the back of my mind for now, it doesn't seem important
enough to worry about anymore.

> > I'll try to get a new version of the patch out ASAP, I'm not sure I'll
> > include everything we talked about right away but I'll stuff in as much
> > as I can. I'll focus on the missing semantics before the more
> > complicated diagnostics changes. I think detection of redeclarations,
> > the correct overriding behavior, and the additional warnings I have
> > been discussing, will come later.
> 
> 
> Makes sense.
> 
> > I think I'm also going to leave investigation of requires expressions
> > in non-dependent contexts that I had mentioned for another time as
> > well. The tests related to those have been fixed and work as expected
> > right now, and I think diving in and trying to figure out whether I can
> > do those tests with non-dependent operands would be a bad use of my
> > time.
> 
> 
> Agreed.

Unfortunately I was a bit under the weather this weekend so I haven't
gotten much work done, I've been going strong today though. Plan stays
the same though.

> > Of course I will make sure all the small changes you requested make it
> > in, that might be the first thing I work on.
> 
> 
> Also note that revisions can slip into Stage 3, though preferably not
> too far.
> 
> Jason

I'll try not to let it come to that, better I don't consider it an
option anyway. :^)

Alex