[Question] type hints for generated methods

[kwreczycki]

What do you think about type hints for generated methods ?
I mean if i've in specification, for example construct behaviour:

->beConstructedWith(Foo $foo, $bar)

generated __construct method should look like that:

use Foo;
...
    public function __construct(Foo $foo, $bar);
...

this solution should avoid some manual work...

[ciaranmcnulty]

It sounds like a good idea. Whent the parameter has multiple types, how would you choose which one to hint?

[mdavis1982]

@ciaranmcnulty I'm not sure that it should ever be ambiguous what the type hint is... We should promote type hinting on an interface and not a concrete implementation for objects. Can you give an example where it might be ambiguous what to add?

The only problem I can see with this is where the beConstructedWith specifies a concrete implementation and the type hint should be an interface. How would that be handled?

[kwreczycki]

I agree with mdavis1982, that we should promote type for interface.
@mdavis1982 Yes, this can be tricky to handle and can introduce some misunderstanding... - IMO.

[ciaranmcnulty]

I was thinking of something like

$this->foo($foo) where $foo is of class Fooand implements BarInterface and BazInterface - we would have to decide which of those three types should be hinted.

However in a spec $foo will always be a double, right? ;-) So we can safely assume it's whatever type the Prophecy double was generated from

e.g.

function it_does_foo(FooInterface $foo) {
    $this->foo($foo);
}

then the generated method can safely be:

function foo(FooInterface $argument1) { ... }

So I am in favour of this feature ;-)

[docteurklein]

Good idea indeed.
However, there is nothing wrong on relying on concrete class typehint. Classes sometimes simply implement their one and only goal, and don't implement any interface.
If you start creating interfaces for the sake of it, then you're doing it wrong. (I mean that a 1 to 1 relation between class and interface is wrong).

Simply let the user choose his type hint, and generate the class accordingly.

[mdavis1982]

@ciaranmcnulty Is there a situation where you would have a double of a concrete class that implements the FooInterface in your spec, rather than having a double of the actual interface?

[ciaranmcnulty]

@mdavis1982 You could but I think mocking the interface is probably a better approach.

[ciaranmcnulty]

@docteurklein I think hinting based on the type used is fine. People who hint without doubles of interfaces will get an interface hint. Prompting against all classes/interfaces the item expresses would add overhead.

[mdavis1982]

@ciaranmcnulty I agree it's probably a better approach - I'm just concerned that trying to help people could result in some things being type hinted incorrectly and people not being aware.

I am, however, in favour of the feature as long as it does the right thing!

[ciaranmcnulty]

@mdavis1982 I agree it will encourage concrete type hinting if people are not already hinting interfaces, but they can always edit the hint ;)

[everzet]

You have a mock of

class ArrayToken implements Token, ArrayAccess, Countable
{
}

What will you typehint? ArrayToken, Token, ArrayAccess or Countable? Defining message signature is a design process - same one as defining message name. And as any other design process, it shouldn't be automated or simplified - you want your brain working there.

[ciaranmcnulty]

@everzet That's not the right way of thinking about it, to me.

function it_does_something(ArrayToken $token)
{
    $this->foo($token)
}

The author here has clearly decided that this function takes an ArrayToken, so it would be appropriate to use ArrayToken as the generated type hint for $token rather than one of the other interfaces it implements

Similarly if the author of the spec had chosen to double Token, that would be a good type hint.

Does this make sense?

[ciaranmcnulty]

I started playing with this, the rules I would propose are:

1. If it's a concrete class, use get_class to work out the type hint
2. If it's a prophecy double of a class, work out what class that was and use as type hint
3. If it's a prophecy double of an interface, work out which interface the double was of and type hint using it

disambiguating 2+3 are giving me trouble.

[docteurklein]

Yes it makes sense. just use the type hint that is used in the spec.

[everzet]

function it_does_something_with_array_token(ArrayToken $token)
{
    $this->foo($token)
}

function it_does_something_else_with_countable_token(CountableToken $token)
{
    $this->foo($token)
}

class YourServiceDude
{
    public function foo(Token $token)
    {
    }
}

?

[ciaranmcnulty]

@everzet We only generate the method on the first instance of its use.

Same issues arise with numbers of parameters etc.

[everzet]

@ciaranmcnulty yes. And as my example shows, it will illogically (for me, as a user) will cause second example to break into a fatal error.

It's not "hey, you made a decision and now it doesn't work anymore - change it" case, it is a "tool silently made a decision for you, now deal with its mistake".

[ciaranmcnulty]

A fatal error about the type hint being wrong will make the author realise that their assumption when they made the first example pass (that they should double ArrayToken) was wrong.

This will make them think more about what they were doubling in the first example and maybe they will realise that they actually wanted to double Token (for instance) rather than ArrayToken, making them improve the readability the specs.

Unless you meant you'd write those two examples without running them in-between, breaking the rules of TDD ( I tease... )

[everzet]

@ciaranmcnulty this particular case (in comparison with number of arguments) is not about my assumptions anymore, it's about tool implicit assumptions that it tried to (mistakenly) digest by observing me :)

For example (it's a keyword here) the fact that I'm talking with you right now doesn't mean that you're the only person I'll talk with for the rest of my life. If you'd end up trying (falsely) generalising everything I do (oh, he bought beer from this pub today, so he obviously can't buy beer from other pubs) - it'd piss me of, not help me be more effective ;)

Abstraction and generalisation are design activities conducted by brain, not program - programs can help you identify wrong generalisation, but they are extremely bad at generalising things in the first place.

[ciaranmcnulty]

Incidentally, I don't believe hinting on Interface is going to be possible without a change to Prophecy (there isn't a way to ask a double which of its many interfaces was the one it was generated from)

[ciaranmcnulty]

@everzet We don't need to always 100% get the right type hint that the author wanted us to auto generate. The question is whether we can 80% of the time get it right, rather than leaving it blank.

[everzet]

@ciaranmcnulty there's no such thing as a "right" or "wrong". It is a design activity and should be conducted by human brain. So any time you get into those 20% of cases you should go "ahh, I was wrong" instead of "why the hell this tool did that".

It's the same thing as with standards. You can automate or standardise everything (especially if you're not caring about 100% of cases). But you'll end up with Interface suffixes, just so people don't need to think about naming anymore.

[ciaranmcnulty]

Doesn't the same argument apply to number of parameters?

PhpSpec tries to do code generation, pulling back from doing it at this stage seems a weird policy decision

[everzet]

@ciaranmcnulty no, it doesn't. You explicitly provide arguments to the method. You manually call method with 3 arguments inside example. You're in full control of the call. So whenever you add another argument in different example, you need to deal with your previous decision.

In case of typehints - you're not explicitly stating the type of object method expects. You're stating that currently, in this example, you're working with this instance. But the fact that you're using specific object doesn't mean that you're not having more generalised version of it in your head.

[ciaranmcnulty]

@everzet I can see the argument there, for not putting it in core.

Because this is something I want for my own style of writing specs, I've knocked together an extension here: https://github.com/ciaranmcnulty/phpspec-typehintedmethods

[docteurklein]

the only thing that could potentitally work would be to aggregate all examples, find the most common denonimator (interface/base class) for a given argument and type hint this one. If two different examples don't share a common denominator, don't type hint.

This stuff obviously won't work if you generated code with only one example.

[docteurklein]

and even then, you can't be sure the guy has no other plan for his argument.

[ciaranmcnulty]

@docteurklein Finding the common superclass won't do it - you'd maybe look at the superset of which methods are stubbed/mocked across all examples and work out the most abstract type that fulfils them -far beyond phpspec's scope.

In my extension I base it on which class was doubled - this relies on the author doubling the most abstract type at the point of writing the example, so it probably isn't suitable for core if people write examples the way @everzet describes above.

I discussed with @MarcelloDuarte today and he also thinks it's not a core feature so I'm going to close this issue - PRs to the extension from those whose style it fits are welcomed.

[MarcelloDuarte]

One of PhpSpec goals is to drive good design. With this feature in core we could end up with encouraging people to leave the type hint to the implementation class instead of the abstract type. Thanks, Ciaran.

[stof]

@docteurklein I moved the discussion to the issue tracker of the extension as you have a valid argument there. Feel free to add more details