[BUG] Can't have a member variable called value in a @union type

[leejy12]

Describe the bug
If a user defined @union type contains a variable called value, it fails to compile.

To Reproduce

U: @union type = {
    value: i32;
}

Error:

example.cpp2: error: a type's implementation may not declare a name that is the same as (i.e., shadows) a type scope name - for example, a type scope function's local variable may not have the same as one of the type's members

Compiler returned: 1

Additional context
This happens because cppfront will produce a member function

void U::set_value(cpp2::in<i32> value)

but since the parameter name is the same as a member variable, cppfront rejects the code.

I think having a member variable called value should be acceptable, so maybe cppfront should emit a parameter name like __value or _Value.

[JohelEGP]

This is really a problem of composition.
Templates don't have this problem
since dependent names need to be explicitly qualified
(e.g., Base::value_type, this->value).
For reference, here's why Cppfront rejects this: #274 (comment).

Some type metafunctions have input names that they transform.
For @enum, it's the enumerators, and
for @union, it's the alternatives.
Their implementation should use the _-prefixed input name when possible.
This particular issue is solved by naming the argument _𝘢𝘭𝘵𝘦𝘳𝘯𝘢𝘵𝘪𝘷𝘦 instead of value.

Looking at the code printed or generated from https://cpp2.godbolt.org/z/aMrfaEvjq,
you can guess other problematic formulations.

optional: @union @print type = {
  empty: std::monostate;
  integer: int;
}
engine: @flag_enum @print type = {
  off;
  on;
}
main: () = { }

optional: type =
{
    _storage: std::aligned_storage_t<cpp2::max(sizeof(std::monostate), sizeof(int))> = ();

    _discriminator: i8 = -1;

    is_empty:(in this) -> move bool = _discriminator == 0;

    empty:(in this) -> forward std::monostate
        [[pre: is_empty()]] = reinterpret_cast<* const std::monostate>(_storage&)*;

    empty:(inout this) -> forward std::monostate
        [[pre: is_empty()]] = reinterpret_cast<* std::monostate>(_storage&)*;

    set_empty:(
        inout this,
        in value: std::monostate
    ) =
    {
        if !is_empty()
        {
            destroy();
            std::construct_at(reinterpret_cast<* std::monostate>(_storage&), value);
        }
        else
        {
            reinterpret_cast<* std::monostate>(_storage&)* = value;
        }
        _discriminator = 0;
    }

    set_empty:(
        inout this,
        forward args...:
    ) =
    {
        if !is_empty()
        {
            destroy();
            std::construct_at(reinterpret_cast<* std::monostate>(_storage&), args...);
        }
        else
        {
            reinterpret_cast<* std::monostate>(_storage&)* = : std::monostate = (args...);
        }
        _discriminator = 0;
    }

    is_integer:(in this) -> move bool = _discriminator == 1;

    integer:(in this) -> forward int
        [[pre: is_integer()]] = reinterpret_cast<* const int>(_storage&)*;

    integer:(inout this) -> forward int
        [[pre: is_integer()]] = reinterpret_cast<* int>(_storage&)*;

    set_integer:(
        inout this,
        in value: int
    ) =
    {
        if !is_integer()
        {
            destroy();
            std::construct_at(reinterpret_cast<* int>(_storage&), value);
        }
        else
        {
            reinterpret_cast<* int>(_storage&)* = value;
        }
        _discriminator = 1;
    }

    set_integer:(
        inout this,
        forward args...:
    ) =
    {
        if !is_integer()
        {
            destroy();
            std::construct_at(reinterpret_cast<* int>(_storage&), args...);
        }
        else
        {
            reinterpret_cast<* int>(_storage&)* = : int = (args...);
        }
        _discriminator = 1;
    }

    private destroy:(inout this) =
    {
        if _discriminator == 0
        {
            std::destroy_at(reinterpret_cast<* std::monostate>(_storage&));
        }
        if _discriminator == 1
        {
            std::destroy_at(reinterpret_cast<* int>(_storage&));
        }
        _discriminator = -1;
    }

    operator=:(move this) =
    {
        destroy();
    }
}


engine: type =
{
    _value: u8;

    private operator=:(
        implicit out this,
        in val: i64
    ) == _value = cpp2::unsafe_narrow<u8>(val);

    get_raw_value:(in this) -> move u8 == _value;

    operator=:(
        out this,
        in that
    ) ==
    {
    }

    operator<=>:(
        in this,
        in that
    ) -> move std::strong_ordering;

    operator|=:(
        inout this,
        in that
    ) == _value |= that._value;

    operator&=:(
        inout this,
        in that
    ) == _value &= that._value;

    operator^=:(
        inout this,
        in that
    ) == _value ^= that._value;

    operator|:(
        in this,
        in that
    ) -> move engine == _value | that._value;

    operator&:(
        in this,
        in that
    ) -> move engine == _value & that._value;

    operator^:(
        in this,
        in that
    ) -> move engine == _value ^ that._value;

    has:(
        inout this,
        in that
    ) -> move bool == _value & that._value;

    set:(
        inout this,
        in that
    ) == _value |= that._value;

    clear:(
        inout this,
        in that
    ) == _value &= that._value~;

    off: engine == 1;

    on: engine == 2;

    none: engine == 0;

    to_string:(in this) -> move std::string =
    {
        ret: std::string = "(";
        comma: std::string = ();
        if this == none
        {
            return "(none)";
        }
        if (this & off) == off
        {
            ret += comma + "off";
            comma = ", ";
        }
        if (this & on) == on
        {
            ret += comma + "on";
            comma = ", ";
        }
        return ret + ")";
    }
}

class optional {
private: std::aligned_storage_t<cpp2::max(sizeof(std::monostate), sizeof(int))> _storage {}; private: cpp2::i8 _discriminator {-1}; public: [[nodiscard]] auto is_empty() const& -> bool;
public: [[nodiscard]] auto empty() const& -> std::monostate const&;
public: [[nodiscard]] auto empty() & -> std::monostate&;
public: auto set_empty(cpp2::in<std::monostate> value) & -> void;
public: auto set_empty(auto&& ...args) & -> void;
public: [[nodiscard]] auto is_integer() const& -> bool;
public: [[nodiscard]] auto integer() const& -> int const&;
public: [[nodiscard]] auto integer() & -> int&;
public: auto set_integer(cpp2::in<int> value) & -> void;
public: auto set_integer(auto&& ...args) & -> void;
private: auto destroy() & -> void;
public: ~optional() noexcept;

  public: optional() = default;
  public: optional(optional const&) = delete; /* No 'that' constructor, suppress copy */
  public: auto operator=(optional const&) -> void = delete;


};
class engine {
private: cpp2::u8 _value; private: constexpr engine(cpp2::in<cpp2::i64> val);

private: constexpr auto operator=(cpp2::in<cpp2::i64> val) -> engine& ;
public: [[nodiscard]] constexpr auto get_raw_value() const& -> cpp2::u8;
public: constexpr engine(engine const& that);
public: constexpr auto operator=(engine const& that) -> engine& ;
public: constexpr engine(engine&& that) noexcept;
public: constexpr auto operator=(engine&& that) noexcept -> engine& ;
public: [[nodiscard]] auto operator<=>(engine const& that) const& -> std::strong_ordering = default;
public: constexpr auto operator|=(engine const& that) & -> void;
public: constexpr auto operator&=(engine const& that) & -> void;
public: constexpr auto operator^=(engine const& that) & -> void;
public: [[nodiscard]] constexpr auto operator|(engine const& that) const& -> engine;
public: [[nodiscard]] constexpr auto operator&(engine const& that) const& -> engine;
public: [[nodiscard]] constexpr auto operator^(engine const& that) const& -> engine;
public: [[nodiscard]] constexpr auto has(engine const& that) & -> bool;
public: constexpr auto set(engine const& that) & -> void;
public: constexpr auto clear(engine const& that) & -> void;
public: static const engine off;
public: static const engine on;
public: static const engine none;
public: [[nodiscard]] auto to_string() const& -> std::string;

};

@union, as part of its API, generates from 𝘢𝘭𝘵𝘦𝘳𝘯𝘢𝘵𝘪𝘷𝘦:

• Members 𝘢𝘭𝘵𝘦𝘳𝘯𝘢𝘵𝘪𝘷𝘦, is_𝘢𝘭𝘵𝘦𝘳𝘯𝘢𝘵𝘪𝘷𝘦, and set_𝘢𝘭𝘵𝘦𝘳𝘯𝘢𝘵𝘪𝘷𝘦.

In its implementation, it uses:

• Members _storage, _discriminator, and destroy.
• Parameters args.

@flag_enum, as part of its API, generates:

• Members 𝘦𝘯𝘶𝘮𝘦𝘳𝘢𝘵𝘪𝘰𝘯, get_raw_value, some operators, has, set, clear, and to_string.

Also, a local variable can't have the name of a member.
This further opens the opportunity for shadowing.
E.g., this fails: grammar: @flag_enum type = { comma; }.

A hint to prevent clashing is in my opening paragraph.

For example, have optional: @union type; derive from cpp2::cpp_union<optional, …>
to declare _storage, _discriminator, and destroy.
Nothing else needs to be moved to cpp2::cpp_union.
The implementation of @union would have to explicitly access that base
to avoid using a member in optional or an ambiguity with another of its bases.
There's still the issue of the parameter args.

@flag_enum's to_string uses the local variable comma.
The algorithm can be refactored as a non-member,
and the member implemented in terms of that.

[hsutter]

Thanks, this is a good learning as we're use more complex metafunctions. A metafunction generally ought to avoid name collisions with user-specified names, both for the names it generates (which should get a nice diagnostic if the user writes a collision) and the names it uses in function implementations (I like the tradition of prefix _).

I'll take a pass at flagging those in basic_enum and union, but I think the metafunction services should give general helper support for this since I think many metafunctions will want to do this.

Thanks!

[JohelEGP]

I had my misgivings, but reading the commit, it looks pretty reasonable.
But rejecting all _-prefixed names when using a type metafunction seems pretty draconian.
Even a type using @struct is prevented from using this convention.

[hsutter]

But rejecting all _-prefixed names when using a type metafunction seems pretty draconian.

The issue is that metafunctions will want to create local variables and private helper functions, and those shouldn't conflict with any names the type author uses. Giving metafunction authors a reserved set of names they can use that doesn't conflict with "nice" user names (or with __ and _Capital C++ implementation reserved names) seemed like it would be important in general.

What would be the alternative... _-suffixed names? individually reserve also the list of private implementation detail names the metafunction uses? something else?

[JohelEGP]

Nope.
I overreacted, thinking that it was a great deal.
But type authors can just use _-postfix instead of _-prefix.

There's still that the chance that, at scale,
type metafunction authors could trample each other's feet.
In those cases, #720 (comment) can be reached for.