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

Reprocess missing class-like storage events #10724

Closed
wants to merge 4 commits into from

Conversation

ohader
Copy link
Contributor

@ohader ohader commented Feb 19, 2024

The scenario described in #10350 fails, since not all other references used in intersection types were complete resolved when scanning a particular file. Instead of catching exceptions during the type resolution process - which would just "mute" the problem and drop important information - those failures in resolving class-like storages are tracked for a potential rescanning.

This PR does the following:

  • introduces a new ClassStorageNotFoundException (instead of global InvalidArgumentException) to handle missing class-like storage references specifically
  • tracks those storage failures during the scanning process in $files_to_rescan for later processing
  • adds bool return type to \Psalm\Internal\Codebase\Scanner::scanAPath() - whether scan was successful
  • modifies Codebase::scanFiles() to actually reprocess those files to be rescanned - after changes of the previous run were populated to the codebase
  • adds test cases for the known scenario of issue Internal Psalm Error: Could not get class storage for <interface> #10350

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


TODO

  • tackle \Psalm\Internal\Codebase\Scanner::scanAPath() return type in forked processes - currently only tested with $pool_size = 1 in test cases

In case not all class storage items were available during
a given file scan, those failed files will be rescanned
later with more populated details in the codebase.

There's a maximum of 10 rescans to avoid endless recursions.
@ohader

This comment was marked as off-topic.

@weirdan
Copy link
Collaborator

weirdan commented Feb 19, 2024

introduces a new ClassStorageNotFoundException (instead of global InvalidArgumentException) to handle missing class-like storage references specifically

It was literally my next step 😁 :

image

@weirdan
Copy link
Collaborator

weirdan commented Feb 19, 2024

tracks those storage failures during the scanning process in $files_to_rescan for later processing

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.

@ohader
Copy link
Contributor Author

ohader commented Feb 19, 2024

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 would have tested if this attempt makes a difference for the crash I still have, but unfortunately you branched it from psalm 6 instead of 5 which I not yet are able to install.

@ohader
Copy link
Contributor Author

ohader commented Feb 20, 2024

I would have tested if this attempt makes a difference for the crash I still have, but unfortunately you branched it from psalm 6 instead of 5 which I not yet are able to install.

Uh, sorry... I usually start working in main (master) and the back-port to stable branches. I forgot it's different here...
Let's close this PR (I can take care of the forward port later, once we found a good foundation code-wise).

5.x PR at #10730

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants