fix: Do not shorten FQN for class resolution if imported symbol is not a class #7705

Wirone · 2024-01-10T00:09:41Z

mvorisek · 2024-01-10T00:13:46Z

in #7704 there is a fix already :)

Wirone · 2024-01-10T00:22:47Z

@mvorisek Are you telling me I wasted my time for implementing this 😩? Eeehhhh....

coveralls · 2024-01-10T00:26:25Z

coverage: 95.63% (-0.007%) from 95.637%
when pulling 375464b on Wirone:codito/7652-ignore-class-resolution-for-function
into 08bc02c on PHP-CS-Fixer:master.

mvorisek · 2024-01-10T08:12:46Z

src/Fixer/Import/FullyQualifiedStrictTypesFixer.php

-        if ($this->cacheUsesLast === $uses) {
-            return;
-        }
+        foreach ($uses as $kind => $kindUses) {


the foreach must be put right before the other foreach

Can you explain a bit more?

It is evident, the lines after the first foreach are unrelated to the loop, ie. must be placed before the loop.

Still don't get it, sorry. Can you provide it as a suggestion (diff)?

Previously all imports in the processed namespace were flattened to one list and use later for shortening. We need to have imports list sliced by import kind, so we can use proper imports in the context of currently processed part of code (e.g. use only class imports when processing function arguments).

Wirone · 2024-05-30T15:24:18Z

src/Tokenizer/Analyzer/NamespaceUsesAnalyzer.php

+        /** @var class-string $fqn */
+        $fqn = $fullName;


I had to do that because of PHPStan, which expects class-string, but infers string from the code above (rightfully), and I couldn't do /** @var class-string $fullName */ before return because Fixer wants to change it to // comment and then PHPStan can't see the enforced type 😕.

Wirone · 2024-05-30T15:24:52Z

src/Fixer/Import/FullyQualifiedStrictTypesFixer.php

-        if ($this->cacheUsesLast === $uses) {
-            return;
-        }
+        foreach ($uses as $kind => $kindUses) {


Can you explain a bit more?

Wirone · 2024-05-30T15:44:40Z

@kubawerlos do you have an idea why this fails? It imports and shortens CoversClass like here, but at the same time leaves FQCN like here 😕. I need to go now, will be AFK until late evening, if anyone can take a look I will be grateful.

mvorisek · 2024-05-30T15:50:06Z

@kubawerlos do you have an idea why this fails? It imports and shortens CoversClass like here, but at the same time leaves FQCN like here 😕. I need to go now, will be AFK until late evening, if anyone can take a look I will be grateful.

@Wirone I guess the PHPUnit48MigrationRiskySetTest does not include the FullyQualifiedStrictTypes fixer rule :)

Wirone · 2024-05-30T15:59:52Z

@mvorisek I've added this rule explicitly to the "highest" config, but it sometimes works and sometimes not 🤔.

kubawerlos · 2024-05-30T22:39:55Z

Well, it looks fine on my PC 😁

I see in the job's log:

2024-05-30T15:37:38.9384682Z   79) tests/RuleSet/Sets/PSR12RiskySetTest.php
2024-05-30T15:37:38.9384841Z       ---------- begin diff ----------
2024-05-30T15:37:38.9385237Z --- /home/runner/work/PHP-CS-Fixer/PHP-CS-Fixer/tests/RuleSet/Sets/PSR12RiskySetTest.php
2024-05-30T15:37:38.9385637Z +++ /home/runner/work/PHP-CS-Fixer/PHP-CS-Fixer/tests/RuleSet/Sets/PSR12RiskySetTest.php
2024-05-30T15:37:38.9385766Z @@ -14,9 +14,10 @@
2024-05-30T15:37:38.9385856Z  
2024-05-30T15:37:38.9386008Z  namespace PhpCsFixer\Tests\RuleSet\Sets;
2024-05-30T15:37:38.9386097Z  
2024-05-30T15:37:38.9386246Z +use PhpCsFixer\RuleSet\Sets\PSR12RiskySet;
2024-05-30T15:37:38.9386337Z +
2024-05-30T15:37:38.9386427Z  /**
2024-05-30T15:37:38.9386531Z   * @internal
2024-05-30T15:37:38.9386642Z - *
2024-05-30T15:37:38.9386854Z - * @covers \PhpCsFixer\RuleSet\Sets\PSR12RiskySet
2024-05-30T15:37:38.9386948Z   */
2024-05-30T15:37:38.9387193Z +#[\PHPUnit\Framework\Attributes\CoversClass(PSR12RiskySet::class)]
2024-05-30T15:37:38.9387421Z  final class PSR12RiskySetTest extends AbstractSetTestCase {}
2024-05-30T15:37:38.9387426Z 
2024-05-30T15:37:38.9387577Z       ----------- end diff -----------

but when I run locally from the branch I have:

kub@:~/code/friendsofphp/PHP-CS-Fixer(codito/7652-ignore-class-resolution-for-function)$ php php-cs-fixer fix --diff --config .php-cs-fixer.php-highest.php tests/RuleSet/Sets/PSR12RiskySetTest.php --dry-run
PHP CS Fixer 3.58.2-DEV 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.3.7
Running analysis on 8 cores with 10 files per process.
Parallel runner is an experimental feature and may be unstable, use it at your own risk. Feedback highly appreciated!
Loaded config default from ".php-cs-fixer.php-highest.php".
Using cache file ".php-cs-fixer.cache".
Paths from configuration file have been overridden by paths provided as command arguments.
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

   1) tests/RuleSet/Sets/PSR12RiskySetTest.php
      ---------- begin diff ----------
--- /home/kuba/code/friendsofphp/PHP-CS-Fixer/tests/RuleSet/Sets/PSR12RiskySetTest.php
+++ /home/kuba/code/friendsofphp/PHP-CS-Fixer/tests/RuleSet/Sets/PSR12RiskySetTest.php
@@ -14,9 +14,11 @@

 namespace PhpCsFixer\Tests\RuleSet\Sets;

+use PhpCsFixer\RuleSet\Sets\PSR12RiskySet;
+use PHPUnit\Framework\Attributes\CoversClass;
+
 /**
  * @internal
- *
- * @covers \PhpCsFixer\RuleSet\Sets\PSR12RiskySet
  */
+#[CoversClass(PSR12RiskySet::class)]
 final class PSR12RiskySetTest extends AbstractSetTestCase {}

      ----------- end diff -----------

and I would expect that in CI here too

kubawerlos · 2024-05-30T22:43:49Z

I guess we can move updating .php-cs-fixer.php-highest.php to separate PR and get the fix done.

Wirone · 2024-05-30T23:41:37Z

@kubawerlos it works differently when you apply the "highest" ruleset to a single file or to the whole codebase. The CI outcome can be reproduced locally by running the exact command that is run in the CI, and the running composer cs:check - some files have FQCNs in attributes 😩. I don't understand why, PHPUnit's rule has higher priority and is applied before fully_qualified_strict_types, so the added attribute should be fixed. It does not make sense because some files are fixed fully, some skip the FQCN import step 🤷‍♂️.

Wirone added the topic/fqcn Fully Qualified Class Name usage and conversions label Jan 10, 2024

Wirone self-assigned this Jan 10, 2024

Wirone marked this pull request as ready for review January 10, 2024 00:21

Wirone mentioned this pull request Jan 10, 2024

fix: fully_qualified_strict_types must honor kind uses #7704

Closed

mvorisek reviewed Jan 10, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

github-actions bot added the status/stale label Apr 9, 2024

Wirone removed the status/stale label Apr 9, 2024

keradus force-pushed the master branch from a968b6e to 28209a2 Compare April 17, 2024 08:10

Wirone mentioned this pull request May 26, 2024

refactor: add FullyQualifiedNameAnalyzer #8048

Open

Wirone added 3 commits May 30, 2024 11:22

Add failing test case

3c352a6

Align the code to the current PHPStan rules

2811185

Wirone force-pushed the codito/7652-ignore-class-resolution-for-function branch from 68491f7 to 2811185 Compare May 30, 2024 15:19

Wirone commented May 30, 2024

View reviewed changes

Fix "PHP 8.3 Fixer with migration rules" job

375464b

Wirone force-pushed the codito/7652-ignore-class-resolution-for-function branch from e8bf8bb to 375464b Compare May 30, 2024 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Do not shorten FQN for class resolution if imported symbol is not a class #7705

fix: Do not shorten FQN for class resolution if imported symbol is not a class #7705

Wirone commented Jan 10, 2024

mvorisek commented Jan 10, 2024

Wirone commented Jan 10, 2024

coveralls commented Jan 10, 2024 •

edited

mvorisek Jan 10, 2024

Wirone May 30, 2024

mvorisek May 30, 2024

Wirone May 30, 2024

This comment was marked as outdated.

Wirone May 30, 2024

Wirone May 30, 2024

Wirone commented May 30, 2024

mvorisek commented May 30, 2024

Wirone commented May 30, 2024

kubawerlos commented May 30, 2024

kubawerlos commented May 30, 2024

Wirone commented May 30, 2024

fix: Do not shorten FQN for class resolution if imported symbol is not a class #7705

Are you sure you want to change the base?

fix: Do not shorten FQN for class resolution if imported symbol is not a class #7705

Conversation

Wirone commented Jan 10, 2024

mvorisek commented Jan 10, 2024

Wirone commented Jan 10, 2024

coveralls commented Jan 10, 2024 • edited

mvorisek Jan 10, 2024

Choose a reason for hiding this comment

Wirone May 30, 2024

Choose a reason for hiding this comment

mvorisek May 30, 2024

Choose a reason for hiding this comment

Wirone May 30, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

Wirone May 30, 2024

Choose a reason for hiding this comment

Wirone May 30, 2024

Choose a reason for hiding this comment

Wirone commented May 30, 2024

mvorisek commented May 30, 2024

Wirone commented May 30, 2024

kubawerlos commented May 30, 2024

kubawerlos commented May 30, 2024

Wirone commented May 30, 2024

coveralls commented Jan 10, 2024 •

edited