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.

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

Avoid exceptions on fetching uninitialized names in ClassLikes #10730

Closed
wants to merge 3 commits into from

Conversation

ohader
Copy link
Contributor

@ohader ohader commented Feb 20, 2024

Similar to the behavior of ClassLikes::classImplements() (#5984),
the following methods will return a false/empty value in case a
specific class name has not been initialized yet:

  • ClassLikes::classExtends() returns false
  • ClassLikes::getParentInterfaces() returns []

Fixes: #7520
Fixes: #10350
Fixes: #10152

@ohader ohader marked this pull request as draft February 20, 2024 14:09
@ohader
Copy link
Contributor Author

ohader commented Feb 20, 2024

Previous comment from master PR are still valid (#10724 (comment))

I wonder if rescanning whole files is really necessary. My idea was to walk classlikes / functionlikes after the codebase population and tighten the intersections. E.g. if the return method was left as MySessionHandler&SessionHandlerInterface where MySessionHandler implements SessionHandlerInterface (because either MySessionHandler or SessionHandlerInterface were unavailable during the initial scan) it would be tightened to MySessionHandler.

I had something similar in my mind as well, but did not find a good way to "communicate" from the specific inner parts to the outer parts that would trigger those optimizations after codebase population. Basically like:

  • hold unresolved/failed statements/names in memory (probably required for anonymous classes)
  • process different strategies to resolve the types (e.g. simplifying the types like ArrayObject&CountableArrayObject; if that fails, reprocess the parser statements; if that fails reprocess the whole file) - this might be an additional setting in psalm.xml config file
  • in case not all previously unresolved statements/names could be resolved, the process could either fail with an exception or continue with a generic type, e.g. TMixed - this might be another config option in psalm.xml

@simonberger
Copy link

I did test it. While my crash pre-scanning the project files does not happen anymore, I got a crash while scanning the project files.

Uncaught Exception: Psalm\Exception\UnpreparedAnalysisException File /var/www/project/src/SomeMockFile.php has not been properly scanned

Emitted in /var/www/project/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/FileAnalyzer.php:124
Stack trace in the forked worker:
#0 /var/www/project/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(1591): Psalm\Internal\Analyzer\FileAnalyzer->analyze()
#1 /var/www/project/vendor/vimeo/psalm/src/Psalm/Internal/Fork/Pool.php(191): Psalm\Internal\Codebase\Analyzer->analysisWorker()
#2 /var/www/project/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(372): Psalm\Internal\Fork\Pool->__construct()
#3 /var/www/project/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(272): Psalm\Internal\Codebase\Analyzer->doAnalysis()
#4 /var/www/project/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php(552): Psalm\Internal\Codebase\Analyzer->analyzeFiles()

@ohader ohader force-pushed the 10350-intersections-5.x branch from 3f7cce4 to c6ad31a Compare February 20, 2024 15:59
@ohader
Copy link
Contributor Author

ohader commented Feb 20, 2024

I did test it. While my crash pre-scanning the project files does not happen anymore, I got a crash while scanning the project files.

Uncaught Exception: Psalm\Exception\UnpreparedAnalysisException File /var/www/project/src/SomeMockFile.php has not been properly scanned

Emitted in /var/www/project/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/FileAnalyzer.php:124

It seems this is a new/different issue now. Can you try running Psalm single-threaded (./psalm --threads=1 ...)?
Just to make sure it's not the issue I referenced in the TODO section of this PR description. Thx!

@ohader ohader force-pushed the 10350-intersections-5.x branch from c6ad31a to f6c76e5 Compare February 20, 2024 16:06
@simonberger
Copy link

It does not fail without threads (more details in the mentioned issue), but as far as I know the whole issue is at least mostly related to using multithreading.
Very few of the people affected, tested it with --threads=1 or --debug.

@ohader ohader force-pushed the 10350-intersections-5.x branch from f6c76e5 to e1d12f8 Compare February 20, 2024 19:28
@ohader
Copy link
Contributor Author

ohader commented Feb 20, 2024

While working on additional tests for unknown/undefined interfaces used in intersections, I realized that the behavior is different. Thus the \Stringable&\JsonSerializable example failed since both the classes actually existed - when those classes are not existing, the process continues and silently ignores that lack of information.

Thus, it seems there might be a much simpler solution than reprocessing, which is more in line with the rest of the assumptions in PsalmPHP:

I've updated the PR accordingly...

@ohader ohader changed the title Reprocess missing class-like storage events Avoid exceptions on fetching uninitialized names in ClassLikes Feb 20, 2024
@ohader ohader marked this pull request as ready for review February 20, 2024 19:31
@ohader ohader marked this pull request as draft February 20, 2024 19:33
Similar to the behavior of `ClassLikes::classImplements()` (vimeo#5984),
the following methods will return a false/empty value in case a
specific class name has not been initialized yet:

+ `ClassLikes::classExtends()` returns `false`
+ `ClassLikes::getParentInterfaces()` returns `[]`
@ohader ohader force-pushed the 10350-intersections-5.x branch from e1d12f8 to 93e2c3e Compare February 20, 2024 19:47
@ohader
Copy link
Contributor Author

ohader commented Feb 20, 2024

Test cases explicitly expect an exception to infer further type checks... which feels super weird:

try {
$this->assertTrue(UnionTypeComparator::isContainedBy(self::$codebase, $callMapType, $expectedType, false, false, null, false, false), "{$msgPrefix} type '{$specified}' is not contained by reflected type '{$reflected}'");
} catch (InvalidArgumentException $e) {
if (preg_match('/^Could not get class storage for (.*)$/', $e->getMessage(), $matches)
&& !class_exists($matches[1])
&& !interface_exists($matches[1])
&& !enum_exists($matches[1])
) {
$this->fail("Class used in CallMap does not exist: {$matches[1]}");
}
}

ClassLikes::classExtends() now returns false instead of throwing an exception by implicitly invoking $this->classlike_storage_provider->get and waiting for that InvalidArgumentException.

@ohader ohader force-pushed the 10350-intersections-5.x branch from 46c5e35 to 14da91a Compare February 20, 2024 22:34
Since the method `ClassLikes::classExtends()` is not throwning an
exception anymore for unknown class names, the `CallMap` tests had
to be adjusted to make the tested behavior a bit more explicit.

Previously any class name that was unknown to the class-like storage
of PsalmPHP, which however was existing in the native PHP scope
(classes, interfaces or enums) was alread enough to make the
type comparison succeed...
@ohader ohader force-pushed the 10350-intersections-5.x branch from 14da91a to f80f840 Compare February 20, 2024 22:46
@ohader
Copy link
Contributor Author

ohader commented Feb 20, 2024

I'll surrender now... still static keyword failures in the CallMap unit tests... but only for PHP 8.2 and 8.3. I'm not able to reproduce that locally with PHP 8.2.

@ohader
Copy link
Contributor Author

ohader commented Feb 21, 2024

→ Phew... I'm dropping this change in favor of the original PR at #10720

@ohader ohader closed this Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants