fix: Phpstan errors in current master build#413
fix: Phpstan errors in current master build#413acoulton wants to merge 5 commits intoBehat:masterfrom
Conversation
Since phpstan@2.1.41, it checks that `implode` is only called with an array of strings. We were not previously checking this for the overall list of `$words`, but were only asserting it in specific places e.g. within the usort callback. Moved the assertion to the start of the loop, which allows us to simplify the checks in later code. Unfortunately I have had to add a phpdoc tag to tell phpstan that the array type has been narrowed - I've not found a way to have it infer that at runtime (I've opened a feature request for this). This also required updating our php-cs-fixer config to keep docblocks that contain `@var` tags even if they are not actually docblocks.
Since phpstan@2.1.40, we have been getting an error on the test due to a
failed type check on calling `assertCount` with the result of
`$this->loader->load()`.
This is because before load, the test was also causing `->supports()`
multiple times with hardcoded strings.
The `->supports()` method in the parent LoaderInterface is tagged with:
```
* @phpstan-assert-if-true =LoaderInterface<TSupportedResourceType> $this
```
This meant that when we asserted `->supports('features/tables.feature')`
was true, phpstan narrowed the type of `$this->loader` to
`GherkinFileLoader&LoaderInterface<'features/tables.feature'>`
The next call that asserted `->supports('features/pystring.feature')`
then caused phpstan to decide that `$this->loader` was now a `*NEVER*`
type. That caused all subsequent calls to `->load()` to fail analysis.
I have refactored the tests so that we are not calling `supports`
multiple times with hardcoded strings in the same test method.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #413 +/- ##
=========================================
Coverage 95.71% 95.71%
Complexity 685 685
=========================================
Files 45 45
Lines 2031 2031
=========================================
Hits 1944 1944
Misses 87 87
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| $this->assertTrue($this->loader->supports('features/tables.feature')); | ||
| $actual = []; | ||
| foreach (array_keys($expected) as $resource) { | ||
| $actual[$resource] = $this->loader->supports($resource); | ||
| } | ||
|
|
||
| $this->assertTrue($this->loader->supports('features/pystring.feature')); |
There was a problem hiding this comment.
On the first assertTrue($this->loader->supports(...)) call, phpstan was narrowing the type of $this->loader to Behat\Gherkin\Loader\GherkinFileLoader&Behat\Gherkin\Loader\LoaderInterface<'features/tables.feature'>.
The second call then caused phpstan to decide that $this->loader was a *NEVER* type.
And this then caused it to decide that the next call to $this->loader->load(...) would return *ERROR*. That then caused an argument type error when we passed it to assertCount().
This appears to be because there's a conflict between:
- the
GherkinFileLoaderbeing tagged as@extends AbstractFileLoader<string> - the
LoaderInterface::supportsbeing tagged with@phpstan-assert-if-true =LoaderInterface<TSupportedResourceType> $this
It's fine if you call the ->supports() with a string variable, because that causes phpstan to infer it as LoaderInterface<string> which matches the class definition. But it causes a problem if you call it with hardcoded values, since phpstan then treats it as LoaderInterface<'value of first string passed'>.
I've created a simplified reproduction case in the phpstan playground
https://phpstan.org/r/6250ecaa-ed3e-48dd-81cf-80d3cf25db3c but I'm not confident enough in phpstan generics to say whether this is a phpstan bug or an issue with our tagging. @stof, do you have an opinion?
There was a problem hiding this comment.
I think the issue is our generic type. supports should refine the type of $resource (to tell phpstan it is compatible with the type of the loader), not of $this.
There was a problem hiding this comment.
@stof thank you!
That makes sense. If I modify the example case to assert the type of the resource rather than the instance, that looks like it works:
…rrowing" This reverts commit 5030837.
Originally, `LoaderInterface::supports()` was tagged with
`@phpstan-assert-if-true` to refine the type of the loader itself.
This was used so that the `Gherkin::resolveLoader()` method could safely
/ automatically narrow from a `LoaderInterface<*>` to a
`LoaderInterface<ActualTypeOfResourceThatWasPassed>`.
The problem with this is that it also means that calling `->supports` on
an instance of a known `LoaderInterface` subclass will change the type
of the instance.
If an implementation returns true for `->supports('some value')` then
phpstan will treat it as `LoaderInterface<'some value'>`. But that's not
necessarily correct - just because `'some value'` passes, doesn't mean
that the instance will *only* accept 'some value'. It could be
`LoaderInterface<string>` or `LoaderInterface<string|Stringable>` or
even `LoaderInterface<mixed>`.
Calling `->supports()` tells us something about the type of the
instance, but not enough to guarantee it.
This becomes a problem if calling `->supports()` multiple times with
different hardcoded values, as phpstan will be unable to resolve the
potential intersection types and will instead treat it as a `*NEVER*`.
The only thing we reliably know is that if `->supports($resource)`
passes then `$resource` must be a type of `TResourceType` and can be
safely passed to `->load($resource)` on the same instance).
Therefore I have flipped the logic of the `@assert-if-true` tag to
refine the type of the resource rather than the instance.
However, we still need to be able to narrow the type of the Loader in
`Gherkin::resolveLoader()`. I think the best way to do that is to
manually hint it within the resolveLoader method. It's safe here,
because we assume that consumers will call `resolveLoader` for each
individual resource, so it doesn't matter if the return typehint is
actually stricter than the class that has been returned.
| * @param TSupportedResourceType $resource Resource to load | ||
| * | ||
| * @phpstan-assert-if-true =LoaderInterface<TSupportedResourceType> $this | ||
| * @phpstan-assert-if-true =TResourceType $resource |
There was a problem hiding this comment.
As @stof identified, calling supports('foo') doesn't guarantee that the instance only accepts 'foo', so we can't safely refine the instance itself to LoaderInterface<'foo'>. Our instance could as easily be (probably is) LoaderInterface<string> or an even wider type.
However we can safely refine $resource - if it passes supports() then it must be a type of TResourceType and safe to pass to ->load($resource).
| /** | ||
| * If the loader supports the provided resource, then it can safely be typed as | ||
| * LoaderInterface<ResourceType>. | ||
| * | ||
| * @var LoaderInterface<TResourceType> $loader | ||
| */ |
There was a problem hiding this comment.
We still need to narrow the type returned from resolveLoader, because $this->loaders is LoaderInterface<*> and the method needs to return LoaderInterface<TResourceType>.
Here, we assume that the caller will call ->resolveLoader($resource)->load($resource) for each resource - they won't attempt to reuse the loader for other resources.
Therefore, it doesn't matter if the type we return is stricter than the actual implementation of the loader we're returning, so long as it accepts the type of resource that was passed in. So the easiest way to do this is just to explicitly tag it the specialised type for phpstan.
|
@stof I've reverted the test refactoring and (I think) fixed the generic / phpstan tagging instead. |
bin/update_i18n
Outdated
| $words = [$words]; | ||
| } | ||
|
|
||
| assert(array_find($words, fn ($word) => is_string($word)) === null, 'Expected $words to be an array of strings'); |
There was a problem hiding this comment.
this assertion looks weird to me. Shouldn't it assert that it cannot find any non-string value instead ?
There was a problem hiding this comment.
Urgh, yeah of course it should. I'd tried a whole range of options for trying to get phpstan to narrow the type itself without success, and mangled the one I'd settled on when I added it back to this file. Will fix.
| * There does not currently seem to be a way to force phpstan to detect this type narrowing at runtime | ||
| * https://github.com/phpstan/phpstan/issues/14360 | ||
| * | ||
| * @var array<string> $words |
There was a problem hiding this comment.
this @var should probably go near the assignment of the value (on line 24) instead.
There was a problem hiding this comment.
Unfortunately it can't go higher up because if it's before the assert(array_filter(...)) then phpstan will complain that the assertions are redundant https://phpstan.org/r/5e86b039-21e8-4bb9-82a1-4877aef0c1c2
Unless we only tag it, and don't assert at runtime at all, but that feels a bit like cheating
Accidentally missed the ! operator when bringing back the syntax from the file I'd been experimenting in.
2 participants
The master branch is failing static analysis due to changes in recent phpstan versions.
One of these I think is a valid issue that had not been detected previously, though fixing it involved working around a limitation in narrowing the types of arrays that I've reported to phpstan in phpstan/phpstan#14360
The other is to do with type narrowing / confusion in how our LoaderInterface and the GherkinFileLoader are tagged as generics. We were previously making an assumption that wasn't guaranteed to be correct. I've updated the generics tagging to fix this.