Bug Report
| Q |
A |
| BC Break |
no |
| Library Version |
1.1.1 |
| PHP version |
8.3 |
Summary
When using --ignore-baseline-linenumbers, adding a new violation to a class that already has other baselined violations of the same rule can cause Violations::remove() to flag the wrong line as "new" — while silently absorbing the actually-new violation as if it were already known. The report doesn't reflect what was really added/changed in the code.
Current behavior
Violations::remove() (src/Rules/Violations.php) matches current violations against baseline violations by (fqcn, normalized error message) only, ignoring the line number as the option name implies. Both lists are pre-sorted by fqcn then by line (Violations::sort(), called in Runner::doRun()), and the matching loop pairs them up positionally, in sorted order — not by stable identity.
This means: if a class already has N baselined violations of the same rule, and a new violation of the same rule is introduced at a line that sorts before one of the existing ones, the position-based pairing shifts and an existing (untouched) violation can end up unmatched and reported as "new", while the actual new one is paired away with an older baseline entry and never reported.
Concrete trace — class Foo already has 3 baselined violations of the same rule at lines 10, 20, 30. A 4th violation of the same rule is introduced at line 15 (between the first two):
- current run, sorted by line:
[10, 15(new), 20, 30]
- baseline, sorted by line:
[10, 20, 30]
Pairing proceeds index by index:
10 ↔ 10 → matched (correct)15 (new) ↔ 20 (pre-existing, untouched) → matched — the genuinely new violation is silently discarded as "already known"20 (pre-existing) ↔ 30 (pre-existing) → matched30 (pre-existing) ↔ nothing left → reported as the "new" violation
The tool reports line 30 — code that was never touched — as the new violation, while the line actually introduced (15) produces no output at all.
This is a different failure mode from #428/#430: that fix addressed baseline entries not being fully cleared when the violation set was unchanged. This one is about misattribution when the set does change, specifically when a new violation's line sorts earlier than existing siblings in the same fqcn.
How to reproduce
- Create a class with 3 dependency violations of the same rule (same fqcn, same rule message) at increasing line numbers, baseline them with
--generate-baseline.
- Insert a 4th violation of the same rule at a line before one of the already-baselined ones (e.g., between the 1st and 2nd).
- Run
check --use-baseline=... --ignore-baseline-linenumbers.
- Observe: the violation reported as new corresponds to a pre-existing, untouched line, not the line that was actually added.
Expected behavior
When ignoring baseline line numbers, adding a new violation among existing same-rule violations in the same class should always report the violation that was actually added, never an unrelated pre-existing one. Matching needs an identity scheme that's robust to both line movement (the whole point of --ignore-baseline-linenumbers) and to having multiple same-rule violations in one class (not purely positional/multiset pairing).
Possible directions (alternatives, not mutually exclusive)
Two ways to give violations a more stable identity than "raw line number", each with different trade-offs:
A. Hash of the source line content, used as part of the matching key instead of the line number: (fqcn, message, hash(line content)).
- Survives line movement, since it follows content rather than position.
- Still collides when two occurrences of the same violation have textually identical line content (e.g. the same call repeated verbatim in two different methods) — narrows the bug rather than eliminating it.
- Whitespace/formatting changes (e.g. a
csfix pass) would need to be normalized before hashing, otherwise unrelated formatting changes could invalidate baseline entries en masse.
- Needs a baseline format change; should be additive/optional to stay backward compatible with existing
baseline.json files.
B. Enclosing class member as part of the identity: (fqcn, message, enclosing method/property/constant name).
FileVisitor already knows which member it's inside while walking the AST when it records a dependency, so this reuses information already available rather than introducing new hashing.- Robust to formatting changes for free (member names aren't touched by
csfix).
- Distinguishes the common duplicate case (same dependency used in two different methods) even when the two call sites are textually identical, which (A) cannot.
- Still collides only in the narrower case of the same dependency appearing twice within the same member — arguably low-impact, since either occurrence is equivalent work to fix.
- As a side benefit, this information could also enrich the violation message itself (e.g. naming the method), independent of which matching fix is chosen.
C. Stop promising per-occurrence precision; match by group and report the whole group. This is what established tools in this space actually do, rather than attempting fuzzy per-occurrence identity:
- PHPStan's baseline matches by
(file, message) only and stores a count per pair; line numbers are kept for display but are not part of the matching key.
- ESLint's bulk suppressions (introduction post) work the same way, matching by
(file, rule) + count, and the docs say so explicitly: "there's no reliable way to determine whether the new violations were introduced recently or already existed" — so when the count for a group grows, ESLint reports all violations in that group rather than guessing which one is new.
- Applied here: when the count of violations for
(fqcn, message) exceeds what's in the baseline, report all current occurrences of that group, not a single best-effort guess. Strictly less helpful than true per-occurrence identity, but it never misattributes — it trades "point at exactly the right line" for "never silently point at the wrong one", which fits a tool that would rather lack a capability than be ambiguous.
D. Don't match against a static snapshot at all — diff against git. golangci-lint's --new-from-rev takes a structurally different approach: instead of reconciling current violations against a frozen baseline file, it considers a violation "new" if and only if it falls on a line that changed relative to a given git ref. This sidesteps the matching problem entirely — there's no fuzzy identity to get wrong, because the answer is read directly from git diff. It would be a bigger, complementary addition (needs git history available in CI, and a configured base ref) rather than a drop-in fix for Violations::remove(), and it doesn't replace the use case of a permanent baseline for debt that's deliberately never going to be fixed — but for the "did I just introduce this" question it answers unambiguously, which today's line/hash/member-based matching cannot.
Bug Report
Summary
When using
--ignore-baseline-linenumbers, adding a new violation to a class that already has other baselined violations of the same rule can causeViolations::remove()to flag the wrong line as "new" — while silently absorbing the actually-new violation as if it were already known. The report doesn't reflect what was really added/changed in the code.Current behavior
Violations::remove()(src/Rules/Violations.php) matches current violations against baseline violations by(fqcn, normalized error message)only, ignoring the line number as the option name implies. Both lists are pre-sorted byfqcnthen byline(Violations::sort(), called inRunner::doRun()), and the matching loop pairs them up positionally, in sorted order — not by stable identity.This means: if a class already has N baselined violations of the same rule, and a new violation of the same rule is introduced at a line that sorts before one of the existing ones, the position-based pairing shifts and an existing (untouched) violation can end up unmatched and reported as "new", while the actual new one is paired away with an older baseline entry and never reported.
Concrete trace — class
Fooalready has 3 baselined violations of the same rule at lines 10, 20, 30. A 4th violation of the same rule is introduced at line 15 (between the first two):[10, 15(new), 20, 30][10, 20, 30]Pairing proceeds index by index:
10↔10→ matched (correct)15(new) ↔20(pre-existing, untouched) → matched — the genuinely new violation is silently discarded as "already known"20(pre-existing) ↔30(pre-existing) → matched30(pre-existing) ↔ nothing left → reported as the "new" violationThe tool reports line 30 — code that was never touched — as the new violation, while the line actually introduced (15) produces no output at all.
This is a different failure mode from #428/#430: that fix addressed baseline entries not being fully cleared when the violation set was unchanged. This one is about misattribution when the set does change, specifically when a new violation's line sorts earlier than existing siblings in the same fqcn.
How to reproduce
--generate-baseline.check --use-baseline=... --ignore-baseline-linenumbers.Expected behavior
When ignoring baseline line numbers, adding a new violation among existing same-rule violations in the same class should always report the violation that was actually added, never an unrelated pre-existing one. Matching needs an identity scheme that's robust to both line movement (the whole point of
--ignore-baseline-linenumbers) and to having multiple same-rule violations in one class (not purely positional/multiset pairing).Possible directions (alternatives, not mutually exclusive)
Two ways to give violations a more stable identity than "raw line number", each with different trade-offs:
A. Hash of the source line content, used as part of the matching key instead of the line number:
(fqcn, message, hash(line content)).csfixpass) would need to be normalized before hashing, otherwise unrelated formatting changes could invalidate baseline entries en masse.baseline.jsonfiles.B. Enclosing class member as part of the identity:
(fqcn, message, enclosing method/property/constant name).FileVisitoralready knows which member it's inside while walking the AST when it records a dependency, so this reuses information already available rather than introducing new hashing.csfix).C. Stop promising per-occurrence precision; match by group and report the whole group. This is what established tools in this space actually do, rather than attempting fuzzy per-occurrence identity:
(file, message)only and stores a count per pair; line numbers are kept for display but are not part of the matching key.(file, rule)+ count, and the docs say so explicitly: "there's no reliable way to determine whether the new violations were introduced recently or already existed" — so when the count for a group grows, ESLint reports all violations in that group rather than guessing which one is new.(fqcn, message)exceeds what's in the baseline, report all current occurrences of that group, not a single best-effort guess. Strictly less helpful than true per-occurrence identity, but it never misattributes — it trades "point at exactly the right line" for "never silently point at the wrong one", which fits a tool that would rather lack a capability than be ambiguous.D. Don't match against a static snapshot at all — diff against git. golangci-lint's
--new-from-revtakes a structurally different approach: instead of reconciling current violations against a frozen baseline file, it considers a violation "new" if and only if it falls on a line that changed relative to a given git ref. This sidesteps the matching problem entirely — there's no fuzzy identity to get wrong, because the answer is read directly fromgit diff. It would be a bigger, complementary addition (needs git history available in CI, and a configured base ref) rather than a drop-in fix forViolations::remove(), and it doesn't replace the use case of a permanent baseline for debt that's deliberately never going to be fixed — but for the "did I just introduce this" question it answers unambiguously, which today's line/hash/member-based matching cannot.