fully_qualified_strict_types breaks @see tag

[kenjis]

Bug report

Description

fully_qualified_strict_types breaks @see tag by AddSeeTestAnnotationRector
https://github.com/rectorphp/rector-phpunit/blob/main/docs/rector_rules_overview.md#addseetestannotationrector

- * @see \CodeIgniter\Pager\PagerRendererTest
+ * @see PagerRendererTest

See also https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc-tags.md#513-see

Runtime version

PHP CS Fixer 3.42.0 Three Keys by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.2.13

Used command

php-cs-fixer fix --ansi --verbose --diff

Configuration file

Code snippet that reproduces the problem

[mvorisek]

I was just about to report this, @Wirone let's revert #5620, the fixing must be done on TypeExpression parsed phpdoc, see #7619 issue for it (which should be then converted to feature request probably).

In our case, the fully_qualified_strict_types fixer is also unexpectedly fixing whitespace:

 /**
- *  @see      http://editor.datatables.net
+ *  @see http://editor.datatables.net
  */
 define('DATATABLES', true);

[Wirone]

What's actually breaking here? If the fixer shortened FQCN it means it was already imported in your class, so it should work anyway. Is navigating in the IDE not working?

[mvorisek]

see my updated post above, the fixer does now crazy things as applied on \S+ regex instead of properly parsed phpdoc which can be invalid, contain actually spaces (callable(): T, (A | B), ...).

[kenjis]

@see [URI | "FQSEN"] [<description>]

https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc-tags.md#513-see

PagerRendererTest is not a FQSEN. \CodeIgniter\Pager\PagerRendererTest is a FQSEN.

[mvorisek]

also see is often like \A\B->name() which is not fixed now, so inconsistency is introduced

[kenjis]

What's actually breaking here?

The Rector rule AddSeeTestAnnotationRector intentionally uses FQCN.
php-cs-fixer breaks the @see tag.
And the Rector rule adds another @see tag like:

1) system/Commands/Database/MigrateStatus.php:18

    ---------- begin diff ----------
@@ @@
  * Displays a list of all migrations and whether they've been run or not.
  *
  * @see MigrateStatusTest
+ * @see \CodeIgniter\Commands\Database\MigrateStatusTest
  */
 class MigrateStatus extends BaseCommand
 {
    ----------- end diff -----------

Applied rules:
 * AddSeeTestAnnotationRector

[Wirone]

First of all, phpDoc is modified only if the short symbol is matched, so it shouldn't touch @see with URL 🤔. Secondly, instead of reverting this we just can introduce the config option and disable it by default, which should fix @GrahamCampbell's concerns.

Anyway, I won't be able to work on it today, family time for me.

[Wirone]

@kenjis thanks for the info. IMHO if short version in the annotation works as expected then maybe it's a bug in Rector, which should not add another @see tag 🤷‍♂️? That's why I asked if navigation works after shortening.

[mvorisek]

First of all, phpDoc is modified only if the short symbol is matched, so it shouldn't touch @see with URL 🤔.

here is the full source: https://github.com/DataTables/Editor-PHP/blob/4447b5996615bf25e0fc189cde3449fc578c755e/DataTables.php#L11

fixer output:

SSSSSSSSSSSSSSSSSSSSFSSSSSSSSSSSS                                 33 / 33 (100%)
Legend: .-no changes, F-fixed, S-skipped (cached or empty file), I-invalid file syntax (file ignored), E-error
   1) ...\datatables-Editor-PHP\DataTables.php (fully_qualified_strict_types)

Fixed 1 of 33 files in 0.047 seconds, 6.000 MB memory used

it seems like the fixer tries to trim the phpdoc even if otherwise unchanged, also, it seems it does so only in this one file, and not in others with the same @see phpdocs... strange.

[Wirone]

In #7622 I've provided some basic improvements for how FQCNs are handled in phpDoc, so it won't touch non-FQCNs anymore and it won't change whitespace between the tag and the value. These are clear bugs that should be fixed. Config option for enabling/disabling phpDoc support is not that urgent IMHO, I will try to provide it but later.

[kenjis]

@Wirone Thank you for your comments.

In terms of class names, the short name is also correct. However, that Rector rule uses FQCN because it does not import (use) the class, i.e., it does not change the source code at all.
php-cs-fixer ignores that intent and imports the class.

[Wirone]

@kenjis Rector also have an option for importing names. On the other hand, Fixer imports symbols only if the config option is enabled - did you do it? Because it wasn't included in any rule set yet.

[kenjis]

We are using $rectorConfig->importNames();.

[Wirone]

@kenjis so it's Rector who imports names, and then Fixer (which you probably run after Rector, as suggested) only shortens already imported names 😉.

[kenjis]

My explanation was not correct.
Rector does not import. php-cs-fixer does not import, but it just converts FQCNs to short classnames.
Because we use the same name space to both class files and the test files for them.

Adding the same class annotation may be a bug in Rector, but I don't expect it to be fixed anytime soon.
So we are having trouble with the php-cs-fixer changing the classname in @see.
So I would appreciate to revert the behavior or to have an option to disable it.

[Wirone]

Because we use the same name space to both class files and the test files for them.

Ah, that's the reason why it is shortened 👍. I've created #7628 which provides ability to configure fully_qualified_strict_types fixer with a list of phpDoc tags that should be processed, so you can disable processing for @see while keeping it for other tags, or disable this feature completely if you want.