Skip to content

Commit a4c10b1

Browse files
mkholtmkholt
authored
Always generate aliased usings (#12)
Context The XrmPluginCore.SourceGenerator ships analyzers + code-fix providers that wire up type-safe Pre/Post image handler signatures. When a RegisterStep<TEntity, TService>(...) 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).
1 parent d14d6dc commit a4c10b1

9 files changed

Lines changed: 1311 additions & 110 deletions

File tree

‎.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,3 +401,6 @@ FodyWeavers.xsd
401401

402402
# Claude local settings
403403
.claude/*.local.json
404+
405+
# Conducktor local files
406+
.conducktor/

‎XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CodeFixTestBase.cs‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using Microsoft.CodeAnalysis.CodeFixes;
44
using Microsoft.CodeAnalysis.CSharp;
55
using Microsoft.CodeAnalysis.Diagnostics;
6+
using System.Collections.Immutable;
67
using XrmPluginCore.SourceGenerator.Tests.Helpers;
78

89
namespace XrmPluginCore.SourceGenerator.Tests.DiagnosticTests;
@@ -82,6 +83,75 @@ protected static async Task<string> ApplyCodeFixAsync(
8283
return newText.ToString();
8384
}
8485

86+
/// <summary>
87+
/// Applies the code fix provider's FixAll provider across every matching diagnostic in the
88+
/// document (Document scope), exercising the custom <c>FixAllProvider</c> rather than a single
89+
/// per-diagnostic fix.
90+
/// </summary>
91+
protected static async Task<string> ApplyFixAllAsync(
92+
string source,
93+
DiagnosticAnalyzer analyzer,
94+
CodeFixProvider codeFixProvider,
95+
string equivalenceKey,
96+
params string[] diagnosticIds)
97+
{
98+
var document = CreateDocument(source);
99+
100+
// Compute diagnostics against the document's own compilation so their locations reference the
101+
// document's syntax tree (the FixAll provider maps locations back to documents).
102+
var compilation = await document.Project.GetCompilationAsync();
103+
var compilationWithAnalyzers = compilation!.WithAnalyzers([analyzer]);
104+
var analyzerDiagnostics = await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync();
105+
var diagnostics = analyzerDiagnostics.Where(d => diagnosticIds.Contains(d.Id)).ToImmutableArray();
106+
107+
if (diagnostics.IsEmpty)
108+
{
109+
return source;
110+
}
111+
112+
var fixAllProvider = codeFixProvider.GetFixAllProvider()!;
113+
var fixAllContext = new FixAllContext(
114+
document,
115+
codeFixProvider,
116+
FixAllScope.Document,
117+
equivalenceKey,
118+
diagnosticIds,
119+
new FixAllDiagnosticProvider(diagnostics),
120+
CancellationToken.None);
121+
122+
var codeAction = await fixAllProvider.GetFixAsync(fixAllContext);
123+
if (codeAction == null)
124+
{
125+
return source;
126+
}
127+
128+
var operations = await codeAction.GetOperationsAsync(CancellationToken.None);
129+
var changedSolution = operations.OfType<ApplyChangesOperation>().Single().ChangedSolution;
130+
var changedDocument = changedSolution.GetDocument(document.Id);
131+
var newText = await changedDocument!.GetTextAsync();
132+
133+
return newText.ToString();
134+
}
135+
136+
private sealed class FixAllDiagnosticProvider : FixAllContext.DiagnosticProvider
137+
{
138+
private readonly ImmutableArray<Diagnostic> _diagnostics;
139+
140+
public FixAllDiagnosticProvider(ImmutableArray<Diagnostic> diagnostics)
141+
{
142+
_diagnostics = diagnostics;
143+
}
144+
145+
public override Task<IEnumerable<Diagnostic>> GetDocumentDiagnosticsAsync(Document document, CancellationToken cancellationToken)
146+
=> Task.FromResult<IEnumerable<Diagnostic>>(_diagnostics);
147+
148+
public override Task<IEnumerable<Diagnostic>> GetAllDiagnosticsAsync(Project project, CancellationToken cancellationToken)
149+
=> Task.FromResult<IEnumerable<Diagnostic>>(_diagnostics);
150+
151+
public override Task<IEnumerable<Diagnostic>> GetProjectDiagnosticsAsync(Project project, CancellationToken cancellationToken)
152+
=> Task.FromResult<IEnumerable<Diagnostic>>(Enumerable.Empty<Diagnostic>());
153+
}
154+
85155
protected static async Task<List<CodeAction>> GetCodeActionsAsync(
86156
string source,
87157
DiagnosticAnalyzer analyzer,

‎XrmPluginCore.SourceGenerator.Tests/DiagnosticTests/CreateHandlerMethodCodeFixProviderTests.cs‎

Lines changed: 191 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using FluentAssertions;
22
using XrmPluginCore.SourceGenerator.Analyzers;
33
using XrmPluginCore.SourceGenerator.CodeFixes;
4+
using XrmPluginCore.SourceGenerator.Tests.Helpers;
45
using Xunit;
56

67
namespace XrmPluginCore.SourceGenerator.Tests.DiagnosticTests;
@@ -55,11 +56,11 @@ public class TestService : ITestService
5556
// Act
5657
var fixedSource = await ApplyCodeFixAsync(source);
5758

58-
// Assert - Method created with PreImage parameter
59-
fixedSource.Should().Contain("void HandleUpdate(PreImage preImage)");
59+
// Assert - Method created with alias-qualified PreImage parameter
60+
fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)");
6061

61-
// Assert - Using directive added
62-
fixedSource.Should().Contain("using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;");
62+
// Assert - Aliased using directive added
63+
fixedSource.Should().Contain("using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;");
6364
}
6465

6566
[Fact]
@@ -107,11 +108,11 @@ public class TestService : ITestService
107108
// Act
108109
var fixedSource = await ApplyCodeFixAsync(source);
109110

110-
// Assert - Method created with both image parameters
111-
fixedSource.Should().Contain("void HandleUpdate(PreImage preImage, PostImage postImage)");
111+
// Assert - Method created with both alias-qualified image parameters
112+
fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage, AccountUpdatePostOperation.PostImage postImage)");
112113

113-
// Assert - Using directive added
114-
fixedSource.Should().Contain("using TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;");
114+
// Assert - Aliased using directive added
115+
fixedSource.Should().Contain("using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;");
115116
}
116117

117118
[Fact]
@@ -165,9 +166,11 @@ public class TestService : ITestService
165166
}
166167

167168
[Fact]
168-
public async Task Should_Avoid_Ambiguous_Usings()
169+
public async Task Should_Use_Aliased_Usings_For_Multiple_Registrations()
169170
{
170-
// Arrange - Using directive already exists for Update registration, method missing for Delete
171+
// Arrange - A plain using already exists for the Update registration; the Delete handler is
172+
// missing from the interface. Creating Delete must requalify the existing Update reference to
173+
// its alias (resolved via the semantic model) and add the Delete using in aliased form.
171174
const string source = """
172175
using System;
173176
using System.ComponentModel;
@@ -211,22 +214,191 @@ public class TestService : ITestService
211214
public void HandleUpdate(PreImage preImage) { }
212215
}
213216
}
217+
218+
namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation
219+
{
220+
public sealed class PreImage { }
221+
public sealed class PostImage { }
222+
}
223+
224+
namespace TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation
225+
{
226+
public sealed class PreImage { }
227+
public sealed class PostImage { }
228+
}
214229
""";
215230

216231
// Act
217232
var fixedSource = await ApplyCodeFixAsync(source);
218233

219-
// Assert - New method created with qualified type
234+
// Assert - New method created with its own alias-qualified type
220235
fixedSource.Should().Contain("void HandleDelete(AccountDeletePostOperation.PreImage preImage)");
221236

222-
// Assert - Existing PreImage references qualified with alias
237+
// Assert - Existing bare PreImage references requalified with the Update alias
223238
CountOccurrences(fixedSource, "AccountUpdatePostOperation.PreImage preImage").Should().BeGreaterOrEqualTo(1);
224239

225-
// Assert - Aliased usings
240+
// Assert - Each namespace aliased exactly once
226241
CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;")
227-
.Should().Be(1, "the existing using should be converted to aliased form");
242+
.Should().Be(1, "the existing plain using should be converted to aliased form");
228243
CountOccurrences(fixedSource, "using AccountDeletePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;")
229244
.Should().Be(1, "the new using should be added in aliased form");
245+
246+
AssertNoAmbiguousReferences(fixedSource);
247+
}
248+
249+
[Fact]
250+
public async Task FixAll_Should_Create_Multiple_Methods_On_Same_Interface()
251+
{
252+
// Arrange - Two same-service registrations, both missing their handler methods.
253+
const string source = """
254+
using System;
255+
using System.ComponentModel;
256+
using Microsoft.Xrm.Sdk;
257+
using XrmPluginCore;
258+
using XrmPluginCore.Enums;
259+
using Microsoft.Extensions.DependencyInjection;
260+
using XrmPluginCore.Tests.Context.BusinessDomain;
261+
262+
namespace TestNamespace
263+
{
264+
public class TestPlugin : Plugin
265+
{
266+
public TestPlugin()
267+
{
268+
RegisterStep<Account, ITestService>(EventOperation.Update, ExecutionStage.PostOperation,
269+
"HandleUpdate")
270+
.AddFilteredAttributes(x => x.Name)
271+
.WithPreImage(x => x.Name, x => x.Revenue);
272+
273+
RegisterStep<Account, ITestService>(EventOperation.Delete, ExecutionStage.PostOperation,
274+
"HandleDelete")
275+
.AddFilteredAttributes(x => x.Name)
276+
.WithPreImage(x => x.Name, x => x.Revenue);
277+
}
278+
279+
protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services)
280+
{
281+
return services.AddScoped<ITestService, TestService>();
282+
}
283+
}
284+
285+
public interface ITestService
286+
{
287+
}
288+
289+
public class TestService : ITestService
290+
{
291+
}
292+
}
293+
294+
namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation
295+
{
296+
public sealed class PreImage { }
297+
public sealed class PostImage { }
298+
}
299+
300+
namespace TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation
301+
{
302+
public sealed class PreImage { }
303+
public sealed class PostImage { }
304+
}
305+
""";
306+
307+
// Act - Apply the consolidating FixAll across all diagnostics in one pass
308+
var fixedSource = await ApplyFixAllAsync(source);
309+
310+
// Assert - Both methods created with their own alias-qualified parameter
311+
fixedSource.Should().Contain("void HandleUpdate(AccountUpdatePostOperation.PreImage preImage)");
312+
fixedSource.Should().Contain("void HandleDelete(AccountDeletePostOperation.PreImage preImage)");
313+
314+
// Assert - One aliased using per distinct namespace (no duplicates)
315+
CountOccurrences(fixedSource, "using AccountUpdatePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation;").Should().Be(1);
316+
CountOccurrences(fixedSource, "using AccountDeletePostOperation = TestNamespace.PluginRegistrations.TestPlugin.AccountDeletePostOperation;").Should().Be(1);
317+
318+
AssertNoAmbiguousReferences(fixedSource);
319+
}
320+
321+
[Fact]
322+
public async Task Should_Add_Method_To_Correct_Interface_When_Names_Collide()
323+
{
324+
// Arrange - A decoy interface with the SAME simple name lives in another namespace and is
325+
// declared FIRST in the document. The registered service is TestNamespace.ITestService. The
326+
// fix must add the method to the registered interface, not the first one matching by name.
327+
const string source = """
328+
using System;
329+
using System.ComponentModel;
330+
using Microsoft.Xrm.Sdk;
331+
using XrmPluginCore;
332+
using XrmPluginCore.Enums;
333+
using Microsoft.Extensions.DependencyInjection;
334+
using XrmPluginCore.Tests.Context.BusinessDomain;
335+
336+
namespace DecoyNamespace
337+
{
338+
public interface ITestService
339+
{
340+
void SomethingElse();
341+
}
342+
}
343+
344+
namespace TestNamespace
345+
{
346+
public class TestPlugin : Plugin
347+
{
348+
public TestPlugin()
349+
{
350+
RegisterStep<Account, ITestService>(EventOperation.Update, ExecutionStage.PostOperation,
351+
"HandleUpdate")
352+
.AddFilteredAttributes(x => x.Name)
353+
.WithPreImage(x => x.Name, x => x.Revenue);
354+
}
355+
356+
protected override IServiceCollection OnBeforeBuildServiceProvider(IServiceCollection services)
357+
{
358+
return services.AddScoped<ITestService, TestService>();
359+
}
360+
}
361+
362+
public interface ITestService
363+
{
364+
}
365+
366+
public class TestService : ITestService
367+
{
368+
}
369+
}
370+
371+
namespace TestNamespace.PluginRegistrations.TestPlugin.AccountUpdatePostOperation
372+
{
373+
public sealed class PreImage { }
374+
public sealed class PostImage { }
375+
}
376+
""";
377+
378+
// Act
379+
var fixedSource = await ApplyCodeFixAsync(source);
380+
381+
// Assert - The method landed on the registered interface, not the decoy
382+
var compilation = CompilationHelper.CreateCompilation(fixedSource);
383+
var registered = compilation.GetTypeByMetadataName("TestNamespace.ITestService");
384+
var decoy = compilation.GetTypeByMetadataName("DecoyNamespace.ITestService");
385+
386+
registered.Should().NotBeNull();
387+
decoy.Should().NotBeNull();
388+
registered!.GetMembers("HandleUpdate").Should().HaveCount(1, "the method must be added to the registered interface");
389+
decoy!.GetMembers("HandleUpdate").Should().BeEmpty("the method must not be added to the same-named decoy interface");
390+
391+
AssertNoAmbiguousReferences(fixedSource);
392+
}
393+
394+
private static void AssertNoAmbiguousReferences(string source)
395+
{
396+
var compilation = CompilationHelper.CreateCompilation(source);
397+
var ambiguous = compilation.GetDiagnostics()
398+
.Where(d => d.Id == "CS0104")
399+
.Select(d => d.GetMessage())
400+
.ToList();
401+
ambiguous.Should().BeEmpty("the fixed source should not contain ambiguous references");
230402
}
231403

232404
private static int CountOccurrences(string source, string search)
@@ -244,4 +416,9 @@ private static int CountOccurrences(string source, string search)
244416
private static Task<string> ApplyCodeFixAsync(string source)
245417
=> ApplyCodeFixAsync(source, new HandlerMethodNotFoundAnalyzer(), new CreateHandlerMethodCodeFixProvider(),
246418
DiagnosticDescriptors.HandlerMethodNotFound.Id);
419+
420+
private static Task<string> ApplyFixAllAsync(string source)
421+
=> ApplyFixAllAsync(source, new HandlerMethodNotFoundAnalyzer(), new CreateHandlerMethodCodeFixProvider(),
422+
nameof(CreateHandlerMethodCodeFixProvider),
423+
DiagnosticDescriptors.HandlerMethodNotFound.Id);
247424
}

0 commit comments

Comments
 (0)