Skip to content

fix: Phpstan errors in current master build #413

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.

Sign up for GitHub

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

Open
acoulton wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: Phpstan errors in current master build #413

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8624690
fix: update_i18n script should check that keywords are strings
acoulton Mar 24, 2026
5030837
test: Refactor GherkinFileLoaderTest to avoid phpstan type narrowing
acoulton Mar 24, 2026
f13e78b
Revert "test: Refactor GherkinFileLoaderTest to avoid phpstan type na…
acoulton Mar 24, 2026
e4bbdeb
fix: phpstan generics tagging of LoaderInterface::supports
acoulton Mar 24, 2026
dbaeb9e
fix: incorrect logic in type assertion
acoulton Mar 24, 2026
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
View file
Open in desktop
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
View file
Open in desktop
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');
Copy link
Copy Markdown
Member

@stof stof Mar 24, 2026

Choose a reason for hiding this comment

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

this assertion looks weird to me. Shouldn't it assert that it cannot find any non-string value instead ?

Copy link
Copy Markdown
Contributor Author

@acoulton acoulton Mar 24, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@acoulton acoulton Mar 24, 2026

Choose a reason for hiding this comment

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

Fixed.

/**
* 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

@stof stof Mar 24, 2026

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

@acoulton acoulton Mar 24, 2026

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
80 changes: 68 additions & 12 deletions tests/Loader/GherkinFileLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,25 @@
use Behat\Gherkin\Lexer;
use Behat\Gherkin\Loader\GherkinFileLoader;
use Behat\Gherkin\Parser;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;

class GherkinFileLoaderTest extends TestCase
{
private GherkinFileLoader $loader;
private string $featuresPath;

private static function featuresPath(): string
{
return dirname(__DIR__) . DIRECTORY_SEPARATOR . 'Fixtures' . DIRECTORY_SEPARATOR . 'features';
}

protected function setUp(): void
{
$parser = new Parser(new Lexer(new CucumberDialectProvider()));
$this->loader = new GherkinFileLoader($parser);

$this->featuresPath = dirname(__DIR__) . DIRECTORY_SEPARATOR . 'Fixtures' . DIRECTORY_SEPARATOR . 'features';
$this->featuresPath = self::featuresPath();
}

public function testSupports(): void
Expand Down Expand Up @@ -93,26 +99,76 @@ public function testParsingCachedFeature(): void
$this->assertEquals('cache', $features[0]);
}

public function testBasePath(): void
/**
* @return array<string, array{?string, array<string, bool>}>
*/
public static function providerSupportsWithBasePath(): array
{
return [
'with no base path set' => [
null,
[
// The default is the current working directory, and there are no files there
'features' => false,
'tables.feature' => false,
'features/tables.feature' => false,
'features/pystring.feature' => false,
'features/multiline_name.feature' => false,
],
],
'with base path set to features directory' => [
self::featuresPath(),
[
'features' => false,
'tables.feature' => true,
'pystring.feature' => true,
'features/tables.feature' => false,
'features/pystring.feature' => false,
],
],
'with base path set to parent of features directory' => [
self::featuresPath() . '/../',
[
'features' => false,
'tables.feature' => false,
'pystring.feature' => false,
'features/tables.feature' => true,
'features/pystring.feature' => true,
],
],
];
}

/**
* @param array<string,bool> $expected
*/
#[DataProvider('providerSupportsWithBasePath')]
public function testSupportsWithBasePath(?string $basePath, array $expected): void
{
$this->assertFalse($this->loader->supports('features'));
$this->assertFalse($this->loader->supports('tables.feature'));
if ($basePath !== null) {
$this->loader->setBasePath($basePath);
}

$this->loader->setBasePath($this->featuresPath . '/../');
$this->assertFalse($this->loader->supports('features'));
$this->assertFalse($this->loader->supports('tables.feature'));
$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'));
Comment on lines -104 to -106
Copy link
Copy Markdown
Contributor Author

@acoulton acoulton Mar 24, 2026

Choose a reason for hiding this comment

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

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 GherkinFileLoader being tagged as @extends AbstractFileLoader<string>
  • the LoaderInterface::supports being 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?

Copy link
Copy Markdown
Member

@stof stof Mar 24, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@acoulton acoulton Mar 24, 2026

Choose a reason for hiding this comment

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

@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:

https://phpstan.org/r/091134b1-4a3e-4514-a63d-17b7c6ccb795

$this->assertSame($expected, $actual);
}

public function testLoadWithBasePath(): void
{
$this->loader->setBasePath(self::featuresPath() . '/../');
$features = $this->loader->load('features/pystring.feature');
$this->assertCount(1, $features);
$this->assertEquals('A py string feature', $features[0]->getTitle());
$this->assertEquals($this->featuresPath . DIRECTORY_SEPARATOR . 'pystring.feature', $features[0]->getFile());
$this->assertEquals(self::featuresPath() . DIRECTORY_SEPARATOR . 'pystring.feature', $features[0]->getFile());

$this->loader->setBasePath($this->featuresPath);
$this->loader->setBasePath(self::featuresPath());
$features = $this->loader->load('multiline_name.feature');
$this->assertCount(1, $features);
$this->assertEquals('multiline', $features[0]->getTitle());
$this->assertEquals($this->featuresPath . DIRECTORY_SEPARATOR . 'multiline_name.feature', $features[0]->getFile());
$this->assertEquals(self::featuresPath() . DIRECTORY_SEPARATOR . 'multiline_name.feature', $features[0]->getFile());
}
}
Loading