Skip to content
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

Force usage of latest patch version to avoid JIT bugs #11270

Merged
merged 2 commits into from
Feb 7, 2025
Merged

Force usage of latest patch version to avoid JIT bugs #11270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ba25ce
Force usage of latest patch version to avoid JIT bugs
danog Feb 7, 2025
b63a208
cs-fix
danog Feb 7, 2025
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
39 changes: 20 additions & 19 deletions src/Psalm/Internal/Analyzer/ProjectAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@
use function number_format;
use function preg_match;
use function rename;
use function sprintf;
use function str_ends_with;
use function str_starts_with;
use function strlen;
Expand All @@ -87,6 +86,7 @@
use function usort;

use const PHP_EOL;
use const PHP_VERSION;
use const PSALM_VERSION;
use const STDERR;

Expand Down Expand Up @@ -280,7 +280,7 @@ private function clearCacheDirectoryIfConfigOrComposerLockfileChanged(): void
clearstatcache(true, $cache_directory);
if (is_dir($cache_directory)) {
$this->progress->debug(
'Composer lockfile change detected, clearing cache directory ' . $cache_directory . "\n",
'Composer lockfile change detected, clearing cache directory ' . $cache_directory . PHP_EOL,
);

Config::removeCacheDirectory($cache_directory);
Expand All @@ -297,7 +297,7 @@ private function clearCacheDirectoryIfConfigOrComposerLockfileChanged(): void
clearstatcache(true, $cache_directory);
if (is_dir($cache_directory)) {
$this->progress->debug(
'Config change detected, clearing cache directory ' . $cache_directory . "\n",
'Config change detected, clearing cache directory ' . $cache_directory . PHP_EOL,
);

Config::removeCacheDirectory($cache_directory);
Expand Down Expand Up @@ -350,7 +350,7 @@ private function visitAutoloadFiles(): void
$now_time = microtime(true);

$this->progress->debug(
'Visiting autoload files took ' . number_format($now_time - $start_time, 3) . 's' . "\n",
'Visiting autoload files took ' . number_format($now_time - $start_time, 3) . 's' . PHP_EOL,
);
}

Expand Down Expand Up @@ -411,12 +411,11 @@ private function generatePHPVersionMessage(): string
$codebase->config->php_extensions_supported_by_psalm_callmaps,
);

$message = sprintf(
"Target PHP version: %d.%d %s",
$codebase->getMajorAnalysisPhpVersion(),
$codebase->getMinorAnalysisPhpVersion(),
$source,
);
$message = "Target PHP version: "
.$codebase->getMajorAnalysisPhpVersion()."."
.$codebase->getMinorAnalysisPhpVersion()." "
.$source
;

$enabled_extensions_names = array_keys(array_filter($codebase->config->php_extensions));
if (count($enabled_extensions_names) > 0) {
Expand All @@ -427,7 +426,9 @@ private function generatePHPVersionMessage(): string
$message .= ' (unsupported extensions: ' . implode(', ', $unsupported_php_extensions) . ')';
}

return "$message.\n";
$message .= '.'.PHP_EOL.PHP_EOL."Running on PHP ".PHP_VERSION.'.'.PHP_EOL.PHP_EOL;

return $message;
}

public function check(string $base_dir, bool $is_diff = false): void
Expand Down Expand Up @@ -479,8 +480,8 @@ public function check(string $base_dir, bool $is_diff = false): void
$this->codebase->infer_types_from_usage = true;
} else {
$this->codebase->diff_run = true;
$this->progress->debug(count($diff_files) . ' changed files: ' . "\n");
$this->progress->debug(' ' . implode("\n ", $diff_files) . "\n");
$this->progress->debug(count($diff_files) . ' changed files: ' . PHP_EOL);
$this->progress->debug(' ' . implode(PHP_EOL." ", $diff_files) . PHP_EOL);

$this->codebase->analyzer->addFilesToShowResults($this->project_files);

Expand Down Expand Up @@ -528,7 +529,7 @@ public function check(string $base_dir, bool $is_diff = false): void
$removed_parser_files = $this->parser_cache_provider->deleteOldParserCaches($start_checks);

if ($removed_parser_files) {
$this->progress->debug('Removed ' . $removed_parser_files . ' old parser caches' . "\n");
$this->progress->debug('Removed ' . $removed_parser_files . ' old parser caches' . PHP_EOL);
}
}
}
Expand Down Expand Up @@ -851,14 +852,14 @@ public function findReferencesTo(string $symbol): void
$selection_start = $selection_bounds[0] - $snippet_bounds[0];
$selection_length = $selection_bounds[1] - $selection_bounds[0];

echo $location->file_name . ':' . $location->getLineNumber() . "\n" .
echo $location->file_name . ':' . $location->getLineNumber() . PHP_EOL .
(
$this->stdout_report_options->use_color
? substr($snippet, 0, $selection_start) .
"\e[97;42m" . substr($snippet, $selection_start, $selection_length) .
"\e[0m" . substr($snippet, $selection_length + $selection_start)
: $snippet
) . "\n" . "\n";
) . PHP_EOL . PHP_EOL;
}
}

Expand Down Expand Up @@ -947,7 +948,7 @@ private function checkDiffFilesWithConfig(Config $config, array $file_list = [])
}

if (!$config->isInProjectDirs($file_path)) {
$this->progress->debug('skipping ' . $file_path . "\n");
$this->progress->debug('skipping ' . $file_path . PHP_EOL);

continue;
}
Expand All @@ -960,7 +961,7 @@ private function checkDiffFilesWithConfig(Config $config, array $file_list = [])

public function checkFile(string $file_path): void
{
$this->progress->debug('Checking ' . $file_path . "\n");
$this->progress->debug('Checking ' . $file_path . PHP_EOL);

$this->config->visitPreloadedStubFiles($this->codebase, $this->progress);

Expand Down Expand Up @@ -1003,7 +1004,7 @@ public function checkPaths(array $paths_to_check): void
$this->codebase->scanner->addFilesToShallowScan($this->extra_files);

foreach ($paths_to_check as $path) {
$this->progress->debug('Checking ' . $path . "\n");
$this->progress->debug('Checking ' . $path . PHP_EOL);

if (is_dir($path)) {
$this->checkDirWithConfig($path, $this->config, true);
Expand Down
19 changes: 14 additions & 5 deletions src/Psalm/Internal/CliUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,9 +524,6 @@ public static function runningInCI(): bool

public static function checkRuntimeRequirements(): void
{
$required_php_version = 8_01_17;
$required_php_version_text = '8.1.17';

// the following list was taken from vendor/composer/platform_check.php
// It includes both Psalm's requirements (from composer.json) and the
// requirements of our dependencies `netresearch/jsonmapper` and
Expand All @@ -545,9 +542,21 @@ public static function checkRuntimeRequirements(): void
];
$issues = [];

if (PHP_VERSION_ID < $required_php_version) {
$issues[] = 'Psalm requires a PHP version ">= ' . $required_php_version_text . '".'
$major_minor = PHP_VERSION_ID - (PHP_VERSION_ID % 100);
foreach ([
8_01_31 => '8.1.31',
8_02_27 => '8.2.27',
8_03_16 => '8.3.16',
8_04_03 => '8.4.3',
] as $version => $txt) {
$version_m = $version - ($version % 100);
if ($version_m === $major_minor) {
if (PHP_VERSION_ID < $version) {
$issues[] = 'Psalm requires a PHP version ">= ' . $txt . '".'
. ' You are running ' . PHP_VERSION . '.';
}
break;
}
Comment on lines +546 to +559
Copy link
Contributor

Choose a reason for hiding this comment

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

Please just declare composer.json "conflict" ranges next time: this has caused me quite a few headaches, yesterday.

I'd rather get an older psalm version, than one that crashes at startup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thing is, the version bump was made for a specific reason, to avoid crashes and other subtle bugs that would have caused more lost time in pointless debugging than a simple PHP version upgrade...

Copy link
Contributor

@Ocramius Ocramius Feb 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

From a consumer perspective, a conflict range would've made this clear, and could've been relaxed until a bugfix is applied (if that would ever allow relaxing the dependency range - it's fine to have the limit in place from here on).

A runtime crash is the worst of both worlds:

  • it doesn't work
  • I don't know it until I run it

Early signaling here is composer update failing to get the latest release.

The current effective result I have right now is me + other people I collaborate with having to manually go in composer.json and pinning vimeo/psalm to an older release.

In fact, I currently use vimeo/psalm:6.4.0 (rollback) on all my systems, and added vimeo/psalm:6.5.0 as an explicit conflict for my colleagues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem with conflict ranges is that composer does not make them at all clear to users of a package; the dependency resolver simply works around them to fit the current version constraints, regardless of the side effects (bugs in older PHP versions).

This is not the only runtime check, and some requirements (like the VM overcommit requirements) cannot even be expressed using composer.

I will not be adding a dependency constraint to composer.json, because as I said, Psalm is a development tool, and the development environment is not as rigid as a production environment (to a certain extent, i.e. minor bumps will still be expressed using composer, as they are somewhat related to the production environment).

Copy link
Contributor

@Ocramius Ocramius Feb 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the dependency resolver simply works around them to fit the current version constraints, regardless of the side effects (bugs in older PHP versions).

We've been working this way for well over a decade now: not getting "latest" is fine, and composer why-not tells you so.

I've even built an entire package around this: https://github.com/Roave/SecurityAdvisories/ - it's designed to exclude dependencies that are a hazard, and people do use it, consciously.

Runtime checks are 100% not the way, and amplify noise around this.

I (relatively experienced PHP dev) for sure wasted a lot of time yesterday dealing with this while trying to work on psalm/psalm-plugin-phpunit#149 - had to roll back vimeo/psalm tons of times while trying out dependency ranges.

What moving this to a runtime check achieved here is:

  1. confusion (people are putting feedback here for a reason: nobody wants to piss you off, rest assured of that :-P )
  2. almost all composer.lock upgrade pipelines ( @renovate / @dependabot ) will fail due to an invalid upgrade
  3. time wasted during development

What should be done next?

The correct way forward here is:

  1. revert patch
  2. tag new patch release (so that upgrades allow jumping to a working release again, including automated upgrades performed so far)
  3. re-apply patch
  4. add dependency range in "conflict"
  5. tag new patch release

Note that deleting a release is non-viable, unless it's a security risk.

"it's a development tool"

As for the "it's a development tool": it does not matter if it is - it's part of the dependency graph, and the dependency graph is designed to say "these work together" or "these don't work together".

In addition to that, even if the dependency graph is split between --dev and --no-dev dependencies, the "these work together" should still apply for the --dev part.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Marco, I value your input, and look forward to collaborating with you (will be taking a look at your PRs next week).

I am not too willing to die on this hill: in general, it's not about this specific change, it's about the possiblity for me as the maintainer of Psalm to direct users to make changes to their environment, to avoid them footguns when working with Psalm.

In my long time as a PHP engineer, I have always tried to proactively prevent bugs from even reaching users, guaranteeing the best possible experience when actually using my tools, be it using warnings to update to the last version on startup (like in MadelineProto, will probably implement this in Psalm as well, given that phpstan has them too), or using runtime exceptions for particularly bad platform bugs (i.e. did something similar for MadelineProto for a recent fiber-related bug, and @trowski did the same inside of amphp itself as well), or even automatic static analysis on code written by the users, to proactively detect possible issues.

I do not want users to waste time debugging weird exceptions and crashes caused by PHP bugs (like recently, I already got two new issues caused by a PHP bug on older versions of PHP, combined with the new amphp-based concurrency model).

A (patch release, not even a major/minor!) PHP version upgrade is a minor inconvenience, bypassable using any modern PHP distribution method (ondrej ppas on debian/ubuntu), and does not require any additional refactoring work on the codebase on behalf of users.

This is also the case for other runtime checks as well (such as #11218).

I am a user and great fan of your security-advisories project, because it allows library developers to bypass composer's long-time limitations, to prevent users from getting vulnerable/broken versions of libraries.

I am trying to apply the same rationale behind security-advisories to Psalm as well, using the only way I have available to me, given that we're talking about PHP platform bugs, and not issues with userland libraries.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the message didn't come through, but I'm also just making a point, and not really willing to explain further than I already did :D

We should probably talk about it over 🍻 at PHPDay or such :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ayy, now that's a nice idea!

}

$missing_extensions = array_filter(
Expand Down