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

[MODUSCarstenScholling]

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

[Arthurvdv]

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?

[MODUSCarstenScholling]

I'll need some days for internal stull. I'll come back to this in a couple of days.

[MODUSCarstenScholling]

Thanks for raising this. I revisited both the comment and pragma handling as part of this PR.

For the original example, deleting // legacy parameter, no longer required is still intentional. That comment describes the parameter being removed, so leaving it behind would either make it look as though it belongs to the next parameter or leave an orphaned comment in the signature.

The important nuance is that parser trivia ownership is not the same as semantic ownership. A comment may be attached to the removed parameter by the syntax tree even though it actually describes a nearby pragma directive. I now handle that explicitly:

• Comments that describe the removed parameter are removed with it.
• Comments immediately before a pragma directive that must be preserved are transferred together with that directive.
• Comments between the removed parameter and the next retained parameter remain with the retained parameter.
• Comments after a retained restore directive remain unchanged.

I considered KeepLeadingTrivia, KeepExteriorTrivia, and KeepDirectives, but they do not give me the required behavior with the AL syntax tree. They can attach trivia to the wrong parameter, leave formatting artifacts, or fail to retain leading pragma trivia. The fix therefore keeps using KeepNoTrivia for the parameter removal and handles the relevant directives and their comments explicitly.

I also addressed the pragma concern in this PR. The behavior is intentionally conservative:

• A balanced #pragma warning disable / restore pair is removed only when it exclusively covers parameters that are removed.
• A balanced pair that also covers a retained parameter stays in place. If its disable directive was attached to the removed parameter, it is moved to the next retained parameter.
• Pragmas that extend beyond the procedure, are unbalanced, or use different warning IDs are preserved. The fix does not try to infer or repair their intended suppression scope.

The Fix All tests now cover line and block comments around transferred pragmas, comments before and after the removed parameter, balanced pragma pairs around only removed parameters, pairs spanning removed and retained parameters, cross-procedure pairs, and unbalanced or mismatched directives.

[Arthurvdv]

@MODUSCarstenScholling Did you close this PR on purpose (to maybe create a new PR) or is this a mistake?

[MODUSCarstenScholling]

@Arthurvdv Sorry. I accidently deleted the branch ❌ It is back now and PR reopened.

[Arthurvdv]

Code Review — ParameterNotReferencedCodeFixProvider.cs

The overall PR architecture is solid — the LC0095/LC0099 split, custom FixAllProvider, annotation-based tracking, and test coverage are all well-structured. The pragma handling is the area with the most complexity risk.

Correctness

1. TakeWhile greedy pragma sweep (line 207)

The TakeWhile walks past the targeted pragma and keeps consuming adjacent trivia by SyntaxKind. If a non-targeted pragma immediately follows a targeted one in the same token's leading trivia, it gets swept into triviaToRemove.

Concrete scenario: a balanced AA0010 pair exclusively wraps a removed parameter, while an adjacent AA0005 disable begins the scope for a retained parameter. The #pragma warning restore AA0010 and #pragma warning disable AA0005 are adjacent in the next parameter's leading trivia. The TakeWhile matches the AA0005 pragma on its SyntaxKind.PragmaWarningDirectiveTrivia and deletes it, leaving the AA0005 restore unbalanced.

The existing test cases don't exercise adjacent targeted/non-targeted pragmas on the same token. Fix: add a pragmaSpans.Contains(trivia.Span) guard within the TakeWhile, or break the consumption into "take this one pragma + its trailing whitespace/EOL" rather than a greedy sweep.

2. String-based pragma error-code comparison (line 560)

GetPragmaErrorCodes returns a raw substring (pragma.ToString().Trim()[prefix.Length..].Trim()), so pragma pairing uses the literal text as a dictionary key. Multi-code pragmas with different spacing or ordering between disable and restore won't pair:

• #pragma warning disable AA0005, AA0006 vs #pragma warning restore AA0005,AA0006 (no space after comma) → strings differ, pair not detected
• Different code ordering between disable and restore → same problem

Low-impact since AL pragmas almost always have a single code, but using the structured PragmaWarningDirectiveTriviaSyntax.ErrorCodes API would be more robust.

Simplification

3. GetPragmaDirectives drops the structured API (line 563)

The .Where(trivia.ToString().Trim().StartsWith("#pragma warning")) filter after .OfType<PragmaWarningDirectiveTriviaSyntax>() is redundant — everything that passed the OfType is already a #pragma warning directive. Dropping to SyntaxTrivia via .Select(d => d.ParentTrivia) forces all downstream code (GetPragmaPairs, GetLocallyEnclosingPragmaPair, GetPragmaTransferPlan) to re-parse via string operations instead of using DisableOrRestoreKeyword and ErrorCodes properties directly.

4. GetSemanticModelIfNeededAsync dead abstraction (line 132)

Unconditionally fetches the semantic model and discards procedureKind via _ = procedureKind. The method name suggests conditional behavior that does not exist — it's a one-line wrapper that could be inlined as document.GetSemanticModelAsync(cancellationToken) at both call sites.

Efficiency

5. root.ToFullString() per parameter (line 509)

GetParameterIndentation calls root.ToFullString() on every invocation, allocating a string proportional to the entire document. It is called once per parameter inside TransferPragmas (line 460) and TransferPragmasToClosingParen (line 495). Hoisting the source string above the loops in RemoveParameters and passing it down would avoid O(N) full-document string allocations.

---

Generated with Claude Opus 4.8

[Arthurvdv]

Thanks, I've let the Code Review run and it comes with some remarks. If I can help and/or create a PR based on this into your branch let me know, happy to help!

[MODUSCarstenScholling]

Trivia is not trivial after all, especially when it can carry an entire inactive conditional branch.

Thanks for the detailed review. I addressed the findings and expanded the coverage around the directive-handling edge cases.

Correctness

1. Greedy pragma trivia removal

Confirmed. The previous cleanup could consume an adjacent preserved pragma directive when both directives were attached to the same token trivia list.

The cleanup now removes only the targeted pragma directive and its associated whitespace. A dedicated regression case covers an immediately adjacent removable pair followed by a preserved pair.

2. String-based warning-code comparison

Confirmed. I replaced the raw substring-based comparison with the structured PragmaWarningDirectiveTriviaSyntax API:

• DisableOrRestoreKeyword determines the directive kind.
• ErrorCodes is normalized as a case-insensitive, deduplicated, sorted set.

This handles reordered IDs, whitespace differences, and duplicate IDs. The tests cover reordered multi-code pragmas and duplicate warning-code entries.

3. Structured directive handling

Confirmed. GetPragmaDirectives now returns PragmaWarningDirectiveTriviaSyntax directly. The redundant text filter was removed, and the pairing logic no longer reparses directive text to determine disable, restore, or the warning-code list.

Inactive directives are also filtered via IsActive. The AL syntax tree exposes directives from inactive conditional branches through GetDirectives(), so this prevents inactive #if / #else branch pragmas from participating in pairing, removal, or transfer.

4. Semantic-model wrapper

Confirmed. The unused GetSemanticModelIfNeededAsync wrapper was removed. Both code paths now call document.GetSemanticModelAsync(...) directly.

5. Repeated full-document allocations

Confirmed. Parameter indentation is now derived from the parameter's leading trivia rather than from root.ToFullString(), removing the repeated document-sized allocation.

Additional robustness work

I also found and addressed a few related cases while extending the matrix:

• Nested pragma pairs with the same warning IDs are paired through a stack rather than positional matching.
• Only pairs wholly contained in one parameter list are eligible for removal.
• Pairs reaching a method body or another scope are preserved and transferred when needed.
• Multiple directives transferred to a closing parenthesis are inserted together in source order.
• Conditional parameters do not receive a CodeFix. A parameter can own #if, #else, #endif, and inactive branch text as trivia; removing it risks a destructive rewrite. The analyzer still reports the diagnostic, but no action is registered.
• Fix All scope isolation is covered for regular procedures and event subscribers in the same document.

Test coverage

The updated tests cover:

• Adjacent removable and preserved directives.
• Reordered, duplicate, empty, mismatched, and partially restored warning-code lists.
• Nested pairs with equal and different warning-code sets.
• Directives around previous, target, next, last, and all removed parameters.
• Scopes extending into method bodies or beyond the procedure.
• Line and block comments around transferred directives.
• Multiple methods with independent pragma pairs.
• LC0095 and LC0099 single-fix, Fix All, no-fix, and mixed-scope behavior.
• Active and inactive nested #if / #else / #endif branches.

[Arthurvdv]

Thanks for look into the feedback, it looks ready now. Let's get this merged for the upcoming v1.1.0 release.