Avoid exceptions on fetching uninitialized names in ClassLikes by ohader · Pull Request #10730 · vimeo/psalm

ohader · 2024-02-20T14:07:09Z

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 · 2024-02-20T14:11:08Z

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&Countable → ArrayObject; 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 · 2024-02-20T15:41:16Z

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 · 2024-02-20T16:04:13Z

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!

simonberger · 2024-02-20T16:14:26Z

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 · 2024-02-20T19:28:44Z

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:

add some more $classlike_storage_provider->has($interface) checks and we're basically good to go
there are already plenty of these try .. catch blocks and ->has(..) checks in \Psalm\Internal\Codebase\ClassLikes(https://github.com/vimeo/psalm/blob/5.x/src/Psalm/Internal/Codebase/ClassLikes.php) already
I dumped the previous draft-code to a separate branch at https://github.com/ohader/psalm/tree/10350-intersections-outdated-5.x in case it gets relevant again

I've updated the PR accordingly...

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 · 2024-02-20T20:43:37Z

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

psalm/tests/Internal/Codebase/InternalCallMapHandlerTest.php

Lines 649 to 659 in 1c36da6

    
           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.

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 · 2024-02-20T22:54:19Z

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 · 2024-02-21T12:25:39Z

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

ohader mentioned this pull request Feb 20, 2024

Reprocess missing class-like storage events #10724

Closed

1 task

ohader marked this pull request as draft February 20, 2024 14:09

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

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

Add more tests for native intersection types in class properties

b0e6db1

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

ohader changed the title ~~Reprocess missing class-like storage events~~ Avoid exceptions on fetching uninitialized names in ClassLikes Feb 20, 2024

ohader marked this pull request as ready for review February 20, 2024 19:31

ohader marked this pull request as draft February 20, 2024 19:33

ohader force-pushed the 10350-intersections-5.x branch from e1d12f8 to 93e2c3e Compare February 20, 2024 19:47

ohader force-pushed the 10350-intersections-5.x branch from 46c5e35 to 14da91a Compare February 20, 2024 22:34

ohader force-pushed the 10350-intersections-5.x branch from 14da91a to f80f840 Compare February 20, 2024 22:46

ohader closed this Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid exceptions on fetching uninitialized names in ClassLikes#10730

Avoid exceptions on fetching uninitialized names in ClassLikes#10730
ohader wants to merge 3 commits intovimeo:5.xfrom
ohader:10350-intersections-5.x

ohader commented Feb 20, 2024 •

edited

Loading

Uh oh!

ohader commented Feb 20, 2024

Uh oh!

simonberger commented Feb 20, 2024

Uh oh!

ohader commented Feb 20, 2024

Uh oh!

simonberger commented Feb 20, 2024

Uh oh!

ohader commented Feb 20, 2024

Uh oh!

ohader commented Feb 20, 2024

Uh oh!

ohader commented Feb 20, 2024 •

edited

Loading

Uh oh!

ohader commented Feb 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ohader commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ohader commented Feb 20, 2024

Uh oh!

simonberger commented Feb 20, 2024

Uh oh!

ohader commented Feb 20, 2024

Uh oh!

simonberger commented Feb 20, 2024

Uh oh!

ohader commented Feb 20, 2024

Uh oh!

ohader commented Feb 20, 2024

Uh oh!

ohader commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ohader commented Feb 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ohader commented Feb 20, 2024 •

edited

Loading

ohader commented Feb 20, 2024 •

edited

Loading