phpDocumentor / fig-standards

Standards either proposed or approved by the Framework Interop Group
http://www.php-fig.org/
Other
360 stars 85 forks source link

Allow overloading with @method #102

Closed mindplay-dk closed 6 years ago

mindplay-dk commented 9 years ago

Currently @method is designated specifically as a means of documenting virtual methods implementing using __call().

Could we extend this to also allow for definitions of overloaded method signatures?

Example from a real-world project I'm working on:

/**
 * @method configure(Closure $func)
 */
class Container
{
    /**
     * @param string $name
     * @param Closure $func
     */
    public function configure($name, $func = null) {
        if ($name instanceof Closure) {
            $func = $name;
            $name = null;
        }
        // ...
    }
}

Here I'm trying to document an alternative method signature of configure(), but it doesn't work (in Storm) and likely because this isn't actually the intended use of @method and not what IDE authors (or anybody else) expected.

In this case, the configure() method has two overloads:

configure(string $name, Closure $func)
configure(Closure $func)`.

I know that, in this particular example, this could be solved easily by swapping the argument order and making $func default to null - but that's not always the case, and, the issue with closures is they tend to span multiple (possibly many) lines, and the $name argument can very easily get "lost" and hard to spot at the end of a long construction like that.

I'm also aware that I could just create two methods with two names - that's often a much better solution, and always the one I shoot for, but in this particular case, there really aren't any meaningful or practical names I could come up with to distinguish these two overloads.

So, bottom line, method overloading is very possible in PHP, by implementing run-time type-checks or counting func_get_args(), but there is currently no way to document them.

e1himself commented 9 years ago

Hi!

Here I raised another issue regarding overloading functions: https://github.com/phpDocumentor/fig-standards/issues/82

Though my suggested approach is different, issues are obviously related.

e1himself commented 9 years ago

Introducing @overload tag is more universal in a case of overloaded functions, not just methods.

mindplay-dk commented 9 years ago

Introducing @overload tag is more universal in a case of overloaded functions, not just methods

Very good point.

ashnazg commented 6 years ago

I'm not sure I'd want such indirection in the documentation, as it seems as though this level of magic can really only be understood when looking in the code itself.

Now, if we're going to do something for these use cases, I think I like overloading @method to do it (pun alert), since its true use case can be described as "document a method that's not an actual coded method".

Ping @ondrejmirtes @muglug @neuro159 @mindplay-dk @GaryJones @mvriel @jaapio for opinions.

muglug commented 6 years ago

I've also seen @method used to overwrite method return types like so (taken from Yii):

class BaseActiveRecord {
  /**
   * @param string $class
   * @param array $link
   * @return ActiveQueryInterface the relational query object.
   */
  public function hasMany($class, $link)
  {
    return new ActiveQuery();
  }
}
/**
 * @method ActiveQuery hasMany($class, array $link)
 */
class ActiveRecord extends BaseActiveRecord {}

Purely from the perspective of static analysis I think this is the wrong direction - if a class returns something specific, its docblock should say that. If we add conditional overrides then we can start to tie ourselves in knots.

jaapio commented 6 years ago

I understand that this is not what you want to do from that perspective. But fact is that people are already missing using the @method tag for this purpose. From a documentation point of view this isn't an issue.

So breaking with this case without an suitable solution is not an option from that perspective. I thought the psr-5 and psr-19 would only standardize the real world usage of phpdoc. So if this use is widely adopt already it should be in?

That said I fully agree that we should not try to overcome with a solution for missing language features. Php doesn't support overloading. So we should not add it to a standard.

neuro159 commented 6 years ago

PhpStorm definitely sees use of multiple @method with same name / different signature in real world.

Basically ppl "explain" their magic stuff to IDE to have proper completion for this.

Internally those tags basically the same as proper methods, and resolution order is: = try to find directly declared normal method = then try to find @method = delegate to all supers/traits.

This behaviour was tuned painstakingly by listening to users. So there is no "conditional" override - its jus as declaring method with super call with tuned types. Works great for frameworks.

muglug commented 6 years ago

So is this permitted by PhpStorm?

class A {
  public function foo(string $s) : string {
    return $s;
  }
}

/** @method stdClass foo(stdClass $s) */
class B extends A {}

(new B)->foo(new stdClass);

or is it only when there are not hard return/param types?

class A {
  /**
   * @param string $s
   * @return string
   */
  public function foo($s) {
    return $s;
  }
}

/** @method stdClass foo(stdClass $s) */
class B extends A {}

(new B)->foo(new stdClass);

The latter seems like a perfectly reasonable solution. The former, not so much.

e1himself commented 6 years ago

@muglug both of the examples above do work: PhpStorm takes the overriding into account.

screenshot from 2018-10-10 11-12-01

screenshot from 2018-10-10 11-13-55

muglug commented 6 years ago

OK, I'll change the behaviour of Psalm to match (and so it will give appropriate warnings about the first, clearly incorrect, case).

ashnazg commented 6 years ago

So the use case described here is to allow for one instance of a @method tag to override a current class method or a parent class method of the same name?

This is not trying to allow for multiple signatures of the same method name at the same class level, as displayed in #82... ?

e1himself commented 6 years ago

This is not trying to allow for multiple signatures of the same method name at the same class level

Correct. So it's kinda off-topic for this thread, which is about allowing overloading.

ashnazg commented 6 years ago

@mindplay-dk , if you wish to pursue this further, please bring this up as a new thread on the FIG mailing list for discussion -- https://groups.google.com/forum/#!forum/php-fig