Change mutator API to return an object we control #494

BackEndTea · 2018-09-27T16:17:20Z

This PR:

Adds the MutatorResult interface, which is returned by the mutate method.
Adds the GeneratorResult & SingleMutationResult classes, which are implementations of MutatorResult
Fixes type hints on mutate doc-blocks
Moves the Mutator class the the Infection namespace.
Deleted the InterfaceParentTrait and moved the method into the PublicVisibility class

This is the implementation of the idea mentioned in #450.

tests-checker

Thank you for contributing to Infection. We noticed you didn't add any tests. Could you please add them to make sure everything works as expected?

BackEndTea · 2018-09-27T16:18:58Z

src/Mutator/Util/Result/SingleMutationResult.php

+    /**
+     * @param Node|Node[] $node
+     */
+    public function __construct($node)


This doesn't have a typehint, as a 'single' mutation can also return an array of nodes to replace a node.

I could also split it up in an ArrayMutationResult or something like that.

I would do it. With this PR, we want to avoid mixed implicit typehint, so why to leave it here. Let it be Node and array|Node[] explicitly

BackEndTea · 2018-09-27T16:20:05Z

Thank you for contributing to Infection. We noticed you didn't add any tests. Could you please add them to make sure everything works as expected?

Weird, i did add some tests, and the bot didn't pick up on it.

sidz · 2018-09-27T17:10:09Z

src/Mutator/Util/Result/MutatorResult.php

+/**
+ * @internal
+ */
+interface MutatorResult


My 5 cents:
What about to call an Interface like MutatedNode and create the same class. Because MutatorResult is smth abstract and doesn't reflect what is it and what for.

I also agree that MutatorResult is too common name, but MutatedNode is also confusing when \Generator is used, even when Node[] is used (because it's not singular but plural, while MutatedNode is strictly singular)

What if we rename it in a way as it is a strategy, not the result of something:

interface NodeReplacer { public function replaceWithNodes(): \Generator; // or getReplacedNodes(): \Generator }

For a single Node

class SingleNodeReplacer implements NodeReplacer { public function __construct(Node $node) { ... } public function replaceWithNodes(): \Generator { yield $this->node; } }

for array of Nodes

class ArrayNodeReplacer implements NodeReplacer { public function __construct(Node[] $node) { ... } public function replaceWithNodes(): \Generator { yield $this->nodes; } }

for Generator

class GeneratorNodeReplacer implements NodeReplacer { public function __construct(\Generator $nodes) { ... } public function replaceWithNodes(): \Generator { return $this->nodes; } }

client code would be

... $nodeReplacer = $mutator->mutate($node); foreach ($nodeReplacer->replaceWithNodes() as $node) { ... } // or foreach ($nodeReplacer->getReplacedNodes() as $node) { ... }

Probably, the previous comment is too much.

Another ideas:

MutatedAst as an interface name (which implies singular/plural)

Or, if we continue PHP-Parser wording (see NodeTraverser), we can name it NodeReplacement:

abstract public function mutate(Node $node): NodeReplacement;

This one also implies singular and plural.

SingleNodeReplacement implements NodeReplacement

ArrayNodeReplacement implements NodeReplacement

...

maks-rafalko · 2018-09-27T20:20:34Z

Weird, i did add some tests, and the bot didn't pick up on it.

I will have a look. Seems like Github API is paginated, and the first page of so many updated files didn't have test file

maks-rafalko · 2018-09-27T20:56:10Z

src/Exception/InvalidMutatorException.php

@@ -9,7 +9,7 @@

 namespace Infection\Exception;

-use Infection\Mutator\Util\Mutator;


any reason behind it?

It seems like the logic behind this is that this is a major class/interface that deserves to be at the root of the similarly named namespace. Just like a Mutation.

Now I wonder if other classes inside Mutator\Util deserve a similar treatment. Maybe moving them all one directory level down would be a better bet? As other directories there correspond to different classes of mutators (say, @cast or @boolean), Util seems like a bit off. What do y'all think?

deserves to be at the root of the similarly named namespace

sounds reasonable

Maybe moving them all one directory level down would be a better bet?

I'm fine with it, the only one note here is to update these lines

infection/tests/Mutator/Util/MutatorProfileTest.php

Lines 42 to 43 in ef45b8e

->in('src/Mutator')

->exclude('Util')

probably, it's out of the scope of this PR

sanmai

This proposal left me baffled. Yeah, it's kind of cool to add these new contracts and interfaces, but other than that... Aren't we about to add more complexity to already very complex project for no particular reason? There must be some rationale for this change, for sanity's sake. What problem is this supposed to solve? What are cons and benefits?

For example, right now adding a mutator is a breeze. With this change that will be ever so slightly more difficult, because one would have to learn two or three more things more than they used to learn to just add a new mutator. Kind of bummer, don't you think?

OK, if you would let me articulate, the purpose of this change is to make things understandable from the first sight. Right now we have a kind of funny of a return declaration which mixes a generator with a node, @return \Generator|Node[], which is, well, not a bad thing if you remember what that means. A year later you won't necessarily so.

So it's either MutatorResult, or a paragraph with an explanation somewhere. The first is better because it's going to be literally everywhere, like jumping in your face. Unlike the other.

sanmai · 2018-10-02T23:45:16Z

src/Mutator/Arithmetic/RoundingFamily.php

+     */
+    public function mutate(Node $node): MutatorResult
+    {
+        return new GeneratorResult($this->getMutatedNodes($node));


Adding an extra function just for a generator is a bummer. This could be like:

return new GeneratorResult(static function () use ($node) { yield $stuff; });

And then GeneratorResult could handle calling this function as needed or even right away.

sanmai · 2018-10-02T23:46:57Z

src/Mutator/Boolean/EqualIdentical.php

    {
-        return new Node\Expr\BinaryOp\Identical($node->left, $node->right, $node->getAttributes());
+        return new Mutator\Util\Result\SingleMutationResult(new Node\Expr\BinaryOp\Identical($node->left, $node->right, $node->getAttributes()));


Long name here, short name in other places?..

sanmai · 2018-10-03T00:04:22Z

src/Mutator/Util/Result/MutatorResult.php

+    /**
+     * @return \Generator|Node[]
+     */
+    public function getResults(): \Generator;


Why not iterable?

sanmai · 2018-10-03T00:04:50Z

src/Mutator/Util/Result/SingleMutationResult.php

+    public function getResults(): \Generator
+    {
+        yield $this->node;
+    }


With an iterable typehint this could return a simple array.

maks-rafalko · 2018-10-03T05:37:15Z

because one would have to learn two or three more things more than they used to learn to just add a new mutator.

this is a price for avoiding mixed return type. As I said in the previous PR, seeing @return Node|Node[]|\Generator is like "what the hell the author was doing..". Though this is of course just my subjective opinion.

Also, I doubt that anyone "used to learn" anything about mutators. Mutators are closed at the moment, we don't expose public interfaces to write custom mutators, and 90%+ of mutators are written by core team. So for the majority of new developers this will be just new thing, no need to re-learn.

With this change that will be ever so slightly more difficult

We can create abstract classes to return that simplicity back again, example for the SingleMutator:

abstract class SingleMutator extends Mutator
{
    abstract protected function getMutatedNode(Node $node): Node;

    public function mutate(Node $node): NodeReplacement
    {
        return SingleNodeReplacement($this->getMutatedNode($node);
    }
}

And then adding new mutator will be easy:

final class Plus extends SingleMutator
{
    /**
     * Replaces "+" with "-"
     *
     * @param Node $node
     *
     * @return Node\Expr\BinaryOp\Minus
     */
    protected function getMutatedNode(Node $node): Node
    {
        return new Node\Expr\BinaryOp\Minus($node->left, $node->right, $node->getAttributes());
    }

    protected function mutatesNode(Node $node): bool
    {
        // ...
    }
}

We will end up with 3 new abstract classes with clear goals, and the main method mutate() will finally have a return type.

sanmai · 2018-10-03T05:43:43Z

seeing @return Node|Node[]|\Generator is like "what the hell the author was doing.."

Yeah, that's kind of funny return declaration, I'd have a similar reaction to it if I wouldn't know what means what. I see the point, that's going to make things easier to maintain in the long-medium term.

(Mutators are not closed at the moment in a sense there are folks who may be planning new mutators right about now, like you see with #500. Bringing simplicity back again will be most beneficial for them.)

sanmai · 2018-10-12T00:46:50Z

I wonder if we can move forward with this.

BackEndTea · 2018-10-22T13:36:50Z

@infection/core sorry about the delay on this PR, i've updated it to use 3 abstract mutator classes that can be used depending on what implementation is needed.

maks-rafalko

I like it, just one comment regarding visibility of the methods

maks-rafalko · 2018-10-22T19:41:31Z

src/Mutator/Arithmetic/Assignment.php

     */
-    public function mutate(Node $node): MutatorResult
+    public function getMutatedNode(Node $node): Node


This should be protected, not public.

abstract protected function getMutatedNode(Node $node): array; in the parent

Same for all classes.

sanmai · 2018-10-24T22:23:57Z

src/Mutator/Util/SingleMutator.php

+
+    final public function mutate(Node $node): NodeReplacement
+    {
+        return new SingleNodeReplacement($this->getMutatedNode($node));


NodeReplacement seems reductant to me.

If Mutator is defined as

public function mutate(Node $node): \Generator;

We can shortcut and remove all these result types, handling differences right inside mutator subclasses.

SingleMutator will be like:

abstract protected function getMutatedNode(Node $node): Node; final public function mutate(Node $node): \Generator { yield $this->getMutatedNode($node); }

SingleMutator and ArrayMutator can share a superclass.
If not, ArrayMutator will be like:

{ abstract protected function getMutatedNode(Node $node): array; final public function mutate(Node $node): \Generator { yield $this->getMutatedNode($node); } }

GeneratorMutator will be like

{ abstract protected function getMutatedNode(Node $node): iterable; final public function mutate(Node $node): \Generator { yield from $this->getMutatedNode($node); } }

iterable typehint gives us flexibility in a case where we would want to return a plain old array with mutations.

BackEndTea · 2019-03-08T09:51:31Z

After some reconsideration i've decided to abandon this PR, it doesn't seem worth it.

ghost assigned BackEndTea Sep 27, 2018

ghost added the Needs Review label Sep 27, 2018

tests-checker bot suggested changes Sep 27, 2018

View reviewed changes

BackEndTea commented Sep 27, 2018

View reviewed changes

BackEndTea force-pushed the fix/mutator-object branch from 64363b6 to 3965336 Compare September 27, 2018 16:24

sidz reviewed Sep 27, 2018

View reviewed changes

maks-rafalko reviewed Sep 27, 2018

View reviewed changes

sanmai reviewed Oct 3, 2018

View reviewed changes

sanmai changed the title ~~Change mutator API to return an object we controll~~ Change mutator API to return an object we control Oct 12, 2018

BackEndTea force-pushed the fix/mutator-object branch from 1a0ed00 to 060fbed Compare October 22, 2018 13:36

BackEndTea force-pushed the fix/mutator-object branch 2 times, most recently from 0abc88f to c7eb237 Compare October 22, 2018 13:41

maks-rafalko previously approved these changes Oct 22, 2018

View reviewed changes

BackEndTea dismissed maks-rafalko’s stale review via 1cefad4 October 24, 2018 15:39

maks-rafalko previously approved these changes Oct 24, 2018

View reviewed changes

BackEndTea dismissed maks-rafalko’s stale review via 3157333 October 24, 2018 15:57

BackEndTea force-pushed the fix/mutator-object branch from 1cefad4 to 3157333 Compare October 24, 2018 15:57

maks-rafalko previously approved these changes Oct 24, 2018

View reviewed changes

sanmai reviewed Oct 24, 2018

View reviewed changes

This was referenced Oct 31, 2018

Enhancement: Implement UnwrapArrayReplace mutator #536

Merged

Argument removal mutators #537

Open

BackEndTea dismissed maks-rafalko’s stale review via 366ffeb November 19, 2018 20:01

BackEndTea force-pushed the fix/mutator-object branch 2 times, most recently from 366ffeb to 0e40c40 Compare November 19, 2018 20:04

Change mutator API to return an object we controll

8f57f36

BackEndTea force-pushed the fix/mutator-object branch from 0e40c40 to 8f57f36 Compare November 19, 2018 20:05

BackEndTea closed this Mar 8, 2019

ghost removed the Needs Review label Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change mutator API to return an object we control #494

Change mutator API to return an object we control #494

BackEndTea commented Sep 27, 2018

tests-checker bot left a comment

BackEndTea Sep 27, 2018

maks-rafalko Sep 27, 2018

BackEndTea commented Sep 27, 2018

sidz Sep 27, 2018 •

edited

maks-rafalko Sep 27, 2018 •

edited

maks-rafalko Sep 28, 2018

maks-rafalko commented Sep 27, 2018

maks-rafalko Sep 27, 2018

sanmai Oct 12, 2018 •

edited

maks-rafalko Oct 13, 2018

sanmai left a comment •

edited

sanmai Oct 2, 2018 •

edited

sanmai Oct 2, 2018

sanmai Oct 3, 2018

sanmai Oct 3, 2018

maks-rafalko commented Oct 3, 2018

sanmai commented Oct 3, 2018 •

edited

sanmai commented Oct 12, 2018

BackEndTea commented Oct 22, 2018

maks-rafalko left a comment

maks-rafalko Oct 22, 2018

sanmai Oct 24, 2018 •

edited

BackEndTea commented Mar 8, 2019

		@@ -9,7 +9,7 @@

		namespace Infection\Exception;

		use Infection\Mutator\Util\Mutator;

Change mutator API to return an object we control #494

Change mutator API to return an object we control #494

Conversation

BackEndTea commented Sep 27, 2018

tests-checker bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BackEndTea commented Sep 27, 2018

sidz Sep 27, 2018 • edited

Choose a reason for hiding this comment

maks-rafalko Sep 27, 2018 • edited

Choose a reason for hiding this comment

For a single Node

for array of Nodes

for Generator

Choose a reason for hiding this comment

maks-rafalko commented Sep 27, 2018

Choose a reason for hiding this comment

sanmai Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanmai left a comment • edited

Choose a reason for hiding this comment

sanmai Oct 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maks-rafalko commented Oct 3, 2018

sanmai commented Oct 3, 2018 • edited

sanmai commented Oct 12, 2018

BackEndTea commented Oct 22, 2018

maks-rafalko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanmai Oct 24, 2018 • edited

Choose a reason for hiding this comment

BackEndTea commented Mar 8, 2019

sidz Sep 27, 2018 •

edited

maks-rafalko Sep 27, 2018 •

edited

sanmai Oct 12, 2018 •

edited

sanmai left a comment •

edited

sanmai Oct 2, 2018 •

edited

sanmai commented Oct 3, 2018 •

edited

sanmai Oct 24, 2018 •

edited