From 4cc00b282c0abbcae21c28aaa74bd93151b65124 Mon Sep 17 00:00:00 2001 From: Morten Holt Date: Fri, 19 Jun 2026 14:14:23 +0200 Subject: [PATCH 1/7] Phase 1 (Build the shared always-aliased helper in `SyntaxFactoryHelper` (with semantic-model requalification)) completed --- .conducktor/SPEC.md | 171 ++++++++++++++++++ .../Helpers/SyntaxFactoryHelper.cs | 98 +++++++--- 2 files changed, 247 insertions(+), 22 deletions(-) create mode 100644 .conducktor/SPEC.md diff --git a/.conducktor/SPEC.md b/.conducktor/SPEC.md new file mode 100644 index 0000000..bebbcac --- /dev/null +++ b/.conducktor/SPEC.md @@ -0,0 +1,171 @@ +# Always generate aliased usings for plugin image registrations + +## Context + +The `XrmPluginCore.SourceGenerator` ships analyzers + code-fix providers that wire up type-safe Pre/Post image handler signatures. When a `RegisterStep(...)` registration declares an image via `WithPreImage`/`WithPostImage`/`AddImage` but the referenced handler method's signature does not match, two diagnostics/code-fixes come into play: + +- **`FixHandlerSignatureCodeFixProvider`** (`XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs`) — fixes an existing handler method's parameter list to accept the registered `PreImage`/`PostImage` wrapper types. Fires on `DiagnosticDescriptors.HandlerSignatureMismatch.Id` and `DiagnosticDescriptors.HandlerSignatureMismatchError.Id`. +- **`CreateHandlerMethodCodeFixProvider`** (`XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs`) — creates a missing handler method on the service interface with the correct image parameters. Fires on `DiagnosticDescriptors.HandlerMethodNotFound.Id`. + +Each registration's image wrapper classes (`PreImage`, `PostImage`) are generated into an isolated namespace produced by `RegisterStepHelper.GetExpectedImageNamespace(...)` (`XrmPluginCore.SourceGenerator/Helpers/RegisterStepHelper.cs:17`), with the shape: + +``` +{PluginNamespace}.PluginRegistrations.{PluginClassName}.{EntityTypeName}{Operation}{Stage} +``` + +e.g. `Some.Namespace.Plugins.PluginRegistrations.SomePlugin.LeadUpdatePostOperation`. The shared marker for these namespaces is the substring `.PluginRegistrations.` (`SyntaxFactoryHelper.IsImageRegistrationNamespace`, `SyntaxFactoryHelper.cs:207`). The alias used is the last namespace segment (`GetLastNamespaceSegment`, `SyntaxFactoryHelper.cs:212`), e.g. `LeadUpdatePostOperation`. Both `PreImage` and `PostImage` are simple type names (`Constants.PreImageTypeName = "PreImage"`, `Constants.PostImageTypeName = "PostImage"`), so two registrations in the same file both expose a `PreImage`/`PostImage` type. + +**The bug:** Today the code fixes try to be clever — they only convert to aliased usings *when an ambiguity is detected*, otherwise they add a plain (non-aliased) `using`. This is driven by `SyntaxFactoryHelper.DetectImageAmbiguity(...)` (`SyntaxFactoryHelper.cs:118`): + +- In `FixHandlerSignatureCodeFixProvider.FixMethodDeclarationsAsync` (lines 199–235): `ambiguity = DetectImageAmbiguity(...)`; `CreateImageParameterList(hasPreImage, hasPostImage, ambiguity.needsAlias ? ambiguity.alias : null)`; then `if (ambiguity.needsAlias) ConvertToAliasedUsingsAndQualifyRefs(...) else AddUsingDirectiveIfMissing(...)`. +- In `CreateHandlerMethodCodeFixProvider.CreateMethodAsync` (lines 135–152): same `DetectImageAmbiguity` / conditional pattern via `CreateMethodDeclaration(..., needsAlias ? alias : null)` and the same `if (needsAlias) ConvertToAliasedUsingsAndQualifyRefs else AddUsingDirectiveIfMissing` branch. + +When multiple plugins use the **same service** and more than one of those registrations declares Pre/Post images, the automatic generation of usings and modification of parameters fails, because the plain-using path produces bare `PreImage`/`PostImage` references that collide across the multiple image namespaces, and the on-demand conversion logic (`ImageAmbiguityRewriter`, `SyntaxFactoryHelper.cs:234`) assumes "there should be exactly one existing plain image using at this point" (`SyntaxFactoryHelper.cs:309`, the `break;` after taking the first entry of `_typeToExistingAlias`). That assumption breaks with multiple image usings. + +**The decision:** Stop trying to convert on demand. **Always** emit aliased usings and **always** qualify the image parameter types with the alias. This makes every emitted reference unambiguous regardless of how many same-service registrations exist in the file. + +Existing tests assert the *old* behavior and must be updated. See: +- `XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs` — `Should_Fix_Signature_And_Add_Using_For_PreImage` (line ~61/64), `..._For_PostImage` (line ~114/117), `..._For_Both_Images` (line ~120), and `Should_Avoid_Ambiguous_Usings` (line 231). +- `XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs` (same directory). + +## Goals + +- Both code-fix providers **always** emit an **aliased** using directive of the form `using LeadUpdatePostOperation = Some.Namespace.Plugins.PluginRegistrations.SomePlugin.LeadUpdatePostOperation;` for the image namespace involved, never a plain `using Some.Namespace...PluginRegistrations...;`. +- Both code-fix providers **always** write image parameters with the alias prefix, e.g. `void OnUpdateLead(LeadUpdatePostOperation.PreImage preImage)` (and `LeadUpdatePostOperation.PostImage postImage`), regardless of whether any ambiguity currently exists. +- Applying either fix to a file where several same-service registrations have Pre/Post images produces compiling, unambiguous code (no `CS0104` ambiguous-reference errors, no collisions between `PreImage`/`PostImage` across registrations). +- On each run, the fix **re-evaluates** existing image-registration usings in the document: if a developer has "cleaned up" / hand-edited the imports in the meantime (e.g. removed an alias, reverted to a plain using, or left a stale bare reference), the fix rewrites those identified usings back to the consistent aliased form and re-qualifies their references. +- The existing test suite is updated to assert the always-aliased behavior and continues to pass. + +## Non-goals + +- Changing the wrapper-class source generation itself (`WrapperClassGenerator`, `PluginImageGenerator`) — namespaces, class shapes, and the `PreImage`/`PostImage` type names stay as-is. The generators emit each wrapper class into its own isolated namespace (`WrapperClassGenerator` writes `namespace {metadata.RegistrationNamespace}` at `WrapperClassGenerator.cs:32` and never emits `using` directives into the consumer's source files), so generated output is already unambiguous by construction and needs no change. The ambiguity this spec addresses is introduced only by the code-fix providers' edits to *user* code. +- Changing the diagnostics/analyzers (`HandlerSignatureMismatchAnalyzer`, `HandlerMethodNotFoundAnalyzer`) — the namespace is still passed via `Constants.PropertyImageNamespace`; only the code-fix output format changes. +- Touching non-image usings. Only usings whose namespace contains `.PluginRegistrations.` are rewritten; all other usings are left untouched. +- Rewriting `member access` expressions like `obj.PreImage`, the alias-name side of a `using X = ...` (`NameEqualsSyntax`), or already-qualified references — these exclusions in `ImageAmbiguityRewriter.VisitIdentifierName` (`SyntaxFactoryHelper.cs:284–300`) must be preserved. + +## Requirements + +- The aliased using format is exactly: `using {alias} = {fullNamespace};` where `alias = GetLastNamespaceSegment(fullNamespace)` (last dot-separated segment) and `fullNamespace` is the value from `Constants.PropertyImageNamespace`. Example: `using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;`. +- The qualified parameter format is exactly: `{alias}.PreImage preImage` and/or `{alias}.PostImage postImage`, in that order (PreImage before PostImage), matching `CreateImageParameterList`'s ordering (`SyntaxFactoryHelper.cs:43–57`). +- Idempotency: applying the fix to a file that already has the correct aliased using must not duplicate it. The existing duplicate-guard in `ConvertToAliasedUsingsAndQualifyRefs` (`SyntaxFactoryHelper.cs:189–192`, checks `u.Alias?.Name.ToString() == newAlias || u.Name?.ToString() == newImageNamespace`) covers this; verify it still holds when the always-alias path is the only path. +- Multiple same-service registrations: when two registrations (e.g. `AccountUpdatePostOperation` and `AccountDeletePostOperation`) both declare images and both handlers are fixed in the same document, each gets its own aliased using and each parameter is prefixed with its own alias. No bare `PreImage`/`PostImage` remains in fixed signatures. +- Re-evaluation of stale imports: when the fix runs, every plain (non-aliased) using whose namespace `IsImageRegistrationNamespace` is true must be converted to its aliased form, and any bare `PreImage`/`PostImage` *type references* under that plain using must be re-qualified to the matching alias. This is the existing `ConvertToAliasedUsingsAndQualifyRefs` + `ImageAmbiguityRewriter` machinery — but it must now correctly handle **more than one** plain image using at a time (the current `break;`-after-first assumption at `SyntaxFactoryHelper.cs:306–310` is insufficient). +- Resolving which alias a bare `PreImage`/`PostImage` reference belongs to (the multi-using case): **use the semantic model to resolve each bare reference's symbol to its declaring namespace, then map that namespace back to its alias.** Do not guess from the bare name alone and do not fall back to "exactly one plain image using" — attempt to resolve every bare reference uniquely via its symbol. If the semantic model resolves the reference to a type whose containing namespace is one of the (now-aliased) image namespaces, qualify the reference with that namespace's alias (`GetLastNamespaceSegment`). If a reference cannot be resolved uniquely to a single image namespace (e.g. the symbol is missing/erroneous, or genuinely ambiguous), leave that reference unqualified rather than guessing — it will surface as a normal compile error for the developer to resolve. This requires threading a `SemanticModel`/`Compilation` (or per-tree `SemanticModel`) into `ConvertToAliasedUsingsAndQualifyRefs` / `ImageAmbiguityRewriter`, which today operate purely on syntax. +- The `DetectImageAmbiguity` method and the conditional `if (needsAlias) ... else AddUsingDirectiveIfMissing(...)` branches in both providers should no longer gate behavior — the aliased path becomes unconditional. `DetectImageAmbiguity` / `AddUsingDirectiveIfMissing` may become dead code; remove them if unused, or keep with updated callers — implementer's discretion, but no caller should rely on the plain-using branch for image namespaces. +- Both providers (`FixHandlerSignatureCodeFixProvider`, `CreateHandlerMethodCodeFixProvider`) must share the always-alias logic rather than each carrying a near-duplicate call site. Today each independently calls `DetectImageAmbiguity` + `CreateImageParameterList` + the using branch. Extract the common steps — compute alias from `imageNamespace`, build the alias-qualified parameter list, and run the always-aliased using conversion/requalification — into a single shared helper (on `SyntaxFactoryHelper`, or a new small helper type) that both providers call. No image-using/alias logic should be duplicated across the two providers. +- `BuildSignatureDescription` (`SyntaxFactoryHelper.cs:66`) is used only for code-action titles (e.g. `Fix signature to 'HandleUpdate(PreImage preImage)'`) and uses unqualified type names; titles may stay unqualified — they are human-facing labels, not emitted code. +- **FixAll must produce the same always-aliased, unambiguous result as a single fix.** Both `FixHandlerSignatureCodeFixProvider` and `CreateHandlerMethodCodeFixProvider` currently return `WellKnownFixAllProviders.BatchFixer` from `GetFixAllProvider()` (`FixHandlerSignatureCodeFixProvider.cs:27–28`, `CreateHandlerMethodCodeFixProvider.cs:24–25`). The batch fixer is unsafe here: it computes each diagnostic's `CodeAction` independently against the *original* document and then merges text changes, so when one document holds several image registrations (the exact same-service-multiple-images scenario this spec targets), the per-diagnostic fixes each add their own aliased using and rewrite the shared `using` block / overlapping references without seeing each other's edits — producing merge conflicts or non-convergent, still-ambiguous output. The goal is to be as unambiguous as possible, so FixAll must be handled explicitly rather than left to `BatchFixer`. See the dedicated phase below: a custom `FixAllProvider` consolidates *all* fixable diagnostics in a document into a single pass that fixes every target signature with its own alias and runs one consolidated always-aliased using rewrite (adding every needed aliased using and requalifying every reference once). This shared FixAll logic must not be duplicated between the two providers. + +## Open questions + +- None outstanding. (Resolved: bare-reference alias resolution uses the semantic model; the two providers share one always-alias helper; FixAll is handled by a custom consolidating provider; generators need no change.) + +## Phases + +### Phase 1: Build the shared always-aliased helper in `SyntaxFactoryHelper` (with semantic-model requalification) + +This phase creates the single shared engine that both code-fix providers will call (resolving the "no duplication" decision) and makes the using-rewrite correct for multiple plain image usings via the semantic model (resolving the "which alias" decision). Edit `XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs`. Three parts: + +1. **Expose the alias helper.** Add `public static string GetAliasForImageNamespace(string ns) => GetLastNamespaceSegment(ns);` (or make `GetLastNamespaceSegment` accessible). Keep `GetLastNamespaceSegment` semantics: substring after the last `.`, or the whole string if no dot (`SyntaxFactoryHelper.cs:212–216`). + +2. **Add the shared always-alias entry point.** Add a single helper that both providers call, e.g.: + ```csharp + // Always emits the aliased using for imageNamespace and re-qualifies/aliases every + // existing image-registration using in the tree. `semanticModel` is the model for `root`'s tree. + public static SyntaxNode ApplyAliasedImageUsings(SyntaxNode root, string imageNamespace, SemanticModel semanticModel) + ``` + It computes `alias = GetAliasForImageNamespace(imageNamespace)`, then delegates to `ConvertToAliasedUsingsAndQualifyRefs(root, imageNamespace, semanticModel)` (see part 3). Guard for null/empty `imageNamespace` (return `root` unchanged / fall back to no-op — the analyzer only sets `Constants.PropertyImageNamespace` when an expected namespace exists; `HandlerSignatureMismatchAnalyzer.cs:119–121`). The alias-qualified **parameter list** is still produced by the existing `CreateImageParameterList(bool, bool, string qualifier)` (`SyntaxFactoryHelper.cs:41–58`), which emits `QualifiedName(IdentifierName(qualifier), IdentifierName(typeName))` (`SyntaxFactoryHelper.cs:218–228`) — both providers pass `alias` as the qualifier (never `null`/conditional). + +3. **Thread the semantic model into `ConvertToAliasedUsingsAndQualifyRefs` + `ImageAmbiguityRewriter` and fix the multi-using requalification.** Today both operate purely on syntax. Change the signature of `ConvertToAliasedUsingsAndQualifyRefs` (`SyntaxFactoryHelper.cs:152`) to also accept a `SemanticModel`, and pass it into the `ImageAmbiguityRewriter` constructor (`SyntaxFactoryHelper.cs:242`). The rewriter currently (a) collects all plain image namespaces into `plainNamespaceToAlias` (lines 159–173 of `ConvertToAliasedUsingsAndQualifyRefs`), (b) adds the new namespace (lines 175–179), (c) runs the rewriter, (d) appends the new aliased using with a duplicate guard checking `u.Alias?.Name.ToString() == newAlias || u.Name?.ToString() == newImageNamespace` (lines 186–202). That collection + dedup logic is correct and stays. + + The broken part is `ImageAmbiguityRewriter.VisitIdentifierName` (`SyntaxFactoryHelper.cs:274–325`), which today picks the alias by iterating `_typeToExistingAlias` and `break`-ing on the first entry with the comment "There should be exactly one existing plain image using at this point" (lines 305–310). Replace this guess with **semantic resolution**: for each bare `PreImage`/`PostImage` identifier (after the existing exclusions at lines 284–300 — already-qualified right side of a `QualifiedNameSyntax`, member-access name like `obj.PreImage`, and the `NameEqualsSyntax` alias side — which MUST be preserved), use the threaded `SemanticModel` to resolve the identifier's symbol (`GetSymbolInfo`/`GetTypeInfo`) to its containing namespace's full name. If that namespace is one of the image namespaces being aliased (present in `plainNamespaceToAlias`, i.e. `IsImageRegistrationNamespace` true and known), qualify the reference with **that** namespace's alias (`GetLastNamespaceSegment`). If the symbol cannot be resolved uniquely to a single image namespace (missing/erroneous symbol, or ambiguous `CS0104`-style binding), leave the reference unqualified — do not guess — so it surfaces as a normal compile error. This makes requalification correct regardless of how many plain image usings exist in the file. Remove the misleading `break;` and its comment. + + Note: because the rewriter converts the plain usings to aliased form in the same pass, resolve symbols against the **original** (pre-rewrite) tree's semantic model, which is what the providers supply (the model for the tree as it was when the diagnostic fired). The bare references were valid under the original plain usings, so the symbols resolve there. + +Verifiable when: `SyntaxFactoryHelper` exposes `GetAliasForImageNamespace` and `ApplyAliasedImageUsings(...)`; `ConvertToAliasedUsingsAndQualifyRefs` accepts a `SemanticModel`; and a unit-level invocation over a tree with two plain image usings (`AccountUpdatePostOperation`, `AccountDeletePostOperation`) plus bare `PreImage` references under each correctly qualifies each reference to its own alias. + +### Phase 2: Wire `FixHandlerSignatureCodeFixProvider` to the shared helper (always-aliased, no duplication) + +Edit `XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs`, method `FixMethodDeclarationsAsync` (lines 124–241), specifically the block at lines 198–235. + +Today it does: +```csharp +var ambiguity = SyntaxFactoryHelper.DetectImageAmbiguity(methodRoot, imageNamespace); +var newParameters = SyntaxFactoryHelper.CreateImageParameterList(hasPreImage, hasPostImage, ambiguity.needsAlias ? ambiguity.alias : null); +// ... replace method decls ... +if (ambiguity.needsAlias) + newRoot = SyntaxFactoryHelper.ConvertToAliasedUsingsAndQualifyRefs(newRoot, imageNamespace); +else + newRoot = SyntaxFactoryHelper.AddUsingDirectiveIfMissing(newRoot, imageNamespace); +``` + +Change it to **always** use the alias via the shared helper. Compute the alias once and always pass it to `CreateImageParameterList`; then call `ApplyAliasedImageUsings` for the using rewrite: +```csharp +var alias = SyntaxFactoryHelper.GetAliasForImageNamespace(imageNamespace); +var newParameters = SyntaxFactoryHelper.CreateImageParameterList(hasPreImage, hasPostImage, alias); +// ... replace method decls (existing loop at lines 216–225 unchanged) ... +newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, imageNamespace, treeSemanticModel); +``` +`ApplyAliasedImageUsings` needs the `SemanticModel` for the tree being rewritten. This provider processes potentially multiple documents grouped by syntax tree (`locationsByTree`, lines 144–162); for each `tree`/`methodDocument`, obtain the model via `compilation.GetSemanticModel(tree)` (the `compilation` is already available as `semanticModel.Compilation`, passed into `FixMethodDeclarationsAsync` at line 119/126). Guard `imageNamespace` null/empty (no-op fallback). Remove the `DetectImageAmbiguity`/conditional branch. + +Verifiable when: applying the fix to a single PreImage registration produces both `void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)` in the signature AND `using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;` in the usings — i.e. the previously-named test `Should_Fix_Signature_And_Add_Using_For_PreImage` now sees the aliased form. And the multi-registration case (two same-service registrations both with PreImages) yields one aliased using per namespace, each signature alias-qualified, no `CS0104`. + +### Phase 3: Wire `CreateHandlerMethodCodeFixProvider` to the shared helper (always-aliased, no duplication) + +Edit `XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs`, method `CreateMethodAsync` (lines 135–154) and `CreateMethodDeclaration` (lines 157–166). + +Today it does: +```csharp +var (needsAlias, alias) = SyntaxFactoryHelper.DetectImageAmbiguity(interfaceRoot, imageNamespace); +var methodDeclaration = CreateMethodDeclaration(methodName, hasPreImage, hasPostImage, needsAlias ? alias : null); +// ... add member ... +if (needsAlias) + newRoot = SyntaxFactoryHelper.ConvertToAliasedUsingsAndQualifyRefs(newRoot, imageNamespace); +else + newRoot = SyntaxFactoryHelper.AddUsingDirectiveIfMissing(newRoot, imageNamespace); +``` + +Change to always-alias via the same shared helper: +```csharp +var alias = SyntaxFactoryHelper.GetAliasForImageNamespace(imageNamespace); +var methodDeclaration = CreateMethodDeclaration(methodName, hasPreImage, hasPostImage, alias); +// ... add member (existing AddMembers/ReplaceNode at lines 141–142 unchanged) ... +newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, imageNamespace, interfaceSemanticModel); +``` +The interface may live in a different document/tree than the trigger; obtain its semantic model via `compilation.GetSemanticModel(interfaceRoot.SyntaxTree)` (the `compilation` is `semanticModel.Compilation`, available in `CreateMethodAsync`, line 76). Apply the same null/empty `imageNamespace` guard. `CreateMethodDeclaration` already forwards the `qualifier` into `CreateImageParameterList` (line 162), so the only change there is passing `alias` instead of the `needsAlias ? alias : null` conditional. Remove the `DetectImageAmbiguity`/conditional branch. After this phase, no image-using/alias logic is duplicated between the two providers — both go through `SyntaxFactoryHelper.ApplyAliasedImageUsings` + `CreateImageParameterList`. + +Verifiable when: invoking the "Create method" fix for a missing handler with a PreImage registration emits `void HandleUpdate(AccountUpdatePostOperation.PreImage preImage);` on the interface plus the aliased using directive, even when no other image using exists in the file; and both providers reference the single shared helper (grep shows no remaining `DetectImageAmbiguity` call sites unless intentionally retained). + +### Phase 4: Replace `BatchFixer` with a custom consolidating `FixAllProvider` for both providers + +Both `FixHandlerSignatureCodeFixProvider` and `CreateHandlerMethodCodeFixProvider` currently return `WellKnownFixAllProviders.BatchFixer` (`FixHandlerSignatureCodeFixProvider.cs:27–28`, `CreateHandlerMethodCodeFixProvider.cs:24–25`). As described in Requirements, `BatchFixer` applies each diagnostic's fix independently against the original document and merges — which breaks when one document has several image registrations, because each fix touches the shared `using` block / overlapping references without seeing the others, yielding merge conflicts or non-convergent ambiguous output. + +Implement a single shared custom `FixAllProvider` (a new type, e.g. `XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs`, used by both providers via `GetFixAllProvider()`) that: +1. Collects **all** fixable diagnostics in the relevant scope (Document/Project/Solution — at minimum Document scope; group remaining work by document). For each document, gather every diagnostic and read each one's `Constants.PropertyServiceType`, `Constants.PropertyMethodName`, `Constants.PropertyHasPreImage`, `Constants.PropertyHasPostImage`, `Constants.PropertyImageNamespace` (same property keys the per-fix path reads, e.g. `FixHandlerSignatureCodeFixProvider.cs:41–49`). +2. Applies all signature/method edits for that document in **one** pass: for each diagnostic, fix its target method signature (or create its interface method) with its **own** alias-qualified parameter list (`SyntaxFactoryHelper.CreateImageParameterList(hasPreImage, hasPostImage, GetAliasForImageNamespace(imageNamespace))`). +3. Runs **one** consolidated always-aliased using rewrite over the resulting tree that adds every distinct aliased image using needed and requalifies every reference once. Prefer extending the shared helper from Phase 1 to accept multiple image namespaces in a single call (e.g. an overload `ApplyAliasedImageUsings(SyntaxNode root, IEnumerable imageNamespaces, SemanticModel semanticModel)` that loops the existing collection/dedup/requalify logic of `ConvertToAliasedUsingsAndQualifyRefs`), so the FixAll path reuses the exact same engine as the single-fix path. No FixAll-specific alias/using logic may be duplicated — it must funnel through the same `SyntaxFactoryHelper` engine that Phases 1–3 use. + +The result must be order-independent and convergent: fixing N registrations in a file via FixAll produces exactly the same unambiguous output as fixing them one-by-one in any order — each namespace aliased exactly once, each signature alias-qualified, no `CS0104`. + +Verifiable when: a FixAll over a document with two (or more) same-service image registrations produces, in a single operation, one aliased `using ... = ...;` per distinct image namespace (no duplicates) and every fixed signature alias-qualified, and the document compiles cleanly; and `GetFixAllProvider()` on both providers returns the new shared provider rather than `WellKnownFixAllProviders.BatchFixer`. + +### Phase 5: Update and extend the code-fix tests + +Edit `XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs` and `XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs`. + +In `FixHandlerSignatureCodeFixProviderTests.cs`: +- `Should_Fix_Signature_And_Add_Using_For_PreImage` (assertions at lines 61, 64): change expected signature from `void HandleUpdate(PreImage preImage)` to `void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)` and the using from plain `using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;` to aliased `using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;`. +- `Should_Fix_Signature_And_Add_Using_For_PostImage` (lines 114, 117): expect `void HandleUpdate(AccountUpdatePostOperation.PostImage postImage)` and the aliased using. +- `Should_Fix_Signature_And_Add_Using_For_Both_Images` (line ~120): expect `void HandleUpdate(AccountUpdatePostOperation.PreImage preImage, AccountUpdatePostOperation.PostImage postImage)` and the aliased using. +- `Should_Avoid_Ambiguous_Usings` (line 231): this already expects the aliased form (`void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)`, `void HandleDelete(AccountDeletePostOperation.PreImage preImage)`, and the two aliased `using ... = ...;` directives counted once each via `CountOccurrences`, lines 285–292). It should keep passing unchanged — verify it does. Consider renaming it (e.g. `Should_Use_Aliased_Usings_For_Multiple_Registrations`) since aliasing is no longer "avoidance of ambiguity" but the default. +- Add a new test asserting idempotency: applying the fix to source that already contains the correct aliased using and aliased signature does not duplicate the using (use the existing `CountOccurrences` helper at lines 295–304, expect count `1`). +- Add a new test for the "cleaned-up imports" re-evaluation: source where a developer reverted one registration's using to plain form (or removed it) — after fix, all image usings are aliased exactly once. +- Add a new test for **multi-using semantic requalification**: two same-service registrations (`AccountUpdatePostOperation` + `AccountDeletePostOperation`) where pre-existing handler bodies/signatures contain bare `PreImage` references under two plain image usings. After fix, each bare reference is qualified to its own alias (`AccountUpdatePostOperation.PreImage` vs `AccountDeletePostOperation.PreImage`) — never cross-qualified — and the result compiles with no `CS0104`. This exercises the Phase 1 semantic-model resolution path (where the old `break`-on-first logic would have mis-qualified one of them). + +- Add a new **FixAll** test (Phase 4): a document with two or more same-service image registrations, all triggering the diagnostic, fixed via the FixAll/batch path. Assert the consolidated single-pass result — one aliased `using ... = ...;` per distinct image namespace (count `1` each via `CountOccurrences`), every signature alias-qualified, no `CS0104`. If `CodeFixTestBase` only exercises single-diagnostic fixes today, extend it (or add a sibling helper) to invoke the provider's `GetFixAllProvider()` across all diagnostics in the document so the custom `FixAllProvider` is actually exercised (not just the per-diagnostic `RegisterCodeFixesAsync`). + +In `CreateHandlerMethodCodeFixProviderTests.cs`: apply the analogous expectation changes — generated interface methods must now use alias-qualified parameter types and the file must gain an aliased using — plus an analogous FixAll test for multiple missing handlers on the same service interface. + +Use the shared `CodeFixTestBase` / `ApplyCodeFixAsync` harness already present (`FixHandlerSignatureCodeFixProviderTests.cs:307–309` wires `HandlerSignatureMismatchAnalyzer` + `FixHandlerSignatureCodeFixProvider` with diagnostic ids `HandlerSignatureMismatch.Id` and `HandlerSignatureMismatchError.Id`). + +Verifiable when: `dotnet test --configuration Release` passes for `XrmPluginCore.SourceGenerator.Tests`, including the updated and newly-added tests, across all target frameworks. diff --git a/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs b/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs index 0db1e3f..598f0fe 100644 --- a/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs +++ b/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs @@ -145,11 +145,50 @@ public static (bool needsAlias, string alias) DetectImageAmbiguity(SyntaxNode ro return (false, null); } + /// + /// Returns the alias used for an image-registration namespace: its last dot-separated segment. + /// + public static string GetAliasForImageNamespace(string ns) => GetLastNamespaceSegment(ns); + + /// + /// Backward-compatible overload without a semantic model. Bare PreImage/PostImage references are + /// left unqualified (no semantic resolution). Prefer the overload that takes a . + /// + public static SyntaxNode ConvertToAliasedUsingsAndQualifyRefs(SyntaxNode root, string newImageNamespace) + => ConvertToAliasedUsingsAndQualifyRefs(root, newImageNamespace, semanticModel: null); + + /// + /// Always emits the aliased using for and re-qualifies/aliases + /// every existing image-registration using in the tree. This is the single shared entry point used + /// by the code-fix providers; alias-qualified parameter lists are produced separately via + /// . + /// + /// The compilation unit (document root) to rewrite. + /// The image-registration namespace to alias and add. + /// The semantic model for 's tree (pre-rewrite). + public static SyntaxNode ApplyAliasedImageUsings(SyntaxNode root, string imageNamespace, SemanticModel semanticModel) + { + if (string.IsNullOrEmpty(imageNamespace)) + { + // The analyzer only sets the image namespace when an expected one exists; nothing to do. + return root; + } + + return ConvertToAliasedUsingsAndQualifyRefs(root, imageNamespace, semanticModel); + } + /// /// Converts existing plain image usings to aliased form, qualifies all bare PreImage/PostImage /// type references, and adds the new aliased using. /// - public static SyntaxNode ConvertToAliasedUsingsAndQualifyRefs(SyntaxNode root, string newImageNamespace) + /// The compilation unit (document root) to rewrite. + /// The image-registration namespace to alias and add. + /// + /// The semantic model for 's original (pre-rewrite) tree, used to resolve + /// which alias a bare PreImage/PostImage reference belongs to. May be null, in which case bare + /// references are left unqualified. + /// + public static SyntaxNode ConvertToAliasedUsingsAndQualifyRefs(SyntaxNode root, string newImageNamespace, SemanticModel semanticModel) { if (root is not CompilationUnitSyntax compilationUnit) { @@ -179,7 +218,7 @@ public static SyntaxNode ConvertToAliasedUsingsAndQualifyRefs(SyntaxNode root, s } // Rewrite the tree - var rewriter = new ImageAmbiguityRewriter(plainNamespaceToAlias); + var rewriter = new ImageAmbiguityRewriter(plainNamespaceToAlias, semanticModel); var newRoot = rewriter.Visit(root); // Add the new aliased using if not already present @@ -235,16 +274,14 @@ private sealed class ImageAmbiguityRewriter : CSharpSyntaxRewriter { private readonly Dictionary _plainNamespaceToAlias; - // Reverse map: for each bare type name, which alias qualifies it - // Built from existing usings only (not the new one being added) - private readonly Dictionary _typeToExistingAlias; + // Semantic model for the original (pre-rewrite) tree, used to resolve which image + // namespace a bare PreImage/PostImage reference actually binds to. May be null. + private readonly SemanticModel _semanticModel; - public ImageAmbiguityRewriter(Dictionary plainNamespaceToAlias) + public ImageAmbiguityRewriter(Dictionary plainNamespaceToAlias, SemanticModel semanticModel) { _plainNamespaceToAlias = plainNamespaceToAlias; - - // Build type-to-alias map for qualifying existing bare references - _typeToExistingAlias = new Dictionary(); + _semanticModel = semanticModel; } public override SyntaxNode VisitUsingDirective(UsingDirectiveSyntax node) @@ -257,9 +294,6 @@ public override SyntaxNode VisitUsingDirective(UsingDirectiveSyntax node) var ns = node.Name?.ToString(); if (ns != null && _plainNamespaceToAlias.TryGetValue(ns, out var alias)) { - // Track which alias maps to which namespace for qualifying references - _typeToExistingAlias[ns] = alias; - // Convert to aliased using return SyntaxFactory.UsingDirective( SyntaxFactory.NameEquals(SyntaxFactory.IdentifierName(alias)), @@ -299,16 +333,13 @@ public override SyntaxNode VisitIdentifierName(IdentifierNameSyntax node) return base.VisitIdentifierName(node); } - // Find the alias for this type reference based on the existing usings that were converted - // We need to figure out which alias applies to this bare reference. - // The bare reference was valid under the old plain using, so find which converted namespace it belonged to. - string matchingAlias = null; - foreach (var kvp in _typeToExistingAlias) - { - matchingAlias = kvp.Value; - break; // There should be exactly one existing plain image using at this point - } - + // Resolve which image namespace this bare reference binds to via the semantic model + // (built from the original, pre-rewrite tree). Only qualify when it resolves uniquely + // to a single image-registration namespace whose alias we know. If the reference is + // ambiguous or unresolved, leave it unqualified so it surfaces as a normal compile error + // rather than guessing — this keeps requalification correct with any number of plain + // image usings in the file. + var matchingAlias = ResolveImageAlias(node); if (matchingAlias == null) { return base.VisitIdentifierName(node); @@ -323,5 +354,28 @@ public override SyntaxNode VisitIdentifierName(IdentifierNameSyntax node) .WithLeadingTrivia(node.GetLeadingTrivia()) .WithTrailingTrivia(node.GetTrailingTrivia()); } + + private string ResolveImageAlias(IdentifierNameSyntax node) + { + if (_semanticModel == null) + { + return null; + } + + var symbol = _semanticModel.GetSymbolInfo(node).Symbol; + if (symbol == null) + { + // Ambiguous (e.g. CS0104) or otherwise unresolved — do not guess. + return null; + } + + var containingNamespace = symbol.ContainingNamespace?.ToDisplayString(); + if (containingNamespace != null && _plainNamespaceToAlias.TryGetValue(containingNamespace, out var alias)) + { + return alias; + } + + return null; + } } } From 51322804cd2270a1e691fede7a9793cf7afcdd12 Mon Sep 17 00:00:00 2001 From: Morten Holt Date: Fri, 19 Jun 2026 14:16:40 +0200 Subject: [PATCH 2/7] Phase 2 (Wire `FixHandlerSignatureCodeFixProvider` to the shared helper (always-aliased, no duplication)) completed --- .../FixHandlerSignatureCodeFixProvider.cs | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs index cb56481..29e1ae5 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs @@ -195,9 +195,11 @@ private static async Task FixMethodDeclarationsAsync( continue; } - // Detect ambiguity - var ambiguity = SyntaxFactoryHelper.DetectImageAmbiguity(methodRoot, imageNamespace); - var newParameters = SyntaxFactoryHelper.CreateImageParameterList(hasPreImage, hasPostImage, ambiguity.needsAlias ? ambiguity.alias : null); + // Always qualify the image parameters with the namespace alias. + var alias = string.IsNullOrEmpty(imageNamespace) + ? null + : SyntaxFactoryHelper.GetAliasForImageNamespace(imageNamespace); + var newParameters = SyntaxFactoryHelper.CreateImageParameterList(hasPreImage, hasPostImage, alias); // Build a set of (containingTypeName, methodName) pairs to replace var targets = new HashSet<(string typeName, string method)>(); @@ -224,15 +226,9 @@ private static async Task FixMethodDeclarationsAsync( } } - // Handle usings - if (ambiguity.needsAlias) - { - newRoot = SyntaxFactoryHelper.ConvertToAliasedUsingsAndQualifyRefs(newRoot, imageNamespace); - } - else - { - newRoot = SyntaxFactoryHelper.AddUsingDirectiveIfMissing(newRoot, imageNamespace); - } + // Always emit the aliased using and requalify existing image references. + var treeSemanticModel = compilation.GetSemanticModel(tree); + newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, imageNamespace, treeSemanticModel); solution = solution.WithDocumentSyntaxRoot(methodDocument.Id, newRoot); } From 2bbb98217b4d4f1865b9609b07553fe75fbafffd Mon Sep 17 00:00:00 2001 From: Morten Holt Date: Fri, 19 Jun 2026 14:18:25 +0200 Subject: [PATCH 3/7] Phase 3 (Wire `CreateHandlerMethodCodeFixProvider` to the shared helper (always-aliased, no duplication)) completed --- .../CreateHandlerMethodCodeFixProvider.cs | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs index 63d4ccc..cc0cdf4 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs @@ -132,24 +132,20 @@ private static async Task CreateMethodAsync( return solution; } - // Detect ambiguity before creating the method - var (needsAlias, alias) = SyntaxFactoryHelper.DetectImageAmbiguity(interfaceRoot, imageNamespace); + // Always qualify the image parameters with the namespace alias. + var alias = string.IsNullOrEmpty(imageNamespace) + ? null + : SyntaxFactoryHelper.GetAliasForImageNamespace(imageNamespace); - // Create the method declaration with qualifier if ambiguous - var methodDeclaration = CreateMethodDeclaration(methodName, hasPreImage, hasPostImage, needsAlias ? alias : null); + var methodDeclaration = CreateMethodDeclaration(methodName, hasPreImage, hasPostImage, alias); var newInterface = interfaceDeclaration.AddMembers(methodDeclaration); var newRoot = interfaceRoot.ReplaceNode(interfaceDeclaration, newInterface); - // Handle usings - if (needsAlias) - { - newRoot = SyntaxFactoryHelper.ConvertToAliasedUsingsAndQualifyRefs(newRoot, imageNamespace); - } - else - { - newRoot = SyntaxFactoryHelper.AddUsingDirectiveIfMissing(newRoot, imageNamespace); - } + // Always emit the aliased using and requalify existing image references. The interface may live + // in a different tree than the trigger, so resolve its semantic model from the compilation. + var interfaceSemanticModel = semanticModel.Compilation.GetSemanticModel(interfaceRoot.SyntaxTree); + newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, imageNamespace, interfaceSemanticModel); return solution.WithDocumentSyntaxRoot(interfaceDocument.Id, newRoot); } From 968d63961250cd34d5e4b657a3fe8ce46cfae2e2 Mon Sep 17 00:00:00 2001 From: Morten Holt Date: Fri, 19 Jun 2026 14:28:45 +0200 Subject: [PATCH 4/7] Phase 4 (Replace `BatchFixer` with a custom consolidating `FixAllProvider` for both providers) completed --- .../AliasedImageUsingsFixAllProvider.cs | 345 ++++++++++++++++++ .../CreateHandlerMethodCodeFixProvider.cs | 6 +- .../FixHandlerSignatureCodeFixProvider.cs | 2 +- .../Helpers/RegisterStepHelper.cs | 34 ++ .../Helpers/SyntaxFactoryHelper.cs | 88 ++++- 5 files changed, 455 insertions(+), 20 deletions(-) create mode 100644 XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs new file mode 100644 index 0000000..123d93a --- /dev/null +++ b/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs @@ -0,0 +1,345 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using XrmPluginCore.SourceGenerator.Helpers; + +namespace XrmPluginCore.SourceGenerator.CodeFixes; + +/// +/// Shared for the image handler code fixes. Unlike +/// — which computes each diagnostic's fix +/// independently against the original document and merges text changes — this provider consolidates +/// every fixable diagnostic in a document into a single pass: it applies each target signature/method +/// edit with its own alias-qualified parameter list, then runs one always-aliased using +/// rewrite per affected document (adding every distinct aliased image using and requalifying every +/// reference once). The result is order-independent and convergent: fixing N registrations via FixAll +/// produces the same unambiguous output as fixing them one-by-one. +/// +internal sealed class AliasedImageUsingsFixAllProvider : FixAllProvider +{ + public static readonly AliasedImageUsingsFixAllProvider Instance = new(); + + private AliasedImageUsingsFixAllProvider() + { + } + + public override IEnumerable GetSupportedFixAllScopes() => new[] + { + FixAllScope.Document, + FixAllScope.Project, + FixAllScope.Solution, + }; + + public override async Task GetFixAsync(FixAllContext fixAllContext) + { + var diagnostics = await GetDiagnosticsAsync(fixAllContext).ConfigureAwait(false); + if (diagnostics.IsDefaultOrEmpty) + { + return null; + } + + var solution = fixAllContext.Solution; + var title = fixAllContext.CodeActionEquivalenceKey ?? "Fix all image handlers"; + + return CodeAction.Create( + title, + c => FixAllAsync(solution, diagnostics, c), + equivalenceKey: fixAllContext.CodeActionEquivalenceKey); + } + + private static async Task> GetDiagnosticsAsync(FixAllContext context) + { + switch (context.Scope) + { + case FixAllScope.Document when context.Document != null: + return await context.GetDocumentDiagnosticsAsync(context.Document).ConfigureAwait(false); + + case FixAllScope.Project: + return await context.GetAllDiagnosticsAsync(context.Project).ConfigureAwait(false); + + case FixAllScope.Solution: + var all = ImmutableArray.CreateBuilder(); + foreach (var project in context.Solution.Projects) + { + all.AddRange(await context.GetAllDiagnosticsAsync(project).ConfigureAwait(false)); + } + + return all.ToImmutable(); + + default: + return ImmutableArray.Empty; + } + } + + private static async Task FixAllAsync( + Solution solution, + ImmutableArray diagnostics, + CancellationToken cancellationToken) + { + // Accumulate every edit by the document it lands in, so each affected document gets a single + // consolidated rewrite regardless of how many diagnostics target it. + var batches = new Dictionary(); + + foreach (var diagnostic in diagnostics) + { + await AccumulateDiagnosticAsync(solution, diagnostic, batches, cancellationToken).ConfigureAwait(false); + } + + foreach (var kvp in batches) + { + solution = await ApplyBatchAsync(solution, kvp.Key, kvp.Value, cancellationToken).ConfigureAwait(false); + } + + return solution; + } + + private static async Task AccumulateDiagnosticAsync( + Solution solution, + Diagnostic diagnostic, + Dictionary batches, + CancellationToken cancellationToken) + { + var diagnosticTree = diagnostic.Location.SourceTree; + if (diagnosticTree == null) + { + return; + } + + var diagnosticDocument = solution.GetDocument(diagnosticTree); + if (diagnosticDocument == null) + { + return; + } + + if (!diagnostic.Properties.TryGetValue(Constants.PropertyMethodName, out var methodName) || methodName == null) + { + return; + } + + diagnostic.Properties.TryGetValue(Constants.PropertyHasPreImage, out var hasPreImageStr); + diagnostic.Properties.TryGetValue(Constants.PropertyHasPostImage, out var hasPostImageStr); + diagnostic.Properties.TryGetValue(Constants.PropertyImageNamespace, out var imageNamespace); + + var hasPreImage = bool.TryParse(hasPreImageStr, out var pre) && pre; + var hasPostImage = bool.TryParse(hasPostImageStr, out var post) && post; + + var semanticModel = await diagnosticDocument.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); + var diagnosticRoot = await diagnosticTree.GetRootAsync(cancellationToken).ConfigureAwait(false); + if (semanticModel == null || diagnosticRoot == null) + { + return; + } + + var serviceType = RegisterStepHelper.ResolveServiceType(diagnosticRoot, semanticModel, diagnostic.Location.SourceSpan, cancellationToken); + if (serviceType == null) + { + return; + } + + if (diagnostic.Id == DiagnosticDescriptors.HandlerMethodNotFound.Id) + { + await AccumulateCreateMethodAsync(solution, serviceType, methodName, hasPreImage, hasPostImage, imageNamespace, batches, cancellationToken).ConfigureAwait(false); + } + else + { + await AccumulateSignatureFixAsync(diagnosticDocument, serviceType, methodName, hasPreImage, hasPostImage, imageNamespace, batches, cancellationToken).ConfigureAwait(false); + } + } + + private static async Task AccumulateCreateMethodAsync( + Solution solution, + INamedTypeSymbol serviceType, + string methodName, + bool hasPreImage, + bool hasPostImage, + string imageNamespace, + Dictionary batches, + CancellationToken cancellationToken) + { + var interfaceDeclaration = await CreateHandlerMethodCodeFixProvider + .FindInterfaceDeclarationAsync(solution, serviceType, cancellationToken) + .ConfigureAwait(false); + if (interfaceDeclaration == null) + { + return; + } + + var interfaceDocument = solution.GetDocument(interfaceDeclaration.SyntaxTree); + if (interfaceDocument == null) + { + return; + } + + var batch = GetBatch(batches, interfaceDocument.Id); + batch.Creations.Add(new MethodEdit(interfaceDeclaration.Identifier.Text, methodName, hasPreImage, hasPostImage, imageNamespace)); + batch.AddNamespace(imageNamespace); + } + + private static async Task AccumulateSignatureFixAsync( + Document diagnosticDocument, + INamedTypeSymbol serviceType, + string methodName, + bool hasPreImage, + bool hasPostImage, + string imageNamespace, + Dictionary batches, + CancellationToken cancellationToken) + { + var solution = diagnosticDocument.Project.Solution; + + var methods = new List(TypeHelper.GetAllMethodsIncludingInherited(serviceType, methodName)); + if (serviceType.TypeKind == TypeKind.Interface) + { + var compilation = await diagnosticDocument.Project.GetCompilationAsync(cancellationToken).ConfigureAwait(false); + if (compilation != null) + { + methods.AddRange(TypeHelper.FindImplementingMethods(compilation, serviceType, methodName)); + } + } + + foreach (var method in methods) + { + foreach (var location in method.Locations) + { + if (!location.IsInSource || location.SourceTree == null) + { + continue; + } + + var document = solution.GetDocument(location.SourceTree); + if (document == null) + { + continue; + } + + var batch = GetBatch(batches, document.Id); + batch.SignatureEdits.Add(new MethodEdit(method.ContainingType.Name, methodName, hasPreImage, hasPostImage, imageNamespace)); + batch.AddNamespace(imageNamespace); + } + } + } + + private static async Task ApplyBatchAsync( + Solution solution, + DocumentId documentId, + DocumentBatch batch, + CancellationToken cancellationToken) + { + var document = solution.GetDocument(documentId); + if (document == null) + { + return solution; + } + + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); + if (root == null) + { + return solution; + } + + var newRoot = root; + + // Fix existing handler signatures (interface + implementations) to their alias-qualified form. + foreach (var edit in DistinctEdits(batch.SignatureEdits)) + { + var newParameters = SyntaxFactoryHelper.CreateImageParameterList(edit.HasPreImage, edit.HasPostImage, GetAlias(edit.ImageNamespace)); + var current = newRoot.DescendantNodes().OfType() + .FirstOrDefault(m => m.Identifier.Text == edit.MethodName && + m.Ancestors().OfType().FirstOrDefault()?.Identifier.Text == edit.TypeName); + if (current != null) + { + newRoot = newRoot.ReplaceNode(current, current.WithParameterList(newParameters)); + } + } + + // Create missing handler methods on their interfaces with the alias-qualified parameter list. + foreach (var edit in DistinctEdits(batch.Creations)) + { + var methodDeclaration = CreateHandlerMethodCodeFixProvider.CreateMethodDeclaration( + edit.MethodName, edit.HasPreImage, edit.HasPostImage, GetAlias(edit.ImageNamespace)); + var interfaceDeclaration = newRoot.DescendantNodes().OfType() + .FirstOrDefault(i => i.Identifier.Text == edit.TypeName); + if (interfaceDeclaration != null) + { + newRoot = newRoot.ReplaceNode(interfaceDeclaration, interfaceDeclaration.AddMembers(methodDeclaration)); + } + } + + // One consolidated always-aliased using rewrite for every namespace touched in this document. + newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, batch.Namespaces, semanticModel); + + return solution.WithDocumentSyntaxRoot(documentId, newRoot); + } + + private static string GetAlias(string imageNamespace) => + string.IsNullOrEmpty(imageNamespace) ? null : SyntaxFactoryHelper.GetAliasForImageNamespace(imageNamespace); + + private static IEnumerable DistinctEdits(IEnumerable edits) + { + var seen = new HashSet<(string, string)>(); + foreach (var edit in edits) + { + if (seen.Add((edit.TypeName, edit.MethodName))) + { + yield return edit; + } + } + } + + private static DocumentBatch GetBatch(Dictionary batches, DocumentId documentId) + { + if (!batches.TryGetValue(documentId, out var batch)) + { + batch = new DocumentBatch(); + batches[documentId] = batch; + } + + return batch; + } + + private sealed class DocumentBatch + { + public List SignatureEdits { get; } = new(); + + public List Creations { get; } = new(); + + public HashSet Namespaces { get; } = new(); + + public void AddNamespace(string imageNamespace) + { + if (!string.IsNullOrEmpty(imageNamespace)) + { + Namespaces.Add(imageNamespace); + } + } + } + + private readonly struct MethodEdit + { + public MethodEdit(string typeName, string methodName, bool hasPreImage, bool hasPostImage, string imageNamespace) + { + TypeName = typeName; + MethodName = methodName; + HasPreImage = hasPreImage; + HasPostImage = hasPostImage; + ImageNamespace = imageNamespace; + } + + public string TypeName { get; } + + public string MethodName { get; } + + public bool HasPreImage { get; } + + public bool HasPostImage { get; } + + public string ImageNamespace { get; } + } +} diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs index cc0cdf4..76c1674 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs @@ -22,7 +22,7 @@ public class CreateHandlerMethodCodeFixProvider : CodeFixProvider ImmutableArray.Create(DiagnosticDescriptors.HandlerMethodNotFound.Id); public sealed override FixAllProvider GetFixAllProvider() => - WellKnownFixAllProviders.BatchFixer; + AliasedImageUsingsFixAllProvider.Instance; public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) { @@ -150,7 +150,7 @@ private static async Task CreateMethodAsync( return solution.WithDocumentSyntaxRoot(interfaceDocument.Id, newRoot); } - private static MethodDeclarationSyntax CreateMethodDeclaration(string methodName, bool hasPreImage, bool hasPostImage, string qualifier = null) + internal static MethodDeclarationSyntax CreateMethodDeclaration(string methodName, bool hasPreImage, bool hasPostImage, string qualifier = null) { return SyntaxFactory.MethodDeclaration( SyntaxFactory.PredefinedType(SyntaxFactory.Token(SyntaxKind.VoidKeyword)), @@ -161,7 +161,7 @@ private static MethodDeclarationSyntax CreateMethodDeclaration(string methodName .WithTrailingTrivia(SyntaxFactory.ElasticCarriageReturnLineFeed); } - private static async Task FindInterfaceDeclarationAsync( + internal static async Task FindInterfaceDeclarationAsync( Solution solution, INamedTypeSymbol typeSymbol, CancellationToken cancellationToken) diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs index 29e1ae5..e8dc67c 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs @@ -25,7 +25,7 @@ public class FixHandlerSignatureCodeFixProvider : CodeFixProvider DiagnosticDescriptors.HandlerSignatureMismatchError.Id); public sealed override FixAllProvider GetFixAllProvider() => - WellKnownFixAllProviders.BatchFixer; + AliasedImageUsingsFixAllProvider.Instance; public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) { diff --git a/XrmPluginCore.SourceGenerator/Helpers/RegisterStepHelper.cs b/XrmPluginCore.SourceGenerator/Helpers/RegisterStepHelper.cs index 90fcaff..ec3f097 100644 --- a/XrmPluginCore.SourceGenerator/Helpers/RegisterStepHelper.cs +++ b/XrmPluginCore.SourceGenerator/Helpers/RegisterStepHelper.cs @@ -1,7 +1,9 @@ using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Text; using System.Linq; +using System.Threading; namespace XrmPluginCore.SourceGenerator.Helpers; @@ -109,6 +111,38 @@ public static bool IsRegisterStepCall(InvocationExpressionSyntax invocation, out return false; } + /// + /// Resolves the service type symbol (the second generic argument of a RegisterStep call) from a + /// diagnostic location. Returns null if no enclosing RegisterStep call is found or the type cannot + /// be resolved. + /// + public static INamedTypeSymbol ResolveServiceType( + SyntaxNode root, + SemanticModel semanticModel, + TextSpan diagnosticSpan, + CancellationToken cancellationToken) + { + var diagnosticNode = root.FindNode(diagnosticSpan); + var registerStepInvocation = diagnosticNode.AncestorsAndSelf() + .OfType() + .FirstOrDefault(i => IsRegisterStepCall(i, out _)); + + if (registerStepInvocation == null) + { + return null; + } + + var genericName = GetGenericName(registerStepInvocation); + if (genericName == null || genericName.TypeArgumentList.Arguments.Count < 2) + { + return null; + } + + var serviceTypeSyntax = genericName.TypeArgumentList.Arguments[1]; + var typeInfo = semanticModel.GetTypeInfo(serviceTypeSyntax, cancellationToken); + return typeInfo.Type as INamedTypeSymbol; + } + /// /// Gets the GenericNameSyntax from a RegisterStep call. /// diff --git a/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs b/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs index 598f0fe..c63a840 100644 --- a/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs +++ b/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs @@ -177,6 +177,31 @@ public static SyntaxNode ApplyAliasedImageUsings(SyntaxNode root, string imageNa return ConvertToAliasedUsingsAndQualifyRefs(root, imageNamespace, semanticModel); } + /// + /// Multi-namespace variant of . + /// Adds every distinct aliased image using needed and requalifies every reference in a single pass. + /// Used by the FixAll path so consolidating N registrations funnels through the same engine as a + /// single fix. + /// + /// The compilation unit (document root) to rewrite. + /// The image-registration namespaces to alias and add. + /// The semantic model for 's tree (pre-rewrite). + public static SyntaxNode ApplyAliasedImageUsings(SyntaxNode root, IEnumerable imageNamespaces, SemanticModel semanticModel) + { + if (imageNamespaces == null) + { + return root; + } + + var namespaces = imageNamespaces.Where(ns => !string.IsNullOrEmpty(ns)).Distinct().ToList(); + if (namespaces.Count == 0) + { + return root; + } + + return ConvertToAliasedUsingsAndQualifyRefs(root, namespaces, semanticModel); + } + /// /// Converts existing plain image usings to aliased form, qualifies all bare PreImage/PostImage /// type references, and adds the new aliased using. @@ -189,6 +214,21 @@ public static SyntaxNode ApplyAliasedImageUsings(SyntaxNode root, string imageNa /// references are left unqualified. /// public static SyntaxNode ConvertToAliasedUsingsAndQualifyRefs(SyntaxNode root, string newImageNamespace, SemanticModel semanticModel) + => ConvertToAliasedUsingsAndQualifyRefs(root, new[] { newImageNamespace }, semanticModel); + + /// + /// Converts existing plain image usings to aliased form, qualifies all bare PreImage/PostImage + /// type references, and adds an aliased using for each of . + /// This is the single engine shared by the single-fix and FixAll code paths. + /// + /// The compilation unit (document root) to rewrite. + /// The image-registration namespaces to alias and add. + /// + /// The semantic model for 's original (pre-rewrite) tree, used to resolve + /// which alias a bare PreImage/PostImage reference belongs to. May be null, in which case bare + /// references are left unqualified. + /// + private static SyntaxNode ConvertToAliasedUsingsAndQualifyRefs(SyntaxNode root, IReadOnlyCollection newImageNamespaces, SemanticModel semanticModel) { if (root is not CompilationUnitSyntax compilationUnit) { @@ -211,32 +251,48 @@ public static SyntaxNode ConvertToAliasedUsingsAndQualifyRefs(SyntaxNode root, s } } - // Also include the new namespace - if (!string.IsNullOrEmpty(newImageNamespace)) + // Also include each new namespace + foreach (var newImageNamespace in newImageNamespaces) { - plainNamespaceToAlias[newImageNamespace] = GetLastNamespaceSegment(newImageNamespace); + if (!string.IsNullOrEmpty(newImageNamespace)) + { + plainNamespaceToAlias[newImageNamespace] = GetLastNamespaceSegment(newImageNamespace); + } } // Rewrite the tree var rewriter = new ImageAmbiguityRewriter(plainNamespaceToAlias, semanticModel); var newRoot = rewriter.Visit(root); - // Add the new aliased using if not already present - if (newRoot is CompilationUnitSyntax newCompUnit && !string.IsNullOrEmpty(newImageNamespace)) + // Add each new aliased using if not already present + if (newRoot is CompilationUnitSyntax newCompUnit) { - var newAlias = GetLastNamespaceSegment(newImageNamespace); - var alreadyExists = newCompUnit.Usings.Any(u => - u.Alias?.Name.ToString() == newAlias || - u.Name?.ToString() == newImageNamespace); - - if (!alreadyExists) + var usingsToAdd = new List(); + foreach (var newImageNamespace in newImageNamespaces) { - var aliasedUsing = SyntaxFactory.UsingDirective( - SyntaxFactory.NameEquals(SyntaxFactory.IdentifierName(newAlias)), - SyntaxFactory.ParseName(newImageNamespace)) - .WithTrailingTrivia(SyntaxFactory.CarriageReturnLineFeed); + if (string.IsNullOrEmpty(newImageNamespace)) + { + continue; + } + + var newAlias = GetLastNamespaceSegment(newImageNamespace); + var alreadyExists = newCompUnit.Usings.Any(u => + u.Alias?.Name.ToString() == newAlias || + u.Name?.ToString() == newImageNamespace) || + usingsToAdd.Any(u => u.Alias?.Name.ToString() == newAlias); + + if (!alreadyExists) + { + usingsToAdd.Add(SyntaxFactory.UsingDirective( + SyntaxFactory.NameEquals(SyntaxFactory.IdentifierName(newAlias)), + SyntaxFactory.ParseName(newImageNamespace)) + .WithTrailingTrivia(SyntaxFactory.CarriageReturnLineFeed)); + } + } - newRoot = newCompUnit.AddUsings(aliasedUsing); + if (usingsToAdd.Count > 0) + { + newRoot = newCompUnit.AddUsings(usingsToAdd.ToArray()); } } From 36540509b02b2aa6b9bd52e5cb1fefb4c7dd3018 Mon Sep 17 00:00:00 2001 From: Morten Holt Date: Fri, 19 Jun 2026 14:45:50 +0200 Subject: [PATCH 5/7] Phase 5 (Update and extend the code-fix tests) completed --- .../DiagnosticTests/CodeFixTestBase.cs | 70 +++ ...CreateHandlerMethodCodeFixProviderTests.cs | 132 +++++- ...FixHandlerSignatureCodeFixProviderTests.cs | 404 ++++++++++++++++-- .../AliasedImageUsingsFixAllProvider.cs | 8 +- .../CreateHandlerMethodCodeFixProvider.cs | 20 +- .../FixHandlerSignatureCodeFixProvider.cs | 12 +- 6 files changed, 593 insertions(+), 53 deletions(-) diff --git a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CodeFixTestBase.cs b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CodeFixTestBase.cs index 2c4ab8d..9d74ef3 100644 --- a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CodeFixTestBase.cs +++ b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CodeFixTestBase.cs @@ -3,6 +3,7 @@ using Microsoft.CodeAnalysis.CodeFixes; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.Diagnostics; +using System.Collections.Immutable; using XrmPluginCore.SourceGenerator.Tests.Helpers; namespace XrmPluginCore.SourceGenerator.Tests.DiagnosticTests; @@ -82,6 +83,75 @@ protected static async Task ApplyCodeFixAsync( return newText.ToString(); } + /// + /// Applies the code fix provider's FixAll provider across every matching diagnostic in the + /// document (Document scope), exercising the custom FixAllProvider rather than a single + /// per-diagnostic fix. + /// + protected static async Task ApplyFixAllAsync( + string source, + DiagnosticAnalyzer analyzer, + CodeFixProvider codeFixProvider, + string equivalenceKey, + params string[] diagnosticIds) + { + var document = CreateDocument(source); + + // Compute diagnostics against the document's own compilation so their locations reference the + // document's syntax tree (the FixAll provider maps locations back to documents). + var compilation = await document.Project.GetCompilationAsync(); + var compilationWithAnalyzers = compilation!.WithAnalyzers([analyzer]); + var analyzerDiagnostics = await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); + var diagnostics = analyzerDiagnostics.Where(d => diagnosticIds.Contains(d.Id)).ToImmutableArray(); + + if (diagnostics.IsEmpty) + { + return source; + } + + var fixAllProvider = codeFixProvider.GetFixAllProvider()!; + var fixAllContext = new FixAllContext( + document, + codeFixProvider, + FixAllScope.Document, + equivalenceKey, + diagnosticIds, + new FixAllDiagnosticProvider(diagnostics), + CancellationToken.None); + + var codeAction = await fixAllProvider.GetFixAsync(fixAllContext); + if (codeAction == null) + { + return source; + } + + var operations = await codeAction.GetOperationsAsync(CancellationToken.None); + var changedSolution = operations.OfType().Single().ChangedSolution; + var changedDocument = changedSolution.GetDocument(document.Id); + var newText = await changedDocument!.GetTextAsync(); + + return newText.ToString(); + } + + private sealed class FixAllDiagnosticProvider : FixAllContext.DiagnosticProvider + { + private readonly ImmutableArray _diagnostics; + + public FixAllDiagnosticProvider(ImmutableArray diagnostics) + { + _diagnostics = diagnostics; + } + + public override Task> GetDocumentDiagnosticsAsync(Document document, CancellationToken cancellationToken) + => Task.FromResult>(_diagnostics); + + public override Task> GetAllDiagnosticsAsync(Project project, CancellationToken cancellationToken) + => Task.FromResult>(_diagnostics); + + public override Task> GetProjectDiagnosticsAsync(Project project, CancellationToken cancellationToken) + => Task.FromResult>(Enumerable.Empty()); + } + protected static async Task> GetCodeActionsAsync( string source, DiagnosticAnalyzer analyzer, diff --git a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs index a2462b1..95f8f19 100644 --- a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs +++ b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs @@ -1,6 +1,7 @@ using FluentAssertions; using XrmPluginCore.SourceGenerator.Analyzers; using XrmPluginCore.SourceGenerator.CodeFixes; +using XrmPluginCore.SourceGenerator.Tests.Helpers; using Xunit; namespace XrmPluginCore.SourceGenerator.Tests.DiagnosticTests; @@ -55,11 +56,11 @@ public class TestService : ITestService // Act var fixedSource = await ApplyCodeFixAsync(source); - // Assert - Method created with PreImage parameter - fixedSource.Should().Contain("void HandleUpdate(PreImage preImage)"); + // Assert - Method created with alias-qualified PreImage parameter + fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)"); - // Assert - Using directive added - fixedSource.Should().Contain("using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); + // Assert - Aliased using directive added + fixedSource.Should().Contain("using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); } [Fact] @@ -107,11 +108,11 @@ public class TestService : ITestService // Act var fixedSource = await ApplyCodeFixAsync(source); - // Assert - Method created with both image parameters - fixedSource.Should().Contain("void HandleUpdate(PreImage preImage, PostImage postImage)"); + // Assert - Method created with both alias-qualified image parameters + fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage, AccountUpdatePostOperation.PostImage postImage)"); - // Assert - Using directive added - fixedSource.Should().Contain("using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); + // Assert - Aliased using directive added + fixedSource.Should().Contain("using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); } [Fact] @@ -165,9 +166,11 @@ public class TestService : ITestService } [Fact] - public async Task Should_Avoid_Ambiguous_Usings() + public async Task Should_Use_Aliased_Usings_For_Multiple_Registrations() { - // Arrange - Using directive already exists for Update registration, method missing for Delete + // Arrange - A plain using already exists for the Update registration; the Delete handler is + // missing from the interface. Creating Delete must requalify the existing Update reference to + // its alias (resolved via the semantic model) and add the Delete using in aliased form. const string source = """ using System; using System.ComponentModel; @@ -211,22 +214,118 @@ public class TestService : ITestService public void HandleUpdate(PreImage preImage) { } } } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } """; // Act var fixedSource = await ApplyCodeFixAsync(source); - // Assert - New method created with qualified type + // Assert - New method created with its own alias-qualified type fixedSource.Should().Contain("void HandleDelete(AccountDeletePostOperation.PreImage preImage)"); - // Assert - Existing PreImage references qualified with alias + // Assert - Existing bare PreImage references requalified with the Update alias CountOccurrences(fixedSource, "AccountUpdatePostOperation.PreImage preImage").Should().BeGreaterOrEqualTo(1); - // Assert - Aliased usings + // Assert - Each namespace aliased exactly once CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;") - .Should().Be(1, "the existing using should be converted to aliased form"); + .Should().Be(1, "the existing plain using should be converted to aliased form"); CountOccurrences(fixedSource, "using AccountDeletePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;") .Should().Be(1, "the new using should be added in aliased form"); + + AssertNoAmbiguousReferences(fixedSource); + } + + [Fact] + public async Task FixAll_Should_Create_Multiple_Methods_On_Same_Interface() + { + // Arrange - Two same-service registrations, both missing their handler methods. + const string source = """ + using System; + using System.ComponentModel; + using Microsoft.Xrm.Sdk; + using XrmPluginCore; + using XrmPluginCore.Enums; + using Microsoft.Extensions.DependencyInjection; + using XrmPluginCore.Tests.Context.BusinessDomain; + + namespace TestNamespace + { + public class TestPlugin : Plugin + { + public TestPlugin() + { + RegisterStep(EventOperation.Update, ExecutionStage.PostOperation, + "HandleUpdate") + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + + RegisterStep(EventOperation.Delete, ExecutionStage.PostOperation, + "HandleDelete") + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + } + + protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services) + { + return services.AddScoped(); + } + } + + public interface ITestService + { + } + + public class TestService : ITestService + { + } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + """; + + // Act - Apply the consolidating FixAll across all diagnostics in one pass + var fixedSource = await ApplyFixAllAsync(source); + + // Assert - Both methods created with their own alias-qualified parameter + fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)"); + fixedSource.Should().Contain("void HandleDelete(AccountDeletePostOperation.PreImage preImage)"); + + // Assert - One aliased using per distinct namespace (no duplicates) + CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;").Should().Be(1); + CountOccurrences(fixedSource, "using AccountDeletePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;").Should().Be(1); + + AssertNoAmbiguousReferences(fixedSource); + } + + private static void AssertNoAmbiguousReferences(string source) + { + var compilation = CompilationHelper.CreateCompilation(source); + var ambiguous = compilation.GetDiagnostics() + .Where(d => d.Id == "CS0104") + .Select(d => d.GetMessage()) + .ToList(); + ambiguous.Should().BeEmpty("the fixed source should not contain ambiguous references"); } private static int CountOccurrences(string source, string search) @@ -244,4 +343,9 @@ private static int CountOccurrences(string source, string search) private static Task ApplyCodeFixAsync(string source) => ApplyCodeFixAsync(source, new HandlerMethodNotFoundAnalyzer(), new CreateHandlerMethodCodeFixProvider(), DiagnosticDescriptors.HandlerMethodNotFound.Id); + + private static Task ApplyFixAllAsync(string source) + => ApplyFixAllAsync(source, new HandlerMethodNotFoundAnalyzer(), new CreateHandlerMethodCodeFixProvider(), + nameof(CreateHandlerMethodCodeFixProvider), + DiagnosticDescriptors.HandlerMethodNotFound.Id); } diff --git a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs index cf56b34..02508ab 100644 --- a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs +++ b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs @@ -1,6 +1,7 @@ using FluentAssertions; using XrmPluginCore.SourceGenerator.Analyzers; using XrmPluginCore.SourceGenerator.CodeFixes; +using XrmPluginCore.SourceGenerator.Tests.Helpers; using Xunit; namespace XrmPluginCore.SourceGenerator.Tests.DiagnosticTests; @@ -57,11 +58,11 @@ public void HandleUpdate() { } // Act var fixedSource = await ApplyCodeFixAsync(source); - // Assert - Signature fixed - fixedSource.Should().Contain("void HandleUpdate(PreImage preImage)"); + // Assert - Signature fixed (alias-qualified) + fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)"); - // Assert - Using directive added - fixedSource.Should().Contain("using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); + // Assert - Aliased using directive added + fixedSource.Should().Contain("using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); } [Fact] @@ -110,11 +111,11 @@ public void HandleUpdate() { } // Act var fixedSource = await ApplyCodeFixAsync(source); - // Assert - Signature fixed - fixedSource.Should().Contain("void HandleUpdate(PostImage postImage)"); + // Assert - Signature fixed (alias-qualified) + fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PostImage postImage)"); - // Assert - Using directive added - fixedSource.Should().Contain("using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); + // Assert - Aliased using directive added + fixedSource.Should().Contain("using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); } [Fact] @@ -164,11 +165,11 @@ public void HandleUpdate() { } // Act var fixedSource = await ApplyCodeFixAsync(source); - // Assert - Signature fixed with both image parameters - fixedSource.Should().Contain("void HandleUpdate(PreImage preImage, PostImage postImage)"); + // Assert - Signature fixed with both image parameters (alias-qualified) + fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage, AccountUpdatePostOperation.PostImage postImage)"); - // Assert - Using directive added - fixedSource.Should().Contain("using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); + // Assert - Aliased using directive added + fixedSource.Should().Contain("using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"); } [Fact] @@ -218,19 +219,22 @@ public void HandleUpdate() { } // Act var fixedSource = await ApplyCodeFixAsync(source); - // Assert - Signature fixed - fixedSource.Should().Contain("void HandleUpdate(PreImage preImage)"); + // Assert - Signature fixed (alias-qualified) + fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)"); - // Assert - Using directive not duplicated (count occurrences) - var usingDirective = "using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;"; - var count = CountOccurrences(fixedSource, usingDirective); - count.Should().Be(1, "the using directive should not be duplicated"); + // Assert - The pre-existing plain using is converted to the aliased form (not duplicated) + CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;") + .Should().Be(1, "the using directive should be converted to aliased form exactly once"); + CountOccurrences(fixedSource, "using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;") + .Should().Be(0, "the plain using should no longer be present"); } [Fact] - public async Task Should_Avoid_Ambiguous_Usings() + public async Task Should_Use_Aliased_Usings_For_Multiple_Registrations() { - // Arrange - Using directive already exists for Update registration + // Arrange - A plain using already exists for the Update registration; the Delete handler is + // missing its image parameter. Fixing Delete must requalify the existing Update reference to + // its alias (resolved via the semantic model) and add the Delete using in aliased form. const string source = """ using System; using System.ComponentModel; @@ -276,20 +280,367 @@ public void HandleUpdate(PreImage preImage) { } public void HandleDelete() { } } } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } """; // Act var fixedSource = await ApplyCodeFixAsync(source); - // Assert - Signature fixed + // Assert - Both signatures alias-qualified (interface + implementation) CountOccurrences(fixedSource, "void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)").Should().Be(2, "both the interface and implementation are updated"); CountOccurrences(fixedSource, "void HandleDelete(AccountDeletePostOperation.PreImage preImage)").Should().Be(2, "both the interface and implementation are updated"); - // Assert - Using directive not duplicated (count occurrences) + // Assert - Each namespace aliased exactly once + CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;") + .Should().Be(1, "the existing plain using should be converted to aliased form exactly once"); + CountOccurrences(fixedSource, "using AccountDeletePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;") + .Should().Be(1, "the new using should be added in aliased form exactly once"); + + // Assert - Result compiles without ambiguous-reference (CS0104) errors + AssertNoAmbiguousReferences(fixedSource); + } + + [Fact] + public async Task Should_Not_Duplicate_Already_Aliased_Using() + { + // Arrange - The aliased using is already present, but the handler still needs its parameter. + const string source = """ + using System; + using System.ComponentModel; + using Microsoft.Xrm.Sdk; + using XrmPluginCore; + using XrmPluginCore.Enums; + using Microsoft.Extensions.DependencyInjection; + using XrmPluginCore.Tests.Context.BusinessDomain; + using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation; + + namespace TestNamespace + { + public class TestPlugin : Plugin + { + public TestPlugin() + { + RegisterStep(EventOperation.Update, ExecutionStage.PostOperation, + nameof(ITestService.HandleUpdate)) + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + } + + protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services) + { + return services.AddScoped(); + } + } + + public interface ITestService + { + void HandleUpdate(); + } + + public class TestService : ITestService + { + public void HandleUpdate() { } + } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + """; + + // Act + var fixedSource = await ApplyCodeFixAsync(source); + + // Assert - Signature fixed with the alias-qualified parameter + CountOccurrences(fixedSource, "void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)").Should().Be(2); + + // Assert - The already-present aliased using is not duplicated + CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;") + .Should().Be(1, "the existing aliased using should not be duplicated"); + + AssertNoAmbiguousReferences(fixedSource); + } + + [Fact] + public async Task Should_Realias_CleanedUp_Plain_Using() + { + // Arrange - The Update registration is fully aliased and correct (no diagnostic). A developer + // has "cleaned up" the Delete import back to a plain using, and the Delete handler is missing + // its parameter. After the fix, every image using must be aliased exactly once. + const string source = """ + using System; + using System.ComponentModel; + using Microsoft.Xrm.Sdk; + using XrmPluginCore; + using XrmPluginCore.Enums; + using Microsoft.Extensions.DependencyInjection; + using XrmPluginCore.Tests.Context.BusinessDomain; + using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation; + using TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation; + + namespace TestNamespace + { + public class TestPlugin : Plugin + { + public TestPlugin() + { + RegisterStep(EventOperation.Update, ExecutionStage.PostOperation, + nameof(ITestService.HandleUpdate)) + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + + RegisterStep(EventOperation.Delete, ExecutionStage.PostOperation, + nameof(ITestService.HandleDelete)) + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + } + + protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services) + { + return services.AddScoped(); + } + } + + public interface ITestService + { + void HandleUpdate(AccountUpdatePostOperation.PreImage preImage); + void HandleDelete(); + } + + public class TestService : ITestService + { + public void HandleUpdate(AccountUpdatePostOperation.PreImage preImage) { } + public void HandleDelete() { } + } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + """; + + // Act + var fixedSource = await ApplyCodeFixAsync(source); + + // Assert - The stale plain Delete using is converted to aliased form + CountOccurrences(fixedSource, "using TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;") + .Should().Be(0, "the plain using should be re-aliased"); + + // Assert - Every image namespace is aliased exactly once CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;") - .Should().Be(1, "the using directive should be de-ambiguified"); + .Should().Be(1); CountOccurrences(fixedSource, "using AccountDeletePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;") - .Should().Be(1, "the using directive should be de-ambiguified"); + .Should().Be(1); + + // Assert - Delete signature fixed, Update signature preserved + CountOccurrences(fixedSource, "void HandleDelete(AccountDeletePostOperation.PreImage preImage)").Should().Be(2); + CountOccurrences(fixedSource, "void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)").Should().Be(2); + + AssertNoAmbiguousReferences(fixedSource); + } + + [Fact] + public async Task Should_Requalify_Each_Reference_Under_Multiple_Plain_Usings() + { + // Arrange - Two plain image usings are present at once. The Update handler references a bare + // PreImage and the Delete handler a bare PostImage (each generated namespace only exposes the + // image type its registration declared, so each bare reference still binds uniquely). The + // Create handler is missing its parameter and triggers the fix. The fix must requalify each + // pre-existing reference to its OWN alias via the semantic model - the old break-on-first + // logic would have mis-qualified one of them. + const string source = """ + using System; + using System.ComponentModel; + using Microsoft.Xrm.Sdk; + using XrmPluginCore; + using XrmPluginCore.Enums; + using Microsoft.Extensions.DependencyInjection; + using XrmPluginCore.Tests.Context.BusinessDomain; + using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation; + using TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation; + + namespace TestNamespace + { + public class TestPlugin : Plugin + { + public TestPlugin() + { + RegisterStep(EventOperation.Update, ExecutionStage.PostOperation, + nameof(ITestService.HandleUpdate)) + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + + RegisterStep(EventOperation.Delete, ExecutionStage.PostOperation, + nameof(ITestService.HandleDelete)) + .AddFilteredAttributes(x => x.Name) + .WithPostImage(x => x.Name, x => x.AccountNumber); + + RegisterStep(EventOperation.Create, ExecutionStage.PostOperation, + nameof(ITestService.HandleCreate)) + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + } + + protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services) + { + return services.AddScoped(); + } + } + + public interface ITestService + { + void HandleUpdate(PreImage preImage); + void HandleDelete(PostImage postImage); + void HandleCreate(); + } + + public class TestService : ITestService + { + public void HandleUpdate(PreImage preImage) { } + public void HandleDelete(PostImage postImage) { } + public void HandleCreate() { } + } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation + { + public sealed class PostImage { } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountCreatePostOperation + { + public sealed class PreImage { } + } + """; + + // Act + var fixedSource = await ApplyCodeFixAsync(source); + + // Assert - Each pre-existing reference requalified to its OWN alias (never cross-qualified) + CountOccurrences(fixedSource, "void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)").Should().Be(2); + CountOccurrences(fixedSource, "void HandleDelete(AccountDeletePostOperation.PostImage postImage)").Should().Be(2); + + // Assert - The triggering handler created with its own alias + CountOccurrences(fixedSource, "void HandleCreate(AccountCreatePostOperation.PreImage preImage)").Should().Be(2); + + // Assert - Each namespace aliased exactly once + CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;").Should().Be(1); + CountOccurrences(fixedSource, "using AccountDeletePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;").Should().Be(1); + CountOccurrences(fixedSource, "using AccountCreatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountCreatePostOperation;").Should().Be(1); + + AssertNoAmbiguousReferences(fixedSource); + } + + [Fact] + public async Task FixAll_Should_Consolidate_Multiple_Registrations() + { + // Arrange - Two same-service registrations both trigger the diagnostic. + const string source = """ + using System; + using System.ComponentModel; + using Microsoft.Xrm.Sdk; + using XrmPluginCore; + using XrmPluginCore.Enums; + using Microsoft.Extensions.DependencyInjection; + using XrmPluginCore.Tests.Context.BusinessDomain; + + namespace TestNamespace + { + public class TestPlugin : Plugin + { + public TestPlugin() + { + RegisterStep(EventOperation.Update, ExecutionStage.PostOperation, + nameof(ITestService.HandleUpdate)) + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + + RegisterStep(EventOperation.Delete, ExecutionStage.PostOperation, + nameof(ITestService.HandleDelete)) + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + } + + protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services) + { + return services.AddScoped(); + } + } + + public interface ITestService + { + void HandleUpdate(); + void HandleDelete(); + } + + public class TestService : ITestService + { + public void HandleUpdate() { } + public void HandleDelete() { } + } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + """; + + // Act - Apply the consolidating FixAll across all diagnostics in one pass + var fixedSource = await ApplyFixAllAsync(source); + + // Assert - Every signature alias-qualified (interface + implementation) + CountOccurrences(fixedSource, "void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)").Should().Be(2); + CountOccurrences(fixedSource, "void HandleDelete(AccountDeletePostOperation.PreImage preImage)").Should().Be(2); + + // Assert - One aliased using per distinct namespace (no duplicates) + CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;").Should().Be(1); + CountOccurrences(fixedSource, "using AccountDeletePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;").Should().Be(1); + + AssertNoAmbiguousReferences(fixedSource); + } + + private static void AssertNoAmbiguousReferences(string source) + { + var compilation = CompilationHelper.CreateCompilation(source); + var ambiguous = compilation.GetDiagnostics() + .Where(d => d.Id == "CS0104") + .Select(d => d.GetMessage()) + .ToList(); + ambiguous.Should().BeEmpty("the fixed source should not contain ambiguous references"); } private static int CountOccurrences(string source, string search) @@ -307,4 +658,9 @@ private static int CountOccurrences(string source, string search) private static Task ApplyCodeFixAsync(string source) => ApplyCodeFixAsync(source, new HandlerSignatureMismatchAnalyzer(), new FixHandlerSignatureCodeFixProvider(), DiagnosticDescriptors.HandlerSignatureMismatch.Id, DiagnosticDescriptors.HandlerSignatureMismatchError.Id); + + private static Task ApplyFixAllAsync(string source) + => ApplyFixAllAsync(source, new HandlerSignatureMismatchAnalyzer(), new FixHandlerSignatureCodeFixProvider(), + nameof(FixHandlerSignatureCodeFixProvider), + DiagnosticDescriptors.HandlerSignatureMismatch.Id, DiagnosticDescriptors.HandlerSignatureMismatchError.Id); } diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs index 123d93a..c9bb063 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs @@ -244,7 +244,10 @@ private static async Task ApplyBatchAsync( return solution; } - var newRoot = root; + // One consolidated always-aliased using rewrite FIRST, on the original tree, so the rewriter + // resolves bare references against a semantic model whose nodes still match. All structural + // edits below produce already alias-qualified parameters, so requalification skips them. + var newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(root, batch.Namespaces, semanticModel); // Fix existing handler signatures (interface + implementations) to their alias-qualified form. foreach (var edit in DistinctEdits(batch.SignatureEdits)) @@ -272,9 +275,6 @@ private static async Task ApplyBatchAsync( } } - // One consolidated always-aliased using rewrite for every namespace touched in this document. - newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, batch.Namespaces, semanticModel); - return solution.WithDocumentSyntaxRoot(documentId, newRoot); } diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs index 76c1674..47571ba 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs @@ -139,13 +139,21 @@ private static async Task CreateMethodAsync( var methodDeclaration = CreateMethodDeclaration(methodName, hasPreImage, hasPostImage, alias); - var newInterface = interfaceDeclaration.AddMembers(methodDeclaration); - var newRoot = interfaceRoot.ReplaceNode(interfaceDeclaration, newInterface); - - // Always emit the aliased using and requalify existing image references. The interface may live - // in a different tree than the trigger, so resolve its semantic model from the compilation. + // Emit the aliased using and requalify existing image references FIRST, on the original tree, + // so the rewriter resolves bare references against a semantic model whose nodes still match. + // The created method's parameters are already alias-qualified, so requalification skips them. + // The interface may live in a different tree than the trigger, so resolve its semantic model + // from the compilation. var interfaceSemanticModel = semanticModel.Compilation.GetSemanticModel(interfaceRoot.SyntaxTree); - newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, imageNamespace, interfaceSemanticModel); + var newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(interfaceRoot, imageNamespace, interfaceSemanticModel); + + // Then add the method to the (re-found) interface declaration. + var targetInterface = newRoot.DescendantNodes().OfType() + .FirstOrDefault(i => i.Identifier.Text == interfaceDeclaration.Identifier.Text); + if (targetInterface != null) + { + newRoot = newRoot.ReplaceNode(targetInterface, targetInterface.AddMembers(methodDeclaration)); + } return solution.WithDocumentSyntaxRoot(interfaceDocument.Id, newRoot); } diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs index e8dc67c..c84966e 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs @@ -213,8 +213,14 @@ private static async Task FixMethodDeclarationsAsync( } } + // Emit the aliased using and requalify existing image references FIRST, on the original + // tree, so the rewriter resolves bare references against a semantic model whose nodes still + // match. The replacement parameters below are already alias-qualified, so the requalifier + // leaves them untouched. + var treeSemanticModel = compilation.GetSemanticModel(tree); + var newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(methodRoot, imageNamespace, treeSemanticModel); + // Replace all matching method declarations one at a time, re-finding after each - SyntaxNode newRoot = methodRoot; foreach (var target in targets) { var current = newRoot.DescendantNodes().OfType() @@ -226,10 +232,6 @@ private static async Task FixMethodDeclarationsAsync( } } - // Always emit the aliased using and requalify existing image references. - var treeSemanticModel = compilation.GetSemanticModel(tree); - newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, imageNamespace, treeSemanticModel); - solution = solution.WithDocumentSyntaxRoot(methodDocument.Id, newRoot); } From c0ba2f62639974014450cecf8054b240a1409360 Mon Sep 17 00:00:00 2001 From: Morten Holt Date: Fri, 19 Jun 2026 14:57:01 +0200 Subject: [PATCH 6/7] Remove the SPEC file --- .conducktor/SPEC.md | 171 -------------------------------------------- .gitignore | 3 + 2 files changed, 3 insertions(+), 171 deletions(-) delete mode 100644 .conducktor/SPEC.md diff --git a/.conducktor/SPEC.md b/.conducktor/SPEC.md deleted file mode 100644 index bebbcac..0000000 --- a/.conducktor/SPEC.md +++ /dev/null @@ -1,171 +0,0 @@ -# Always generate aliased usings for plugin image registrations - -## Context - -The `XrmPluginCore.SourceGenerator` ships analyzers + code-fix providers that wire up type-safe Pre/Post image handler signatures. When a `RegisterStep(...)` registration declares an image via `WithPreImage`/`WithPostImage`/`AddImage` but the referenced handler method's signature does not match, two diagnostics/code-fixes come into play: - -- **`FixHandlerSignatureCodeFixProvider`** (`XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs`) — fixes an existing handler method's parameter list to accept the registered `PreImage`/`PostImage` wrapper types. Fires on `DiagnosticDescriptors.HandlerSignatureMismatch.Id` and `DiagnosticDescriptors.HandlerSignatureMismatchError.Id`. -- **`CreateHandlerMethodCodeFixProvider`** (`XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs`) — creates a missing handler method on the service interface with the correct image parameters. Fires on `DiagnosticDescriptors.HandlerMethodNotFound.Id`. - -Each registration's image wrapper classes (`PreImage`, `PostImage`) are generated into an isolated namespace produced by `RegisterStepHelper.GetExpectedImageNamespace(...)` (`XrmPluginCore.SourceGenerator/Helpers/RegisterStepHelper.cs:17`), with the shape: - -``` -{PluginNamespace}.PluginRegistrations.{PluginClassName}.{EntityTypeName}{Operation}{Stage} -``` - -e.g. `Some.Namespace.Plugins.PluginRegistrations.SomePlugin.LeadUpdatePostOperation`. The shared marker for these namespaces is the substring `.PluginRegistrations.` (`SyntaxFactoryHelper.IsImageRegistrationNamespace`, `SyntaxFactoryHelper.cs:207`). The alias used is the last namespace segment (`GetLastNamespaceSegment`, `SyntaxFactoryHelper.cs:212`), e.g. `LeadUpdatePostOperation`. Both `PreImage` and `PostImage` are simple type names (`Constants.PreImageTypeName = "PreImage"`, `Constants.PostImageTypeName = "PostImage"`), so two registrations in the same file both expose a `PreImage`/`PostImage` type. - -**The bug:** Today the code fixes try to be clever — they only convert to aliased usings *when an ambiguity is detected*, otherwise they add a plain (non-aliased) `using`. This is driven by `SyntaxFactoryHelper.DetectImageAmbiguity(...)` (`SyntaxFactoryHelper.cs:118`): - -- In `FixHandlerSignatureCodeFixProvider.FixMethodDeclarationsAsync` (lines 199–235): `ambiguity = DetectImageAmbiguity(...)`; `CreateImageParameterList(hasPreImage, hasPostImage, ambiguity.needsAlias ? ambiguity.alias : null)`; then `if (ambiguity.needsAlias) ConvertToAliasedUsingsAndQualifyRefs(...) else AddUsingDirectiveIfMissing(...)`. -- In `CreateHandlerMethodCodeFixProvider.CreateMethodAsync` (lines 135–152): same `DetectImageAmbiguity` / conditional pattern via `CreateMethodDeclaration(..., needsAlias ? alias : null)` and the same `if (needsAlias) ConvertToAliasedUsingsAndQualifyRefs else AddUsingDirectiveIfMissing` branch. - -When multiple plugins use the **same service** and more than one of those registrations declares Pre/Post images, the automatic generation of usings and modification of parameters fails, because the plain-using path produces bare `PreImage`/`PostImage` references that collide across the multiple image namespaces, and the on-demand conversion logic (`ImageAmbiguityRewriter`, `SyntaxFactoryHelper.cs:234`) assumes "there should be exactly one existing plain image using at this point" (`SyntaxFactoryHelper.cs:309`, the `break;` after taking the first entry of `_typeToExistingAlias`). That assumption breaks with multiple image usings. - -**The decision:** Stop trying to convert on demand. **Always** emit aliased usings and **always** qualify the image parameter types with the alias. This makes every emitted reference unambiguous regardless of how many same-service registrations exist in the file. - -Existing tests assert the *old* behavior and must be updated. See: -- `XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs` — `Should_Fix_Signature_And_Add_Using_For_PreImage` (line ~61/64), `..._For_PostImage` (line ~114/117), `..._For_Both_Images` (line ~120), and `Should_Avoid_Ambiguous_Usings` (line 231). -- `XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs` (same directory). - -## Goals - -- Both code-fix providers **always** emit an **aliased** using directive of the form `using LeadUpdatePostOperation = Some.Namespace.Plugins.PluginRegistrations.SomePlugin.LeadUpdatePostOperation;` for the image namespace involved, never a plain `using Some.Namespace...PluginRegistrations...;`. -- Both code-fix providers **always** write image parameters with the alias prefix, e.g. `void OnUpdateLead(LeadUpdatePostOperation.PreImage preImage)` (and `LeadUpdatePostOperation.PostImage postImage`), regardless of whether any ambiguity currently exists. -- Applying either fix to a file where several same-service registrations have Pre/Post images produces compiling, unambiguous code (no `CS0104` ambiguous-reference errors, no collisions between `PreImage`/`PostImage` across registrations). -- On each run, the fix **re-evaluates** existing image-registration usings in the document: if a developer has "cleaned up" / hand-edited the imports in the meantime (e.g. removed an alias, reverted to a plain using, or left a stale bare reference), the fix rewrites those identified usings back to the consistent aliased form and re-qualifies their references. -- The existing test suite is updated to assert the always-aliased behavior and continues to pass. - -## Non-goals - -- Changing the wrapper-class source generation itself (`WrapperClassGenerator`, `PluginImageGenerator`) — namespaces, class shapes, and the `PreImage`/`PostImage` type names stay as-is. The generators emit each wrapper class into its own isolated namespace (`WrapperClassGenerator` writes `namespace {metadata.RegistrationNamespace}` at `WrapperClassGenerator.cs:32` and never emits `using` directives into the consumer's source files), so generated output is already unambiguous by construction and needs no change. The ambiguity this spec addresses is introduced only by the code-fix providers' edits to *user* code. -- Changing the diagnostics/analyzers (`HandlerSignatureMismatchAnalyzer`, `HandlerMethodNotFoundAnalyzer`) — the namespace is still passed via `Constants.PropertyImageNamespace`; only the code-fix output format changes. -- Touching non-image usings. Only usings whose namespace contains `.PluginRegistrations.` are rewritten; all other usings are left untouched. -- Rewriting `member access` expressions like `obj.PreImage`, the alias-name side of a `using X = ...` (`NameEqualsSyntax`), or already-qualified references — these exclusions in `ImageAmbiguityRewriter.VisitIdentifierName` (`SyntaxFactoryHelper.cs:284–300`) must be preserved. - -## Requirements - -- The aliased using format is exactly: `using {alias} = {fullNamespace};` where `alias = GetLastNamespaceSegment(fullNamespace)` (last dot-separated segment) and `fullNamespace` is the value from `Constants.PropertyImageNamespace`. Example: `using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;`. -- The qualified parameter format is exactly: `{alias}.PreImage preImage` and/or `{alias}.PostImage postImage`, in that order (PreImage before PostImage), matching `CreateImageParameterList`'s ordering (`SyntaxFactoryHelper.cs:43–57`). -- Idempotency: applying the fix to a file that already has the correct aliased using must not duplicate it. The existing duplicate-guard in `ConvertToAliasedUsingsAndQualifyRefs` (`SyntaxFactoryHelper.cs:189–192`, checks `u.Alias?.Name.ToString() == newAlias || u.Name?.ToString() == newImageNamespace`) covers this; verify it still holds when the always-alias path is the only path. -- Multiple same-service registrations: when two registrations (e.g. `AccountUpdatePostOperation` and `AccountDeletePostOperation`) both declare images and both handlers are fixed in the same document, each gets its own aliased using and each parameter is prefixed with its own alias. No bare `PreImage`/`PostImage` remains in fixed signatures. -- Re-evaluation of stale imports: when the fix runs, every plain (non-aliased) using whose namespace `IsImageRegistrationNamespace` is true must be converted to its aliased form, and any bare `PreImage`/`PostImage` *type references* under that plain using must be re-qualified to the matching alias. This is the existing `ConvertToAliasedUsingsAndQualifyRefs` + `ImageAmbiguityRewriter` machinery — but it must now correctly handle **more than one** plain image using at a time (the current `break;`-after-first assumption at `SyntaxFactoryHelper.cs:306–310` is insufficient). -- Resolving which alias a bare `PreImage`/`PostImage` reference belongs to (the multi-using case): **use the semantic model to resolve each bare reference's symbol to its declaring namespace, then map that namespace back to its alias.** Do not guess from the bare name alone and do not fall back to "exactly one plain image using" — attempt to resolve every bare reference uniquely via its symbol. If the semantic model resolves the reference to a type whose containing namespace is one of the (now-aliased) image namespaces, qualify the reference with that namespace's alias (`GetLastNamespaceSegment`). If a reference cannot be resolved uniquely to a single image namespace (e.g. the symbol is missing/erroneous, or genuinely ambiguous), leave that reference unqualified rather than guessing — it will surface as a normal compile error for the developer to resolve. This requires threading a `SemanticModel`/`Compilation` (or per-tree `SemanticModel`) into `ConvertToAliasedUsingsAndQualifyRefs` / `ImageAmbiguityRewriter`, which today operate purely on syntax. -- The `DetectImageAmbiguity` method and the conditional `if (needsAlias) ... else AddUsingDirectiveIfMissing(...)` branches in both providers should no longer gate behavior — the aliased path becomes unconditional. `DetectImageAmbiguity` / `AddUsingDirectiveIfMissing` may become dead code; remove them if unused, or keep with updated callers — implementer's discretion, but no caller should rely on the plain-using branch for image namespaces. -- Both providers (`FixHandlerSignatureCodeFixProvider`, `CreateHandlerMethodCodeFixProvider`) must share the always-alias logic rather than each carrying a near-duplicate call site. Today each independently calls `DetectImageAmbiguity` + `CreateImageParameterList` + the using branch. Extract the common steps — compute alias from `imageNamespace`, build the alias-qualified parameter list, and run the always-aliased using conversion/requalification — into a single shared helper (on `SyntaxFactoryHelper`, or a new small helper type) that both providers call. No image-using/alias logic should be duplicated across the two providers. -- `BuildSignatureDescription` (`SyntaxFactoryHelper.cs:66`) is used only for code-action titles (e.g. `Fix signature to 'HandleUpdate(PreImage preImage)'`) and uses unqualified type names; titles may stay unqualified — they are human-facing labels, not emitted code. -- **FixAll must produce the same always-aliased, unambiguous result as a single fix.** Both `FixHandlerSignatureCodeFixProvider` and `CreateHandlerMethodCodeFixProvider` currently return `WellKnownFixAllProviders.BatchFixer` from `GetFixAllProvider()` (`FixHandlerSignatureCodeFixProvider.cs:27–28`, `CreateHandlerMethodCodeFixProvider.cs:24–25`). The batch fixer is unsafe here: it computes each diagnostic's `CodeAction` independently against the *original* document and then merges text changes, so when one document holds several image registrations (the exact same-service-multiple-images scenario this spec targets), the per-diagnostic fixes each add their own aliased using and rewrite the shared `using` block / overlapping references without seeing each other's edits — producing merge conflicts or non-convergent, still-ambiguous output. The goal is to be as unambiguous as possible, so FixAll must be handled explicitly rather than left to `BatchFixer`. See the dedicated phase below: a custom `FixAllProvider` consolidates *all* fixable diagnostics in a document into a single pass that fixes every target signature with its own alias and runs one consolidated always-aliased using rewrite (adding every needed aliased using and requalifying every reference once). This shared FixAll logic must not be duplicated between the two providers. - -## Open questions - -- None outstanding. (Resolved: bare-reference alias resolution uses the semantic model; the two providers share one always-alias helper; FixAll is handled by a custom consolidating provider; generators need no change.) - -## Phases - -### Phase 1: Build the shared always-aliased helper in `SyntaxFactoryHelper` (with semantic-model requalification) - -This phase creates the single shared engine that both code-fix providers will call (resolving the "no duplication" decision) and makes the using-rewrite correct for multiple plain image usings via the semantic model (resolving the "which alias" decision). Edit `XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs`. Three parts: - -1. **Expose the alias helper.** Add `public static string GetAliasForImageNamespace(string ns) => GetLastNamespaceSegment(ns);` (or make `GetLastNamespaceSegment` accessible). Keep `GetLastNamespaceSegment` semantics: substring after the last `.`, or the whole string if no dot (`SyntaxFactoryHelper.cs:212–216`). - -2. **Add the shared always-alias entry point.** Add a single helper that both providers call, e.g.: - ```csharp - // Always emits the aliased using for imageNamespace and re-qualifies/aliases every - // existing image-registration using in the tree. `semanticModel` is the model for `root`'s tree. - public static SyntaxNode ApplyAliasedImageUsings(SyntaxNode root, string imageNamespace, SemanticModel semanticModel) - ``` - It computes `alias = GetAliasForImageNamespace(imageNamespace)`, then delegates to `ConvertToAliasedUsingsAndQualifyRefs(root, imageNamespace, semanticModel)` (see part 3). Guard for null/empty `imageNamespace` (return `root` unchanged / fall back to no-op — the analyzer only sets `Constants.PropertyImageNamespace` when an expected namespace exists; `HandlerSignatureMismatchAnalyzer.cs:119–121`). The alias-qualified **parameter list** is still produced by the existing `CreateImageParameterList(bool, bool, string qualifier)` (`SyntaxFactoryHelper.cs:41–58`), which emits `QualifiedName(IdentifierName(qualifier), IdentifierName(typeName))` (`SyntaxFactoryHelper.cs:218–228`) — both providers pass `alias` as the qualifier (never `null`/conditional). - -3. **Thread the semantic model into `ConvertToAliasedUsingsAndQualifyRefs` + `ImageAmbiguityRewriter` and fix the multi-using requalification.** Today both operate purely on syntax. Change the signature of `ConvertToAliasedUsingsAndQualifyRefs` (`SyntaxFactoryHelper.cs:152`) to also accept a `SemanticModel`, and pass it into the `ImageAmbiguityRewriter` constructor (`SyntaxFactoryHelper.cs:242`). The rewriter currently (a) collects all plain image namespaces into `plainNamespaceToAlias` (lines 159–173 of `ConvertToAliasedUsingsAndQualifyRefs`), (b) adds the new namespace (lines 175–179), (c) runs the rewriter, (d) appends the new aliased using with a duplicate guard checking `u.Alias?.Name.ToString() == newAlias || u.Name?.ToString() == newImageNamespace` (lines 186–202). That collection + dedup logic is correct and stays. - - The broken part is `ImageAmbiguityRewriter.VisitIdentifierName` (`SyntaxFactoryHelper.cs:274–325`), which today picks the alias by iterating `_typeToExistingAlias` and `break`-ing on the first entry with the comment "There should be exactly one existing plain image using at this point" (lines 305–310). Replace this guess with **semantic resolution**: for each bare `PreImage`/`PostImage` identifier (after the existing exclusions at lines 284–300 — already-qualified right side of a `QualifiedNameSyntax`, member-access name like `obj.PreImage`, and the `NameEqualsSyntax` alias side — which MUST be preserved), use the threaded `SemanticModel` to resolve the identifier's symbol (`GetSymbolInfo`/`GetTypeInfo`) to its containing namespace's full name. If that namespace is one of the image namespaces being aliased (present in `plainNamespaceToAlias`, i.e. `IsImageRegistrationNamespace` true and known), qualify the reference with **that** namespace's alias (`GetLastNamespaceSegment`). If the symbol cannot be resolved uniquely to a single image namespace (missing/erroneous symbol, or ambiguous `CS0104`-style binding), leave the reference unqualified — do not guess — so it surfaces as a normal compile error. This makes requalification correct regardless of how many plain image usings exist in the file. Remove the misleading `break;` and its comment. - - Note: because the rewriter converts the plain usings to aliased form in the same pass, resolve symbols against the **original** (pre-rewrite) tree's semantic model, which is what the providers supply (the model for the tree as it was when the diagnostic fired). The bare references were valid under the original plain usings, so the symbols resolve there. - -Verifiable when: `SyntaxFactoryHelper` exposes `GetAliasForImageNamespace` and `ApplyAliasedImageUsings(...)`; `ConvertToAliasedUsingsAndQualifyRefs` accepts a `SemanticModel`; and a unit-level invocation over a tree with two plain image usings (`AccountUpdatePostOperation`, `AccountDeletePostOperation`) plus bare `PreImage` references under each correctly qualifies each reference to its own alias. - -### Phase 2: Wire `FixHandlerSignatureCodeFixProvider` to the shared helper (always-aliased, no duplication) - -Edit `XrmPluginCore.SourceGenerator/CodeFixes/FixHandlerSignatureCodeFixProvider.cs`, method `FixMethodDeclarationsAsync` (lines 124–241), specifically the block at lines 198–235. - -Today it does: -```csharp -var ambiguity = SyntaxFactoryHelper.DetectImageAmbiguity(methodRoot, imageNamespace); -var newParameters = SyntaxFactoryHelper.CreateImageParameterList(hasPreImage, hasPostImage, ambiguity.needsAlias ? ambiguity.alias : null); -// ... replace method decls ... -if (ambiguity.needsAlias) - newRoot = SyntaxFactoryHelper.ConvertToAliasedUsingsAndQualifyRefs(newRoot, imageNamespace); -else - newRoot = SyntaxFactoryHelper.AddUsingDirectiveIfMissing(newRoot, imageNamespace); -``` - -Change it to **always** use the alias via the shared helper. Compute the alias once and always pass it to `CreateImageParameterList`; then call `ApplyAliasedImageUsings` for the using rewrite: -```csharp -var alias = SyntaxFactoryHelper.GetAliasForImageNamespace(imageNamespace); -var newParameters = SyntaxFactoryHelper.CreateImageParameterList(hasPreImage, hasPostImage, alias); -// ... replace method decls (existing loop at lines 216–225 unchanged) ... -newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, imageNamespace, treeSemanticModel); -``` -`ApplyAliasedImageUsings` needs the `SemanticModel` for the tree being rewritten. This provider processes potentially multiple documents grouped by syntax tree (`locationsByTree`, lines 144–162); for each `tree`/`methodDocument`, obtain the model via `compilation.GetSemanticModel(tree)` (the `compilation` is already available as `semanticModel.Compilation`, passed into `FixMethodDeclarationsAsync` at line 119/126). Guard `imageNamespace` null/empty (no-op fallback). Remove the `DetectImageAmbiguity`/conditional branch. - -Verifiable when: applying the fix to a single PreImage registration produces both `void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)` in the signature AND `using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;` in the usings — i.e. the previously-named test `Should_Fix_Signature_And_Add_Using_For_PreImage` now sees the aliased form. And the multi-registration case (two same-service registrations both with PreImages) yields one aliased using per namespace, each signature alias-qualified, no `CS0104`. - -### Phase 3: Wire `CreateHandlerMethodCodeFixProvider` to the shared helper (always-aliased, no duplication) - -Edit `XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs`, method `CreateMethodAsync` (lines 135–154) and `CreateMethodDeclaration` (lines 157–166). - -Today it does: -```csharp -var (needsAlias, alias) = SyntaxFactoryHelper.DetectImageAmbiguity(interfaceRoot, imageNamespace); -var methodDeclaration = CreateMethodDeclaration(methodName, hasPreImage, hasPostImage, needsAlias ? alias : null); -// ... add member ... -if (needsAlias) - newRoot = SyntaxFactoryHelper.ConvertToAliasedUsingsAndQualifyRefs(newRoot, imageNamespace); -else - newRoot = SyntaxFactoryHelper.AddUsingDirectiveIfMissing(newRoot, imageNamespace); -``` - -Change to always-alias via the same shared helper: -```csharp -var alias = SyntaxFactoryHelper.GetAliasForImageNamespace(imageNamespace); -var methodDeclaration = CreateMethodDeclaration(methodName, hasPreImage, hasPostImage, alias); -// ... add member (existing AddMembers/ReplaceNode at lines 141–142 unchanged) ... -newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(newRoot, imageNamespace, interfaceSemanticModel); -``` -The interface may live in a different document/tree than the trigger; obtain its semantic model via `compilation.GetSemanticModel(interfaceRoot.SyntaxTree)` (the `compilation` is `semanticModel.Compilation`, available in `CreateMethodAsync`, line 76). Apply the same null/empty `imageNamespace` guard. `CreateMethodDeclaration` already forwards the `qualifier` into `CreateImageParameterList` (line 162), so the only change there is passing `alias` instead of the `needsAlias ? alias : null` conditional. Remove the `DetectImageAmbiguity`/conditional branch. After this phase, no image-using/alias logic is duplicated between the two providers — both go through `SyntaxFactoryHelper.ApplyAliasedImageUsings` + `CreateImageParameterList`. - -Verifiable when: invoking the "Create method" fix for a missing handler with a PreImage registration emits `void HandleUpdate(AccountUpdatePostOperation.PreImage preImage);` on the interface plus the aliased using directive, even when no other image using exists in the file; and both providers reference the single shared helper (grep shows no remaining `DetectImageAmbiguity` call sites unless intentionally retained). - -### Phase 4: Replace `BatchFixer` with a custom consolidating `FixAllProvider` for both providers - -Both `FixHandlerSignatureCodeFixProvider` and `CreateHandlerMethodCodeFixProvider` currently return `WellKnownFixAllProviders.BatchFixer` (`FixHandlerSignatureCodeFixProvider.cs:27–28`, `CreateHandlerMethodCodeFixProvider.cs:24–25`). As described in Requirements, `BatchFixer` applies each diagnostic's fix independently against the original document and merges — which breaks when one document has several image registrations, because each fix touches the shared `using` block / overlapping references without seeing the others, yielding merge conflicts or non-convergent ambiguous output. - -Implement a single shared custom `FixAllProvider` (a new type, e.g. `XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs`, used by both providers via `GetFixAllProvider()`) that: -1. Collects **all** fixable diagnostics in the relevant scope (Document/Project/Solution — at minimum Document scope; group remaining work by document). For each document, gather every diagnostic and read each one's `Constants.PropertyServiceType`, `Constants.PropertyMethodName`, `Constants.PropertyHasPreImage`, `Constants.PropertyHasPostImage`, `Constants.PropertyImageNamespace` (same property keys the per-fix path reads, e.g. `FixHandlerSignatureCodeFixProvider.cs:41–49`). -2. Applies all signature/method edits for that document in **one** pass: for each diagnostic, fix its target method signature (or create its interface method) with its **own** alias-qualified parameter list (`SyntaxFactoryHelper.CreateImageParameterList(hasPreImage, hasPostImage, GetAliasForImageNamespace(imageNamespace))`). -3. Runs **one** consolidated always-aliased using rewrite over the resulting tree that adds every distinct aliased image using needed and requalifies every reference once. Prefer extending the shared helper from Phase 1 to accept multiple image namespaces in a single call (e.g. an overload `ApplyAliasedImageUsings(SyntaxNode root, IEnumerable imageNamespaces, SemanticModel semanticModel)` that loops the existing collection/dedup/requalify logic of `ConvertToAliasedUsingsAndQualifyRefs`), so the FixAll path reuses the exact same engine as the single-fix path. No FixAll-specific alias/using logic may be duplicated — it must funnel through the same `SyntaxFactoryHelper` engine that Phases 1–3 use. - -The result must be order-independent and convergent: fixing N registrations in a file via FixAll produces exactly the same unambiguous output as fixing them one-by-one in any order — each namespace aliased exactly once, each signature alias-qualified, no `CS0104`. - -Verifiable when: a FixAll over a document with two (or more) same-service image registrations produces, in a single operation, one aliased `using ... = ...;` per distinct image namespace (no duplicates) and every fixed signature alias-qualified, and the document compiles cleanly; and `GetFixAllProvider()` on both providers returns the new shared provider rather than `WellKnownFixAllProviders.BatchFixer`. - -### Phase 5: Update and extend the code-fix tests - -Edit `XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs` and `XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs`. - -In `FixHandlerSignatureCodeFixProviderTests.cs`: -- `Should_Fix_Signature_And_Add_Using_For_PreImage` (assertions at lines 61, 64): change expected signature from `void HandleUpdate(PreImage preImage)` to `void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)` and the using from plain `using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;` to aliased `using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;`. -- `Should_Fix_Signature_And_Add_Using_For_PostImage` (lines 114, 117): expect `void HandleUpdate(AccountUpdatePostOperation.PostImage postImage)` and the aliased using. -- `Should_Fix_Signature_And_Add_Using_For_Both_Images` (line ~120): expect `void HandleUpdate(AccountUpdatePostOperation.PreImage preImage, AccountUpdatePostOperation.PostImage postImage)` and the aliased using. -- `Should_Avoid_Ambiguous_Usings` (line 231): this already expects the aliased form (`void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)`, `void HandleDelete(AccountDeletePostOperation.PreImage preImage)`, and the two aliased `using ... = ...;` directives counted once each via `CountOccurrences`, lines 285–292). It should keep passing unchanged — verify it does. Consider renaming it (e.g. `Should_Use_Aliased_Usings_For_Multiple_Registrations`) since aliasing is no longer "avoidance of ambiguity" but the default. -- Add a new test asserting idempotency: applying the fix to source that already contains the correct aliased using and aliased signature does not duplicate the using (use the existing `CountOccurrences` helper at lines 295–304, expect count `1`). -- Add a new test for the "cleaned-up imports" re-evaluation: source where a developer reverted one registration's using to plain form (or removed it) — after fix, all image usings are aliased exactly once. -- Add a new test for **multi-using semantic requalification**: two same-service registrations (`AccountUpdatePostOperation` + `AccountDeletePostOperation`) where pre-existing handler bodies/signatures contain bare `PreImage` references under two plain image usings. After fix, each bare reference is qualified to its own alias (`AccountUpdatePostOperation.PreImage` vs `AccountDeletePostOperation.PreImage`) — never cross-qualified — and the result compiles with no `CS0104`. This exercises the Phase 1 semantic-model resolution path (where the old `break`-on-first logic would have mis-qualified one of them). - -- Add a new **FixAll** test (Phase 4): a document with two or more same-service image registrations, all triggering the diagnostic, fixed via the FixAll/batch path. Assert the consolidated single-pass result — one aliased `using ... = ...;` per distinct image namespace (count `1` each via `CountOccurrences`), every signature alias-qualified, no `CS0104`. If `CodeFixTestBase` only exercises single-diagnostic fixes today, extend it (or add a sibling helper) to invoke the provider's `GetFixAllProvider()` across all diagnostics in the document so the custom `FixAllProvider` is actually exercised (not just the per-diagnostic `RegisterCodeFixesAsync`). - -In `CreateHandlerMethodCodeFixProviderTests.cs`: apply the analogous expectation changes — generated interface methods must now use alias-qualified parameter types and the file must gain an aliased using — plus an analogous FixAll test for multiple missing handlers on the same service interface. - -Use the shared `CodeFixTestBase` / `ApplyCodeFixAsync` harness already present (`FixHandlerSignatureCodeFixProviderTests.cs:307–309` wires `HandlerSignatureMismatchAnalyzer` + `FixHandlerSignatureCodeFixProvider` with diagnostic ids `HandlerSignatureMismatch.Id` and `HandlerSignatureMismatchError.Id`). - -Verifiable when: `dotnet test --configuration Release` passes for `XrmPluginCore.SourceGenerator.Tests`, including the updated and newly-added tests, across all target frameworks. diff --git a/.gitignore b/.gitignore index e1a1752..a47f294 100644 --- a/.gitignore +++ b/.gitignore @@ -401,3 +401,6 @@ FodyWeavers.xsd # Claude local settings .claude/*.local.json + +# Conducktor local files +.conducktor/ From a20b0c8e19226925b248397382976c5da85b7992 Mon Sep 17 00:00:00 2001 From: Morten Holt Date: Mon, 22 Jun 2026 09:27:23 +0200 Subject: [PATCH 7/7] Address code-fix review feedback (alias guard, interface targeting, FixAll title) - SyntaxFactoryHelper: only treat a plain (non-aliased) using as "already present" for the image namespace. A namespace imported under a different alias no longer suppresses the standard alias, which the emitted parameter types reference. - CreateHandlerMethodCodeFixProvider: track the target interface with a SyntaxAnnotation (binding via a ReplaceSyntaxTree'd semantic model) instead of re-finding by identifier text, so same-named interfaces across namespaces or nested types resolve correctly. - AliasedImageUsingsFixAllProvider: use a descriptive constant title for the FixAll code action; keep the equivalence key for identity only. - Add regression tests for both behavioral fixes (verified failing without the fix); full suite 80/80. Co-Authored-By: Claude via Conducktor --- ...CreateHandlerMethodCodeFixProviderTests.cs | 73 ++++++++++++++++ ...FixHandlerSignatureCodeFixProviderTests.cs | 83 +++++++++++++++++++ .../AliasedImageUsingsFixAllProvider.cs | 7 +- .../CreateHandlerMethodCodeFixProvider.cs | 32 ++++--- .../Helpers/SyntaxFactoryHelper.cs | 8 +- 5 files changed, 190 insertions(+), 13 deletions(-) diff --git a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs index 95f8f19..b11b814 100644 --- a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs +++ b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs @@ -318,6 +318,79 @@ public sealed class PostImage { } AssertNoAmbiguousReferences(fixedSource); } + [Fact] + public async Task Should_Add_Method_To_Correct_Interface_When_Names_Collide() + { + // Arrange - A decoy interface with the SAME simple name lives in another namespace and is + // declared FIRST in the document. The registered service is TestNamespace.ITestService. The + // fix must add the method to the registered interface, not the first one matching by name. + const string source = """ + using System; + using System.ComponentModel; + using Microsoft.Xrm.Sdk; + using XrmPluginCore; + using XrmPluginCore.Enums; + using Microsoft.Extensions.DependencyInjection; + using XrmPluginCore.Tests.Context.BusinessDomain; + + namespace DecoyNamespace + { + public interface ITestService + { + void SomethingElse(); + } + } + + namespace TestNamespace + { + public class TestPlugin : Plugin + { + public TestPlugin() + { + RegisterStep(EventOperation.Update, ExecutionStage.PostOperation, + "HandleUpdate") + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + } + + protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services) + { + return services.AddScoped(); + } + } + + public interface ITestService + { + } + + public class TestService : ITestService + { + } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + """; + + // Act + var fixedSource = await ApplyCodeFixAsync(source); + + // Assert - The method landed on the registered interface, not the decoy + var compilation = CompilationHelper.CreateCompilation(fixedSource); + var registered = compilation.GetTypeByMetadataName("TestNamespace.ITestService"); + var decoy = compilation.GetTypeByMetadataName("DecoyNamespace.ITestService"); + + registered.Should().NotBeNull(); + decoy.Should().NotBeNull(); + registered!.GetMembers("HandleUpdate").Should().HaveCount(1, "the method must be added to the registered interface"); + decoy!.GetMembers("HandleUpdate").Should().BeEmpty("the method must not be added to the same-named decoy interface"); + + AssertNoAmbiguousReferences(fixedSource); + } + private static void AssertNoAmbiguousReferences(string source) { var compilation = CompilationHelper.CreateCompilation(source); diff --git a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs index 02508ab..881e6d8 100644 --- a/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs +++ b/XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/FixHandlerSignatureCodeFixProviderTests.cs @@ -633,6 +633,76 @@ public sealed class PostImage { } AssertNoAmbiguousReferences(fixedSource); } + [Fact] + public async Task Should_Add_Standard_Alias_When_Namespace_Imported_Under_Different_Alias() + { + // Arrange - The image namespace is already imported, but under a NON-standard alias ("Foo"). + // The emitted parameter type uses the standard alias (the last namespace segment), so the fix + // must still add the standard aliased using - otherwise the qualified type fails to resolve. + const string source = """ + using System; + using System.ComponentModel; + using Microsoft.Xrm.Sdk; + using XrmPluginCore; + using XrmPluginCore.Enums; + using Microsoft.Extensions.DependencyInjection; + using XrmPluginCore.Tests.Context.BusinessDomain; + using Foo = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation; + + namespace TestNamespace + { + public class TestPlugin : Plugin + { + public TestPlugin() + { + RegisterStep(EventOperation.Update, ExecutionStage.PostOperation, + nameof(ITestService.HandleUpdate)) + .AddFilteredAttributes(x => x.Name) + .WithPreImage(x => x.Name, x => x.Revenue); + } + + protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services) + { + return services.AddScoped(); + } + } + + public interface ITestService + { + void HandleUpdate(); + } + + public class TestService : ITestService + { + public void HandleUpdate() { } + } + } + + namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation + { + public sealed class PreImage { } + public sealed class PostImage { } + } + """; + + // Act + var fixedSource = await ApplyCodeFixAsync(source); + + // Assert - Signature uses the standard alias + CountOccurrences(fixedSource, "void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)").Should().Be(2); + + // Assert - The standard aliased using is added even though "Foo" already imports the namespace + CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;") + .Should().Be(1, "the standard alias must be present so the qualified parameter type resolves"); + + // Assert - The pre-existing non-standard alias is left untouched + CountOccurrences(fixedSource, "using Foo = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;") + .Should().Be(1, "the existing alias should not be removed"); + + // Assert - Compiles cleanly: no ambiguity AND no unresolved standard alias + AssertCompilesWithoutReferenceErrors(fixedSource); + } + private static void AssertNoAmbiguousReferences(string source) { var compilation = CompilationHelper.CreateCompilation(source); @@ -643,6 +713,19 @@ private static void AssertNoAmbiguousReferences(string source) ambiguous.Should().BeEmpty("the fixed source should not contain ambiguous references"); } + private static void AssertCompilesWithoutReferenceErrors(string source) + { + var compilation = CompilationHelper.CreateCompilation(source); + + // CS0104 = ambiguous reference; CS0246/CS0234 = unresolved type / namespace member, which is + // how a missing alias (qualified type whose alias was never added) surfaces. + var errors = compilation.GetDiagnostics() + .Where(d => d.Id is "CS0104" or "CS0246" or "CS0234") + .Select(d => d.GetMessage()) + .ToList(); + errors.Should().BeEmpty("the fixed source should resolve all image references"); + } + private static int CountOccurrences(string source, string search) { var count = 0; diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs index c9bb063..383e8e3 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/AliasedImageUsingsFixAllProvider.cs @@ -25,6 +25,10 @@ internal sealed class AliasedImageUsingsFixAllProvider : FixAllProvider { public static readonly AliasedImageUsingsFixAllProvider Instance = new(); + // User-facing label shown in the FixAll UI. The equivalence key is a stable identity token and is + // not suitable as a title. + private const string Title = "Fix all image handler signatures"; + private AliasedImageUsingsFixAllProvider() { } @@ -45,10 +49,9 @@ public override async Task GetFixAsync(FixAllContext fixAllContext) } var solution = fixAllContext.Solution; - var title = fixAllContext.CodeActionEquivalenceKey ?? "Fix all image handlers"; return CodeAction.Create( - title, + Title, c => FixAllAsync(solution, diagnostics, c), equivalenceKey: fixAllContext.CodeActionEquivalenceKey); } diff --git a/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs b/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs index 47571ba..a46538c 100644 --- a/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs +++ b/XrmPluginCore.SourceGenerator/CodeFixes/CreateHandlerMethodCodeFixProvider.cs @@ -139,17 +139,29 @@ private static async Task CreateMethodAsync( var methodDeclaration = CreateMethodDeclaration(methodName, hasPreImage, hasPostImage, alias); - // Emit the aliased using and requalify existing image references FIRST, on the original tree, - // so the rewriter resolves bare references against a semantic model whose nodes still match. + // Annotate the exact interface node so we can locate it precisely after the using rewrite, + // rather than re-finding by identifier text (which is ambiguous when several interfaces share + // a name across namespaces or as nested types). + var interfaceAnnotation = new SyntaxAnnotation(); + var annotatedRoot = interfaceRoot.ReplaceNode( + interfaceDeclaration, + interfaceDeclaration.WithAdditionalAnnotations(interfaceAnnotation)); + + // Emit the aliased using and requalify existing image references FIRST, so the rewriter + // resolves bare references against a semantic model whose nodes still match. Annotating the + // node above produced a new tree, so build the semantic model from a compilation with that + // tree swapped in — the annotation doesn't affect binding, so references resolve identically. // The created method's parameters are already alias-qualified, so requalification skips them. - // The interface may live in a different tree than the trigger, so resolve its semantic model - // from the compilation. - var interfaceSemanticModel = semanticModel.Compilation.GetSemanticModel(interfaceRoot.SyntaxTree); - var newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(interfaceRoot, imageNamespace, interfaceSemanticModel); - - // Then add the method to the (re-found) interface declaration. - var targetInterface = newRoot.DescendantNodes().OfType() - .FirstOrDefault(i => i.Identifier.Text == interfaceDeclaration.Identifier.Text); + // The interface may live in a different tree than the trigger. + var annotatedTree = annotatedRoot.SyntaxTree; + var annotatedCompilation = semanticModel.Compilation.ReplaceSyntaxTree(interfaceRoot.SyntaxTree, annotatedTree); + var interfaceSemanticModel = annotatedCompilation.GetSemanticModel(annotatedTree); + var newRoot = SyntaxFactoryHelper.ApplyAliasedImageUsings(annotatedRoot, imageNamespace, interfaceSemanticModel); + + // Then add the method to the annotated interface declaration. + var targetInterface = newRoot.GetAnnotatedNodes(interfaceAnnotation) + .OfType() + .FirstOrDefault(); if (targetInterface != null) { newRoot = newRoot.ReplaceNode(targetInterface, targetInterface.AddMembers(methodDeclaration)); diff --git a/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs b/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs index c63a840..876e842 100644 --- a/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs +++ b/XrmPluginCore.SourceGenerator/Helpers/SyntaxFactoryHelper.cs @@ -276,9 +276,15 @@ private static SyntaxNode ConvertToAliasedUsingsAndQualifyRefs(SyntaxNode root, } var newAlias = GetLastNamespaceSegment(newImageNamespace); + + // The standard alias is "already present" only if some using binds exactly that alias + // name, or a plain (non-aliased) using imports the namespace (which the rewriter above + // would already have converted to the standard alias). A using that imports the same + // namespace under a DIFFERENT alias does NOT count: the emitted parameter types are + // qualified with the standard alias, so we must still add it or the code won't compile. var alreadyExists = newCompUnit.Usings.Any(u => u.Alias?.Name.ToString() == newAlias || - u.Name?.ToString() == newImageNamespace) || + (u.Alias == null && u.Name?.ToString() == newImageNamespace)) || usingsToAdd.Any(u => u.Alias?.Name.ToString() == newAlias); if (!alreadyExists)