Skip to content

Commit 825c1a3

Browse files
natemcmasternatemcmaster
authored
Fix AOT code generation type safety issues (#583)
## Summary Fixes critical and high-severity bugs in the source generator that were identified during review of #582. ## Issues Fixed ### Bug 1: Value Type Constructor Parameters (Critical - CS0077) **Problem:** Generated code used `as` operator with non-nullable value types: ```csharp var p0_0 = _services.GetService(typeof(int)) as int; // CS0077 compiler error ``` **Fix:** Check service object for null, then use direct cast: ```csharp var service0_0 = _services.GetService(typeof(int)); if (service0_0 != null) return new T((int)service0_0!); ``` **Impact:** Any command with value type constructor parameters (int, bool, etc.) would fail to compile with CS0077. ### Bug 2: Nullable Array RemainingArguments (High Severity) **Problem:** Used fragile string comparison for type detection: ```csharp if (sp.RemainingArgumentsPropertyType == "string[]") // Fails for "string[]?" ``` **Fix:** Use actual type analysis with `IArrayTypeSymbol`: ```csharp info.SpecialProperties.RemainingArgumentsIsArray = property.Type is IArrayTypeSymbol; ``` **Impact:** Properties like `string[]?` would use wrong code path, causing `InvalidCastException` at runtime. ### Additional Fixes - Added `System.Linq` using to generated code for `ToArray()` support - Fixed `CommandData` property name to `CommandInfo` to match interface - Organized Claude planning documents in `.claude/plans/` ## Test Coverage Added comprehensive tests demonstrating both bugs: 1. **GeneratorBugReproduction/** - Compilation test project that demonstrates bugs would cause CS0077 2. **GeneratedModelFactoryTests.cs** - Unit tests for value type handling patterns 3. **SpecialPropertiesMetadataTests.cs** - Tests for array type conversion logic All tests now pass ✅
2 parents cb57bcd + 0fe1352 commit 825c1a3

File tree

10 files changed

+508
-9
lines changed

10 files changed

+508
-9
lines changed

aot-plan.md renamed to .claude/plans/aot-plan.md

File renamed without changes.

‎CLAUDE.md‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,17 +103,29 @@ return app.Execute(args);
103103
Uses xUnit with FluentAssertions and Moq. Convention tests inherit from `ConventionTestBase` and use `Create<T>()` factory method.
104104

105105
```bash
106+
# Quick test run (may not catch all issues)
106107
dotnet test --collect:"XPlat Code Coverage"
108+
109+
# Full validation (REQUIRED before committing)
110+
pwsh -File build.ps1
107111
```
108112

113+
**IMPORTANT:** Always run the full `build.ps1` script before committing changes. `dotnet test` alone may pass while the full build fails due to:
114+
- Sample project compilation issues
115+
- Source generator output problems
116+
- Integration test failures
117+
- Code coverage requirements
118+
119+
The build script runs the complete validation pipeline including tests, samples, and packaging.
120+
109121
## Development Workflow
110122

111123
**Test-Driven Development:** When implementing new features or fixing bugs, prefer writing tests first:
112124

113125
1. Write a failing test that demonstrates the desired behavior or reproduces the bug
114126
2. Run the test to confirm it fails as expected
115127
3. Implement the minimum code needed to make the test pass
116-
4. Run tests to verify the fix
128+
4. Run the **full build script** (`pwsh -File build.ps1`) to verify the fix
117129
5. Refactor if needed while keeping tests green
118130

119131
This approach ensures code correctness, prevents regressions, and validates that tests actually catch the issues they're meant to detect. The test suite already has good coverage and patterns to follow.

‎src/CommandLineUtils.Generators/CommandMetadataGenerator.cs‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,8 @@ private static void ExtractSpecialProperties(INamedTypeSymbol typeSymbol, Comman
190190
{
191191
info.SpecialProperties.RemainingArgumentsPropertyName = property.Name;
192192
info.SpecialProperties.RemainingArgumentsPropertyType = property.Type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
193+
// Track whether this is an array type (needs special handling for conversion from IReadOnlyList)
194+
info.SpecialProperties.RemainingArgumentsIsArray = property.Type is IArrayTypeSymbol;
193195
}
194196
}
195197
}
@@ -823,6 +825,7 @@ private static string GenerateMetadataProvider(CommandData info)
823825
sb.AppendLine("using System;");
824826
sb.AppendLine("using System.Collections.Generic;");
825827
sb.AppendLine("using System.ComponentModel.DataAnnotations;");
828+
sb.AppendLine("using System.Linq;");
826829
sb.AppendLine("using System.Threading;");
827830
sb.AppendLine("using System.Threading.Tasks;");
828831
sb.AppendLine("using McMaster.Extensions.CommandLineUtils;");
@@ -1043,7 +1046,7 @@ private static void GenerateSubcommandsProperty(StringBuilder sb, CommandData in
10431046
private static void GenerateCommandDataProperty(StringBuilder sb, CommandData info, string indent)
10441047
{
10451048
var cmd = info.CommandAttribute;
1046-
sb.AppendLine($"{indent} public CommandMetadata? CommandData => new CommandMetadata");
1049+
sb.AppendLine($"{indent} public CommandMetadata? CommandInfo => new CommandMetadata");
10471050
sb.AppendLine($"{indent} {{");
10481051
// Use explicit name if set, otherwise use inferred name from class name
10491052
var commandName = cmd.Name ?? info.InferredName;
@@ -1216,8 +1219,9 @@ private static void GenerateSpecialPropertiesProperty(StringBuilder sb, CommandD
12161219

12171220
if (sp.RemainingArgumentsPropertyName != null)
12181221
{
1219-
// Handle string[] vs IReadOnlyList<string>
1220-
if (sp.RemainingArgumentsPropertyType == "string[]")
1222+
// Handle string[] vs IReadOnlyList<string> based on actual type analysis (not string comparison)
1223+
// Array types need conversion from IReadOnlyList, collection types can be cast directly
1224+
if (sp.RemainingArgumentsIsArray)
12211225
{
12221226
sb.AppendLine($"{indent} RemainingArgumentsSetter = static (obj, val) => (({info.ClassName})obj).{sp.RemainingArgumentsPropertyName} = val is string[] arr ? arr : ((System.Collections.Generic.IReadOnlyList<string>)val!).ToArray(),");
12231227
}
@@ -1306,19 +1310,20 @@ private static void GenerateModelFactory(StringBuilder sb, CommandData info, str
13061310
var ctor = constructorsWithParams[ctorIdx];
13071311

13081312
// Generate variable declarations for each parameter
1313+
// Use intermediate 'service' variable to avoid 'as' operator issues with value types
13091314
for (int paramIdx = 0; paramIdx < ctor.Parameters.Count; paramIdx++)
13101315
{
13111316
var param = ctor.Parameters[paramIdx];
1312-
sb.AppendLine($"{indent} var p{ctorIdx}_{paramIdx} = _services.GetService(typeof({param.TypeName})) as {param.TypeName};");
1317+
sb.AppendLine($"{indent} var service{ctorIdx}_{paramIdx} = _services.GetService(typeof({param.TypeName}));");
13131318
}
13141319

1315-
// Check if all parameters were resolved
1316-
var allParamsCheck = string.Join(" && ", Enumerable.Range(0, ctor.Parameters.Count).Select(i => $"p{ctorIdx}_{i} != null"));
1320+
// Check if all parameters were resolved (check service objects, not cast results)
1321+
var allParamsCheck = string.Join(" && ", Enumerable.Range(0, ctor.Parameters.Count).Select(i => $"service{ctorIdx}_{i} != null"));
13171322
sb.AppendLine($"{indent} if ({allParamsCheck})");
13181323
sb.AppendLine($"{indent} {{");
13191324

1320-
// Create instance with resolved parameters
1321-
var paramList = string.Join(", ", Enumerable.Range(0, ctor.Parameters.Count).Select(i => $"p{ctorIdx}_{i}"));
1325+
// Create instance with resolved parameters (cast to correct types)
1326+
var paramList = string.Join(", ", Enumerable.Range(0, ctor.Parameters.Count).Select(i => $"({ctor.Parameters[i].TypeName})service{ctorIdx}_{i}!"));
13221327
sb.AppendLine($"{indent} return new {info.ClassName}({paramList});");
13231328
sb.AppendLine($"{indent} }}");
13241329
}

‎src/CommandLineUtils.Generators/SpecialPropertiesData.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,11 @@ internal sealed class SpecialPropertiesData
3838
/// </summary>
3939
public string? RemainingArgumentsPropertyType { get; set; }
4040

41+
/// <summary>
42+
/// Whether the RemainingArguments property is an array type (vs IReadOnlyList, IEnumerable, etc).
43+
/// </summary>
44+
public bool RemainingArgumentsIsArray { get; set; }
45+
4146
/// <summary>
4247
/// Whether any special properties exist.
4348
/// </summary>

‎test/CommandLineUtils.Tests/SourceGeneration/GeneratedModelFactoryTests.cs‎

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,239 @@
1+
// Copyright (c) Nate McMaster.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using McMaster.Extensions.CommandLineUtils.SourceGeneration;
7+
using Xunit;
8+
9+
namespace McMaster.Extensions.CommandLineUtils.Tests.SourceGeneration
10+
{
11+
/// <summary>
12+
/// Tests that simulate patterns used in generated model factories.
13+
/// These tests verify the correctness of code generation patterns.
14+
/// </summary>
15+
public class GeneratedModelFactoryTests
16+
{
17+
#region Test Models
18+
19+
private class ModelWithValueTypeConstructorParameters
20+
{
21+
public int Port { get; }
22+
public bool Verbose { get; }
23+
24+
public ModelWithValueTypeConstructorParameters(int port, bool verbose)
25+
{
26+
Port = port;
27+
Verbose = verbose;
28+
}
29+
}
30+
31+
private class ModelWithMixedConstructorParameters
32+
{
33+
public string Name { get; }
34+
public int Count { get; }
35+
36+
public ModelWithMixedConstructorParameters(string name, int count)
37+
{
38+
Name = name;
39+
Count = count;
40+
}
41+
}
42+
43+
#endregion
44+
45+
#region Test Service Provider
46+
47+
private class TestServiceProvider : IServiceProvider
48+
{
49+
private readonly Dictionary<Type, object?> _services = new();
50+
51+
public void Register<T>(T service)
52+
{
53+
_services[typeof(T)] = service;
54+
}
55+
56+
public object? GetService(Type serviceType)
57+
{
58+
_services.TryGetValue(serviceType, out var service);
59+
return service;
60+
}
61+
}
62+
63+
#endregion
64+
65+
#region Bug Documentation: 'as' operator with value types
66+
67+
/// <summary>
68+
/// Documents the bug in generated code: using 'as' with non-nullable value types doesn't compile.
69+
///
70+
/// The current generator produces:
71+
/// var p0_0 = _services.GetService(typeof(int)) as int;
72+
///
73+
/// This is invalid C# because 'as' can only be used with reference types or nullable value types.
74+
/// The generated code will FAIL TO COMPILE for any command with value type constructor parameters.
75+
/// </summary>
76+
[Fact]
77+
public void Bug_AsOperatorWithNonNullableValueType_DoesNotCompile()
78+
{
79+
// This test documents the compilation failure.
80+
// We can't write code that fails to compile in a test, so we demonstrate
81+
// the issue conceptually:
82+
83+
var services = new TestServiceProvider();
84+
services.Register<int>(8080);
85+
86+
var service = services.GetService(typeof(int));
87+
88+
// THIS IS THE BUG: The generator produces this pattern which doesn't compile:
89+
// var p0_0 = service as int; // ERROR CS0077: The 'as' operator must be used with a reference type or nullable value type
90+
91+
// To make it compile, we'd need to use nullable:
92+
var p0_0_nullable = service as int?; // This compiles but defeats the purpose
93+
94+
// Or better yet, use the correct pattern (check then cast):
95+
var p0_0_correct = service != null ? (int)service : default;
96+
97+
Assert.NotNull(p0_0_nullable);
98+
Assert.Equal(8080, p0_0_nullable.Value);
99+
Assert.Equal(8080, p0_0_correct);
100+
}
101+
102+
#endregion
103+
104+
#region Correct Pattern: Check service then cast
105+
106+
/// <summary>
107+
/// This factory simulates the CORRECT pattern that the generator should produce:
108+
/// check if service exists, then cast using direct cast operator.
109+
/// </summary>
110+
private class CorrectGeneratedFactory_CheckThenCast : IModelFactory<ModelWithValueTypeConstructorParameters>
111+
{
112+
private readonly IServiceProvider? _services;
113+
114+
public CorrectGeneratedFactory_CheckThenCast(IServiceProvider? services) => _services = services;
115+
116+
public ModelWithValueTypeConstructorParameters Create()
117+
{
118+
if (_services != null)
119+
{
120+
var instance = _services.GetService(typeof(ModelWithValueTypeConstructorParameters)) as ModelWithValueTypeConstructorParameters;
121+
if (instance != null) return instance;
122+
123+
// CORRECT PATTERN: Check service object, then cast
124+
var service0_0 = _services.GetService(typeof(int));
125+
var service0_1 = _services.GetService(typeof(bool));
126+
127+
if (service0_0 != null && service0_1 != null)
128+
{
129+
return new ModelWithValueTypeConstructorParameters((int)service0_0, (bool)service0_1);
130+
}
131+
}
132+
133+
throw new InvalidOperationException("Unable to create instance of ModelWithValueTypeConstructorParameters");
134+
}
135+
136+
object IModelFactory.Create() => Create();
137+
}
138+
139+
[Fact]
140+
public void CorrectPattern_CheckThenCast_WorksForValueTypeConstructorParameters()
141+
{
142+
// Arrange
143+
var services = new TestServiceProvider();
144+
services.Register<int>(8080);
145+
services.Register<bool>(true);
146+
147+
var factory = new CorrectGeneratedFactory_CheckThenCast(services);
148+
149+
// Act
150+
var instance = factory.Create();
151+
152+
// Assert
153+
Assert.NotNull(instance);
154+
Assert.Equal(8080, instance.Port);
155+
Assert.True(instance.Verbose);
156+
}
157+
158+
[Fact]
159+
public void CorrectPattern_CheckThenCast_WorksForMixedReferenceAndValueTypes()
160+
{
161+
// Arrange
162+
var services = new TestServiceProvider();
163+
services.Register<string>("test-name");
164+
services.Register<int>(42);
165+
166+
var factory = new CorrectGeneratedFactory_MixedTypes(services);
167+
168+
// Act
169+
var instance = factory.Create();
170+
171+
// Assert
172+
Assert.NotNull(instance);
173+
Assert.Equal("test-name", instance.Name);
174+
Assert.Equal(42, instance.Count);
175+
}
176+
177+
private class CorrectGeneratedFactory_MixedTypes : IModelFactory<ModelWithMixedConstructorParameters>
178+
{
179+
private readonly IServiceProvider? _services;
180+
181+
public CorrectGeneratedFactory_MixedTypes(IServiceProvider? services) => _services = services;
182+
183+
public ModelWithMixedConstructorParameters Create()
184+
{
185+
if (_services != null)
186+
{
187+
var instance = _services.GetService(typeof(ModelWithMixedConstructorParameters)) as ModelWithMixedConstructorParameters;
188+
if (instance != null) return instance;
189+
190+
var service0_0 = _services.GetService(typeof(string));
191+
var service0_1 = _services.GetService(typeof(int));
192+
193+
if (service0_0 != null && service0_1 != null)
194+
{
195+
return new ModelWithMixedConstructorParameters((string)service0_0, (int)service0_1);
196+
}
197+
}
198+
199+
throw new InvalidOperationException("Unable to create instance");
200+
}
201+
202+
object IModelFactory.Create() => Create();
203+
}
204+
205+
#endregion
206+
207+
#region Edge Cases
208+
209+
[Fact]
210+
public void CorrectPattern_FailsGracefully_WhenServiceNotRegistered()
211+
{
212+
// Arrange: Don't register the services
213+
var services = new TestServiceProvider();
214+
215+
var factory = new CorrectGeneratedFactory_CheckThenCast(services);
216+
217+
// Act & Assert
218+
var ex = Assert.Throws<InvalidOperationException>(() => factory.Create());
219+
Assert.Contains("Unable to create instance", ex.Message);
220+
}
221+
222+
[Fact]
223+
public void CorrectPattern_FailsGracefully_WhenPartialServicesRegistered()
224+
{
225+
// Arrange: Only register one of two required services
226+
var services = new TestServiceProvider();
227+
services.Register<int>(8080);
228+
// bool not registered
229+
230+
var factory = new CorrectGeneratedFactory_CheckThenCast(services);
231+
232+
// Act & Assert
233+
var ex = Assert.Throws<InvalidOperationException>(() => factory.Create());
234+
Assert.Contains("Unable to create instance", ex.Message);
235+
}
236+
237+
#endregion
238+
}
239+
}

0 commit comments

Comments
 (0)