[BUG] forwarding a member of class from method with const `this` returns non-const reference that drops `const`

filipsajdak commented 1 year ago

In the current implementation (a981011782a972f1b6474f41c071517f1f061446) cppfront does not handle forwarding of class attributes well. The following code:

spec: type = {
    name:        std::string = "spec";
    get_name: (in    this) -> forward std::string = this.name;
    get_name: (inout this) -> forward std::string = this.name;
}

Generates:

class spec  {
    private: std::string name {"spec"}; 
    public: [[nodiscard]] auto get_name() const -> std::string& { return (*this).name; }
    public: [[nodiscard]] auto get_name() -> std::string& { return (*this).name; }
}

When compiles using cpp1 compiler:

../tests/type_forward.cpp2:4:74: error: binding reference of type 'basic_string<...>' to value of type 'const basic_string<...>' drops 'const' qualifier
    public: [[nodiscard]] auto get_name() const -> std::string& { return (*this).name; }

Expectation

I expect to get std::string const& when forwarding the member of the class when in this is used (explicitly or implicitly). I can now achieve that by adding const to the return type:

    get_name: (in    this) -> forward const std::string = this.name;

that will generate:

    public: [[nodiscard]] auto get_name() const -> std::string const& { return (*this).name; }

That works but breaks the idea of writing intentions rather expected generated code.

Side notes

Playing a little bit with that I come to the conclusion that what I am missing here is something that will mimic Deducing this (P0847). I imagine writing something like the following:

spec: type = {
    name:        std::string = "spec";
    get_name: (this : _) -> forward std::string = this.name; 
    // alternatively: get_name: (deduce this) -> forward std::string = this.name;
}

I understand that P0847 is a C++23 feature that is not yet implemented by our supported compilers but maybe we can mimic it here somehow? Using that syntax we could at least generate both versions of methods (const & non-const).

I am also a little disapointed with version that returns forward _. The following code:

    get_name: (in    this) -> forward _ = this.name;
    get_name: (inout this) -> forward _ = this.name;

Generates:

    public: [[nodiscard]] auto get_name() const -> decltype(auto) { return (*this).name; }
    public: [[nodiscard]] auto get_name() -> decltype(auto) { return (*this).name; }

In both methods return type is deduced to std::string. What I was expecting was behaviour like with return type auto&& which for non-const version returns std::string& and for const version returns std::string const&.

filipsajdak commented 1 year ago

Maybe this is also a good example for forward this?

hsutter commented 1 year ago

Good points, thanks!

Re -> forward X: For now, I think it's good to explore the path of having -> forward X be implicitly -> X const& (not -> X&) from an in this function (aka const member function)... all the time. Even though the function may be returning something other than a member, it seems like a const function would prefer to be const. Let's try this path and see if there's real demand for being able to have a const member function return a reference to non-onst. (Const FTW!)

Re -> forward _: Somewhere I had the notion (maybe from some previous issue/PR comment thread?) that decltype(auto) was the thing... but you're right that auto&& seems to make more sense. For now I'm switching back to auto&& to try that path.

Thanks!

hsutter commented 1 year ago

Manually citing 413de0ed5747a74e8226c6474072925cf21bacd8 here since I mistyped "closes $274" in the title... it's been a long day :)

filipsajdak commented 1 year ago

Regarding decltype(auto), there was a discussion about returning a temporary variable (https://github.com/hsutter/cppfront/issues/175) and here https://github.com/hsutter/cppfront/issues/248#issuecomment-1412166343

In situations where we return member of the class definately auto&& is the right thing to do!

filipsajdak commented 1 year ago

Now my code changed from:

        get_name:        (this) -> forward const std::string = this.name;
        get_description: (this) -> forward const std::string = this.description;
        get_config:      (this) -> forward const execspec::config_t = this.config;
        get_stages:      (this) -> forward const std::vector<execspec::process_stage> = this.stages;

to

        get_name:        (this) -> forward _ = this.name;
        get_description: (this) -> forward _ = this.description;
        get_config:      (this) -> forward _ = this.config;
        get_stages:      (this) -> forward _ = this.stages;

Looks promissing!

filipsajdak commented 1 year ago

And @TartanLlama is using auto&& in his work demonstrating P0847 (Deducing this) -> https://devblogs.microsoft.com/cppblog/cpp23-deducing-this/#de-duplication-quadruplication

hsutter commented 1 year ago

Mmm, but those references remind me that we'll now be returning a dangling reference for return of a local variable. I should ban that... ok, done!

hsutter commented 1 year ago

See 43cdf3e651ebe51cf1d68efbe50128c248f63364

hsutter commented 1 year ago

Re these:

    get_name:        (this) -> forward _ = this.name;
    get_description: (this) -> forward _ = this.description;
    get_config:      (this) -> forward _ = this.config;
    get_stages:      (this) -> forward _ = this.stages;

Is the this. needed? I think that

    get_name:        (this) -> forward _ = name;
    get_description: (this) -> forward _ = description;
    get_config:      (this) -> forward _ = config;
    get_stages:      (this) -> forward _ = stages;

should work too, right?

JohelEGP commented 1 year ago

Yes. A difference is that the former necessarily breaks when a data member declaration is renamed but not its use. The latter may continue to work and change meaning.

filipsajdak commented 1 year ago

@hsutter yes, that works. I did not know that it will work. I thought that this is required.

I just checked. Deducing this (P0847) described also here https://devblogs.microsoft.com/cppblog/cpp23-deducing-this/#design requires use of self when calling class member.

From the blog mentioned above:

One name resolution change is that inside such a member function, you are not allowed to explicitly or implicitly refer to this.
struct cat {
std::string name;

void print_name(this const cat& self) {
    std::cout << name;       //invalid
    std::cout << this->name; //also invalid
    std::cout << self.name;  //all good
}

};



Should we align to that?

hsutter commented 1 year ago

[updated answer]

Should we align to that?

If you mean should we ban using this: No. Note P0847 still allows this-qualification, just using the name self instead (and with a . instead of ->). What P0847 is doing is effectively trying to rename this to self, but can't fully do that as it heroically bolts an "explicit this" into the Cpp1 syntax that already has a magical this everywhere. So it bans using the name this if self is available, not because this is bad, but as an escape hatch to use the new name instead of the old name in a complex syntax that supports both names. There's no such complexity or ambiguity in Cpp2... there's just a this parameter, which is fully explicit and can be decorated and used like any other parameter. Oh, and it uses . all the time. :) So it's all good.

After I wrote that I realized maybe you meant should we require qualifying this.: Maybe. That would actually be consistent with making this a non-magical parameter. Interesting idea, let me noodle on it!

AbhinavK00 commented 1 year ago

I think forward could be divided into 2 different return types, T& const and T&. A function will either return references to it's arguments (this parameter is also an argument) or some global. In case of arguments, if it is an in argument, you'll always return back T& const. If it is an out or inout parameter, then both cases are possible, so maybe this can be a motivating case for distinction. Similar with globals, you may return back const or mutating references to globals. move arguments should not be forward returned. And as for forward parameters, as they're just wrapper functions, it really depends on the underlying function that you're forwarding your argument to, not sure about this case. I'm not clear about forward parameters, so maybe someone can educate me about those?

filipsajdak commented 1 year ago

@hsutter Yes, I was referring to requiring qualifying this.. I recall that in one of the issues (https://github.com/hsutter/cppfront/issues/257#issuecomment-1460317217) you mention

in Cpp2 I've tried to follow "explicit is better than implicit"

P0847 is similar here: when the method defines this explicitly, it is forbidden to use it implicitly. And the other thing is that it was natural for me that when I have this as a method argument, I should use it (as I usually would use other function arguments).

filipsajdak commented 1 year ago

@AbhinavK00 regarding forwarding take a look here: https://github.com/hsutter/cppfront/issues/248

I will provide more info later.

hsutter commented 1 year ago

"Should we require this. qualification?" is a good question. After sleeping on it once, here's where I am with this...

I'm not yet convinced about requiring this. qualification. The argument in favor of requiring it is on the basis of consistency to make this be totally non-magical. But the two emphasized parts of that argument are the parts that leave me unconvinced... I think that requiring it isn't really removing magic, and requiring it only for data members fails to fully realize consistency:

Optional (default), not implicit (magic). I think this is partly about how we view the ability to omit this.... if we view this. to be "implicit," I agree that would feel magical. But my view is different: I view this. to be optional (not implicit), and then it's just another default (not magic). That is, just like other defaults (e.g., parameters default to in, return types default to -> void, and this's type defaults to the enclosing type because there's nothing else it could be) this isn't a second syntax to say the same thing, it's the same syntax with optional parts defaulted for convenience especially when there's nothing else it could be (*) -- you don't have to spell out the default if you're using it. (Insert obligatory reference to @KateGregory's classic talk title "What You Say When You Say Nothing At All.")
Consistency. If we did require this. on the grounds of requiring it to be explicit, then we should require it for all member accesses, not just data members. And it feels weird (at least clunky, and there might be usability problems I can't think of right now) to require this.f() syntax for every member function call. It's the kind of thing that adds verbosity for insufficient (if any?) benefit.

(*) But this did motivate me to make a change: Let's try improving shadowing by banning member name shadowing. One of the reasons you really need the ability to qualify this-> today is that you might have a local name that shadows a member name, so you need to say this-> to incant the member name. That's confusing for other reasons too, and a pitfall, so it seems worth avoiding in its own right: I just pushed commits 96c4126fb973a855d73172fa5e56bb781fbbbd2f and a2c71a99b9effebde5b6e4088ad4a2ddd4fb3bf1 that prevent a type's implementation from declaring any local name that shadows a type scope name (in particular, but not limited to, a parameter or local variable in a member function that tries to reuse the same name as a data member of the type).

Summarizing, I'd say that in terms of solving known problems:

I'm not sure how requiring programmers to write explicit this. is solving a problem we have in C++ today in practice that hasn't already been addressed in Cpp2 (once we ban shadowing).
I do think it's a known minor confusion that a parameter/local name can have the same name as a member (I myself rightly got dinged for it in this repo in cpp2util.h, in #260 which I fixed, thanks again @hypatia-of-sva! and I think I have a few of them left elsewhere), and so I think ensuring that can't happen does solve a problem.

And the other thing is that it was natural for me that when I have this as a method argument, I should use it (as I usually would use other function arguments).

Yes, and you can still do that and I agree that's important.

There's a snapshot of my current thinking about, anyway. Again, good question! Thanks.

AbhinavK00 commented 1 year ago

I personally think that this. should be required.

But my view isn't that this. is implicit, it's that it's optional, and then it's just another default (not magic).

Defaults are still kind of magic. In case of in, having a default is reasonable since there are other options that can be used according to use case but in case of this, there's only one option so it may as well be spelled out. It's one less thing on mind and one more thing on screen. But this argument can go both ways. Requiring this to be written establishes consistency between member functions and free functions, you will write smth.something in both cases.

If we did require this. on the grounds of requiring it to be explicit, then we should require it for all member accesses, not just data members.

Isn't that what we do today? We write classname.membfunc to call member functions or am I missing something?

And it feels weird (at least clunky, and there might be usability problems I can't think of right now)

One problem I can think of having to write inout this.func to call an inout member functions, same with other parameter qualifiers. We can definitely come up with some better syntax than this to address this problem and the current syntax is not final so this problem may not as well exist in future.

I'm not sure how requiring programmers to write explicit this. is solving a problem we have in C++ today in practice that hasn't already been addressed in Cpp2 (once we ban shadowing).

This can go both ways, not having to write this is also not solving any problem, just making the code shorter.

I think in this case, sticking to explicit this is better as it is familiar to current cpp developers.

By the way @filipsajdak , I understand forward returning, it's just forward parameters that I'm unable to see use case for (other than wrapper functions).

filipsajdak commented 1 year ago

@hsutter I agree that having to use this. also for member functions starts to feels clunky. And using:

this.fun(x,y);

will trigger UFCS and will end up with:

CPP2_UFCS(fun, (*this), x, y);

There is no issue when there are member functions but if you will make mistake you can call external function instead. So, we need to pay more attention how it will interact with UFCS as that might not be obvious.

And it makes currently difference if you will call a method with or without this. one will trigger UFCS the other will not.

filipsajdak commented 1 year ago

@AbhinavK00 argument forwarding are useful for constructors and factory methods when you want to forward the type you provides as an argument (its called perfect forwarding - see here: https://en.cppreference.com/w/cpp/utility/forward#Example). In cppfront Herb shows how to perfect forward a specific type using requires clause that makes it even better (see here: https://youtu.be/ELeZAKCN4tY?t=5330).

hsutter commented 1 year ago

And it makes currently difference if you will call a method with or without this. one will trigger UFCS the other will not.

Right. It's generally true that obj.f(x) is able to call f(obj, value) as a fallback, and I like your observation that this is true everywhere including inside member function bodies for member function calls... that's consistent (enables us to remember/teach one thing that's the same everywhere) and seems just as useful inside member function bodies as outside them... and because this. is required to opt into it, it also has like what seems the right default, as you just observed. :)

Good observation, thanks.

filipsajdak commented 1 year ago

It's a way to provide customization points.

JohelEGP commented 1 year ago

That should be reserved for the type's author, right? Or the namespace's owner.

hsutter commented 1 year ago

Right, only the type's author can use this. qualification because only they can write type-scope (member) functions and have a this.

SebastianTroy commented 1 year ago

Even if you allow the reopening of types? (like you need for metaclass style namespaces?)

On 13 March 2023 20:57:37 Herb Sutter @.***> wrote:

Right, only the type's author can use this. qualification because only they can write type-scope (member) functions and have a this.

— Reply to this email directly, view it on GitHubhttps://github.com/hsutter/cppfront/issues/274#issuecomment-1466945578, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALUZQLTOYUKYXIF3R5ED33W36C33ANCNFSM6AAAAAAVXWMTCY. You are receiving this because you are subscribed to this thread.Message ID: @.***>

hsutter / cppfront

[BUG] forwarding a member of class from method with const `this` returns non-const reference that drops `const` #274

Expectation

Side notes