Repository navigation
[dx] Report rule-scoped skips that matched a file but never changed it - #8853
Merged
Merged
Conversation
reportUnusedSkips() so far only flagged a skip whose path never matched any file. A skip can also match a file while the rule would not have changed it anyway - the clone-probe used to catch this per node, removed for performance. On a dry run, re-run each rule-scoped skip on its exact file with skipping off; if the rule still does not change the file, report the skip as unnecessary. Paid once per skipped file, only when reporting is enabled.
TomasVotruba
changed the title
…runs The verifier re-processes skipped files only to read which rules would change them. Building its own dry-run Configuration keeps FileProcessor from writing the skipped file, so the check is safe outside a dry run too. Drop the dry-run gate in ProcessCommand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
->reportUnusedSkips()so far only flags a skip whose path never matched any file. But a skip can also match a file while the rule would not have changed it anyway - a genuinely unnecessary skip. That precision used to come from the per-node clone-probe inAbstractRector::enterNode(), removed in #8560 for performance (it ranrefactor()on a deep clone of every skipped node).What
This restores the detection, paid once at end-of-run instead of per node:
Skipperrecords the real file paths skipped per rule-scoped path skip, but only whenreportUnusedSkips()is on, so normal runs pay nothing.NeededSkipVerifierre-runs each skipped file through the full rule set with skipping off (a transient flag onSkipper), and reads back which rules actually changed the file. A skipped rule that is not among them never prevented a change, so its skip is reported as unnecessary.MissConfigurationReporter::reportUnnecessarySkips()prints them grouped under their rule, next to the existing never-matched report.Scope and guards
Configuration, so it detects the change but never writes the skipped file - safe on apply runs too.Capture happens in the main process, so coverage is single-process runs; parallel workers are a possible follow-up.