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

[waffl3x <waffl3x@protonmail.com>]

> > > > OKAY, I figured out SOMETHING, I think this should be fine. As noted in
> > > > the comments, this might be a better way of handling the static lambda
> > > > case too. This is still a nasty hack so it should probably be done
> > > > differently, but I question if making a whole new fntype node in
> > > > tsubst_lambda_expr makes sense. On the other hand, maybe there will be
> > > > problems if a lambda is already instantiated? I'm not sure, mutating
> > > > things this way makes me uneasy though.
> > > 
> > > A problem with changing TYPE_METHOD_BASETYPE for a FUNCTION_TYPE is that
> > > all equivalent FUNCTION_TYPE share the same tree node (through
> > > type_hash_canon), so you're messing with the type of unrelated functions
> > > at the same time. I think it's better to stick with the way static
> > > lambdas are handled.
> > 
> > Yes, that was my concern as well, without even thinking about hashing.
> > I will investigate it deeper but I'm pretty sure TYPE_METHOD_BASETYPE,
> > while a valid field, is not used at all for function_type nodes. I had
> > to look into this when looking into the DECL_CONTEXT stuff previously,
> > so I'm pretty confident in this. Come to think of it, does hashing even
> > take fields that aren't "supposed" to be set for a node into account?
> 
> 
> It doesn't. The issue is messing with the type of (potentially) a lot
> of different functions. Even if it doesn't actually break anything, it
> seems like the kind of hidden mutation that you were objecting to.

Oh... yeah..., I see the issue now. I still don't think the solution
used for static lambdas will work, or be painless anyhow, but if I
can't find something better I will try to use that one.

> > Well, even so, I can just clear it after it gets used as we just need
> > it to pass the closure type down. Perhaps I should have led with this,
> > but as it stands the version that uses TYPE_METHOD_BASETYPE bootstraps
> > with no regressions. I'll still look deeper but I'm pretty confident in
> > my decision here, I really don't want to try to unravel what
> > build_memfn_type does, I would rather find a whole different way of
> > passing that information down.
> 
> 
> But the existing code already works fine, it's just a question of
> changing the conditions so we handle xob lambdas the same way as static.

I'm still concerned it wont cooperate with xobj parameters of unrelated
type, but like I said, you've indicated my solution is definitely wrong
so I'll look at fixing it.

> > Anyway, due to this, I am not currently concerned about the lack of a
> > diagnostic, and I believe my implementation is the most correct way to
> > do it. The problem with resolve_address_of_overloaded_function that I
> > go into below (depending on if you agree that it's a problem) is what
> > needs to be fixed to allow the current diagnostic to be properly
> > emitted. As of right now, there is no execution path that I know of
> > where the diagnostic will be properly emitted.
> > 
> > I go into more detail about this in the comment in
> > tsubst_function_decl, although I do have to revise it a bit as I wrote
> > it before I had a discussion with another developer on the correct
> > behavior of the following case. I previously wondered if the overload
> > resolution from initializing p1 should result in a hard fault.
> > 
> > template<typename T>
> > struct S : T {
> > using T::operator();
> > void operator()(this int&, auto) {}
> > };
> > 
> > int main()
> > {
> > auto s0 = S{[](this auto&&, auto){}};
> > // error, ambiguous
> > void (*p0)(int&, int) = &decltype(s0)::operator();
> > 
> > auto s1 = S{[x = 42](this auto&&, auto){}};
> > // SFINAE's, calls S::operator()
> > void (*p1)(int&, int) = &decltype(s1)::operator();
> > }
> > 
> > The wording in [expr.prim.lambda.closure-5] is similar to that in
> > [dcl.fct-15], and we harness SFINAE in that case, so he concluded that
> > this case (initializing p1) should also harness SFINAE.
> 
> 
> Agreed.

Perfect, glad we are on the same page there.

> > > > The other problem I'm having is
> > > > 
> > > > auto f0 = [n = 5, &m](this auto const&){ n = 10; };
> > > > This errors just fine, the lambda is unconditionally const so
> > > > LAMBDA_EXPR_MUTABLE_P flag is set for the closure.
> > > > 
> > > > This on the other hand does not. The constness of the captures depends
> > > > on (I assume) LAMBDA_EXPR_MUTABLE_P so all the captures are non-const
> > > > here.
> > > > auto f1 = [n = 5](this auto&& self){ n = 10; };
> > > > as_const(f1)();
> > > 
> > > That sounds correct, why would this be an error?
> > > 
> > > The constness of the captures doesn't depend on LAMBDA_EXPR_MUTABLE_P,
> > > it depends on the type of the object parameter, which in this case is
> > > non-const, so so are the captures.
> > 
> > Oops, I should have just made a less terse example, you missed the
> > "as_const" in the call to the lambda. The object parameter should be
> > deduced as const here. I definitely should have made that example
> > better, my bad.
> 
> 
> Ah, yes, I see it now.

I don't remember if I relayed my planned fix for this to you. My
current idea is to modify the tree during instantiation of the lambda's
body somewhere near tsubst and apply const to all it's members. This is
unfortunately the best idea I have so far and it feels like an awful
hack. I am open to better ideas, but I don't think we can do anything
until the template is instantiated so I think it has to be there.

> > > > I was going to close out this message there but I just realized why
> > > > exactly you think erroring in instantiate_body is too late, it's
> > > > because at that point an error is an error, while if we error a little
> > > > bit earlier during substitution it's not a hard error. I'm glad I
> > > > realized this now, because I am much more confident in how to implement
> > > > the errors for unrelated type now. I'm still a little confused on what
> > > > the exact protocol for emitting errors at this stage is, as there
> > > > aren't many explicit errors written in. It seems to me like the errors
> > > > are supposed to be emitted when calling back into non-template stages
> > > > of the compiler with substituted types. It also seems like that hasn't
> > > > always been strictly followed, and I hate to contribute to code debt
> > > > but I'm not sure how I would do it in this case, nor if I actually have
> > > > a valid understanding of how this all works.
> > > 
> > > Most errors that could occur in template substitution are guarded by if
> > > (complain & tf_error) so that they aren't emitted during deduction
> > > substitution. It's better if those are in code that's shared with the
> > > non-template case, but sometimes that isn't feasible.
> > 
> > Yeah, makes sense, but the case going through
> > resolve_address_of_overloaded_function -> fn_type_unification ->
> > instantiate_template -> tsubst_decl -> tsubst_function_decl does not
> > behave super nicely. I tried adding (complain & tf_error) to the
> > explain_p parameter of fn_type_unification in
> > resolve_address_of_overloaded_function, but I immediately learned why
> > that wasn't being handled that way. I think
> > resolve_address_of_overloaded_function has a number of problems and
> > should probably use print_z_candidates instead of print_candidates in
> > the long term. This would actually solve the problem in
> > tsubst_function_decl where it never emits an error for an unrelated
> > type, print_z_candidates does call fn_type_unification with true passed
> > to explain_p. I go into this in detail in the large comment in
> > tsubst_function_decl (that I have to revise) so I won't waste time
> > rehashing it here.
> 
> 
> Makes sense, we won't see a message about the unrelated type because
> print_candidates doesn't repeat deduction like print_z_candidates does.
> 
> > Thank you for explaining though, some of the code doesn't reflect the
> > method you've relayed here (such as an unguarded error_at in
> > tsubst_function_decl) so I was starting to get confused on what exactly
> > is supposed to be going on. This clarifies things for me.
> 
> 
> That unguarded error does indeed look suspicious, though it could be
> that OMP reductions are called in a sufficiently unusual way that it
> isn't actually a problem in practice. /shrug
> 
> Jason

Fair enough.

Luckily, since I found that other test case, I have been able to
confirm that my diagnostic is properly reported when print_z_candidates
is called. So I feel like the diagnostic is implemented correctly.

Should I wait until I fix the issue in tsubst_lambda_expr before
submitting the patch? I'm fine to do it either way, just whatever you
prefer. If I finish cleaning up these tests before I hear back I'll go
ahead and submit it and then start looking at different solutions in
there.

Alex