Fix inconsistent analysis on case-insensitive filesystems #3988

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

Jump to bottom

Open

staabm wants to merge 1 commit into phpstan:2.1.x

from staabm:bug12972

Open

Fix inconsistent analysis on case-insensitive filesystems #3988

staabm wants to merge 1 commit into phpstan:2.1.x from staabm:bug12972

Conversation

@staabm

Copy link

Contributor

@staabm staabm commented May 12, 2025 •

edited

Loading

closes phpstan/phpstan#12972

since on macos filesystem paths are case-insensitive before this PR we ran into the following error:

➜ bug-12972 git:(bug12972) ../../bin/phpstan analyse --debug 
 ! [NOTE] The Xdebug PHP extension is active, but "--xdebug" is not used. 
 ! The process was restarted and it will not halt at breakpoints. 
 ! Use "--xdebug" if you want to halt at breakpoints. 
Note: Using configuration file /Users/m.staab/Documents/GitHub/phpstan-src/e2e/bug-12972/phpstan.dist.neon.
/Users/m.staab/Documents/GitHub/phpstan-src/e2e/bug-12972/src/folder/file2.php
/Users/m.staab/Documents/GitHub/phpstan-src/e2e/bug-12972/src/other/file.php
 ------ --------------------------------------------------------------------------------------------- 
 Line other/file.php 
 ------ --------------------------------------------------------------------------------------------- 
 6 Method other12972\MyClass::doSomething() should return int but return statement is missing. 
 ------ ---------------------------------------------------------------------------------------------

-> PHPStan is using the simple-parser instead of the rich-parser as it does not handle the analyzed path case-insentive as macOS would do (same for windows)

see

➜ phpstan-src git:(bug12972) ls -l e2e/bug-12972/src/ 
total 0
drwxr-xr-x@ 3 m.staab staff 96 May 12 14:59 folder
drwxr-xr-x@ 3 m.staab staff 96 May 12 14:52 other
➜ phpstan-src git:(bug12972) ls e2e/bug-12972/src/other/file.php 
e2e/bug-12972/src/other/file.php
➜ phpstan-src git:(bug12972) ls e2e/bug-12972/src/OTHER/file.php
e2e/bug-12972/src/OTHER/file.php
➜ phpstan-src git:(bug12972) php -r 'var_dump(is_file("e2e/bug-12972/src/OTHER/file.php"));'
bool(true)
➜ phpstan-src git:(bug12972) php -r 'var_dump(is_file("e2e/bug-12972/src/other/file.php"));'
bool(true)
➜ phpstan-src git:(bug12972) php -v 
PHP 8.4.7 (cli) (built: May 6 2025 12:31:58) (NTS)
Copyright (c) The PHP Group
Built by Homebrew
Zend Engine v4.4.7, Copyright (c) Zend Technologies

@clxmstaab clxmstaab force-pushed the bug12972 branch 2 times, most recently from 0bfa507 to 23a7403 Compare

May 12, 2025 13:39

@staabm staabm marked this pull request as ready for review

May 12, 2025 13:49

@phpstan-bot

Copy link

Collaborator

phpstan-bot commented May 12, 2025

This pull request has been marked as ready for review.

@staabm staabm mentioned this pull request

May 12, 2025

Inconsisten analysis results phpstan/phpstan#12972

Closed

staabm

staabm commented

May 12, 2025

View reviewed changes

.github/workflows/e2e-tests.yml

Comment on lines +394 to +440

- name: "Install GNU Patch on macOS" # see https://github.com/cweagans/composer-patches/issues/326

if: runner.os == 'macOS'

run: |

brew install gpatch

echo "/opt/homebrew/opt/gpatch/libexec/gnubin" >> $GITHUB_PATH

Copy link

Contributor Author

@staabm staabm May 12, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

on macOS we need a GNU compatible patch command, as the apple-shipped patch command cannot apply our patch files:

Error: Cannot apply patch 0 (patches/cloudflare-ca.patch)!
In Patches.php line 331:
 
 Cannot apply patch 0 (patches/cloudflare-ca.patch)!

staabm

staabm commented

May 12, 2025

View reviewed changes

src/Analyser/AnalysedFilesResolver.php

Comment on lines 21 to 42

public function __construct(array $files = [])

{

$this->caseInsensitiveFilesystem = PHP_OS_FAMILY === 'Darwin';

$this->setAnalysedFiles($files);

}

/**

* @param string[] $files

public function setAnalysedFiles(array $files): void

{

if ($this->caseInsensitiveFilesystem) {

$files = array_map(static fn (string $file): string => strtolower($file), $files);

}

$this->analysedFiles = array_fill_keys($files, true);

}

public function isInAnalyzedFiles(string $file): bool

{

if ($this->caseInsensitiveFilesystem) {

$file = strtolower($file);

}

return isset($this->analysedFiles[$file]);

}

Copy link

Contributor Author

@staabm staabm May 12, 2025

Choose a reason for hiding this comment

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

actual fix is here, everything else are adjustments to make sure this new logic is used all over the place

staabm

staabm commented

May 13, 2025

View reviewed changes

src/Analyser/AnalysedFilesResolver.php Outdated Show resolved Hide resolved

@staabm staabm force-pushed the bug12972 branch from f8715fb to 3f9eb31 Compare

May 16, 2025 13:42

@clxmstaab clxmstaab force-pushed the bug12972 branch 3 times, most recently from 72d0d20 to d283dbe Compare

May 19, 2025 07:04

@staabm staabm force-pushed the bug12972 branch from d283dbe to 4716b6d Compare

May 20, 2025 09:44

@staabm


 Fix inconsistent analysis on case-insensitive filesystems

10c222a

Update e2e-tests.yml
Update e2e-tests.yml
Update e2e-tests.yml
Update e2e-tests.yml
Update e2e-tests.yml
Update e2e-tests.yml
Update e2e-tests.yml
fix PathRoutingParser
extract and re-use AnalysedFilesResolver
use AnalysedFilesResolver more
use AnalysedFilesResolver more
simplify
test also on windows
added failling ubuntu test
added feature detect
simplify detect
more assertions
simplify
Update AnalysedFilesResolver.php

@clxmstaab clxmstaab force-pushed the bug12972 branch from 4716b6d to 10c222a Compare

May 21, 2025 07:16

ondrejmirtes

ondrejmirtes requested changes

May 21, 2025

View reviewed changes

.github/workflows/e2e-tests.yml

- script: |

cd e2e/bug-12972

../../bin/phpstan analyze

os: "windows-latest"

Copy link

Member

@ondrejmirtes ondrejmirtes May 21, 2025

Choose a reason for hiding this comment

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

I don't get it. Why do you expect different outcomes on Linux vs. the rest?

Copy link

Contributor Author

@staabm staabm May 21, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

because linux does not have a case-insensitive filesystem.

Copy link

Member

@ondrejmirtes ondrejmirtes May 21, 2025

Choose a reason for hiding this comment

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

So you have a project that deliberately works only on case-insensitive filesystems?

Copy link

Contributor Author

@staabm staabm May 21, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

the test mimics what php-src will do at runtime.
it will happily accept a different-cased path on windows/macos but doesn't on linux (acutally depending on the filesystem case-sensitivy not the OS itself).

Copy link

Member

@ondrejmirtes ondrejmirtes May 21, 2025

Choose a reason for hiding this comment

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

I'd like if the e2e test mimiced how your project actually worked. This just looks like PHPStan fixes macOS but breaks Linux.

Copy link

Contributor Author

@staabm staabm May 21, 2025

Choose a reason for hiding this comment

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

ok, lets look into #4003 which mimics our project setup more closely

Labels

None yet

3 participants

@staabm @phpstan-bot @ondrejmirtes

Uh oh!

Fix inconsistent analysis on case-insensitive filesystems #3988

Are you sure you want to change the base?

Fix inconsistent analysis on case-insensitive filesystems #3988

Uh oh!

Conversation

@staabm staabm commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phpstan-bot commented May 12, 2025

Uh oh!

@staabm staabm May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@staabm staabm May 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@ondrejmirtes ondrejmirtes May 21, 2025

Choose a reason for hiding this comment

Uh oh!

@staabm staabm May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes May 21, 2025

Choose a reason for hiding this comment

Uh oh!

@staabm staabm May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes May 21, 2025

Choose a reason for hiding this comment

Uh oh!

@staabm staabm May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

@staabm staabm commented May 12, 2025 •

edited

Loading

@staabm staabm May 12, 2025 •

edited

Loading

@staabm staabm May 21, 2025 •

edited

Loading

@staabm staabm May 21, 2025 •

edited

Loading