Skip to content
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

DX: Narrow docblock types in Tokenizer #6293

Merged
merged 2 commits into from Jul 19, 2022
Merged

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Feb 17, 2022

No description provided.

@coveralls
Copy link

coveralls commented Feb 17, 2022

Coverage Status

Coverage increased (+0.002%) to 92.889% when pulling b13205c on jrmajor:stan into 76bd49d on FriendsOfPHP:master.

src/Tokenizer/Tokens.php Show resolved Hide resolved
src/Tokenizer/Tokens.php Show resolved Hide resolved
@jrmajor jrmajor marked this pull request as ready for review February 17, 2022 18:24
* @var CaseAnalysis[]
* @var list<CaseAnalysis>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎🏻 I don't see the value of this change, tbh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static analysis tools interpret CaseAnalysis[] as array<array-key, CaseAnalysis>, which tells you nothing about keys. This change makes it easier to understand what do they actually mean:

  • array<string, CaseAnalysis> means that keys are strings and probably are meaningful, maybe case name?
  • array<int, CaseAnalysis> means that keys are integers and may mean something, maybe index of T_CASE token?
  • list<CaseAnalysis> means that this is just a list, keys are not meaningful,
  • CaseAnalysis[] may be any of the above.

Take a look at changes in FunctionsAnalyser. This method also uses Foo[] type, but turns out that in that case keys are actually argument names:

  /**
-  * @return ArgumentAnalysis[]
+  * @return array<string, ArgumentAnalysis>
   */
  public function getFunctionArguments(Tokens $tokens, int $functionIndex): array

@@ -1641,6 +1646,9 @@ private function getBlockEdgeCachingTestTokens(): Tokens
]);
}

/**
* @param Tokens::BLOCK_TYPE_* $type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls don't, there is no such type as Tokens::BLOCK_TYPE_*

some sca tools will try to convert it to

-private static function assertFindBlockEnd(int $expectedIndex, string $source, int $type, int $searchIndex): void
+private static function assertFindBlockEnd(int $expectedIndex, string $source, Tokens::BLOCK_TYPE_* $type, int $searchIndex): void

which will obviously fail

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param Tokens::BLOCK_TYPE_* $type
* @param int $type one of Tokens::BLOCK_TYPE_*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no such type as Tokens::BLOCK_TYPE_*

Surely there's no such native PHP type, but neither there is ArgumentAnalysis[].

As for modern static analysis tools, they understand it — here's an example in PHPStan and Psalm.

some sca tools will try to convert it to

What tools? That would probably mean they're broken; they should validate that the PHPDoc type is a correct PHP type before making such changes. If they don't do that, ArgumentAnalysis[] would also break them.

Comment on lines 743 to 748
$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessageMatches('/^Invalid param type: "-1"\.$/');

// @phpstan-ignore-next-line
$tokens->findBlockEnd(-1, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keradus This is a place where Tokens::BLOCK_TYPE_* type actually helps to find invalid usage of this method.

@jrmajor jrmajor changed the title Narrow docblock types in Tokenizer DX: Narrow docblock types in Tokenizer Jun 30, 2022
@jrmajor jrmajor force-pushed the stan branch 2 times, most recently from 2ffcb08 to 98d47d1 Compare July 8, 2022 11:43
@julienfalque julienfalque merged commit fab645b into PHP-CS-Fixer:master Jul 19, 2022
@julienfalque
Copy link
Member

Thank you @jrmajor.

@jrmajor jrmajor deleted the stan branch July 19, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants