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

Add support for result cache meta extensions #3765

Merged

Conversation

Wirone
Copy link
Contributor

@Wirone Wirone commented Jan 3, 2025

Important

This feature was brought to you thanks to GetResponse - Marketing Software for Professional Email Marketing, which paid for my work on this.

This introduces contract for extending the way how result cache metadata is calculated, so PHPStan extensions can take part into deciding whether analysis should be re-run or its result can be re-use from cache.

It is a base for phpstan/phpstan-symfony#255, which needs separate PR that introduces ResultCacheMetaExtension for Symfony DI container. It will be done when this gets merged (and released?).

@ondrejmirtes I need some help / your decisions:

  • I did not add any tests, as I don't see any tests for ResultCacheManager. Please point me how/if I should test this extension point. Just run make build and verified everything passes.
  • I've added phpstanExtensions entry to the array with metadata but I am not 100% satisfied with it. Maybe it should be metaExtensions or metaFromPhpstanExtensions or something different. Or maybe it shouldn't be nested, but merged with the main array? Let me know.
  • I've added this to ConditionalTagsExtension because after code analysis and seeing how e.g. DiagnoseExtension is implemented I've encountered such usage and thought ResultCacheManager should be there too. But it's a wild guess only, please let me know if I should keep it there.
  • Should any of existing ResultCacheManager::getMeta() items be converted to an extension? That could act as e2e test, but also could require some hacks in order to keep backward compatibility with existing result caches (depending on the scheme of the array with metadata).

@Wirone

This comment was marked as outdated.

@ondrejmirtes
Copy link
Member

is there a way to start working on phpstan/phpstan-symfony extension before this is merged and released

No need to do that. First we need to agree how it should work here, then merge it, then you can require ^2.1.1 in phpstan-symfony

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Really nice! You almost nailed it :)

If you wanted to add a test, it'd make sense to do an E2E test similar to what we do here:

- script: |
cd e2e/bug-9622-trait
echo -n > phpstan-baseline.neon
../../bin/phpstan -vvv
patch -b src/Foo.php < patch-1.patch
cat baseline-1.neon > phpstan-baseline.neon
../../bin/phpstan -vvv
mv src/Foo.php.orig src/Foo.php
echo -n > phpstan-baseline.neon
../../bin/phpstan -vvv

  1. Create an example project under e2e directory
  2. Create some sample result cache meta extension there that would return different hash based on something we'll change later
  3. Run PHPStan, Run PHPStan again with --fail-without-result-cache - should exit with 0
  4. Change the thing that will change the hash. Can be some txt file within the project.
  5. Run PHPStan again with --fail-without-result-cache, should error. You can assert the output with bashunit similarly to here
    OUTPUT=$(../bashunit -a exit_code "1" "../../bin/phpstan analyze --no-progress --level 8 --error-format raw data/")
    echo "$OUTPUT"
    ../bashunit -a line_count 2 "$OUTPUT"
    ../bashunit -a matches "Note: Using configuration file .+phpstan.neon." "$OUTPUT"
    ../bashunit -a contains 'Method class@anonymous/TestClassUsingTrait.php:20::doBar() should return stdClass but returns Exception.' "$OUTPUT"

Wirone added 4 commits January 7, 2025 12:04

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This introduces contract for extending the way how result cache metadata is calculated, so PHPStan extensions can take part into deciding whether analysis should be re-run or its result can be re-use from cache.
Did not think that new version of PHPStan invalidates result cache anyway.
@Wirone Wirone force-pushed the codito/result-cache-meta-extension branch 3 times, most recently from 23a4300 to 2584564 Compare January 7, 2025 11:11
@Wirone Wirone force-pushed the codito/result-cache-meta-extension branch from 2584564 to 041f6d0 Compare January 7, 2025 11:18
@Wirone Wirone requested a review from ondrejmirtes January 7, 2025 11:21
@Wirone
Copy link
Contributor Author

Wirone commented Jan 7, 2025

@ondrejmirtes rebased on top of 2.1.x and updated to follow your review. Thanks for the guidance, it was really helpful 🙂.

@ondrejmirtes ondrejmirtes merged commit 2d93b10 into phpstan:2.1.x Jan 7, 2025
427 checks passed
@ondrejmirtes
Copy link
Member

Thank you. Now you can send the portion in phpstan-symfony while requiring PHPStan ^2.1.2 :)

@ondrejmirtes
Copy link
Member

Docs: phpstan/phpstan@3fa1a7c

@Wirone Wirone deleted the codito/result-cache-meta-extension branch January 7, 2025 14:01
@Wirone
Copy link
Contributor Author

Wirone commented Jan 7, 2025

Thank you. Now you can send the portion in phpstan-symfony while requiring PHPStan ^2.1.2 :)

@ondrejmirtes is 2.1.2 going to be tagged soon? I would like to start working on Symfony extension 🙂.

@ondrejmirtes
Copy link
Member

You can put that requirement in composer.json in the extension now and it's going to download the current dev version.

@Wirone
Copy link
Contributor Author

Wirone commented Jan 7, 2025

Yeah, I just did it and was surprised it worked (Installing phpstan/phpstan (2.1.x-dev 3fa1a7c)) 😅. What kind of sorcery is this? Is this Composer's thing (like automatically take 2.1.x-dev if the constraint requires not released version and there's "minimum-stability": "dev") 🤔?

@ondrejmirtes
Copy link
Member

Yeah, probably something like that.

@AJenbo
Copy link
Contributor

AJenbo commented Jan 21, 2025

@Wirone thank you so so much for this it's going finally allow Bladestan to move to a 1.0. The timing is also perfect since I just finished implementing most other features this week :)

@ondrejmirtes
Copy link
Member

@AJenbo Please share how this extension helps you. Is it really a good fit?

@AJenbo
Copy link
Contributor

AJenbo commented Jan 21, 2025

Based on the description it sounds like a good fit. Once I get going with implementing it I will make sure to update you on how it works out.

@AJenbo
Copy link
Contributor

AJenbo commented Jan 21, 2025

@ondrejmirtes on a kind of related note I was wondering the other day if there could be a way to add extra context to the printout without having to duplicate the whole formatter and require users to run PHPStan with extra arguments. Having to duplicate the ErrorFormatter seems a bit silly and it keeps going out of sync meaning that people either loose out of improvements from PHPStan or extra context from Bladestan: https://github.com/bladestan/bladestan/blob/main/src/ErrorReporting/PHPStan/ErrorFormatter/BladeTemplateErrorFormatter.php#L146

@AJenbo
Copy link
Contributor

AJenbo commented Jan 21, 2025

@ondrejmirtes going over the docs I see that this is at a project level and not on a per file level. It would be much better for Bladestan if it can tell PHPStan that the meta for a specific file has changed and only that one needs to be re-analyzed.

Still being able to mark the cache as stale at all will solve the issue, even if the performance penalty is higher then it needs to be.

@ondrejmirtes
Copy link
Member

@AJenbo Yes, I suspected it's not a great fit. Changing the hash returned from the extension will invalidate the whole project which will be reanalysed again.

We could add another extension interface (or two) that would be a better fit for Bladestan. As you can see from this PR, it's not difficult at all.

Result cache uses two important mechanisms you might want to tap into:

  1. File dependency tree - Result cache keeps track of dependencies between files. When a function signature changes in one file, we have to know about all the files we have to reanalyse (the files that call the function). This is done in https://github.com/phpstan/phpstan-src/blob/2.1.x/src/Dependency/DependencyResolver.php. I suppose Bladestan introduces dependencies between files where this is not obvious to PHPStan. Like when a template file changes, we have to reanalyse the controller that renders the template. We could introduce "ResultCacheNodeDependencyExtension" to tap into that.

  2. Exported nodes - when a file hash changes, files dependent on it are not reanalysed without further consideration. For example, if you just change a method body, PHPStan doesn't have to reanalyse files that call this method. But if you change a public method signature, it does. This is done in https://github.com/phpstan/phpstan-src/blob/2.1.x/src/Dependency/ExportedNodeResolver.php. I suppose that template files are not like that. For example, when a template file starts using a new variable, you want to reanalyse its controller. A new extension "ResultCacheExportedNodesExtension" could do that.

on a kind of related note I was wondering the other day if there could be

This is completely unrelated to this PR, please open a separate discussion and provide the whole context. From this paragraph I have no idea what it's about.

Also, I'd love if more projects followed what Twigstan (https://github.com/twigstan/twigstan). It uses its own executable to drive how PHPStan is called, calls it multiple times, uses collectors heavily, and doesn't need to use any ugly hacks to achieve its goal. It'd be great if you got inspired by that :)

@AJenbo
Copy link
Contributor

AJenbo commented Jan 21, 2025

Ok, I will try and look in to it. I have a few other tasks to finish at work before I can get back to working on this.

We should also make sure that if a template is calling something that changed, a fresh analysis of the template and controller is triggered.

I'd love if more projects followed what Twigstan

I have looked at Twigstan previously, it is well made. One problem with how Twigstan does things is that it doesn't work with various IDEs that already have a PHPStan integration. Another is the time I have to dedicate to rewriting things, I didn't even intend to become the main maintainer for Bladestan :)

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.

None yet

3 participants