Skip to content

feat(LC0095, LC0099): split unreferenced-parameter rule into LC0095 and LC0099 - #425

Open
MODUSCarstenScholling wants to merge 1 commit into
ALCops:mainfrom
MODUSCarstenScholling:dev-cs-paramnotref-split+fixall
Open

feat(LC0095, LC0099): split unreferenced-parameter rule into LC0095 and LC0099#425
MODUSCarstenScholling wants to merge 1 commit into
ALCops:mainfrom
MODUSCarstenScholling:dev-cs-paramnotref-split+fixall

Conversation

@MODUSCarstenScholling

@MODUSCarstenScholling MODUSCarstenScholling commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR splits the previous unreferenced-parameter behavior into two diagnostics with clear scope boundaries:

  • LC0095: regular non-local procedures (internal/public), severity Warning
  • LC0099: event subscribers, severity Info

It also keeps a shared code fix implementation and updates tests and documentation accordingly.

What changed

  • Analyzer:

    • Added LC0099 descriptor and routing in ParameterNotReferenced analyzer
    • Kept LC0095 for non-subscriber paths only
  • Code fix:

    • Shared provider handles both LC0095 and LC0099
    • Distinct equivalence keys for regular procedures vs event subscribers
    • Custom Fix All kept and refined for shared separated-list edits
    • Preserved fallback behavior for empty fixAllSpans document scope
  • Tests:

    • Added/updated subscriber-specific diagnostic and fix coverage
    • Added/updated fix-all coverage for both scopes
    • Renamed cases for clearer intent and consistency

Why

AA0137 does not cover this full space:

  • It handles local procedures only
  • Event subscribers need separate guidance and a lower-friction severity

Splitting the diagnostics improves clarity, allows better policy tuning, and keeps fix behavior deterministic.

Validation

  • Focused tests for ParameterNotReferenced pass
  • Solution test run passes in the current workspace
  • CI-targeted compatibility behavior for multi-target builds remains respected

Breaking changes

No runtime breaking changes.
Diagnostic behavior changes:

  • Event subscriber findings now report as LC0099 instead of LC0095.

Implements #426

…nd LC0099

Separate unreferenced parameter diagnostics by procedure kind:
- Keep LC0095 for regular non-local procedures (internal/public)
- Introduce LC0099 for event subscribers (Info severity)

Implement a shared code fix provider for both diagnostics:
- Keep scoped equivalence keys for regular procedures and event subscribers
- Use custom Fix All with one-pass RemoveNodes on separated parameter lists
- Keep fallback behavior when fixAllSpans is empty in document scope

Improve maintainability and consistency:
- Align naming in tests and fix-all scenarios
- Keep netstandard2.1 compatibility behavior intact

Expand and update test coverage:
- Add and adjust diagnostic, single-fix, and fix-all cases for both scopes
- Verify focused ParameterNotReferenced test suite passes
@Arthurvdv

Arthurvdv commented Aug 18, 2026

Copy link
Copy Markdown
Member

Code review

Reviewed the LC0095/LC0099 split, the CodeFix changes, and the new FixAll implementation. No significant issues found in the core implementation. Summary of what was verified:

Builds & tests

  • Compiles clean on net10.0, net8.0, and netstandard2.1 (against the oldest pinned SDK, v12.0.13).
  • All 26 ParameterNotReferenced tests pass, including the new HasFixAll batch cases; full LinterCop suite passes (319/319).

FixAll correctness

  • Confirmed via decompilation that FixAllProvider.Create(...) exists in all shipped SDK versions, so replacing WellKnownFixAllProviders.BatchFixer won't break CI.
  • The fixAllSpans.HasValue && !IsDefaultOrEmpty guard with fallback to GetDocumentDiagnosticsAsync is required and correct: the SDK's FixAllState.GetFixAllSpansAsync always returns empty span arrays for Document/Project/Workspace scopes.
  • Cross-ID leakage (LC0095 spans picked up during an LC0099 fix-all, or vice versa) is correctly neutralized by the semantic IsEventSubscriber() filter keyed off CodeActionEquivalenceKey.

Plumbing

  • LC0099 is consistently wired through DiagnosticIds, DiagnosticDescriptors (Design/Info/help URI), all four resx entries, SupportedDiagnostics, and FixableDiagnosticIds. Skip logic (IsObsolete, handler/callback/trigger/interface exclusions) preserved.

Question: is deleting the adjacent comment intentional?

In HasFix/RemoveMiddleParameterMultiline, the comment // legacy parameter, no longer required is leading trivia of the removed parameter. The fix uses SyntaxRemoveOptions.KeepNoTrivia, so the comment is deleted along with the parameter, and expected.al asserts this.

Was this a deliberate choice? Our view is that a code fix shouldn't silently delete user comments; the developer should decide whether a comment is still relevant. If you agree, this needs a change (e.g., SyntaxRemoveOptions.KeepLeadingTrivia or KeepExteriorTrivia) plus updated fixtures. If it was intentional, we'd like to hear the reasoning.

Related: directive trivia (#pragma)

KeepNoTrivia also drops directives. If a #pragma warning disable sat on the removed parameter, the fix could silently unbalance a pragma pair. SyntaxRemoveOptions.KeepDirectives exists for exactly this scenario, and there's currently no test covering it. Is this something we should address in this PR (possibly combined with the trivia change above), or track as a follow-up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants