Perl-Apollo / Corinna

Corinna - Bring Modern OO to the Core of Perl
Artistic License 2.0
157 stars 19 forks source link

UNIVERSAL::Cor Feedback #8

Closed Ovid closed 2 years ago

Ovid commented 4 years ago

Use this issue to provide feedback on the UNIVERSAL::Cor Proposal.

HaraldJoerg commented 4 years ago

I have my doubts about the value of a generic clone method. Usually, I would expect that values in slots from the original object would be copied to the clone, regardless of whether they are available as constructor arguments (therefore available for update) or not. But that does not always give correct results. Let's start with the following example, a combination from the UNIVERSAL::Cor and Constructor pages:

class Box {
    has ($height, $width, $depth) :reader :new;
    has $volume :reader = $width * $height * $depth;
}

my $original_box = Box->new(height=>1, width=>2, depth=>3);
my $updated_box  = $original_box->clone(depth=>9);  # h=1, w=2, d=9

It would be wrong if the value of the $volume slot was copied from $original_box->volume, or if the value of $updated_box->volume would depend on whether the lazy calculation of $original_box->volume had already been processed before. I also doubt that it is always possible to "just re-calculate" slot values during cloning.

With clonebeing part of UNIVERSAL::Cor, every module author needs to test / override / document this method. I suggest making it a separate role (or even separate roles for shallow and deep cloning, respectively) instead, so that module authors can decide whether and how they want to support cloning.

Abigail commented 4 years ago

@HaraldJoerg Yes. And if you don't use a role, you may use an attribute to indicate which slots can be copied in a clone, and which should not. You probably don't want to clone your database handles, and your file handles, you probably only want to "clone" them by after "dupping" on system level. A blanket copy everything when cloning is handy in a lot of cases, but it's a real hassle when it shouldn't.

Also, shallow copying? Deep copying? If the latter, what if a slot contains an object which isn't a Cor object, how to copy that?

Ovid commented 4 years ago

@HaraldJoerg @Abigail Point taken about the clone method. I'm definitely less clear on it now. It's fair to say that this might have been simpler had we not had a mix of OO systems, but even then the edge cases being pointed out would be an issue.

haarg commented 4 years ago

Changing the signature of existing methods seems like a bad idea. Especially having the can method return "true" since it currently returns subrefs.

This seems to be proving a lot of unnecessary methods, including introducing new mechanisms that already have established ways to do the same thing. Overriding to_string is obviously easier than using overload, but is it really used often enough that a new mechanism needs to be invented? While obviously this is meant to be an improvement on what perl currently provides, unnecessary differences will add confusion. Aside from new, meta and the upper case methods, these don't seem fundamental to the system, and I'd probably prefer they weren't included.

clone could provide a function that is more fundamental, but it's also the hardest to get right, as indicated by the other comments. In addition, if roles are not allowed to have method modifiers, how would they customize the cloning of their attributes?

Ovid commented 4 years ago

Changing the signature of existing methods seems like a bad idea. Especially having the can method return "true" since it currently returns subrefs.

Agreed. Needs to be fixed.

This seems to be proving a lot of unnecessary methods, including introducing new mechanisms that already have established ways to do the same thing.

Agreed. Will rethink that.

Overriding to_string is obviously easier than using overload, but is it really used often enough that a new mechanism needs to be invented?

This one's important. String overloading in Perl isn't done often because it's not-intuitive for most. Java's very handy toString method is great. I kinda like Raku's gist.

clone could provide a function that is more fundamental, but it's also the hardest to get right, as indicated by the other comments. In addition, if roles are not allowed to have method modifiers, how would they customize the cloning of their attributes?

The intent behind clone is to provide a starting point. When we're using "dumb classes" (e.g., "structs with methods"), the clone is trivial. Otherwise, yes, it gets thorny.

That being said, yes, pushing cloning to a role is probably better.

haarg commented 4 years ago

This one's important. String overloading in Perl isn't done often because it's not-intuitive for most. Java's very handy toString method is great. I kinda like Raku's gist.

I strongly disagree. I have rarely wanted string overloading, and more often wished it hadn't been used.

haarg commented 4 years ago

The intent behind clone is to provide a starting point. When we're using "dumb classes" (e.g., "structs with methods"), the clone is trivial. Otherwise, yes, it gets thorny.

That being said, yes, pushing cloning to a role is probably better.

I think providing a default method that handles the simple case, but can't be extended to cover the complex ones (such as roles) is worse than just not providing the method. It definitely seems like a worthwhile function, but given the complexities, it might be better to push it off to Cor v2 or such.

b2gills commented 4 years ago

I don't see why clone is seen as that controversial.

It's in Raku, and I don't recall ever seeing a single mention of a possible problem.

Sure it can be a problem:

'2000-02-29'.Date.clone( year => 2001 )

But then you just write your own clone that does something like around to check the arguments.
(Or in this specific case Date.clone() just calls new with the combination of old and new values.)

ERROR: Day out of range. Is: 29, should be in 1..28

(Perhaps the biggest problem to it being public by default I can think of is the lack of a strong type system.)


What makes it even less of a problem is that clone is almost never used outside of the class itself in practice.
It is often instead used by other methods in the class. Ones which know more about the class.

'2000-02-29'.Date.later( :1year )
# 2001-02-28

'2000-02-29'.Date.truncated-to( 'month' )
# 2000-02-01

So if it is a problem, just add the idea for a private method and make clone one of those. (Or a similar idea.) That way if someone wanted it public they could just delegate to the existing private one. Or if they want to create a method which returns a new instance which is mostly the same they can use the built-in one.


There is a really good reason to have it built-in, it uses meta knowledge of the class to do the cloning. That is it knows what attributes exist and need to be copied without it having to be hard-coded.

It also would be heavily tested so people don't end up creating new versions with new bugs. (There have been cases where clone was created from scratch for speed; which accidently “forgot” certain attributes.)

clone is one of the things that vastly reduces the annoyance with using immutable objects. It doesn't make sense to me to have immutable by default, but not include something like clone.

Ovid commented 4 years ago

@b2gills clone isn't controversial in Raku, in part, because it doesn't have to worry about the gap between the new style and the old style.

As has been pointed out, there are a number of unanswered questions, not the least of which is that holding a reference to a non-Cor object means we don't have a standard way to clone.

Also, you wrote:

It doesn't make sense to me to have immutable by default, but not include something like clone.

I agree. Working with immutable objects means being able to spawn new ones and with new state.

That being said, I'm unsure that the issue is as serious as people think.

  1. There's going to be a problem withe Cor/non-Cor divide. I can't fix that.
  2. @HaraldJoerg's concern about invalid data being copied might be mitigated by stating that only slots passed to the constructor are copied: slots without a :new attribute would be derived (but then, we're spawning new copies and not cloning?)
  3. @Abigail's concerns about cloning things like file and database handles is certainly an issue I've been thinking about
  4. @haarg's concern about "roles without method modifiers" is largely mitigated by me finally admitting that we're probably going to need modifiers in roles

Actually, I'll need to rethink point 2. I may be too tightly coupling cloning with construction.

duncand commented 3 years ago

Regarding the object_id() it is important to distinguish 2 concepts. One concept is that every object instance has a guaranteed unique value, and this is essentially the same as refaddr(). The other concept, which only applies to so-called "value types" whose identity is their value, it would produce the same value for 2 objects considered to be the "same value" and these tend to be immutable objects. The former concept has to be system-defined and not user-overridable; the second concept needs to be user-overridable and if not defined would default to the same value as the former.

These methods are important for doing things like having a collection of objects and being able to distinguish if a particular object is already in the collection or not.

We would also want predicate methods for comparing 2 objects where one is system-defined and not overridable and returns true if they are the exact same object instance, the second is user-overridable and returns true if they are conceptually the same value.

duncand commented 3 years ago

I believe that a generic clone() method is useless or counter-productive in practice and is a code smell. For most objects there is no value in cloning them, either because they should instead be immutable or because they are conceptually one of a kind entities. Where cloning has a business case, that should be entirely user-defined case by case for the class.

b2gills commented 3 years ago

@duncand You are completely missing the point of clone, and that is that it can take arguments.
That is you want a copy of the object, except with some small changes.

Even if it weren't available outside of the class it can still be very useful as a tool to implement methods

For example truncated-to could use clone to implement itself.

DateTime.now.truncated-to( 'hours' )

DateTime.now.clone( minute => 0, second => 0 )

At the very least it means that you don't have to remember all of the attributes as they are included automatically.


The main reason people create mutating classes is that it is often easier than creating a new one that is only slightly different.

If your character takes a hit it is much easier to decrement an attribute.

class Character {
  has $.hitpoints = 100;
  has $.name;

  method hit () {
    $!hitpoints--
  }
}
…
$a.hit();

With clone it makes it just as easy to not have a mutating object.

class Character {
  has $.hitpoints = 100;
  has $.name;

  method hit () {
    self.clone( hitpoints => $!hitpoints - 1 )
  }
}
…
$a .= hit();
duncand commented 3 years ago

@b2gills So are you saying that clone() is semantically the same as new() except that it can provide a subset of constructor arguments for you from an existing object of the same kind, and thus it would work for a fully immutable object that is only defined at creation?

In that case I agree it would be useful, but practically speaking to keep things simple and predictable a generated clone() would only really work for classes where all of their properties are directly settable with constructor arguments and there aren't extra properties that are set by other means.

In the general case a class would need manually written clone logic so it does the right thing when there is an interaction between different slots or any slots aren't directly settable by constructor arguments.

b2gills commented 3 years ago

In Raku clone actually copies all of the contents (vm opcode) and then directly changes some of them. Though some of that is just implementation detail.

For Cor it may make more sense to have it hook into the construction mechanism.

It may even make sense for it to just be a [private] subroutine that is only available inside of a class, and only can operate on said class.

duncand commented 3 years ago

I highly recommend adding a generic multi_assign() operator that works the same as clone() except it modifies the current object rather than making a new one. The auto-generated :write mutator methods should NOT return any value, and anyone wanting to chain calls by suggesting the innocent is returned from a mutator, they should instead use the new multi_assign() generic method for closest fit, or use constructor arguments when it is known at the time.

duncand commented 3 years ago

I highly recommend that Corinna does NOT implement any standard Perl overloading operators on objects by default in any way to reflect something meaningful. Instead, the default behavior of someone calling those on a Corinna object should be similar to how it works for blessed references now, such as returning a memory address of sorts and/or class name etc. This is how a default to-string should be.

Ovid commented 2 years ago

Closing old tickets because the MVP has been accepted. We'll start fresh with real code.