New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Debug] Detect virtual methods using @method #28902

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ro0NL
Contributor

ro0NL commented Oct 17, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28897 (comment)
License MIT
Doc PR symfony/symfony-docs#10504

My first Debug PR, so im still on it. But early feedback welcome.

In #28901 we'll introduce a new virtual interface method using @method annotation. IIUC the idea is to trigger whenever such a method is overridden.

@stof

This comment has been minimized.

Member

stof commented Oct 17, 2018

No, the idea of @method on an interface is to trigger whenever a concrete class implementing the interface does not implement the method (directly or inherited from a parent class, we don't care).
The idea is to be able to add the method as an actual interface method in the next major.

@stof

This comment has been minimized.

Member

stof commented Oct 17, 2018

And only interface-level annotations should be considered IMO. Class-level @method annotation should not trigger deprecations when not being added in a child class, as such annotations are meant to document magic methods implement through __call

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 17, 2018

I've updated the regex for @method specific due newlines, and perhaps some future optimizing.

See https://regex101.com/r/Jw5R11/1 i think this is sufficient to move forward.

continue;
}
if (false !== \strpos($doc, $annotation) && preg_match_all('#\n \* @'.$annotation.'(?:( .+?)\.?)(?=\r?\n \*(?: @|/$|\r?\n))#s', $doc, $notice)) {

This comment has been minimized.

@stof

stof Oct 17, 2018

Member

you should look for @method here, not for method

@@ -223,7 +224,19 @@ public function checkAnnotations(\ReflectionClass $refl, $class)
// Detect annotations on the class
if (false !== $doc = $refl->getDocComment()) {
foreach (array('final', 'deprecated', 'internal') as $annotation) {
foreach (array('final', 'deprecated', 'internal', 'method') as $annotation) {
if ('method' === $annotation) {

This comment has been minimized.

@stof

stof Oct 17, 2018

Member

as the implementation for @method is entirely different, I would not put it in the foreach (array('final', 'deprecated', 'internal') as $annotation) {. Put a separate block of code before or after the loop with the different handling.

Show resolved Hide resolved src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php Outdated

@ro0NL ro0NL force-pushed the ro0NL:debug branch 2 times, most recently from 34087bb to 9fa8ca0 Oct 17, 2018

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 17, 2018

This is fun :) I think i got to the tricky part, so last commit needs review :) deprecations so far:

array(7) {
  [0] =>
  string(173) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "sameLineInterfaceMethod()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [1] =>
  string(181) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "sameLineInterfaceMethodNoBraces()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [2] =>
  string(172) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "newLineInterfaceMethod()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [3] =>
  string(180) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "newLineInterfaceMethodNoBraces()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [4] =>
  string(172) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "invalidInterfaceMethod()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [5] =>
  string(180) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtualParent" should implement "invalidInterfaceMethodNoBraces()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualInterface""
  [6] =>
  string(165) ""Test\Symfony\Component\Debug\Tests\ExtendsVirtual" should implement "subInterfaceMethod()" per contract "Symfony\Component\Debug\Tests\Fixtures\VirtualSubInterface""
}
@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 17, 2018

The issue is with sameLineInterfaceMethodNoBraces, it triggers for ExtendsVirtualParent but is declared on ExtendsVirtual. I think we should skip if the parent is abstract/not-final so the current behavior is actually expected. Correct?

@ro0NL ro0NL force-pushed the ro0NL:debug branch from 9fa8ca0 to 79845ba Oct 17, 2018

@stof

This comment has been minimized.

Member

stof commented Oct 17, 2018

abstract classes should be excluded from this check, as it is fine for them to provide incomplete implementations of an interface (a child class extending them would complain about the missing method).
not-final is irrelevant here. If you implement the interface in a concrete class, we must warn if you don't implement the extra method, otherwise adding the method in the interface would break the code.

@ro0NL ro0NL force-pushed the ro0NL:debug branch from 3d03b6e to 1334ad6 Oct 17, 2018

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 17, 2018

abstract classes should be excluded from this check, as it is fine for them to provide incomplete implementations of an interface

I think it works correct now 🎉

@@ -249,6 +267,9 @@ public function checkAnnotations(\ReflectionClass $refl, $class)
if (!isset(self::$checkedClasses[$use])) {
$this->checkClass($use);
}
if ($refl->isAbstract() && isset(self::$method[$use])) {
self::$method[$refl->name][] = $use;

This comment has been minimized.

@ro0NL

ro0NL Oct 17, 2018

Contributor

so self::$method stores @methods per interface, or interfaces per abstract, and is resolved below as such. This way we preserve the original interface name. At least that's how i intended it :)

@@ -249,6 +267,9 @@ public function checkAnnotations(\ReflectionClass $refl, $class)
if (!isset(self::$checkedClasses[$use])) {
$this->checkClass($use);
}
if ($refl->isAbstract() && isset(self::$method[$use])) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 17, 2018

Member

operands should be swapped for performance

@ro0NL ro0NL force-pushed the ro0NL:debug branch from 061aa97 to 1a6585f Oct 17, 2018

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 17, 2018

https://github.com/nrk/predis/blob/v1.1/src/ClientInterface.php#L27 makes test fail, something wrong with description parsing but more important, it made me discover we should skip the deprecration if __call is declared :)

edit: ready!

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 18, 2018

From phpDocumentor/phpDocumentor2#1855 i discovered we might be able to support static virtual methods as well.

Do we want to include it in this PR? Support it even?

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 18, 2018

Turns out it's supported as of phpdoc3 (https://github.com/phpDocumentor/phpDocumentor2/blob/v3.0.0-alpha1/docs/references/phpdoc/tags/method.rst)

I haven tested phpstorm yet, but if it works we might want to have a future proof implementation. IF virtual statics are worth it :)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 18, 2018

It's a few keystrokes away, isn't it? Let's do it :)

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 19, 2018

fabpot added a commit that referenced this pull request Oct 19, 2018

minor #28915 [Config] Fix @method annotation (ro0NL)
This PR was squashed before being merged into the 4.1 branch (closes #28915).

Discussion
----------

[Config] Fix @method annotation

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

```
@method [[static] return type] [name]([[type] [parameter]<, ...>]) [<description>]
```

I dont think this was intended to be the method description, which will be taken into account after #28902

Let me know if there's a better description, other then `Gets the child node definitions` :)

Commits
-------

0d7a961 [Config] Fix @method annotation

symfony-splitter pushed a commit to symfony/form that referenced this pull request Oct 19, 2018

minor #28916 [Form] Fix @method annotation (ro0NL)
This PR was squashed before being merged into the 4.2-dev branch (closes #28916).

Discussion
----------

[Form] Fix @method annotation

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Same as #28915 for 4.2 (these are the only occurrences for interfaces)

Actually provides a use case for symfony/symfony#28902 (comment) 🎉

Commits
-------

13f0db718e [Form] Fix @method annotation

fabpot added a commit that referenced this pull request Oct 19, 2018

minor #28916 [Form] Fix @method annotation (ro0NL)
This PR was squashed before being merged into the 4.2-dev branch (closes #28916).

Discussion
----------

[Form] Fix @method annotation

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Same as #28915 for 4.2 (these are the only occurrences for interfaces)

Actually provides a use case for #28902 (comment) 🎉

Commits
-------

13f0db7 [Form] Fix @method annotation

lyrixx added a commit to lyrixx/symfony that referenced this pull request Oct 19, 2018

minor symfony#28915 [Config] Fix @method annotation (ro0NL)
This PR was squashed before being merged into the 4.1 branch (closes symfony#28915).

Discussion
----------

[Config] Fix @method annotation

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

```
@method [[static] return type] [name]([[type] [parameter]<, ...>]) [<description>]
```

I dont think this was intended to be the method description, which will be taken into account after symfony#28902

Let me know if there's a better description, other then `Gets the child node definitions` :)

Commits
-------

0d7a961 [Config] Fix @method annotation

@ro0NL ro0NL force-pushed the ro0NL:debug branch from 857dcc7 to 704d87f Oct 20, 2018

@ro0NL ro0NL force-pushed the ro0NL:debug branch from 704d87f to 0166299 Oct 20, 2018

@ro0NL ro0NL force-pushed the ro0NL:debug branch from 0166299 to aee2a2b Oct 20, 2018

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 20, 2018

Ready :)

Status: needs review

ro0NL added some commits Oct 20, 2018

@ro0NL ro0NL force-pushed the ro0NL:debug branch from 0f4f906 to 61d8708 Oct 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment