coding-standard
coding-standard copied to clipboard
doctrine
Drop MissingTraversable* typehint sniffs
Remove CS checks for missing traversable type hints
I propose to disable these sniffs as we have and already adopted tools that are more capable to check for such issues (PHPStan/Psalm).
WDYT? If we agree, I'll go and adopt related tests.
Basic example
PHPStan complains about not getting a type spec: https://phpstan.org/r/9a2a974a-d647-4c95-99d1-e87ef7fc9207
function food(iterable $i): void
{
return;
}
Implementing a method
❌ Missing type hint is complained about https://phpstan.org/r/cebc3f17-4c08-4e6f-a6d8-cfae8ebc4338
interface I {
public function foo(): iterable;
}
final class C implements I {
public function foo(): iterable {
return [];
}
}
✅ Adding a type hint on the interface fixes the error without having a need to also provide @inheritDoc to all implementations. Implementations inherit that from interface by default so @inheritDoc becomes redundant in most cases we use it nowadays.
Phpstan is not limited to single file evaluation like phpcs but can evaluate the whole context.
https://phpstan.org/r/6c748d83-62b3-4c94-b22b-4d852696050e
interface I {
/** @return iterable<mixed> */
public function foo(): iterable;
}
final class C implements I {
public function foo(): iterable {
return [];
}
}
PHPStan or Psalm check for these better.
They check them once the spec is written no? It's up to CS to enforce specifying what's inside a traversable. Unless I'm missing knowledge about some features in those tools :)
What spec do you mean? 🤔
I should've added a playground link though: https://phpstan.org/r/198b1899-3695-4bf6-b158-0c3f3445095d
I guess:
❌ https://psalm.dev/r/6189d0e1e3 without type
✅ https://psalm.dev/r/dacb0340d2 with type
Yeh, but that still allows passing on an iterable<mixed> to somebody else (when not consuming it directly, or when declaring a return type).
In practice, this iterable rule has been one of the biggest drivers in type safety a couple years ago, and dropping it still allows for declaring untyped iterables in abstract types 🤔
Before calling it "done", it would be best to have this also in psalm mixed rules, IMO.
The point here was to drop the phpcs check as we have a smarter and more bulletproof alternative in PHPStan. So even though Psalm does no have the feature parity in this matter now, it does not seem like an obstacle to me.
Not having to spam inheritDoc in every implementation is a nice bonus.
TBH I think we all have to use both psalm and phpstan simultaneously to be safe.
Not really: I stopped using phpstan for my own work (also OSS), and will likely pick it up again in a few years, if things change 🤷♀️
We still tend to use both PHPStan and Psalm in all major Doctrine repositories so there's no need for an additional CS rule
@doctrine/coding-standard-approvers I'm not sure if this can be merged as a patch release so I'm deferring clicking merge button to somebody more informed :)
Though in my opinion we can drop features here that are superseded by other tools if those tools are not newborn or experimental or w.e. and other repositories should too. That was the point of this PR.
Other repositories can eventually reenable those.
I have been always thinking of the coding standard requiring type annotations as a way to enable static analysis tools, not as a solution on its own. As long as PHPStan no longer needs it, I'd say we could disable this requirement in the coding standard. Those meaningless {@inheritDoc}s alone would be a good reason to do that.
Eventually, I'd like to see this tool focusing on code formatting which it's originally designed for, not on static analysis.
Today, I found that using a type alias to replace occurrences of array<int, int|string|Type|null>|array<string, int|string|Type|null> with ArrayOfTypes results in phpcs erroring out with @param annotation of method \Doctrine\DBAL\Query\QueryBuilder::setParameters() does not specify type hint for items of its traversable parameter $types.
So now I'd say this rule is making the maintenance harder: after that refactoring I was planning to narrow down int to Connection::PARAM_*|ParameterType::*, but now it's going to be verbose and impractical.
@Ocramius can you please reconsider?
As for the target branch, I think it's ok to target 9.0.x since it's unlikely to make build fail.
using a type alias to replace occurrences ... results in phpcs erroring out
Sounds like a bug, rather than related to this RFC (which proposes dropping the rule overall).
@Ocramius can you please reconsider?
No, not really - it's something that helped me on countless projects to get type declarations in, even when phpstan/phpstan or vimeo/psalm were installed. It has and is a huge driver for increasing type safety inside codebases.
I reported the bug here FTR: https://github.com/slevomat/coding-standard/issues/1334, hopefully it's fixable. If not, we might consider disabling it on a per-project basis.
