jkoudys / immutable.php

Immutable collections, with filter, map, join, sort, slice, and other methods. Well-suited for functional programming and memory-intensive applications. Runs especially fast in PHP7.
MIT License
346 stars 20 forks source link

Immutability and loss of information #13

Open jclaveau opened 5 years ago

jclaveau commented 5 years ago

Hi @jkoudys ,

Thanks a lot for sharing your super-accurate work. I have a question concerning Immutability. Presently, you make your collections immutable by calling return new static(SplFixedArray $fixedArray); at each end of mutator methods.

This works perfectly as long as the collection itself as no more property than the content of the $fixedArray parameter.

For example if I extend ImmArray this way :

class MyLovelyCollection extends ImmArray
{
    protected $feeling = 'loneliness';

    public function __construct($fixedArray, $feeling)
    {
        $this->feeling = $feeling;
        parent::__construct($fixedArray);
    }

    public function getFeeling()
    {
        return $this->feeling;
    }
}

And then call something like

$faces = new MyLovelyCollection([':)', ':/', ':D', 'XD'], 'happiness');

echo $faces->getFeeling() // => 'happiness'

$faces->filter(function($face) {
    return ! preg_match("#[^/]$#", $face);
});

echo $faces->getFeeling() // => 'loneliness'

Because of this, I cannot use together the quality of your iterable ImmArray (it has no point inside a composition) and some properties (mutable or not) that I would attach to. Simple things like composition becomes super hard due to it.

So, more pragmatically, my question is "Why don't do clone $collection; return $collection->doSomething(); instead of return new static(...);?".

Thanks in advance for your time, Jean

jkoudys commented 5 years ago

Thanks Jean! This library has definitely always targeted the composition-based approach; while we do support extension (hence the heavy return static approach you've noted), I did consider declaring the ImmArray as final and then providing some interfaces. I think at the time I'd hoped the php-fig would have some kind of focus on collections (e.g. class Foo implements Immutable instead of class Foo extends ImmArray). I should probably revisit this for a PHP7.2+ version that defines an interface where you'd expect mutating methods to fail.

Not all constructor calls create a new SplFixedArray. We DI this and only require it be a Traversable. A bunch of the methods (e.g. slice, concat) actually builds from an Iterator, which gives a huge advantage over wrapping many Array methods, and takes advantage of the immutability requirement. If you know the underlying SplFixedArray can't be changed, then you can chain $myStuff->slice(0, 5)->concat($otherStuff)->map(function ($v) { return $v * 2; }), and each step is doing almost nothing (only defining an iterator), which finally builds a new SplFixedArray at the end when you map.

I am thinking now if it's worth implementing the __clone magic method. I wrote __construct as private by design, because we only want to use the factory methods to create new instances. To me, I think __clone would make the most sense if it built a new instance with an SplFixedArray. e.g. right now $foo = $bar->slice(0, 5); keeps the SplFixedArray inside $bar in-scope, and that could be a very large collection. $foo = clone $bar->slice(0, 5); could make $foo have an sfa of count 5, and then $bar can fall out of scope and get GC'd.

So getting back to your example, what you've provided actually fails a bit earlier on the __construct, because parent::__construct is private. You're also missing an assignment, since $faces->filter will do nothing and needs to be $faces = $faces->filter. All that said, I think you're right that if we supported this via its __clone method instead, you could extend your own methods. I'm not too comfortable with relying on the clone if we can avoid it, and ImmArrays that have mutable properties don't make sense (since they're not immutable), and set-once properties at construction don't make sense either (since then it's not an array). Another project had an interesting discussion on the topic.

The ability to extend was really meant more for adding accessor methods, e.g. if I had Csvable that's meant to allow an ImmArray to export as CSV, I could say:

public function csvExport(): string {
  return $this
    ->map('someEscapingFunction')
    ->join(',');
}
jclaveau commented 5 years ago

Thanks a lot for your detailed answer.

My little example

I would first notice than my example wasn't meant to work perfectly but just to illustrate my purpose. Thus said I didn't realize your __construct() was private, which highlights your integration by composition.

Workflow issue

I understand your point even if extending seems to be a really natural way of integrating an external lib, add it some features temporary and, why not, finally merge it through a Pull Request. This is a super obvious workflow that I try to generalize in my work, especially to use the extended class as sandbox for my coworkers before I have the time to challenge deeply their modifications and take the time to explain why (or not) their work is satisfying our expectations.

Maybe I should search for simple compositions enabling to achieve the same goal but I'm pretty sure the workflow would be much less intuitive.

Built-in immutability

I totally agree that a builtin immutability would be appreciable. Thus said, I'm not really sure an interface would be the best representation of it. I would probably more vote for a dedicated keyword like "static" or "final", by class, property or method.

immutable class String
{
    protected $value;

    public function __construct($string)
    {
        $this->value = $string;
    }

    public function strToLower()
    {
        // here PHP would handle the clone by itself as there is a mutation
        $this->value = strtolower( $string );
        return $this;
    }

    // alternative for a partial Immutability 
    public immutable function strToLower()
    {
        // here PHP would handle the clone by itself as there is a mutation
        $this->value = strtolower( $string );
        return $this;
    }
}

After digging a little it looks like my wish is in discussion https://wiki.php.net/rfc/immutability

Workaround waiting for it

Meanwhile, instead of an interface, I experimented a way to ease the implementation of immutability by providing helpers gathered in a trait and the only way I found to don't break dependency (without Reflection) was by cloning: https://github.com/jclaveau/php-immutable-trait As you can see it's a little more tricky but I kind of like the hack with the bt :) .

So much questions

All this led me to a lot of questions about immutability the way I find it around:

Concerning your implementation

Not all constructor calls create a new SplFixedArray. We DI this and only require it be a Traversable. A bunch of the methods (e.g. slice, concat) actually builds from an Iterator, which gives a huge advantage over wrapping many Array methods, and takes advantage of the immutability requirement. If you know the underlying SplFixedArray can't be changed, then you can chain $myStuff->slice(0, 5)->concat($otherStuff)->map(function ($v) { return $v * 2; }), and each step is doing almost nothing (only defining an iterator), which finally builds a new SplFixedArray at the end when you map.

This is the key sentence for me :

If you know the underlying SplFixedArray can't be changed, then you can chain $myStuff->slice(0, 5)->concat($otherStuff)->map(function ($v) { return $v * 2; })

This answers my xmas list of questions above (but i'd love to have your opinion on some so I let them :) ). Here you require the immutability of SplFixedArray to iterate after the chain of actions is defined, step by step.

But does this point requires the ImmArray itself to be immutable or only to copy the last SplFixedArray everytime we use it?

Concerning your implementation 2

I am thinking now if it's worth implementing the clone magic method. I wrote construct as private by design, because we only want to use the factory methods to create new instances. To me, I think __clone would make the most sense if it built a new instance with an SplFixedArray. e.g. right now $foo = $bar->slice(0, 5); keeps the SplFixedArray inside $bar in-scope, and that could be a very large collection. $foo = clone $bar->slice(0, 5); could make $foo have an sfa of count 5, and then $bar can fall out of scope and get GC'd.

Here, I'm not sure I understood well what bothers you : do you wan't to avoid this copy of the SplFixedArray to save space or force this copy to let the previous SplFixedArray to the GC?

Something interesting waiting for your clarification

In your example you apply the clone after the slice $foo = clone $bar->slice(0, 5);. In my opinion, for true immutability, the clone must be done first (so the given instance wouldn't be changed by later calls).

If I start from the use of my trait to have the clone before the "mutator" call, I get something like:

    public function slice($begin = 0, $end = null)
    {
        if ($this->callOnCloneIfImmutable($result))
            return $result;

        // $this is now the cloned instance
        $this->sfa = new SliceIterator($this->sfa, $begin, $end);
        return $this;
    }

    public function __clone()
    {
        // I wonder if copying the sfa here is what you need or what you wouldn't.
        $this->sfa = clone $this->sfa;
    }

And the reflection would continue later if I don't take to much of your time :)

Thank you very much again for it! Jean

PS : meanwhile I visited your website ^^