Do not invalidate cache during suppression

[ro0NL]

see #9918 for background

im not aware of psalm internals, but i tried this change first and it reduces runtime practically 100% for us when running psalm --set-baseline=psalm-baseline.xml with cache

I also cannot spot any side-effects still this may cause, eg. expected issues are still being written to, and removed from the baseline 😳

Im curious what tests do here with this change, hence a PR.

cc @orklah i must be missing something 😄

[ygottschalk]

Try rebasing this onto current master. Some failing test should be fixed there.

[ro0NL]

@ygottschalk thank you 🙏

[ro0NL]

            && !isset($options['set-baseline'])
            && !isset($options['update-baseline']);

i cant really rationalize this. i feel like it hacks the caching for whatever purpose. IMHO then --no-diff should be passed explicitly (ref #7681)

[orklah]

I fear Psalm may be unable to refresh or recreate the baseline unless it has analyzed your whole codebase. It would make sense at least. So whenever your ask it to do that, it can't just analyze files that changed or were removed and launch the whole analysis.

You may have to choose between having a squeaky clean baseline and a fast analysis

[ro0NL]

im not sure i follow, it seems psalm is able to recreate the baseline from cache just fine 🤔

i can confirm after rm psalm-baseline.xml psalm regenerates the same baseline file using vendor/bin/psalm --set-baseline=psalm-baseline.xml

[ro0NL]

i can also confirm after rm psalm-baseline.xml AND intentionally forcing an error (referenced an unknown class in src/ somewhere), the baseline is updated as expected

[ro0NL]

i can also confirm after fixing that error i get Baseline for issue "UndefinedClass" has 1 extra entry. (see https://psalm.dev/316) AND it's removed from the baseline file

[orklah]

There's two things that can reduce the time of Psalm's analysis:

• the cache (we store parsed files and also store method/class definitions)
• the diff option (Psalm will only check changed files instead of analyzing everything again)

Both can be controlled through --no-cache and --no-diff (though I think --no-cache implies --no-diff).

I think you may have misinterpreted Psalm using the --no-diff mode when you ask it to regenerate baseline vs Psalm using no cache at all.

Could you make a few tests with --no-diff, --no-cache and --set-baseline to see if it matches your expectations?

[ro0NL]

@orklah im afraid the matrix is too big to mentally coop for me 😿

(with matrix i mean: local/global install vs phar/src vs v5/master vs no-diff/no-cache)

I think --no-cache implies --no-diff

I think so. Perhaps this PR is about: Do not invalidate diff during suppression :)

I still believe the fix looks legit, ppl can opt-in to the previous behaviour by passing --no-diff explicitly.

I've now updated our global phar installation to master. Our main project runs 2 psalm instances/configs, for non-legacy/legacy respectively.

Command running:

psalm --no-progress --output-format=github --set-baseline=psalm-baseline.xml && ko=0 || ko=1; \
psalm --no-progress --output-format=github -c psalm-legacy.xml --set-baseline=psalm-legacy-baseline.xml && exit $ko

Some numbers (all without this PR);

global phar psalm v5 no-cache: 7-8m
global phar psalm v5 cached: 6-7m

global phar psalm master no-cache: 8-9m
global phar psalm master cached: 6-7m

At this point im assuming local/global install vs phar/src does not matter (or shouldnt ;))

For comparison, we also have 2 symfony kernels (for frontend/backend respectively) that we compile in 2 envs (prod/test respectively). All wiith APP_DEBUG=1 (so the cache is invalidated based on mtime), and it takes 2m roughly.

[ro0NL]

testing on my local machine with a local phar (vendor/bin/psalm.phar)

(all this is php7.4 btw)

local phar psalm master no-cache: 6-7m
local phar psalm master cached: 5-6m

at this point ive added continue-on-error: true for the psalm job in GHA, so eg. unit tests will run either way.

then for any commit to develop we just take >10m jobs for granted :)

[ro0NL]

You've used 100% of included services

🤣

@orklah any chance to get this merged? given --no-diff remains available to opt-in?

or do you suggest to not suppress large codebase realtime, but rather eg. daily?

[orklah]

@orklah any chance to get this merged? given --no-diff remains available to opt-in?

I went back four years and found this:
https://github.com/vimeo/psalm/blame/fe294a4dc08d72c89289fb40968421a7aed4066f/src/psalm.php#L684

So yeah, this was definitely by design, there must be something deep that disallow using diff mode while also reseting the baseline

[ro0NL]

@orklah, im still wondering about --set-baseline --no-diff v --set-baseline, and also why i need to repeat baseline file in --set-baseline, but that's a different story 😅

eventually i removed the psalm supression step from our GHA job, allow the psalm step to continue on error, disabled findUnusedBaselineEntry, and added daily psalm suppression to our backlog

gonna leave it at this :)

[orklah]

--set-baseline --no-diff v --set-baseline

There's no difference between those two commands. --set-baseline implies --no-diff.

We could have kept an error when using --set-baseline without --no-diff but it's noise

[ro0NL]

right, i meant --set-baseline --no-diff v --set-baseline --diff

so the TLDR here is; patching the baseline based on diff (aka fast) is not (yet) supported

[ro0NL]

global phar psalm master cached: 6-7m

related to #9884, ive cleaned up all the includes, so they are included once at front controller level

adding each function file as stub is still needed (it cannot be put in our composer.json due the non-legacy part of the project)

BUT this speeds up psalm suppression, im now seeing stable real 3m39,926s

still slow 🙄 but OK.

the side-effect is every function is loaded from stub, which ignores at least @deprecated at the function level. But we dont care, since IDE still strikes them.