[BUG] Converting assignment generated from non-converting constructor

[JohelEGP]

Title: Converting assignment generated from non-converting constructor.

Description:

Relevant extracts from https://github.com/hsutter/cppfront/wiki/Cpp2:-operator=,-this-&-that
always talk about generating converting assignment from a converting constructor,
but cppfront currently generate converting assignment from an explicit constructor.

• implicit: On a two-parameter operator= function, writing operator=: (implicit out this, /*...*/) defines a conversion function that can be used to perform implicit conversions to this type.

operator= is the generalized name for all value-setting functions, including the six combinations of { copy, move, converting } x { constructors, assignment operators }.

Note: The concept of a converting assignment operator is first-class in Cpp2, and makes this 3x2 matrix of options symmetric. In Cpp1, it's common to write a converting constructor from some other type, but not an assignment operator from that other type, which leads to asymmetries like mytype var = other; working but var = other; not working.

• (A)ssignment, A1, A2, A3: If you write a copy or move or converting constructor, but not a corresponding copy or move or converting assignment operator, the latter is generated.

Note: Generating inout this (assignment) from out this also generates converting assignment from converting construction, which is a new thing. Today in Cpp1, if you write a converting constructor from another type X, you may or may not write the corresponding assignment from X; in Cpp2 you will get that by default, and it sets the object to the same state as the converting constructor from X does.

Minimal reproducer (https://cpp2.godbolt.org/z/zzK9qc4Gx):

quantity: type = {
  value: int = ();
  operator=: (out this, val: int) = { value = val; }
}

main: () = {
  x := quantity(0);
  x = 1;
}

Commands:

cppfront -clean-cpp1 main.cpp2
clang++17 -std=c++2b -stdlib=libc++ -lc++abi -pedantic-errors -Wall -Wextra -Wconversion -I . main.cpp

Expected result:

An error at:

  x = 1;

Actual result and error:

Cpp2 lowered to Cpp1.

#include "cpp2util.h"

class quantity;
  
class quantity {
  private: int value {}; 
  public: explicit quantity(cpp2::in<int> val);
  public: auto operator=(cpp2::in<int> val) -> quantity& ;

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

auto main() -> int;
  

  quantity::quantity(cpp2::in<int> val)
                                      : value{ val }
  {}
  auto quantity::operator=(cpp2::in<int> val) -> quantity& {
                                      value = val;
                                      return *this;
  }

auto main() -> int{
  auto x {quantity(0)}; 
  x = 1;
}

Output:

Program returned: 0

[hsutter]

@realgdman

As a side note, I find that standalone {} suspicious, like it's failed generation of something operator= related.
I have ran grep '^\s*{\s*}' *.cpp on my cpp2 dir and see it it in several files.

Besides the function body cases, in my own code I regularly use {} to mean a default value. For example, for a function that returns a pointer, return {}; and return nullptr; are equivalent. For a function that returns a string, return {}; and return ""; are equivalent. For many standard types, it also happens work today for "default assignment" (e.g., my_vector = {};) to reset to essentially the default-constructed state, and so parallel "default construction" (e.g., vector<int> my_vector{};). Maybe those are what you're seeing?

[hsutter]

@JohelEGP - thanks! I think this is intended behavior though... given your example type:

quantity: type = {
  value: int = ();
  operator=: (out this, val: int) = { value = val; }
}

The first example is:

main: () = {
  x: quantity = 0; // explicit construction
  x = 1;           // explicit assignment

This is what I intended... I considered both lines to be equivalently "explicit" syntaxes. My view is that both are explicitly asking x to be set to 0 or 1. Note that with delayed initialization the above can be written equivalently like this:

  y: quantity;
  y = 0;          // explicit construction (first use)
  y = 1;          // explicit assignment
}

and with this version the = equivalence is even more direct.

My understanding of the danger of implicit conversions is when Cpp1 silently creates a temporary quantity object without any clue in the source that it's happening. In this case there are only explicit quantity objects being constructed/assigned.

Does that make sense?

[JohelEGP]

Yes!
It definitely makes much more sense when portrayed with Cpp2,
especially

  y: quantity;
  y = 0;          // explicit construction (first use)
  y = 1;          // explicit assignment

The use of the C++ standard term converting constructor in the wiki
made me think this should only apply to operator= generated from implicit this.

Now, how does one opt-out of this generation?
Types representing quantities don't want assignment from numbers, as that's type unsafe.
For example, the physical meaning of x = 1 changes silently under a refactor of x's type.

[hsutter]

Opt-out is via implicit.

But I think what you're asking is maybe what if the user writes y = Z(); where Z has a conversion to int and then the assignment to y works? But then the implicit conversion is the conversion from Z to int, so it's the responsibility of the author of Z to make that conversion explicit. (Which, if they're writing it in Cpp2, is the default...)

[JohelEGP]

That doesn't work: https://cpp2.godbolt.org/z/zTbj843v7.
I want the explicit assignment x = 1 to be an error.

[hsutter]

Oh, I see -- you want an opt-out of even generating the assignment?

Assuming you want to disable all mutation, not just assignment, one way is to make a const quantity object. Or to make the value member be const.

If you want to allow mutating operations, and allow construction from int but not allow assignment from int, this should work (but currently doesn't, that's just a bug I should fix):

quantity: type = {
  value: int = ();
  operator=: (out this, val: int) = { value = val; }
  private operator=: (inout this, val: int) = { value = val; } // opt out with private assignment (doesn't currently work)
}

There's a bug here because I should not generate the converting assignment when it's user-written, but this would be the way to do it.

But let me ask: Given x: quantity;, can you elaborate on why x = 0; x = 1; should be an error only on the second statement? And x = quantity(1); would still work? That is, I think you're asking for this:

x: quantity;
x = 0; // ok
x = 1; // I think you want this to be an error, even though...
x = quantity(1); // still ok

I'm trying to imagine a situation where that would be desirable... the reason why Cpp2 generates converting assignment from converting construction is that usually they should be symmetric and it's a surprise when Cpp1 class authors support construction from a different type but not assignment from that same type. Is there a use case you came across?

[JohelEGP]

But let me ask: Given x: quantity;, can you elaborate on why x = 0; x = 1; should be an error only on the second statement? And x = quantity(1); would still work? That is, I think you're asking for this:

x: quantity;
x = 0; // ok
x = 1; // I think you want this to be an error, even though...
x = quantity(1); // still ok

I'm trying to imagine a situation where that would be desirable... the reason why Cpp2 generates converting assignment from converting construction is that usually they should be symmetric and it's a surprise when Cpp1 class authors support construction from a different type but not assignment from that same type. Is there a use case you came across?

The use case is unit/quantity types.
x: metres = 1; defines x to be 1 m.
If x = 1; were allowed, that would set x to 1 m.
But if the type of x were to change to
x: millimetres = 1;.
Suddenly, any following x = 1 changes from setting x to 1 m to setting x to 1 mm.
So for added safety, unit libraries require the assignment from a quantity type and not a number:
x = metres(1); // OK, meaning doesn't change under a refactor of the type of `x`.

[hsutter]

Suddenly, any following x = 1 changes from setting x to 1 m to setting x to 1 mm.

Thanks. OK, so I see why you want to suppress assignment, and you'll soon be able to do that by writing a private converting assignment operator (that will work as soon as I fix the bug to not generate it if the user wrote it).

FWIW, this works nicely if you want the UDL-like suffix syntax, which in Cpp2 works for both literals and non-literals (below, an int object named forty_two)... the following already works now:

meters: @value type = {
  value: int = ();
  operator=: (out this, val: int) = { value = val; }
}

main: () = {
  forty_two := 42;

  x := 0.meters();
  x = 1.meters();
  x = forty_two.meters();
}

Note this way all the cases can use the same consistent syntax (though x: quantity = 0; would still work, I'm just saying it's possible to use a single consistent style).

I'll look at fixing the double generation bug so you can opt out and make x = 1; assignment an error... thanks for the feedback and helping me notice the bug.

[JohelEGP]

Thank you.
What about clarifying or removing the uses of "converting"
from the quotes of the wiki article in the opening comment?

[hsutter]

[Edited to add "default or converting"]

Ah, good point, I should address this phrase:

but cppfront currently generate converting assignment from an explicit constructor.

An explicit converting constructor is still a converting constructor, right? A converting constructor (and, now that we've generalized this with the other Issues in this area #375 #398 #450, any non-copy operator=) can be explicit (the default) or implicit.

I've added a note to the implicit bullet to try to clarify this:

• implicit: On a two-parameter operator= function, writing operator=: (implicit out this, /*...*/) defines a conversion function that can be used to perform implicit conversions to this type. Note that implicit only has an effect when the operator= function is used as a default or converting constructor (or is a conversion operator) -- the option governs whether this function can be used to implicitly generate a temporary object of this type, which is something only a constructor or conversion operator can do, an assignment operator does not.

Does that help?

[JohelEGP]

An explicit converting constructor is still a converting constructor, right?

If it's

the C++ standard term converting constructor

then only an explicit constructor can be a converting constructor.

Does that help?

Not really.
It's the use of "converting" that led me to believe
that generated assignments should be tied to non-explicit constructors only.
But you pointed out that's not actually the case.
So it turns out that whether an operator='s this parameter is implicit or not has no effect on emitted assignments.
So the use of "converting" in the wiki are misleading,
because what they state also apply to non-converting (explicit) constructors.

[hsutter]

Ah, I didn't notice that "converting constructor" was already a Cpp1 standard term of art. The Cpp1 term seems like it would be better named an "implicitly converting constructor." A Cpp1 explicit constructor still performs conversions, after all, just in a subset of contexts.

In contrast, the Cpp1 standard uses the term "conversion function" whether it's implicit or explicit, then qualifies the term as needed, e.g., to say things like "a conversion operator may be explicit".

So restricting "converting constructor" to mean only a non-explicit one seems like inconsistent terminology between converting constructors and conversion operators, as well as inconsistent in that explicit constructors also perform type conversions just in a subset of contexts.

My inclination is to keep using the term consistently as I've been doing in Cpp2, but if it's confusing I may need to come up with a different term just to avoid the confusion.

[JohelEGP]

I actually noticed that term when writing #468 (comment) 2 days ago.
But when I opened this issue,
for some reason,
I was convinced that all uses of "converting" in the wiki article
were referring to implicit conversions.
So yes, I think it's confusing to use "converting".

[JohelEGP]

Suddenly, any following x = 1 changes from setting x to 1 m to setting x to 1 mm.

Thanks. OK, so I see why you want to suppress assignment, and you'll soon be able to do that by writing a private converting assignment operator (that will work as soon as I fix the bug to not generate it if the user wrote it).

The opt-out by commit eac30d8 turns out to be rather noisy.

tiles: type = {
  public vertices: sf::VertexArray;

  operator=: (out this, size: i32) = vertices = (sf::PrimitiveType::Triangles, (size * tile_vertices) as u32);
  private operator=: (inout this, size: i32)
    [[pre Type: false]] // A "must-not-be-used" hint (`private` doesn't suffice).
  = {
    vertices = (); // Avoid Cpp2 error (_could_ be moved to the member's initializer).
    _        = size; // Avoid Cpp1 warning.
  }

And because the solution only looks at the parameter's type,
it could fail in more involved signatures (e.g., using requires clauses).

[JohelEGP]

An alternative could be to discard the signature:
_ = operator=: (inout this, size: i32);.

[hsutter]

Thanks! Can you elaborate please...

[[pre Type: false]] // A "must-not-be-used" hint (`private` doesn't suffice).

Why isn't private enough?

vertices = (); // Avoid Cpp2 error (could be moved to the member's initializer).

Yes, I'd put it in the initializer. Is there a reason it can't be?

_        = size; // Avoid Cpp1 warning.

I tried making this code warn using -Wunused-value but didn't succeed in making it fire. Was that warning you saw, or something else?

But assuming it was that: Perhaps it'd be useful to support giving a parameter the name _ and emitting [[maybe_unused]] in that case, which cppfront already does for local variables named _?

[JohelEGP]

Why isn't private enough?

To prevent it being accidentally invoked because it exists.

Yes, I'd put it in the initializer. Is there a reason it can't be?

None in this case.
But that might not always be possible, or scale.
Or it might seem out of place.
Because its reason for being is as a workaround to something far off.

_        = size; // Avoid Cpp1 warning.

I tried making this code warn using -Wunused-value but didn't succeed in making it fire. Was that warning you saw, or something else?

Waarudo.cpp2:219:45: warning: unused parameter 'size' [-Wunused-parameter]
  219 |   auto tiles::operator=(cpp2::in<cpp2::i32> size) -> tiles& 
      |                                             ^
1 warning generated.

But assuming it was that: Perhaps it'd be useful to support giving a parameter the name _ and emitting [[maybe_unused]] in that case, which cppfront already does for local variables named _?

See also #305 (comment), where by "discard a parameter's name" I meant not emitting it.
_ = E lowers to (void) E,
and _ : T = E to T auto_line_column = E;,
which doesn't use [[maybe_unused]].
And that's good, because otherwise you wouldn't get a warning when T doesn't have a non-trivial destructor.

[JohelEGP]

Why isn't private enough?

To prevent it being accidentally invoked because it exists.

Yes, I'd put it in the initializer. Is there a reason it can't be?

None in this case. But that might not always be possible, or scale. Or it might seem out of place. Because its reason for being is as a workaround to something far off.

Here's an example where it's not possible, and actually made me discover a bug in @basic_value: #453 (comment) (now #779).

[JohelEGP]

Here's an example where it's not possible, and actually made me discover a bug in @basic_value: #453 (comment) (now #779).

In that particular case, now I can just omit the initialization by assigning _ to the member: https://cpp2.godbolt.org/z/eGja1q5Mf.