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

AnalyseApplication: Do not re-analyse stubs on every run #730

Closed
wants to merge 1 commit into from
Closed

AnalyseApplication: Do not re-analyse stubs on every run #730

wants to merge 1 commit into from

Conversation

dktapps
Copy link
Contributor

@dktapps dktapps commented Oct 23, 2021

Instead, only analyse them on full runs. Since a change in any stub already invalidates the whole result cache, this means we only need to analyse them on full runs and then remember their errors for later.

On pmmp/PocketMine-MP, this reduces the time to re-analyse with a result cache from ~1300ms to ~1000ms on an XPS 17 9710.

This is a rough shot at implementing phpstan/phpstan#5826.

Instead, only analyse them on full runs. Since a change in any stub already invalidates the whole result cache, this means we only need to analyse them on full runs and then remember their errors for later.
@dktapps
Copy link
Contributor Author

dktapps commented Oct 23, 2021

As an added bonus (?) this also fixes ignoring stub errors - on 0.12.99, they'd be added to auto-generated baselines, but PHPStan wouldn't pay any attention to the baseline. I'm not sure ignoring stub errors is desirable, but the way it works (or doesn't) on 0.12.99 is obviously broken, regardless of what it was supposed to do.

@dktapps
Copy link
Contributor Author

dktapps commented Oct 23, 2021

Build failure looks unrelated.

@ondrejmirtes
Copy link
Member

I've looked really hard, couldn't find a flaw in your logic, and almost merged it. But I've found it! This makes errors from stubs effectively ignorable which isn't desired.

I merged your commit as: d96e8f0

And added this: a1abfc3

@dktapps
Copy link
Contributor Author

dktapps commented Jan 31, 2022

Yeah I did notice this when I first implemented it, but I didn't think anything of it. Since they were able to end up in baseline but wouldn't get ignored anyway, I assumed it was a bug.

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