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

[waffl3x <waffl3x@protonmail.com>]

On Sunday, November 26th, 2023 at 2:30 PM, Jason Merrill <jason@redhat.com> wrote:


> 
> 
> On 11/24/23 20:14, waffl3x wrote:
> 
> > 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?
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.

> > I don't like that pt.cc feels like it has a ton of hidden mutations,
> > it's really hard to follow through it. Would you agree it's in need for
> > cleanup or am I just not experienced enough in this area yet?
> 
> 
> I'm sure there are things that could use cleaning up, but I'm not
> thinking of anything specific offhand. Any particular examples?

Not at the moment, just after 8 hours of following the execution path
you start to feel like something could be done better. I'm sorry if I
sound bitter but to be honest I kind of am. :^)

I don't think it's going to be my first goal to look at pt.cc for
things to clean up, I honestly didn't want to go through here at all.
Maybe if I look at it for longer I will feel better about it but I want
to make that a task for the future.

> > Regarding the error handling, I just had a thought about it, I have a
> > hunch it definitely needs to go in tsubst_template_decl or
> > tsubst_function_decl. There might need to be more changes to determine
> > the actual type of the lambda in there, but everything else I've done
> > changes the implicit object argument to be treated more like a regular
> > argument, doing error handling for the object type in
> > tsubst_lambda_expr would be inconsistent with that.
> 
> 
> But the rule about unrelated type is specific to lambdas, so doing it in
> tsubst_lambda_expr seems preferable to adding a lambda special case to
> generic code.

No, while that might seem intuitive, the error is not during
instantiation of the closure object's type, it's during instantiation
of the function template. The type of the closure will always be the
type of the closure, but the type of the explicit object parameter can
be different. In the following example we don't go into
tsubst_lambda_expr at all.

template<typename T>
struct S : T {
  using T::operator();
  using s_tag = void;
};

int main()
{
  auto f = [](this auto self){
    if constexpr ( requires{typename decltype(self)::s_tag;} ) {
      return 5;
    }
    else {
      return 10;
    }
  };
  auto s = S{f};

  if (f () != 10)
    __builtin_abort ();
  if (s () != 5)
    __builtin_abort ();
}

I hope this demonstrates that placing the check in tsubst_function_decl
is the correct way to do this.

Now with that out of the way, I do still kind of feel icky about it.
This really is a MASSIVE edge case that will almost never matter. As
far as I know, instantiating explicitly like so... 

auto f = [x = 42](this auto&&) -> int { return x; };
int (*fp)(int&) = &decltype(f)::operator();

...is the only way to coerce the explicit object parameter of the
lambda's call operator into deducing as an unrelated type. Cases that
are not deduced can be caught trivially while parsing the lambda and
are the only reasonable cases where you might have an unrelated type.
Perhaps it might become relevant in the future if a proposal like
https://atomgalaxy.github.io/isocpp-1107/D1107.html ever made it in,
but we aren't there yet. So as it stands, I'm pretty certain that it's
just impossible to instantiate a lambda's call operator with an
unrelated xobj parameter type except for the above case with
address-of. If you can think of another, please do share, but I've
spent a fair amount of time on it and came up with nothing.

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. Luckily for me,
that is what my implementation already did. The big comment in
tsubst_function_decl does not reflect this conclusion yet so I will
have to revise it a little bit before I can submit v6, but once I do
that I will follow up this e-mail with it.
https://eel.is/c++draft/dcl.fct#15
https://eel.is/c++draft/expr.prim#lambda.closure-5

> > 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.

Anyway, yes I agree that it's -supposed- to depend on the object
parameter, but that isn't the behavior I'm observing right now. Perhaps
you're right that it's not based on the LAMBDA_EXPR_MUTABLE_P flag, I
mostly just assumed that was the case, but it does seem to be based on
the closure object. Now that I'm clear of the previous problem today
will be spent focusing on this bug.

> > 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.

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.

> > One side note before I close up here, I don't like when *_P macros are
> > used to set flags. Is this something else we should clean up in the future?
> 
> 
> I don't think so, a wholesale renaming would just confuse existing
> developers.

I don't think changing *_P macros to be immutable, and adding
corresponding *_SET or *_FLAG macros (or functions) would be confusing.
I think it's much more confusing to have the inconsistent behavior.

In case it's not clear, I'm not proposing we change any of the names
here. I disliked the *_P name scheme at first, but as I became familiar
with it I think it's good, especially once I realized that it stands
for predicate. Even at that point though I was pretty sure that
convention should stick as it's not worth the confusion just because a
single developer, let alone a new one (me), doesn't like it. And I like
it now so I absolutely wouldn't propose making that change.

But I do think they should be immutable, this has other benefits anyway
since it will be more clear where we observe state, and where we mutate
it.

> > I'm beginning to
> > wonder if an overhaul that gets rid of the macros from the public
> > interface is a good idea. I'm reluctant to suggest that as I've really
> > warmed up to the macros a lot. They are used in a consistent and easy
> > to understand way which is highly unlike the bad uses of macros that
> > I've seen before that really obscure what's actually going on. But they
> > are still macros, so maybe moving away from them is the right call,
> > especially since there has started to be a mix-up of macros and
> > functions for the same purposes. I'm mildly of the opinion that only
> > one style should be used (in the public interface) because mixing them
> > causes confusion, it did for me anyway. Perhaps I should open a thread
> > on the general mail list and see what others think, get some input
> > before I decide which direction to go with it. To be clear, when I say
> > getting rid of macros, I want to emphasize I mean only in the public
> > interface, I don't see any value in getting rid of macros under the
> > hood as the way the checking macros are implemented is already really
> > good and works. It would only cause problems to try to move away from
> > that. I think I'll probably start to mess around with this idea as soon
> > as this patch is done.
> 
> 
> There has been some movement from macros toward inline functions since
> the switch to C++, but that also has complications with
> const-correctness, declaration order, and bit-fields. It's probably
> good to use inlines instead of larger macros when feasible, but the
> accessors should probably stay macros.
> 
> Jason

Yeah I've noticed the pains with const-correctness, it seems like a
nightmare. However, thats one of the reasons I want to tackle it, I
think fixing and propagating const correctness throughout the code base
would be the biggest thing that could be done to improve it. I have
really poor short term memory so once you start piling on mutations I
have a hard time reasoning about code really quickly.

I see what you mean regarding accessors, trying to access or mutate
bitfields through functions might be way more trouble than it's worth,
especially if we wanted to keep the `SOME_FIELD (some_node) =
new_value` pattern. We would need to return proxy objects and those
just don't always optimize properly in my (limited) experience. I plan
on playing around with some solutions once this patch is done, I'll see
what feels good, what feels bad, whether it's actually viable to change
the accessors into functions, etc.

One developer in IRC expressed that he wished that accessors were
member functions, which I also have kind of wished before. I feel like
it shouldn't be the first refactor though.

I will follow up this e-mail shortly with v6, I just have to clean up
that comment as I said. I don't want to spend too much time on it
because I want to get a proof-of-concept fix for the "lambda capture's
not const" bug today but I'll try to throw it together with a little
more effort than v5.

I do want to note though, despite my claims above that the patch is
bootstrapped and tested without regressions, I did have to make a small
change in tsubst_lambda_expr. When I refactored the changes there I
made a thinko and checked for iobj member function in the condition
before build_memfn_type, completely forgetting about the static lambda
case. I changed it back to !DECL_XOBJ_MEMBER_FUNC_P and ran the 1 test
that failed (static-operator-call5.C) and confirmed that it worked
again, but running the full test suite takes me about an hour and a
half. SO, point is, I'm confident that change broke nothing, but due to
the change technically this version is not regression tested. I am
running a bootstrap since that will finish before I'm done revising the
comment though. Hope this isn't a problem.

I will try to finish up quickly so you can get your eyes on the patch
ASAP.

Alex