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

Fix stale result cache for require-extends/require-implements #2866

Merged
merged 28 commits into from Jan 14, 2024

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jan 11, 2024

@staabm staabm changed the title Fix stale result cache for Fix stale result cache for require-extends/require-implements Jan 11, 2024
@staabm staabm marked this pull request as ready for review January 11, 2024 20:27
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
../../bin/phpstan -vvv
mv src/Baz.php.orig src/Baz.php
echo -n > phpstan-baseline.neon
../../bin/phpstan -vvv
Copy link
Member

Choose a reason for hiding this comment

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

I tried result-cache-8 after removing changes from src/ and it does not fail for me locally. Please first submit a draft PR with just the E2E tests so it's obvious they fail.

Copy link
Contributor Author

@staabm staabm Jan 12, 2024

Choose a reason for hiding this comment

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

after having slept about it, I think the situation is as follows:

we only need to support require-extends for result-cache invalidation because thats the only new part in the reflection hierarchy.

require-implements works with reflection information which was available before, we just added some rules arround it.

see failling test
and the fix

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.

I just figured out when @phpstan-require-implements has to be handled in DependencyResolver.

/** @phpstan-require-implements Bar */
trait Foo
{
}

When the file with Bar changes, we need to reanalyse trait Foo, because the RequireImplementsRule might find some new error, or get rid of an existing error. Like for example when Foo changes type from interface to class and so on.

Please write a failing E2E test number 7 with this use-case, make sure it fails, then fix it in DependencyResolver. Thanks :)

@staabm
Copy link
Contributor Author

staabm commented Jan 13, 2024

Thank you, I will have a look until monday

@staabm
Copy link
Contributor Author

staabm commented Jan 14, 2024

took me a few attempts.. :-/.

failling test in https://github.com/phpstan/phpstan-src/actions/runs/7518819436/job/20466608449?pr=2866

I think its ready for another round of review

@ondrejmirtes ondrejmirtes merged commit 38eb365 into phpstan:1.10.x Jan 14, 2024
426 of 428 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

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