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

[filipsajdak]

In the current implementation (a981011) 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]

Maybe this is also a good example for forward this?

[hsutter]

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]

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

[filipsajdak]

Regarding decltype(auto), there was a discussion about returning a temporary variable (#175) and here #248 (comment)

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

[filipsajdak]

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]

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

[hsutter]

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]

See 43cdf3e

[hsutter]

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]

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]

@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]

[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]

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?