Skip to content

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

Merged
ondrejmirtes merged 28 commits intophpstan:1.10.xfrom
staabm:e2e
Jan 14, 2024
Merged

Fix stale result cache for require-extends/require-implements#2866
ondrejmirtes merged 28 commits intophpstan:1.10.xfrom
staabm:e2e

Conversation

@staabm
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

staabm commented Jan 13, 2024

Thank you, I will have a look until monday

@staabm
Copy link
Copy Markdown
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
@ondrejmirtes
Copy link
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

3 participants