Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .php-cs-fixer.dist.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,8 @@
'single_line_throw' => false,
'ternary_to_null_coalescing' => true,
'global_namespace_import' => false,
'phpdoc_to_comment' => [ // Keep as a phpdoc if it contains the `@var` annotation
'ignored_tags' => ['var'],
],
])
->setFinder($finder);
24 changes: 18 additions & 6 deletions bin/update_i18n
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ foreach ($gherkinLanguages as $lang => $keywords) {
$words = [$words];
}

assert(array_find($words, fn ($word) => !is_string($word)) === null, 'Expected $words to be an array of strings');
/**
* Here we force phpstan to recognise that we have narrowed the type of $words to a list of strings with the
* assertion above. Otherwise it will report an error from the later `implode()` call that $words must be a
* list<string>.
*
* 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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this @var should probably go near the assignment of the value (on line 24) instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

*/
if ($type === 'scenarioOutline') {
$type = 'scenario_outline';
}
Expand All @@ -32,7 +43,6 @@ foreach ($gherkinLanguages as $lang => $keywords) {
$formattedKeywords = [];

foreach ($words as $word) {
assert(is_string($word));
$formattedWord = trim($word);

if ($formattedWord === $word) {
Expand All @@ -45,12 +55,14 @@ foreach ($gherkinLanguages as $lang => $keywords) {
$words = $formattedKeywords;
}

usort($words, static function ($type1, $type2) {
assert(is_string($type1));
assert(is_string($type2));
foreach ($words as $word) {
assert(is_string($word));
}

return [mb_strlen($type2, 'utf8'), $type1] <=> [mb_strlen($type1, 'utf8'), $type2];
});
usort(
$words,
static fn (string $type1, string $type2) => [mb_strlen($type2, 'utf8'), $type1] <=> [mb_strlen($type1, 'utf8'), $type2]
);

$langMessages[$type] = implode('|', $words);
}
Expand Down
6 changes: 6 additions & 0 deletions src/Gherkin.php
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,12 @@ public function resolveLoader(mixed $resource)
{
foreach ($this->loaders as $loader) {
if ($loader->supports($resource)) {
/**
* If the loader supports the provided resource, then it can safely be typed as
* LoaderInterface<ResourceType>.
*
* @var LoaderInterface<TResourceType> $loader
*/
Comment on lines +154 to +159
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

return $loader;
}
}
Expand Down
6 changes: 2 additions & 4 deletions src/Loader/LoaderInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,9 @@ interface LoaderInterface
/**
* Checks if current loader supports provided resource.
*
* @template TSupportedResourceType
* @param mixed $resource Resource to load
*
* @param TSupportedResourceType $resource Resource to load
*
* @phpstan-assert-if-true =LoaderInterface<TSupportedResourceType> $this
* @phpstan-assert-if-true =TResourceType $resource
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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).

*
* @return bool
*/
Expand Down
Loading