fix(FC0004): regression fixtures for permission lists with compiler directives; CodeFix directive policy

.claude/rules/codefix-development.md

@@ -115,7 +115,7 @@ Key details:
 - **Delegate signature.** `FixAllProvider.Create(...)` requires `Task<Document?>` (nullable). Returning `Task<Document>` compiles but binds to a different overload path and can misbehave.
 - **`Optional<ImmutableArray<TextSpan>>` quirk.** In the AL SDK, `fixAllSpans.HasValue` can be `true` while `fixAllSpans.Value.IsDefaultOrEmpty` is also `true` (observed with RoslynTestKit's default document-scope FixAll). Always guard with `!IsDefaultOrEmpty` and fall back to `fixAllContext.GetDocumentDiagnosticsAsync(document)` so FixAll works both in VS Code and in tests.
 - **One-pass rewrite for `SeparatedSyntaxList`.** Use `root.RemoveNodes(collection, SyntaxRemoveOptions.KeepNoTrivia)` (plural). It handles separator removal correctly across siblings in the same list, whereas per-diagnostic `ReplaceNode` calls conflict.
-- **Trivia handling.** For node removals, prefer `SyntaxRemoveOptions.KeepNoTrivia` so multi-line signatures do not leave dangling comments or blank continuation lines. This also removes directives attached to the node; if a directive can be paired outside the removed node, explicitly preserve, transfer, or remove its matching directive before the node rewrite. See `ParameterNotReferencedCodeFixProvider` for a parameter-list implementation.
+- **Trivia handling.** For node removals, prefer `SyntaxRemoveOptions.KeepNoTrivia` so multi-line signatures do not leave dangling comments or blank continuation lines. This also removes directives attached to the node; if a directive can be paired outside the removed node, explicitly preserve, transfer, or remove its matching directive before the node rewrite. See `ParameterNotReferencedCodeFixProvider` for a parameter-list implementation and "Compiler directives and disabled text" below for the policy.
 - **Scope filter via `CodeActionEquivalenceKey`.** Register multiple `CodeAction`s with distinct `EquivalenceKey`s if a rule needs "Fix all of kind X" variants. Read `fixAllContext.CodeActionEquivalenceKey` inside `FixAllAsync` to know which action the user invoked.
 - **Single-fix path stays consistent.** Use `root.RemoveNode(node, SyntaxRemoveOptions.KeepNoTrivia)` (singular) for the individual quick-fix so single and Fix-All behave identically.
 
@@ -133,9 +133,29 @@ Copy the nearest existing provider rather than inventing a transformation:
 | Add `Locked = true` to a label | `EmptyCaptionLocked`, `LabelWithTokSuffixMustBeLocked` |
 | Remove siblings from a `SeparatedSyntaxList` with FixAll | `ParameterNotReferencedCodeFixProvider` |
 | Insert a statement before another, using the semantic model | `UsePartialRecordsOnRead` |
+| Move or remove nodes that may carry `#if` / `#pragma` / `#region` trivia | `PermissionDeclarationOrderCodeFixProvider`, `ParameterNotReferencedCodeFixProvider` |
 
 Always build the replacement from the existing nodes and tokens (`WithTriviaFrom`, keep the receiver expression) and guard every navigation step: a missing optional node returns the unchanged document instead of throwing.
 
+## Compiler directives and disabled text
+
+**Policy: preserve, else bail out.** A fix keeps every directive (`#if`/`#elif`/`#else`/`#endif`, `#pragma`, `#region`/`#endregion`, `#define`/`#undef`) and any disabled text inside the span it rewrites, removes or moves. When it cannot, it does not register: check in `RegisterCodeFixesAsync` so no lightbulb appears (LC0095's `HasConditionalDirective`), or return the unchanged document. Deleting a directive is a correctness bug. `#if` decides what compiles, `#pragma warning` decides which warnings are suppressed, and AppSource apps use both to guard obsoleted objects (`#if not CLEAN25` plus `#pragma warning disable AL0432`). Record a per-fix deviation in the rule doc's CodeFix table.
+
+**Where directives live.** A directive takes its own line and is *leading trivia of the next token*. So a directive after the last element of a list sits on the closing token (`;`, `)`, `}`) or on the next sibling, outside the node being edited, and one before the first element belongs to that element. An inactive `#if` branch is a single `DisabledTextTrivia`: the "nodes" in it do not exist in the tree, so counts, `SeparatedSyntaxList` indexes and "is this the last entry" checks see fewer elements than the source shows. Tests define no preprocessor symbols, which is why `#if CLEAN25` yields disabled text and `#if not CLEAN25` yields real nodes.
+
+**What silently drops them:**
+
+- `WithoutTrivia()`, `WithLeadingTrivia(SyntaxFactory.TriviaList())`, or `WithTriviaFrom(other)` on a node that carried directives.
+- Rebuilding a list or node from `SyntaxFactory` (FC0004's `BuildMultiLinePermissionValue` strips every entry's trivia; it is safe there only because the analyzer never reports lists with directives).
+- `RemoveNode(s)` with `KeepNoTrivia`, which drops every directive in the removed node's trivia. `KeepUnbalancedDirectives` keeps `#define`/`#undef` and those `#if`/`#region` directives whose partners (`GetRelatedDirectives`) are not all inside the removed span, and still drops `#pragma`. `KeepDirectives` keeps all of them. Source: `SyntaxNodeRemover.AddDirectives` in `Microsoft.Dynamics.Nav.CodeAnalysis.Syntax/SyntaxNodeRemover.cs` (same logic at 12.0 and 18.x).
+- A `SourceText.WithChanges` edit, or `SyntaxFactory.Parse*` output, over a span that contains directive lines.
+
+**Detection.** `node.ContainsDirectives` / `token.ContainsDirectives` is the cheap gate. `trivia.IsDirective`, `trivia.GetStructure() is ConditionalDirectiveTriviaSyntax` (`#if`/`#elif`) or `is PragmaWarningDirectiveTriviaSyntax`, `node.GetDirectives(filter)` / `GetFirstDirective` and `DirectiveTriviaSyntax.IsActive` identify them. All of these, and `SyntaxRemoveOptions.KeepDirectives` / `KeepUnbalancedDirectives`, are `yes` at every column of the nav-sdk-docs reference tables, including `ns2.0 12.0`, so they need no guard. The `SyntaxKind` values (`DisabledTextTrivia`, `IfDirectiveTrivia`, ...) exist at every version but their ordinals move, so compare them through `EnumProvider.SyntaxKind`. `EnumProvider` exposes `PragmaWarningDirectiveTrivia`, `RegionDirectiveTrivia` and `EndRegionDirectiveTrivia`; add any other kind you need there.
+
+**Preserving.** Move trivia explicitly and keep the directives in order. FC0004 lifts `#region` runs into a group tree and re-emits them where each group now starts and ends. LC0095 pairs `#pragma warning disable`/`restore`, deletes a balanced pair that only wrapped removed parameters, and transfers the others to the next remaining parameter or to the `)`. The nav-sdk-docs page `docs/60-code-fixes/syntax-editor-and-editing.md` covers `SyntaxRemoveOptions` and the `KeepDirectives` pitfall.
+
+**Fixtures.** A fix that removes, moves or rebuilds nodes ships the directive fixture set in `testing.md` (Testing Code Fixes).
+
 ## Passing data from analyzer to CodeFix via diagnostic properties
 
 When the CodeFix needs information computed by the analyzer (e.g. a replacement name), the analyzer passes it through `ImmutableDictionary<string, string>` properties on the diagnostic. **Always use the `CodeFixProperties` record pattern** below. Do not use raw dictionary lookups, `out` parameters, or magic strings.

.claude/rules/diagnostics/fc0004-permission-declaration-order.md

@@ -32,7 +32,7 @@ Registers `RegisterCompilationAction`, one diagnostic per object on the `Propert
 
 ## Deliberate non-reports
 
-- Lists containing any non-region directive (`#if`, `#pragma`, ...) or unbalanced regions: AZ refuses to sort them, so no diagnostic and no fix.
+- Lists containing any non-region directive (`#if`, `#pragma`, ...) or unbalanced regions: AZ refuses to sort them, so no diagnostic and no fix. This covers both `#if` branches. In an active branch (`#if not X`) the entries are real nodes and the `#if`/`#pragma` lines are leading trivia of the next entry or of the `;`. In an inactive branch the entries are one disabled-text trivia, so the list has fewer entries than the source shows. The region tree refuses both, so the directive deletion the pre-AZ fix did when it rebuilt the whole list ([#454](https://github.com/ALCops/Analyzers/issues/454)) cannot come back.
 - An `#endregion` on the line after the `;` (a common hand-written layout) is outside the property, so the list counts as unbalanced and is never checked; AZ behaves the same because it does not pass the closing token for separated lists.
 - Entries with equal sort keys in any relative order: the sort is stable, so they are never reported.
 
@@ -57,3 +57,4 @@ Registers `RegisterCompilationAction`, one diagnostic per object on the `Propert
 | Empty `#region ... #endregion` pairs stay with the entry that carried them | An empty group has nothing to sort; as a tree node it would be flattened after the entries and drift to the end of the list. |
 | Single-line lists (no newline separators, no directives) are rewritten as multi-line via `BuildMultiLinePermissionValue` | Unchanged behaviour from the original rule. |
 | The fix bails out defensively on non-region directives or unbalanced regions | Mirrors the analyzer, which never reports such lists. |
+| Directives outside the list (on the line before `Permissions`, or after the `;`) are left alone | They are trivia of the property's own tokens or of the next sibling, and `PropertySyntax.WithValue` keeps both on the reorder path and on the single-line rebuild path. The `HasFix` directive fixtures pin this. |

.claude/rules/testing.md

@@ -123,6 +123,17 @@ HasFix files use current/expected pairs:
 
 `HasFix` / `HasFixAll` test methods and their `current.al` / `expected.al` layout: `.claude/skills/new-codefix/references/hasfix-tests.md` (used by `/new-codefix`). Key rule: `TestCodeFix` takes the `DiagnosticDescriptor` object, `TestFixAll` takes the ID string.
 
+### Directive fixtures (mandatory for fixes that remove, move or rebuild nodes)
+
+Such a fix ships a fixture for each of these around the edited node (or, where the node cannot carry one, around its closest sibling):
+
+1. an active `#if not CLEAN25 ... #endif`;
+2. an inactive `#if CLEAN25 ... #endif` (disabled text);
+3. a `#pragma warning disable ... restore` pair;
+4. a `#region ... #endregion` pair.
+
+Each one is a `HasFix` case whose `expected.al` keeps every directive line, a `NoFix` case (`fixture.NoCodeFix(code, descriptor)`, LC0095 layout) when the fix bails out, or a `NoDiagnostic` case when the analyzer itself declines (FC0004). Tests define no preprocessor symbols, so `#if CLEAN25` is disabled text and `#if not CLEAN25` keeps its contents as real nodes; the two exercise different trees. Confirm a directive `HasFix` case fails when `expected.al` omits one directive line, or it may pass for the wrong reason. The policy behind this is in `codefix-development.md` (Compiler directives and disabled text).
+
 ## Version-Conditional Test Skipping
 
 ### Skipping an entire test method (net8.0-only rules)

.claude/skills/new-codefix/SKILL.md

@@ -21,13 +21,14 @@ Work out the fix, then **stop and confirm before editing any file**:
 | FixAll: `BatchFixer` or custom `FixAllProvider`? | Custom when several diagnostics edit a shared ancestor node (`ParameterListSyntax`, property lists). |
 | Does the fix need analyzer data via `Diagnostic.Properties` (`CodeFixProperties`)? | If yes, the analyzer and its tests change in the same PR. |
 | Does the cop already reference `…CodeAnalysis.Workspaces.dll` and `System.Composition.AttributedModel.dll`? | `DocumentationCop` and `TestAutomationCop` currently have no `CodeFixes/`; adding one means adding these references to the `.csproj`. |
+| Which directives can occur inside the span the fix rewrites, removes or moves (`#region`, active `#if`, inactive `#if` = disabled text, `#pragma`), and what happens to each? | Preserve or bail out, never delete; name the fixtures that prove it. See `codefix-development.md` (Compiler directives and disabled text) and `testing.md` (Directive fixtures). |
 | Which nav-sdk-docs pages back the fix shape? | `/nav-sdk-docs:sdk-lookup` on the `SyntaxEditor`, `CodeAction` or `FixAllProvider` member you intend to use; the `docs/60-code-fixes/` pages name the pitfalls (`FindNode` ties, trivia, `BatchFixer` merge semantics). |
 
 ## Steps
 
 1. **Resx:** add `{RuleName}CodeAction` (the fix title) to `ALCops.{Cop}Analyzers.resx`.
 2. **Provider:** create `src/ALCops.{Cop}/CodeFixes/{RuleName}CodeFixProvider.cs` from `references/codefix-template.md`; `FixableDiagnosticIds` from `DiagnosticDescriptors.{RuleName}.Id`; preserve trivia; compare AL names via `SemanticFacts`; use `SyntaxFactory` per the reference section in `codefix-development.md`.
-3. **Tests:** `HasFix/{Case}/current.al` + `expected.al` and the `HasFix` method from `references/hasfix-tests.md`; add `HasFixAll` with ≥2 markers on sibling nodes when the answer to the FixAll question was "custom".
+3. **Tests:** `HasFix/{Case}/current.al` + `expected.al` and the `HasFix` method from `references/hasfix-tests.md`; add `HasFixAll` with ≥2 markers on sibling nodes when the answer to the FixAll question was "custom". A fix that removes, moves or rebuilds nodes also gets the directive fixture set (`testing.md`, Directive fixtures).
 4. **Run:** `dotnet build ALCops.sln`; `dotnet test src/ALCops.{Cop}.Test/ --filter "FullyQualifiedName~{RuleName}"`. Report real output. Then `/code-review` on the branch (`REVIEW.md` carries the code-fix checklist); fix or justify every correctness finding.
 5. **Document:** append `## CodeFix: {RuleName}CodeFixProvider` with a Decision | Rationale table (fix shape, FixAll choice, trivia handling, intentionally unfixed cases) to the rule doc.
 6. Commit `feat({ID}): add CodeFix …` on a `feat/` branch.
@@ -45,5 +46,6 @@ Work out the fix, then **stop and confirm before editing any file**:
 | Dropping a qualified receiver (`Rec.`, `Customer.`) when rewriting an invocation (#441 PC0035) | Rewrite only the member/arguments; keep the receiver expression and its trivia. |
 | A fix that assumes a `MemberAccessExpressionSyntax` receiver and returns the document unchanged on bare self or `this` | Handle the receiver-less `IdentifierNameSyntax` / `InvocationExpressionSyntax` shape too, and add `*BareSelf*` and `*ThisSelf*` `HasFix` cases whenever the diagnostic can fire on those forms (`receiver-forms.md`). |
 | Rewriting a node that may be missing (unblocked `then` branch, #398 PC0035) | Guard every navigation step; return the unchanged document instead of throwing. |
+| Stripping or rebuilding trivia (`WithoutTrivia`, `WithLeadingTrivia(TriviaList())`, `RemoveNode(…, KeepNoTrivia)`, a `SyntaxFactory`-built list) on a node that may carry directives (#454 FC0004) | Gate on `ContainsDirectives`, preserve or bail out, and add the directive fixtures. |
 | Comparing AL identifiers with `StringComparison.OrdinalIgnoreCase` | `SemanticFacts` name comparison. |
 | Forgetting the rule doc's CodeFix section | Step 5 is part of "done". |

REVIEW.md

@@ -4,7 +4,7 @@ Two parts. The first is specific to this repository and names the `.claude/rules
 
 ## ALCops house rules
 
-Severity: a finding is a **correctness** finding when it changes what a rule reports, whether the analyzer loads in `alc` or the editor, or whether the netstandard2.1 build compiles. Findings about the `.claude/` documentation contract are nits unless the change adds a rule without its rule doc.
+Severity: a finding is a **correctness** finding when it changes what a rule reports, what a CodeFix produces, whether the analyzer loads in `alc` or the editor, or whether the netstandard2.1 build compiles. Findings about the `.claude/` documentation contract are nits unless the change adds a rule without its rule doc.
 
 | Check | Why | Reasoning lives in |
 |---|---|---|
@@ -17,6 +17,7 @@ Severity: a finding is a **correctness** finding when it changes what a rule rep
 | Mutable instance fields; accumulate-here report-there patterns; per-invocation registrations where a per-body registration would do | Instances are shared across passes and projects; per-file passes see one file; cost is paid per callback. | `.claude/rules/sdk-analysis-scope.md`, `.claude/rules/analyzer-performance.md` |
 | A C# feature or SDK member that does not exist on `netstandard2.1` used without `#if NETSTANDARD2_1` or a version gate; a change that was not built for all three target frameworks | Local builds target `net10.0` only; the CI matrix builds `netstandard2.1;net8.0;net10.0` and analyzers evaluate one compilation at a time. | `.claude/rules/netstandard21-compatibility.md` |
 | Tests: `[TestCase("X")]` without `X.al`; `NoDiagnostic` fixtures without `[|...|]` markers; `expected.al` that still contains markers; `TestCodeFix` given an id string or `TestFixAll` given a descriptor; a `this` fixture without `SkipTestIfVersionIsTooLow`; a fixture that does not compile without the `InDocumentWithErrors` arrangement | Each of these passes for the wrong reason or fails with an unrelated message. | `.claude/rules/testing.md` |
+| A CodeFix that strips or rebuilds trivia (`WithoutTrivia`, `WithLeadingTrivia(SyntaxFactory.TriviaList())`, `RemoveNode(…, KeepNoTrivia)`, a `SyntaxFactory`-built replacement list) or applies a text edit on a span that can contain `#if`, `#pragma` or `#region` lines, without a `ContainsDirectives` gate or explicit re-attachment; a fix that removes, moves or rebuilds nodes without the directive fixtures | Directives are leading trivia of the token that follows them and an inactive `#if` branch is disabled-text trivia; dropping either silently changes what compiles or which warnings are suppressed. | `.claude/rules/codefix-development.md` (Compiler directives and disabled text), `.claude/rules/testing.md` |
 | A new rule without `DiagnosticIds`, the three resx entries, a `DiagnosticDescriptors` entry with the `alcops.dev` help URI, and `.claude/rules/diagnostics/{id}-{slug}.md`; a new CodeFix without the rule doc's `## CodeFix` section; a changed design decision without its rule-doc row | The rule doc is the contract false-positive triage reads first; the help URI is what users click. | `CLAUDE.md` (Keeping `.claude/` in sync), `.claude/skills/new-analyzer/references/rule-doc.md` |
 | `ALCopsSettings.cs` changed without `alcops.schema.json` (or the reverse) | A parity test enforces it; the finding saves a CI round-trip. | `.claude/rules/settings-schema.md` |
 | Issue or PR numbers in code comments, XML docs or `.claude/` general guides; measurements or "not yet implemented" phrasing in general guides | The repository keeps deep context in rule docs and GitHub, not in code; `Validate-Rules.ps1` rejects the stale-fact patterns. | `CLAUDE.md` (Workflow), `.claude/scripts/Validate-Rules.ps1` |

src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/PreserveDirectivesAroundProperty/current.al

@@ -0,0 +1,39 @@
+table 50100 Alpha
+{
+    Caption = '', Locked = true;
+    fields
+    {
+        field(1; MyField; Integer) { }
+    }
+}
+
+table 50101 Bravo
+{
+    Caption = '', Locked = true;
+    ObsoleteState = Pending;
+    ObsoleteReason = 'Replaced by table Alpha.';
+    ObsoleteTag = '25.0';
+    fields
+    {
+        field(1; MyField; Integer) { }
+    }
+}
+
+table 50102 Charlie
+{
+    Caption = '', Locked = true;
+    fields
+    {
+        field(1; MyField; Integer) { }
+    }
+}
+
+permissionset 50100 "My Permission Set"
+{
+    Assignable = true;
+#pragma warning disable AL0432
+    [|Permissions = tabledata Charlie = R,
+                  tabledata Bravo = R,
+                  tabledata Alpha = R|];
+#pragma warning restore AL0432
+}

src/ALCops.FormattingCop.Test/Rules/PermissionDeclarationOrder/HasFix/PreserveDirectivesAroundProperty/expected.al

@@ -0,0 +1,39 @@
+table 50100 Alpha
+{
+    Caption = '', Locked = true;
+    fields
+    {
+        field(1; MyField; Integer) { }
+    }
+}
+
+table 50101 Bravo
+{
+    Caption = '', Locked = true;
+    ObsoleteState = Pending;
+    ObsoleteReason = 'Replaced by table Alpha.';
+    ObsoleteTag = '25.0';
+    fields
+    {
+        field(1; MyField; Integer) { }
+    }
+}
+
+table 50102 Charlie
+{
+    Caption = '', Locked = true;
+    fields
+    {
+        field(1; MyField; Integer) { }
+    }
+}
+
+permissionset 50100 "My Permission Set"
+{
+    Assignable = true;
+#pragma warning disable AL0432
+    Permissions = tabledata Alpha = R,
+                  tabledata Bravo = R,
+                  tabledata Charlie = R;
+#pragma warning restore AL0432
+}