phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

Add --only-remove-errors analyse command option

Open N-Silbernagel opened this issue 10 months ago • 8 comments

Following the discussion I created, this is my attempt at adding a command line option for ignoring new errors to trim down the baseline file.

This enables the following workflow:

  1. Make changes to a codebase. This might add new errors but solve existing, ignored errors as well
  2. Run Analysis, displaying some new errors mixed with unmatched ignored errors
  3. Run --generate-baseline --ignore-new-errors to remove the fixed ignored errors from the baseline

This way, the unmatched ignored errors are removed from the baseline and from the analysis output, allowing me to focus on the new ones.

As a first step, I wanted to open this PR to validate the idea, I'm not 100% confident in the changes I made and their consequences.

The biggest change here is, that I changed configuration loading in such a way, that the baseline configuration would be loaded, even when generating the baseline. That means, that we need to remove the ignoreErrors configuration in the "IgnoredErrorHelperResult". I hope this is the only place where that configuration change effects the code, the tests seem to support it. There is now more "argument drilling" than before and the Fixer command needs to pass the baseline and ignoreNewErrors arguments, which I have set to false on that part for now, because I don't think the fixer uses baselines.

I'd be happy to hear some early feedback on this.

N-Silbernagel avatar Mar 02 '25 17:03 N-Silbernagel

A problem I just realized while creating the PR: ignoreError configurations done outside of the baseline would now be put in the baseline as well, which we probably want to avoid.

N-Silbernagel avatar Mar 02 '25 17:03 N-Silbernagel

Also noticed ci failing because a bigger baseline is generated, looking into it

N-Silbernagel avatar Mar 02 '25 17:03 N-Silbernagel

This needs a different approach.

  1. Remove all the changes around config loading etc. because they need to stay the same, otherwise we're risking unintended side effects.
  2. BaselineNeonErrorFormatter and BaselinePhpErrorFormatter need to be aware of the new CLI option user passed in, and also about the baseline file name currently being overwritten.
  3. BaselineNeonErrorFormatter already does that to a degree, to preserve the number of newlines at the end of the file.
  4. Based on the current file and the fact that the user only wants to remove errors, both BaselinePhpErrorFormatter and BaselineNeonErrorFormatter could implement this logic, without changing anything else in PHPStan.

ondrejmirtes avatar Mar 07 '25 09:03 ondrejmirtes

hey @ondrejmirtes thank you for the feedback!

That's the approach im currently trying as well. Im basically going through IgnoredErrorHelperResult again with only the ignores from the baseline. One Problem I am having is how to best load only the baseline config. I'm currently loading it in the CommandHelper, because the Loader is available at that point. I'm thinking maybe we could add the loader to the DI Container to use that in the generateBaseline method, I am not sure yet though. Will report back with a new version of the chnges.

N-Silbernagel avatar Mar 07 '25 09:03 N-Silbernagel

I don't think we need to touch IgnoredErrorHelperResult or CommandHelper.

AnalyseCommand should just pass the information about the file and the option to the baseline formatters. Nothing else should be touched.

ondrejmirtes avatar Mar 07 '25 10:03 ondrejmirtes

Tried a new approach. I did not add the code the baseline formatters but to the generateBaseline function of AnalyseCommand as to not duplicate the logic. To avoid side effects, the code is only run when the new cli option is passed.

I'm not quite happy with the structure of the tests but found them to be quite tricky to get right because the File names need to stay the same while changing the content. Needed some ignores, which is probably not ideal.

Looking forward to hear your feedback on the code changes.

N-Silbernagel avatar Mar 13 '25 19:03 N-Silbernagel

This pull request has been marked as ready for review.

phpstan-bot avatar Mar 13 '25 19:03 phpstan-bot

Thanks for the review.

Extracted the new logic into BaselineIgnoredErrorHelper, which I added to the config. Deleted the old integration tests to replace them with unit tests.

N-Silbernagel avatar Mar 20 '25 20:03 N-Silbernagel