fix: NativeAOT/Trimming for generated tools #20
Conversation
|
Gunpal Jain seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThe changes span several areas of the project. A new testing project has been introduced and configured within the solution. The generator project now enables unsafe blocks and updates formatting, while many new helper, model, and resource files have been added to support JSON schema and source generation. Additionally, namespaces have been reorganized and new attributes, flags, and extension methods have been added to enhance JSON serialization configuration and diagnostics. Finally, the test projects and snapshot tests have been extended with new service classes and auto‐generated serialization context files. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Build Process
participant GS as JsonSchemaGenerator
participant JSG as JsonSourceGenerator
participant E as Emitter
participant R as Roslyn Parser
participant S as Generated Source Files
U->>GS: Invoke generator methods (e.g. AsCalls2)
GS->>JSG: Initialize JSON source generator (Initialize/Initialize2)
JSG->>E: Delegate emission of source code
E->>R: Pass syntax trees and context for analysis
R->>S: Produce structured source output
S-->>U: Updated code with JSON schema metadata
sequenceDiagram
participant T as Test Runner
participant WS as WeatherService
participant JS as JsonSerializer
participant CTX as Generated JsonSerializerContext
participant O as Output Object
T->>WS: Invoke GetCurrentWeather3 (or async call)
WS->>CTX: Use generated serializer context for conversion
CTX->>JS: Serialize/Deserialize payload
JS-->>CTX: JSON type information retrieval
CTX-->>WS: Return deserialized complex object
WS-->>T: Provide verification and test assertions
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (77)
src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/ClassType.cs (1)
6-23: Well-defined class type enumerationThe
ClassTypeenum provides a comprehensive set of types needed for JSON serialization with proper documentation for special cases. This will be valuable for determining how to handle different types during serialization context generation.Consider adding XML documentation for all enum values, not just some of them, to maintain consistency and improve clarity.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/Hash.cs (1)
13-15: Consider documenting removed functionality for future reference.The comment indicates that some code was removed but may need to be re-added in the future. For better maintainability, consider:
- Adding a reference to where the original code can be found (e.g., a specific commit or reference implementation)
- Including more specific conditions for when the removed code should be restored
src/tests/CSharpToJsonSchema.IntegrationTests/ComplexClassSerializerTools.cs (2)
6-6: Consider adding default constructor and XML documentation.For test classes that will be used with serialization, it's helpful to have a parameterless constructor and XML documentation for clarity.
+/// <summary> +/// Complex class for testing JSON serialization with various property types. +/// </summary> public partial class ComplexClassSerializerTools { + public ComplexClassSerializerTools() + { + Tags = new List<string>(); + Metadata = new Dictionary<string, object>(); + Details = new NestedClass(); + }
16-21: Consider adding constructor for NestedClass.Similar to the parent class, the nested class should have a constructor to initialize the collection property.
public class NestedClass { public string Description { get; set; } public double Value { get; set; } public List<int> Numbers { get; set; } + + public NestedClass() + { + Numbers = new List<int>(); + } }src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.zh-Hant.xlf (2)
97-97: Minor typo in translation for JsonSerializerContext.There appears to be an inconsistent casing in the translation where "JsonSerializerCoNtext" has a capital N in the middle.
-<target state="translated">類型 '{0}' 已使用 JsonSerializableAttribute 標註，但並非衍生自 JsonSerializerCoNtext。將不會產生原始程式碼。</target> +<target state="translated">類型 '{0}' 已使用 JsonSerializableAttribute 標註，但並非衍生自 JsonSerializerContext。將不會產生原始程式碼。</target>
102-102: Same typo in another translation string.The same "JsonSerializerCoNtext" typo appears in this translation as well.
-<target state="translated">以 JsonSerializableAttribute 標註的類型必須為衍生自 JsonSerializerCoNtext 的類別。</target> +<target state="translated">以 JsonSerializableAttribute 標註的類型必須為衍生自 JsonSerializerContext 的類別。</target>src/tests/CSharpToJsonSchema.AotTests/Services/ComplexClass.cs (2)
7-13: Add property initializers to prevent null reference exceptions.The properties are declared without initializers, which could lead to NullReferenceExceptions when accessing collections. Consider adding initializers to the collection properties.
- public string Name { get; set; } + public string Name { get; set; } = string.Empty; public int Age { get; set; } public bool IsActive { get; set; } public DateTime CreatedAt { get; set; } - public List<string> Tags { get; set; } - public Dictionary<string, object> Metadata { get; set; } - public NestedClass Details { get; set; } + public List<string> Tags { get; set; } = new(); + public Dictionary<string, object> Metadata { get; set; } = new(); + public NestedClass Details { get; set; } = new();
17-19: Add property initializers to the nested class as well.Similar to the parent class, the nested class properties should have initializers to prevent null reference exceptions.
- public string Description { get; set; } + public string Description { get; set; } = string.Empty; public double Value { get; set; } - public List<int> Numbers { get; set; } + public List<int> Numbers { get; set; } = new();src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/GeneratorAttributeSyntaxContext.cs (1)
35-35: Consider removing extra blank line.There's an unnecessary blank line before the constructor declaration.
- internal GeneratorAttributeSyntaxContext(src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueEqualityImmutableDictionary.cs (1)
28-39: Consider thread-safety for cached hash code.The _hash field isn't protected against race conditions in a multi-threaded environment, which could lead to the hash being calculated multiple times or even getting a partially computed value.
For a struct that's meant to be immutable, consider making the hash code initialization thread-safe:
public override int GetHashCode() { - if (_hash.HasValue) - return _hash.Value; - - _hash = 0; - foreach (var kvp in Map.ToImmutableArray().Sort()) - { - _hash = HashCode.Combine(_hash, kvp.Key, kvp.Value); - } - return _hash.Value; + return _hash ??= CalculateHashCode(); + + int CalculateHashCode() + { + int hash = 0; + foreach (var kvp in Map.ToImmutableArray().Sort()) + { + hash = HashCode.Combine(hash, kvp.Key, kvp.Value); + } + return hash; + } }src/libs/CSharpToJsonSchema.Generators/JsonSchemaGenerator.cs (2)
40-41: Initialization of JsonSourceGenerator needs documentation.The purpose of this new JsonSourceGenerator and its relationship to the existing generator is not immediately clear. Consider adding a comment explaining how this enhances NativeAOT support.
+// Initialize the JSON source generator to support JsonSerializerContext generation for AOT compatibility var generator = new JsonSourceGenerator(); generator.Initialize2(context);
69-72: Document the purpose of AsCalls2 method.This method appears to duplicate functionality from AsCalls but returns a SourceText instead of FileWithName. Consider adding documentation to explain its specific purpose related to NativeAOT support.
+/// <summary> +/// Returns the source text for calls in a format compatible with JsonSerializerContext for AOT scenarios. +/// </summary> +/// <param name="interface">The interface data to generate calls for.</param> +/// <returns>A tuple containing the hint name and source text for the generated calls.</returns> public static (string hintName, SourceText sourceText) AsCalls2(InterfaceData @interface) { return ($"{@interface.Name}.Calls.generated.cs", SourceText.From(Sources.GenerateCalls(@interface))); }src/libs/CSharpToJsonSchema/GenerateJsonTypeInfoAttribute.cs (1)
10-16: Unused constructor parameter.The constructor takes a Type parameter but doesn't use it, requiring the IDE0060 warning suppression. Consider documenting why the parameter is stored or used elsewhere.
#pragma warning disable IDE0060 /// <summary> /// Initializes a new instance of <see cref="JsonSerializableAttribute"/> with the specified type. /// </summary> /// <param name="type">The type to generate source code for.</param> - public GenerateJsonTypeInfoAttribute(Type type) { } + public GenerateJsonTypeInfoAttribute(Type type) + { + // Type parameter is required for reflection-based attribute usage + // but not used at runtime - it's processed during source generation + } #pragma warning restore IDE0060src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SequenceEqualImmutableArray.cs (2)
24-26: Add documentation for indexer and Length property.The indexer and Length property lack documentation. Consider adding XML documentation to maintain consistency with the rest of the code.
+ /// <summary> + /// Gets the element at the specified index in the array. + /// </summary> + /// <param name="i">The zero-based index of the element to get.</param> + /// <returns>The element at the specified index.</returns> public T this[int i] { get => Array[i]; } + /// <summary> + /// Gets the number of elements in the array. + /// </summary> public int Length => Array.Length;
27-28: Maintain consistent method styling.The Insert method uses an expression-bodied member while the Add method uses a block body. Consider using a consistent style throughout the code.
- public SequenceEqualImmutableArray<T> Insert(int index, T item) - => new SequenceEqualImmutableArray<T>(Array.Insert(index, item), Comparer); + public SequenceEqualImmutableArray<T> Insert(int index, T item) + { + return new SequenceEqualImmutableArray<T>(Array.Insert(index, item), Comparer); + }src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/GlobalAlias.cs (1)
63-67: Potential optimization for equality comparisonThe current implementation has a potential performance issue. When the spans have different lengths,
SequenceEqualwill still attempt to compare elements until it reaches the end of the shorter span.Consider adding a length check before the
SequenceEqualcall:- return this.AliasAndSymbolNames.AsSpan().SequenceEqual(aliases.AliasAndSymbolNames.AsSpan()); + return this.AliasAndSymbolNames.Length == aliases.AliasAndSymbolNames.Length && + this.AliasAndSymbolNames.AsSpan().SequenceEqual(aliases.AliasAndSymbolNames.AsSpan());src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueEqualityImmutableDictionaryHelperExtensions.cs (1)
14-17: Consider adding nullability annotationThe method doesn't handle null input, which could cause a NullReferenceException.
- public static ValueEqualityImmutableDictionary<TKey, TValue> ToValueEquals<TKey, TValue>(this ImmutableDictionary<TKey, TValue> source) + public static ValueEqualityImmutableDictionary<TKey, TValue> ToValueEquals<TKey, TValue>(this ImmutableDictionary<TKey, TValue> source) + where TKey : notnullAlso consider adding a null check:
public static ValueEqualityImmutableDictionary<TKey, TValue> ToValueEquals<TKey, TValue>(this ImmutableDictionary<TKey, TValue> source) + where TKey : notnull { + ArgumentNullException.ThrowIfNull(source); return new ValueEqualityImmutableDictionary<TKey, TValue>(source); }src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/SourceGenerationOptionsSpec.cs (1)
69-72: Single-expression method is concise and clear.Using a single-expression body nicely conveys the fallback behavior. It may be helpful to add a unit test verifying the return of
JsonKnownNamingPolicy.CamelCasewhenDefaultsisJsonSerializerDefaults.Web.src/libs/CSharpToJsonSchema.Generators/Sources.Calls.cs (2)
23-24: Optional: Add explanatory doc comments for the generated extension class.Providing doc comments for the generated partial class (e.g. describing its purpose and usage) can help new contributors understand the rationale behind code generation.
197-200: Empty partial serializer context.This partial class is declared but not yet populated with
[JsonSerializable]attributes or[JsonSourceGenerationOptions]. Adding them would ensure the source generator can properly generate the metadata for serialization.public partial class {extensionsClassName}JsonSerializerContext: global::System.Text.Json.Serialization.JsonSerializerContext {{ + [global::System.Text.Json.Serialization.JsonSerializable(typeof(SomeType))] + [global::System.Text.Json.Serialization.JsonSourceGenerationOptions(WriteIndented = true)] // Additional definitions as needed }}src/libs/CSharpToJsonSchema.Generators/JsonGen/System.Text.Json/JsonSerializableAttribute.cs (2)
3-4: Specify applicable target(s) with an[AttributeUsage]declaration.Defining where this attribute can be applied will clarify usage. Consider limiting to classes or structs if appropriate:
+[System.AttributeUsage(System.AttributeTargets.Class | System.AttributeTargets.Struct)] sealed class JsonSerializableAttribute : JsonAttribute {
10-10: Unused constructor parameter.The constructor accepts a
Typebut doesn't store or expose it. If the goal is simply an attribute marker, consider removing the parameter or storing it for later use.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueListBuilder.cs (2)
13-16: Potential type safety issue in constructor.There's a potentially unsafe cast in the constructor. The
scratchBufferparameter is defined asSpan<T?>(nullable), but it's then cast toSpan<T>(non-nullable) without checking for null values, which could lead to runtime errors.-public ValueListBuilder(Span<T?> scratchBuffer) -{ - _span = scratchBuffer!; -} +public ValueListBuilder(Span<T?> scratchBuffer) +{ + // Safer to validate that no elements are null before casting + _span = MemoryMarshal.CreateSpan(ref Unsafe.As<T?, T>(ref MemoryMarshal.GetReference(scratchBuffer)), scratchBuffer.Length); +}You may need to add the following import if you implement this suggestion:
using System.Runtime.InteropServices;
7-203: Well-implemented high-performance collection type.This
ValueListBuilder<T>implementation shows excellent attention to performance optimizations:
- Using
ArrayPool<T>for efficient memory management- Proper method annotations with
MethodImplOptions- Good separation of common and uncommon code paths
- Careful handling of resizing logic
One suggestion: Consider adding XML documentation for public methods to improve API discoverability and usage.
src/libs/CSharpToJsonSchema.Generators/JsonGen/System.Text.Json/JsonSourceGenerationAttribute.cs (1)
152-155: Fix documentation mismatch.The documentation for
IndentSizereferencesJsonSerializerOptions.IndentCharacterrather than the corresponding size property. Consider clarifying this to avoid confusion.-/// <summary> -/// Specifies the default value of <see cref="JsonSerializerOptions.IndentCharacter"/> when set. -/// </summary> +/// <summary> +/// Specifies the default value of <see cref="JsonSerializerOptions.IndentSize"/> when set. +/// </summary>src/libs/CSharpToJsonSchema.Generators/CSharpToJsonSchema.Generators.csproj (1)
14-14: Minor formatting changes only.These changes just add spaces before closing tags and don't affect functionality.
Also applies to: 26-27
src/libs/CSharpToJsonSchema.Generators/Conversion/SymbolGenerator.cs (3)
49-52: Commented-out code should be removed or implementedThe commented-out code for adding attribute lists with enum converters should either be implemented or removed to avoid confusion.
if (p.Type.TypeKind == TypeKind.Enum) { - //decleration = decleration.AddAttributeLists(AttributeList(SingletonSeparatedList(GetConverter(propertyTypeName)))); + // TODO: Implement enum converter or remove this block }
82-82: Commented-out code should be removedThis line appears to be a leftover from development and should be removed.
var compilation = originalCompilation .AddSyntaxTrees(syntaxTree); - //.WithAssemblyName(assemblyName);
76-76: Commented-out code should be removedRemove the commented-out code to improve code cleanliness.
var syntaxTree =CSharpSyntaxTree.Create(compilationUnit,parseOptions); -//CSharpSyntaxTree.Create(ns.NormalizeWhitespace());src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherAsyncArgs.g.verified.cs (1)
107-114: Add null check for Unit property.The code checks if
Locationis null before writing, but doesn't do the same forUnit. While the setter usesvalue!which suggests it shouldn't be null, adding a null check would improve robustness.writer.WritePropertyName(PropName_unit); -global::System.Text.Json.JsonSerializer.Serialize(writer, ((global::CSharpToJsonSchema.IntegrationTests.GetCurrentWeatherAsyncArgs)value).Unit, Unit); +var unit = ((global::CSharpToJsonSchema.IntegrationTests.GetCurrentWeatherAsyncArgs)value).Unit; +if (unit is not null) +{ + global::System.Text.Json.JsonSerializer.Serialize(writer, unit, Unit); +} +else +{ + writer.WriteNullValue(); +}src/tests/CSharpToJsonSchema.AotTests/JsonSerializationTests.cs (3)
46-49: Uncomment or remove metadata validation codeThere are commented-out sections for metadata validation. Either uncomment this code if the validation is important or remove it completely if it's not needed.
10-59: Test method should be asyncThe test method signature has a return type of
Taskbut usesTask.CompletedTaskinstead of being marked asasync. Consider making it consistent with the second test method.- public Task ShouldDeserializeWithJsonTypeInfo() + public async Task ShouldDeserializeWithJsonTypeInfo()And at the end of the method:
- return Task.CompletedTask;
13-32: Extract common test object creation logicBoth test methods create nearly identical
ComplexClassSerializerToolsobjects. Consider extracting this initialization into a helper method or setup method to reduce duplication.+ private ComplexClassSerializerTools CreateComplexClassInstance() + { + return new ComplexClassSerializerTools() + { + Name = "Example Name", + Age = 30, + IsActive = true, + CreatedAt = DateTime.UtcNow, + Tags = new List<string> { "tag1", "tag2", "tag3" }, + Metadata = new Dictionary<string, object> + { + { "Key1", "Value1" }, + { "Key2", 12345 }, + { "Key3", true } + }, + Details = new ComplexClassSerializerTools.NestedClass + { + Description = "Nested description", + Value = 99.99, + Numbers = new List<int> { 1, 2, 3, 4, 5 } + } + }; + }Then use it in both test methods:
- var args = new ComplexClassSerializerTools() - { - Name = "Example Name", - Age = 30, - IsActive = true, - CreatedAt = DateTime.UtcNow, - Tags = new List<string> { "tag1", "tag2", "tag3" }, - Metadata = new Dictionary<string, object> - { - { "Key1", "Value1" }, - { "Key2", 12345 }, - { "Key3", true } - }, - Details = new ComplexClassSerializerTools.NestedClass - { - Description = "Nested description", - Value = 99.99, - Numbers = new List<int> { 1, 2, 3, 4, 5 } - } - }; + var args = CreateComplexClassInstance();Also applies to: 67-86
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SourceWriter.cs (3)
109-115: Document the unsafe code block with its rationaleThe unsafe code used for performance optimization when appending character spans should be documented with a comment explaining why this approach was chosen over safer alternatives.
+ // Using unsafe code for direct memory access to optimize span operations + // which provides better performance than character-by-character copying private static unsafe void AppendSpan(StringBuilder builder, ReadOnlySpan<char> span) { fixed (char* ptr = span) { builder.Append(ptr, span.Length); } }
20-24: Consider simplifying exception throwing patternThe pattern of using a local function just to throw an exception is uncommon and adds complexity. Consider using a more direct approach unless there's a specific performance reason for this pattern.
if (value < 0) { - Throw(); - static void Throw() => throw new ArgumentOutOfRangeException(nameof(value)); + throw new ArgumentOutOfRangeException(nameof(value)); }
7-116: Consider adding fluent API methods for indentation managementThe class would benefit from fluent API methods for managing indentation such as
Indent()andDedent()to make the usage more readable and less error-prone than directly manipulating theIndentationproperty.+ public SourceWriter Indent() + { + Indentation++; + return this; + } + + public SourceWriter Dedent() + { + if (Indentation > 0) + Indentation--; + return this; + }src/tests/CSharpToJsonSchema.AotTests/Services/WeatherTools.cs (2)
69-91: Unnecessary async keywordThe method
GetComplexDataTypeis marked with theasynckeyword but doesn't contain anyawaitoperations, making the async keyword unnecessary. This creates a synchronous method wrapped in an async Task result.-public async Task<ComplexClassSerializerTools> GetComplexDataType(Weather weatherName, Unit unit = Unit.Celsius, CancellationToken cancellationToken = default) +public Task<ComplexClassSerializerTools> GetComplexDataType(Weather weatherName, Unit unit = Unit.Celsius, CancellationToken cancellationToken = default) { - return new ComplexClassSerializerTools() + return Task.FromResult(new ComplexClassSerializerTools() { Name = "Example Name", Age = 30, IsActive = true, CreatedAt = DateTime.UtcNow, Tags = new List<string> { "tag1", "tag2", "tag3" }, Metadata = new Dictionary<string, object> { { "Key1", "Value1" }, { "Key2", 12345 }, { "Key3", true } }, Details = new ComplexClassSerializerTools.NestedClass { Description = "Nested description", Value = 99.99, Numbers = new List<int> { 1, 2, 3, 4, 5 } } - }; + }); }
16-22: Add null checks or use required keyword for Weather propertiesThe
Weatherclass has several properties that could be null but lacks null checks. Consider using C# required properties or adding null validation.public class Weather { - public string Location { get; set; } = string.Empty; - public double Temperature { get; set; } - public Unit Unit { get; set; } - public string Description { get; set; } = string.Empty; + public required string Location { get; set; } = string.Empty; + public double Temperature { get; set; } + public Unit Unit { get; set; } + public required string Description { get; set; } = string.Empty; }src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/RoslynExtensions_2.cs (3)
64-68: Improve null check clarityThe code uses
is objectfor a null check, which is less readable than more explicit alternatives. Consider using!= nullor pattern matching for clearer intent.-if (type is object) +if (type != null) { // Multiple visible types with the same metadata name are present return null; }
165-172: Commented code should be removed or explainedThe method
GetUnqualifiedNameis commented out. Either remove it if it's unused or explain why it's being kept but commented out.- // public static SimpleNameSyntax GetUnqualifiedName(this NameSyntax name) - // => name switch - // { - // AliasQualifiedNameSyntax alias => alias.Name, - // QualifiedNameSyntax qualified => qualified.Right, - // SimpleNameSyntax simple => simple, - // _ => throw new InvalidOperationException("Unreachable"), - // };
147-163:❓ Verification inconclusive
Consider performance optimization for ToImmutableArray
The implementation of
ToImmutableArraycreates a new builder when the span length is greater than 4. For large spans, this could be optimized further by using unsafe code or Memory.
🌐 Web query:
Are there more efficient ways to convert a ReadOnlySpan<T> to ImmutableArray<T> for large spans?💡 Result:
To efficiently convert a
ReadOnlySpan<T>to anImmutableArray<T>, especially for large spans, useImmutableArray.Create<T>(ReadOnlySpan<T> items)introduced in .NET 5+. This method directly copies the span's contents to a new array in a single step[1][4][12], avoiding intermediate allocations:ReadOnlySpan<int> span = ...; ImmutableArray<int> immutable = ImmutableArray.Create(span);Key Advantages:
Single Copy Operation:
Unlike older patterns (e.g.,span.ToArray().ToImmutableArray()),Createavoids creating a temporary mutable array, reducing overhead for large data[1][4].Builder Optimization:
For incremental construction, useImmutableArray<T>.BuilderwithAddRange(ReadOnlySpan<T>)to minimize array resizing[9][12]:var builder = ImmutableArray.CreateBuilder<int>(span.Length); builder.AddRange(span); ImmutableArray<int> result = builder.ToImmutable();Performance Notes:
- Avoid
ToArray(): Directly passing the span toCreateis more efficient than manualToArray()conversions[1][4].- Unsupported Scenarios: If using older .NET versions (<5.0),
ToArray()is unavoidable but less optimal[4][7].For most cases,
ImmutableArray.Createprovides the best balance of simplicity and performance.Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.readonlyspan-1?view=net-9.0
- 2: dotnet/runtime#22928
- 3: https://louthy.github.io/language-ext/LanguageExt.Core/Immutable%20Collections/
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.collections.immutable.immutablearray-1?view=net-9.0
- 5: https://blog.ndepend.com/improve-c-code-performance-with-spant/
- 6: https://learn.microsoft.com/en-us/archive/msdn-magazine/2018/january/csharp-all-about-span-exploring-a-new-net-mainstay
- 7: https://www.codeproject.com/Articles/140138/Immutable-Array-for-NET
- 8: https://www.benjamin.pizza/posts/2021-05-18-some-notes-on-variables-and-binding.html
- 9: https://learn.microsoft.com/en-us/dotnet/api/system.collections.immutable.immutablearray-1.builder?view=net-9.0
- 10: https://reflection1451.rssing.com/chan-63183954/latest.php
- 11: https://www.codemag.com/Article/2207031/Writing-High-Performance-Code-Using-SpanT-and-MemoryT-in-C
- 12: dotnet/runtime#53154
- 13: dotnet/roslyn-analyzers#5607
Optimize ToImmutableArray for Large Spans
Based on recent findings, you can improve performance for spans with more than four elements by replacing the manual builder-based loop with the built-in conversion method available in .NET 5+. Instead of iterating through the span to populate the builder, you can directly use:
return ImmutableArray.Create(span);This method performs a single copy of the span’s contents into the immutable array, reducing overhead and avoiding unnecessary allocations. Be sure to verify that your project targets .NET 5 or later prior to adopting this change.
Suggested Diff for the Default Case:
- default: - var builder = ImmutableArray.CreateBuilder<T>(span.Length); - foreach (var item in span) - builder.Add(item); - - return builder.MoveToImmutable(); + default: + return ImmutableArray.Create(span);src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.Emitter.ExceptionMessages.cs (1)
10-32: Standardize exception message formatThe exception messages use different formats - some include quotes around placeholders, others don't. Consider standardizing the format for consistency.
private static class ExceptionMessages { public const string InaccessibleJsonIncludePropertiesNotSupported = - "The member '{0}.{1}' has been annotated with the JsonIncludeAttribute but is not visible to the source generator."; + "The member '{0}.{1}' has been annotated with the JsonIncludeAttribute but is not visible to the source generator."; public const string IncompatibleConverterType = - "The converter '{0}' is not compatible with the type '{1}'."; + "The converter '{0}' is not compatible with the type '{1}'."; public const string InitOnlyPropertySetterNotSupported = "Setting init-only properties is not supported in source generation mode."; public const string InvalidJsonConverterFactoryOutput = - "The converter '{0}' cannot return null or a JsonConverterFactory instance."; + "The converter '{0}' cannot return null or a JsonConverterFactory instance."; public const string InvalidSerializablePropertyConfiguration = - "Invalid serializable-property configuration specified for type '{0}'. For more information, see 'JsonSourceGenerationMode.Serialization'."; + "Invalid serializable-property configuration specified for type '{0}'. For more information, see 'JsonSourceGenerationMode.Serialization'."; public const string PropertyGetterDisallowNull = - "The property or field '{0}' on type '{1}' doesn't allow getting null values. Consider updating its nullability annotation."; + "The property or field '{0}' on type '{1}' doesn't allow getting null values. Consider updating its nullability annotation.";src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/DiagnosticInfo.cs (1)
14-19: Consider minimizing array creation overhead.
messageArgs: MessageArgs.Array.ToArray()introduces an extra array creation, which might have a slight overhead if diagnostics are generated frequently. Consider reusing the underlying array or passing the immutable array directly if performance is critical.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SyntaxValueProviderExtensions_2.cs (2)
52-57: Remove or clarify#if falseblocks if no longer needed.Multiple
#if falseblocks reference unused or language-specific functionality (e.g., VB global aliases, object pooling). If these are permanently unneeded, removing them may reduce code clutter. If you plan to enable them later, consider adding explanatory comments.Also applies to: 115-133, 219-238
185-253: Second overload complements the first; be mindful of duplication.The second
ForAttributeWithSimpleNameoverload duplicates logic from the first, with differences in how it retrieves the compilation provider. Consider refactoring common code into a shared helper to reduce duplication and ensure consistency.src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/RoslynExtension2.cs (2)
35-76: Validate null inputs forfullyQualifiedMetadataName.Currently, there's no explicit check for invalid or null inputs in
GetBestTypeByMetadataName(…). While it relies on the Roslyn API, adding defensive checks or documentation would improve robustness and clarify behavior for edge cases.public static INamedTypeSymbol? GetBestTypeByMetadataName(this Compilation compilation, string fullyQualifiedMetadataName) { + if (string.IsNullOrEmpty(fullyQualifiedMetadataName)) + throw new ArgumentNullException(nameof(fullyQualifiedMetadataName)); + // ... }
157-176: Optimize array creation for larger spans if performance is critical.For large spans, multiple expansions and iteration may have a minor overhead. However, given this runs at compile-time, it’s likely acceptable. Continue usage unless profiling indicates a performance bottleneck.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SyntaxValueProviderExtensions.cs (3)
57-60: Handle potential missing '.' infullyQualifiedMetadataName.Using
Debug.Assert(lastDotIndex > 0)ensures correctness in debug, but a runtime exception in production might be preferable if the string is malformed. Consider raising a more explicit error or describing it in the method’s contract.var lastDotIndex = fullyQualifiedMetadataName.LastIndexOf('.'); - Debug.Assert(lastDotIndex > 0); + if (lastDotIndex <= 0) + { + throw new ArgumentException("fullyQualifiedMetadataName must contain at least one '.'"); + }
75-103: Validate memory usage ofValueListBuilder<T>for large projects.While typical usage is safe, confirm that building large lists from numerous syntax nodes won’t cause resource spikes. If usage grows, consider incremental streaming or chunked processing of nodes.
173-190: Ensure consistent approach in both overloads.Both
ForAttributeWithMetadataName(…)overloads are nearly identical. If future logic changes, ensure you keep them in sync or factor out common functionality to reduce duplication and potential bugs.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueAsyncArgs.g.verified.cs (1)
58-68: Empty JSON object serializer.
GetValueAsyncArgsSerializeHandleronly writes{}for non-null instances. IfGetValueAsyncArgsholds data that must be serialized, it needs properties written here.Would you like a code snippet to serialize specific properties if required?
src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/JsonHelpers.cs (1)
90-170: Handle cycles more explicitly in Kahn's algorithm.
The current implementation usesDebug.Assertfor cycle detection. In release configurations, this could yield silent failures if the graph contains cycles. Consider throwing a descriptive exception or returning partial outputs with an error indicator to handle cycles gracefully.src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.Roslyn4.0.cs (3)
28-65: Unify or better differentiate the initialization methods.Two similarly structured methods,
Initialize2(interface-based) andInitialize(class-based), could cause confusion or code duplication in the future. Consider consolidating them or renaming them to reflect their distinct purposes more clearly.
106-113: Remove or restore diagnostic reporting code.The diagnostic reporting lines are commented out. If these diagnostics are no longer needed, consider removing the dead code. Otherwise, re-enable the logic to provide valuable build-time feedback.
114-119: Clarify no-op behavior when context spec is null.Returning early when
ContextGenerationSpecis null avoids generation. If this is an expected scenario, consider adding a comment to clarify why generation is skipped and whether we might need warnings or logs.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.g.verified.cs (1)
13-38: Well-Structured Default Serializer OptionsThese options comprehensively set up features like property naming policies, number handling, and ignore conditions. The presence of
IndentCharacterandIndentSizeis a small nitpick sinceWriteIndentedis set tofalse(meaning the indentation settings might not take effect). If you intend to produce indented JSON, setWriteIndented = true. Otherwise, consider removing the indentation properties for clarity.src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/HashHelpers.cs (3)
28-35: Consider converting prime numbers span to a static readonly field.Using a static readonly field instead of a property with an implicit array creation could improve performance by avoiding repeated array allocations.
- internal static ReadOnlySpan<int> Primes => - [ - 3, 7, 11, 17, 23, 29, 37, 47, 59, 71, 89, 107, 131, 163, 197, 239, 293, 353, 431, 521, 631, 761, 919, - 1103, 1327, 1597, 1931, 2333, 2801, 3371, 4049, 4861, 5839, 7013, 8419, 10103, 12143, 14591, - 17519, 21023, 25229, 30293, 36353, 43627, 52361, 62851, 75431, 90523, 108631, 130363, 156437, - 187751, 225307, 270371, 324449, 389357, 467237, 560689, 672827, 807403, 968897, 1162687, 1395263, - 1674319, 2009191, 2411033, 2893249, 3471899, 4166287, 4999559, 5999471, 7199369 - ]; + private static readonly int[] s_primes = + { + 3, 7, 11, 17, 23, 29, 37, 47, 59, 71, 89, 107, 131, 163, 197, 239, 293, 353, 431, 521, 631, 761, 919, + 1103, 1327, 1597, 1931, 2333, 2801, 3371, 4049, 4861, 5839, 7013, 8419, 10103, 12143, 14591, + 17519, 21023, 25229, 30293, 36353, 43627, 52361, 62851, 75431, 90523, 108631, 130363, 156437, + 187751, 225307, 270371, 324449, 389357, 467237, 560689, 672827, 807403, 968897, 1162687, 1395263, + 1674319, 2009191, 2411033, 2893249, 3471899, 4166287, 4999559, 5999471, 7199369 + }; + + internal static ReadOnlySpan<int> Primes => s_primes;
89-91: Consider adding bounds checking for divisor parameter.The
GetFastModMultipliermethod should verify that the divisor is not zero to prevent division by zero exceptions.public static ulong GetFastModMultiplier(uint divisor) => + divisor == 0 ? throw new DivideByZeroException() : ulong.MaxValue / divisor + 1;
110-116: Consider adding XML documentation for the Combine method.Similar to other methods in this class, the
Combinemethod would benefit from XML documentation describing its purpose and parameters.+/// <summary> +/// Combines two hash codes into a new hash code using bitwise rotation. +/// </summary> +/// <param name="h1">The first hash code to combine.</param> +/// <param name="h2">The second hash code to combine.</param> +/// <returns>A combined hash code.</returns> public static int Combine(int h1, int h2) { // RyuJIT optimizes this to use the ROL instruction // Related GitHub pull request: https://github.com/dotnet/coreclr/pull/1830 uint rol5 = ((uint)h1 << 5) | ((uint)h1 >> 27); return ((int)rol5 + h1) ^ h2; }src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ImmutableEquatableArray.cs (2)
27-36: Optimize GetHashCode for better null handling and performance.The current implementation may not handle all cases optimally. Consider using
EqualityComparer<T>.Defaultfor better type handling and potentially better performance.public override int GetHashCode() { int hash = 0; - foreach (T value in _values) - { - hash = HashHelpers.Combine(hash, value is null ? 0 : value.GetHashCode()); - } + var comparer = EqualityComparer<T>.Default; + foreach (T value in _values) + { + hash = HashHelpers.Combine(hash, comparer.GetHashCode(value)); + } return hash; }
42-67: Consider implementing IEnumerator for the Enumerator struct.For more consistent behavior with other collection types, the Enumerator struct should implement IEnumerator.
-public struct Enumerator +public struct Enumerator : IEnumerator<T> { private readonly T[] _values; private int _index; internal Enumerator(T[] values) { _values = values; _index = -1; } public bool MoveNext() { int newIndex = _index + 1; if ((uint)newIndex < (uint)_values.Length) { _index = newIndex; return true; } return false; } public readonly T Current => _values[_index]; + + readonly object IEnumerator.Current => Current; + + public readonly void Reset() => _index = -1; + + public readonly void Dispose() { } }src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/AbstractSyntaxHelper.cs (2)
5-28: Enhance documentation with XML comments for this abstract class.Adding XML documentation to the abstract class and its methods would improve developer experience and code maintainability, especially since this class serves as a contract for concrete implementations.
+/// <summary> +/// Abstract base implementation of the <see cref="ISyntaxHelper"/> interface that provides +/// syntax analysis functionality for C# code. +/// </summary> internal abstract class AbstractSyntaxHelper : ISyntaxHelper { + /// <summary> + /// Gets a value indicating whether the syntax is case-sensitive. + /// </summary> public abstract bool IsCaseSensitive { get; } + /// <summary> + /// Determines if the specified name is a valid identifier. + /// </summary> + /// <param name="name">The name to validate.</param> + /// <returns>True if the name is a valid identifier; otherwise, false.</returns> public abstract bool IsValidIdentifier(string name); + /// <summary> + /// Gets the unqualified identifier token from a name node. + /// </summary> + /// <param name="name">The name node.</param> + /// <returns>The unqualified identifier token.</returns> public abstract SyntaxToken GetUnqualifiedIdentifierOfName(SyntaxNode name); // Add similar documentation for other methods
24-27: Consider adding documentation for AddAliases methods.These methods would benefit from documentation explaining the purpose of the ValueListBuilder and when the global parameter should be true.
+ /// <summary> + /// Adds aliases from the specified node to the provided list builder. + /// </summary> + /// <param name="node">The syntax node to extract aliases from.</param> + /// <param name="aliases">The list builder to add aliases to.</param> + /// <param name="global">Indicates whether to add global aliases.</param> public abstract void AddAliases(SyntaxNode node, ref ValueListBuilder<(string aliasName, string symbolName)> aliases, bool global); + /// <summary> + /// Adds aliases from compilation options to the provided list builder. + /// </summary> + /// <param name="options">The compilation options to extract aliases from.</param> + /// <param name="aliases">The list builder to add aliases to.</param> public abstract void AddAliases(CompilationOptions options, ref ValueListBuilder<(string aliasName, string symbolName)> aliases);src/libs/CSharpToJsonSchema.Generators/JsonGen/System.Text.Json/Flags.cs (2)
40-76: Consider adding a combined flag for AllowReadingFromString and WriteAsString.Since developers might frequently want to use both flags together, consider adding a convenience combined flag.
[Flags] public enum JsonNumberHandling { /// <summary> /// Numbers will only be read from <see cref="JsonTokenType.Number"/> tokens and will only be written as JSON numbers (without quotes). /// </summary> Strict = 0x0, /// <summary> /// Numbers can be read from <see cref="JsonTokenType.String"/> tokens. /// Does not prevent numbers from being read from <see cref="JsonTokenType.Number"/> token. /// Strings that have escaped characters will be unescaped before reading. /// Leading or trailing trivia within the string token, including whitespace, is not allowed. /// </summary> AllowReadingFromString = 0x1, /// <summary> /// Numbers will be written as JSON strings (with quotes), not as JSON numbers. /// <remarks> /// This behavior is not defined by the JSON specification. Altering the default number handling can potentially produce JSON that cannot be parsed by other JSON implementations. /// </remarks> /// </summary> WriteAsString = 0x2, + /// <summary> + /// Numbers can be read from string tokens and will be written as strings. + /// Combines <see cref="AllowReadingFromString"/> and <see cref="WriteAsString"/>. + /// </summary> + StringAsNumber = AllowReadingFromString | WriteAsString, + /// <summary> /// The "NaN", "Infinity", and "-Infinity" <see cref="JsonTokenType.String"/> tokens can be read as /// floating-point constants, and the <see cref="float"/> and <see cref="double"/> values for these /// constants (such as <see cref="float.PositiveInfinity"/> and <see cref="double.NaN"/>) /// will be written as their corresponding JSON string representations. /// Strings that have escaped characters will be unescaped before reading. /// Leading or trailing trivia within the string token, including whitespace, is not allowed. /// <remarks> /// This behavior is not defined by the JSON specification. Altering the default number handling can potentially produce JSON that cannot be parsed by other JSON implementations. /// </remarks> /// </summary> AllowNamedFloatingPointLiterals = 0x4 }
207-226: Consider adding ReferenceHandler type documentation.The enum references ReferenceHandler which is not documented here. Consider adding a reference to the appropriate documentation.
/// <summary> -/// The <see cref="ReferenceHandler"/> to be used at run time. +/// The <see cref="ReferenceHandler"/> to be used at run time for handling reference cycles during serialization. +/// <remarks> +/// Reference handlers control how circular references are detected and handled during the serialization process. +/// </remarks> /// </summary> public enum JsonKnownReferenceHandlersrc/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.DiagnosticDescriptors.cs (2)
7-14: Keep diagnostic descriptors synchronized with upstream references.
This comment references .NET runtime diagnostics at https://github.com/dotnet/runtime/docs/project/list-of-diagnostics.md. Be sure to periodically update these descriptors if changes occur in upstream documentation to prevent drifting from official definitions.
63-90: Consider localizable message formats.
If you anticipate localizing diagnostic messages, consider retrieving message formats from resource files, instead of embedding them directly in code, for improved maintainability.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Calls.generated.verified.cs (1)
74-95: Potential duplication in #if / #else blocks.
Multiple methods contain repeated logic for reflection vs. source-generated context. Consider extracting that into a small helper method to reduce duplication.- #if NET6_0_OR_GREATER - if(global::System.Text.Json.JsonSerializer.IsReflectionEnabledByDefault) { - // reflection-based path - } else { - // context-based path - } - #else - // fallback path - #endif + // Proposed approach: A single method that decides and returns the correct deserialization + // strategy or context, which your AsXYZArgs() calls can reuse.src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.Emitter.cs (3)
19-44: Constants define generator behavior clearly.
These constant strings and references to methods (“CreateValueInfo”, etc.) make the code generation logic more readable. Ensure they remain in sync if any renaming occurs elsewhere.
605-719: Property metadata initialization.
You store property type info, getters, setters, and possible default ignore conditions. This approach is thorough, though it’s quite large. Consider splitting out into smaller methods for clarity if you foresee adding more logic.
1472-1522: Collection method naming approach is straightforward.
TheGetCollectionInfoMethodNameswitch enumerates typical .NET collection patterns. Make sure newly-introduced collection types are added here if needed in future expansions.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/DiagnosticExtensions.cs (2)
8-50: Consider consolidating repetitive overloads.
MultipleCreateDiagnosticoverloads with and withoutpropertiesparameters share nearly identical logic. You could factor out common functionality into a private helper to reduce duplication and streamline maintenance.
112-215: Unify the Diagnostic and DiagnosticInfo creation if desired.
Repeated patterns exist in theCreateDiagnosticInfomethods that mirror theCreateDiagnosticlogic. While this repetition is acceptable, you could unify them behind a shared helper for easier future maintenance.src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/RoslynExtensions.cs (1)
287-323: Maintain coverage for nullability resolution.
The logic for determining nullability annotations from the original definition is consistent with limitations inSystem.Text.Json. Adding thorough unit tests, particularly for complex generic instantiations, could help confirm the approach remains robust.src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/KnownTypeSymbols.cs (3)
25-125: Leverage cohesive caching strategy.
The caching pattern withOption<INamedTypeSymbol?>fields andGetOrResolveTypeworks well. If you find the number of properties growing further, consider centralizing logic (e.g., a dictionary-based structure) to simplify expansions.
265-316: Review large conditional chain in IsImmutableEnumerableType.
The sequentialifchecks for matching generic type definitions are clear, but you could replace them with a lookup table keyed by the relevant generic type definitions. This would reduce repetitive conditional blocks and simplify future maintenance.
318-343: Consider similar lookup optimization for IsImmutableDictionaryType.
Just likeIsImmutableEnumerableType, this method relies on sequential comparisons. A mapping of types to their corresponding factory names would be more direct and reduce branching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
CSharpToJsonSchema.sln(3 hunks)src/libs/CSharpToJsonSchema.Generators/CSharpToJsonSchema.Generators.csproj(3 hunks)src/libs/CSharpToJsonSchema.Generators/Conversion/SymbolGenerator.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/Conversion/ToModels.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/AbstractSyntaxHelper.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/CSharpSyntaxHelper.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/CSharpSyntaxUtilities.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/CollectionExtensions.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/DiagnosticExtensions.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/DiagnosticInfo.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/GeneratorAttributeSyntaxContext.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/GlobalAlias.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/Hash.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/HashCode.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ISyntaxHelper.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ImmutableEquatableArray.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SequenceEqualImmutableArray.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SourceWriter.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SyntaxValueProviderExtensions.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SyntaxValueProviderExtensions_2.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueEqualityImmutableDictionary.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueEqualityImmutableDictionaryHelperExtensions.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueListBuilder.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/DiagnosticHelper.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/HashHelpers.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/JsonHelpers.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/KnownTypeSymbols.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/RoslynExtension2.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/RoslynExtensions.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/RoslynExtensions_2.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/SourceGeneratorHelpers.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/TypeModelHelper.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonConstants.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.DiagnosticDescriptors.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.Emitter.ExceptionMessages.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.Emitter.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.Roslyn4.0.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/ClassType.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/CollectionType.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/ContextGenerationSpec.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/JsonPrimitiveTypeKind.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/ObjectConstructionStrategy.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/ParameterGenerationSpec.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/PropertyGenerationSpec.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/PropertyInitializerGenerationSpec.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/SourceGenerationOptionsSpec.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/TypeGenerationSpec.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/TypeRef.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/Strings.resx(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.cs.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.de.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.es.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.fr.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.it.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.ja.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.ko.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.pl.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.pt-BR.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.ru.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.tr.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.zh-Hans.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.zh-Hant.xlf(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/System.Text.Json/Flags.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/System.Text.Json/JsonSerializableAttribute.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonGen/System.Text.Json/JsonSourceGenerationAttribute.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/JsonSchemaGenerator.cs(3 hunks)src/libs/CSharpToJsonSchema.Generators/Models/ClassData.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/Models/InterfaceData.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/Models/MethodData.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/Models/OpenApiSchema.cs(1 hunks)src/libs/CSharpToJsonSchema.Generators/Sources.Calls.cs(6 hunks)src/libs/CSharpToJsonSchema.Generators/Sources.Tools.cs(1 hunks)src/libs/CSharpToJsonSchema/GenerateJsonTypeInfoAttribute.cs(1 hunks)src/tests/CSharpToJsonSchema.AotTests/CSharpToJsonSchema.AotTests.csproj(1 hunks)src/tests/CSharpToJsonSchema.AotTests/JsonSerializationTests.cs(1 hunks)src/tests/CSharpToJsonSchema.AotTests/Services/ComplexClass.cs(1 hunks)src/tests/CSharpToJsonSchema.AotTests/Services/WeatherTools.cs(1 hunks)src/tests/CSharpToJsonSchema.IntegrationTests/CSharpToJsonSchema.IntegrationTests.csproj(1 hunks)src/tests/CSharpToJsonSchema.IntegrationTests/ComplexClassSerializerTools.cs(1 hunks)src/tests/CSharpToJsonSchema.IntegrationTests/VariousTypesTools.cs(2 hunks)src/tests/CSharpToJsonSchema.IntegrationTests/WeatherStrictTools.cs(2 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/CSharpToJsonSchema.SnapshotTests.csproj(3 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/SnapshotTests.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Boolean.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Calls.generated.verified.cs(5 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.DateOnly.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.DateTime.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Double.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetCurrentWeather3Args.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetJsonTypeInfo.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueArgs.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueAsyncArgs.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Int32.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Int64.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.PropertyNames.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.SetValueArgs.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.SetValueAsyncArgs.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Single.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Tools.generated.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Calls.generated.verified.cs(4 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Double.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherArgs.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherAsyncArgs.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetJsonTypeInfo.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.PropertyNames.g.verified.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.String.g.verified.cs(1 hunks)
⛔ Files not processed due to max files limit (15)
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Unit.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Weather.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Calls.generated.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Double.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetCurrentWeather2Args.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetCurrentWeatherAsync2Args.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetJsonTypeInfo.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.PropertyNames.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.String.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Tools.generated.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Unit2.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Weather2.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/TestHelper.cs
🧰 Additional context used
🪛 GitHub Check: Test / Build, test and publish
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueEqualityImmutableDictionary.cs
[warning] 6-6:
The type 'T' cannot be used as type parameter 'TKey' in the generic type or method 'ImmutableDictionary<TKey, TValue>'. Nullability of type argument 'T' doesn't match 'notnull' constraint.
[warning] 6-6:
The type 'T' cannot be used as type parameter 'TKey' in the generic type or method 'ImmutableDictionary<TKey, TValue>'. Nullability of type argument 'T' doesn't match 'notnull' constraint.
🔇 Additional comments (162)
src/tests/CSharpToJsonSchema.SnapshotTests/SnapshotTests.cs (1)
6-6: Blank Line Added for ReadabilityA blank line was added at line 6 in the
ToolTestsclass. This minor formatting change improves the readability of the class without impacting functionality.src/tests/CSharpToJsonSchema.IntegrationTests/CSharpToJsonSchema.IntegrationTests.csproj (1)
5-5: Formatting Adjustment in Project FileA blank line added within the
<PropertyGroup>(line 5) enhances visual separation of properties. This change is purely cosmetic and does not affect the build or functionality.src/libs/CSharpToJsonSchema.Generators/Models/InterfaceData.cs (1)
1-1: Namespace ReorganizationThe namespace has been updated to
CSharpToJsonSchema.Generators.Models, aligning this record struct with the overall project structure. This improves code clarity and maintainability.src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/JsonPrimitiveTypeKind.cs (1)
1-15: New Enum for JSON Primitive TypesThe file introduces the
JsonPrimitiveTypeKindenum, which clearly lists supported primitive JSON types. The inclusion of standard license headers and a dedicated namespace further enhances code quality and maintainability.src/libs/CSharpToJsonSchema.Generators/Models/MethodData.cs (1)
1-1: Updated Namespace for MethodDataThe namespace of the
MethodDatarecord struct has been updated toCSharpToJsonSchema.Generators.Models, providing better coherence with the reorganized codebase. This change supports improved clarity and consistency.src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/CollectionType.cs (1)
6-34: Well-structured collection type enumerationThe
CollectionTypeenum is well-organized with a clear separation between dictionary types and non-dictionary types. This comprehensive categorization will be valuable for the JsonSerializerContext generation.src/libs/CSharpToJsonSchema.Generators/Models/OpenApiSchema.cs (1)
1-1:❓ Verification inconclusive
Namespace change looks good
The namespace change from
CSharpToJsonSchema.Generators.CoretoCSharpToJsonSchema.Generators.Modelsis appropriate for better organization of model classes.Make sure to update any imports in files that reference this class to prevent compilation errors.
🏁 Script executed:
#!/bin/bash # Find all files that might reference OpenApiSchema with the old namespace rg -l "CSharpToJsonSchema\.Generators\.Core\.OpenApiSchema" --type csLength of output: 71
Action: Verify Import References for Namespace Update
The namespace change from
CSharpToJsonSchema.Generators.CoretoCSharpToJsonSchema.Generators.Modelsappears correctly applied. However, the automated search for references to the old namespace returned no results, which may be inconclusive. Please manually verify that all affected files have updated their imports, ensuring no residual references toCSharpToJsonSchema.Generators.Coreremain.
- Confirm that all import statements in the codebase now point to
CSharpToJsonSchema.Generators.Models(e.g., in files that might previously have containedusing CSharpToJsonSchema.Generators.Core;).src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/ObjectConstructionStrategy.cs (1)
6-23: Well-documented object construction strategy enumThe
ObjectConstructionStrategyenum is well-structured with clear XML documentation for each value, which will be helpful for developers using this enum. The values cover the essential construction strategies needed for object serialization.src/libs/CSharpToJsonSchema.Generators/Conversion/ToModels.cs (2)
2-2: Using internal model namespace instead of external dependency.The code now imports from the project's internal models namespace rather than an external dependency. This reduces external dependencies and gives the project more control over the model definitions.
5-5:❓ Verification inconclusive
Namespace hierarchy simplified.
Namespace has been updated from
CSharpToJsonSchema.Generators.Core.ConversiontoCSharpToJsonSchema.Generators.Conversion, simplifying the namespace hierarchy. This change must be consistent across the codebase.
🏁 Script executed:
#!/bin/bash # Check for any references to the old namespace that need to be updated rg -p "CSharpToJsonSchema\.Generators\.Core\.Conversion" --type csLength of output: 68
Namespace update verified, but please perform a quick manual check
The automated search for the old namespace (
CSharpToJsonSchema.Generators.Core.Conversion) didn’t return any occurrences, which indicates that the update toCSharpToJsonSchema.Generators.Conversionhas been applied consistently. However, since the automated output was minimal, please manually confirm that no overlooked references remain elsewhere in the codebase.
- Verified file:
src/libs/CSharpToJsonSchema.Generators/Conversion/ToModels.cs- No additional references to the old namespace were detected via automated search.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.ja.xlf (1)
1-167: Added Japanese localization support for error messages.This file adds comprehensive Japanese translations for error messages and diagnostics, enhancing internationalization. The XLIFF format is standard and properly structured with source English and target Japanese translations.
Some error messages are specifically related to NativeAOT requirements and JSON serialization context generation, which aligns with the PR's objective to enhance support for NativeAOT:
- The non-generic
JsonStringEnumConverterwarning (lines 105-114)- Init-only property serialization limitations (lines 65-74)
- Context class partialness requirement (lines 5-14)
These translations will provide clearer error messages for Japanese users when working with NativeAOT and JSON serialization.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/Hash.cs (1)
3-12: Add a hashing utility for generating deterministic hash codes.The
Hash.Combinemethod provides a way to combine hash values using a specific multiplication factor (0xA5555529). This is commonly used in generated code to create deterministic hash codes for objects.The implementation uses
uncheckedto prevent overflow exceptions, which is appropriate for hash code generation where overflow is expected and not problematic.src/libs/CSharpToJsonSchema.Generators/Sources.Tools.cs (1)
1-2: Namespace reorganization looks good.The change from using
CSharpToJsonSchema.Generators.CoretoCSharpToJsonSchema.Generators.Modelsaligns well with the PR's objective to enhance NativeAOT support through better organization of model classes.src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/TypeRef.cs (1)
1-38: Well-structured TypeRef implementation for type identity.This new class provides a clean implementation for representing type identity with proper equality comparison. The
CanBeNullproperty calculation and use ofFullyQualifiedNamefor identity will be helpful for JSON serialization context generation.src/tests/CSharpToJsonSchema.IntegrationTests/ComplexClassSerializerTools.cs (1)
1-22: Good test class structure for serialization testing.The partial class with various property types provides excellent coverage for testing JSON serialization. This will help validate the JsonSerializerContext generation for NativeAOT.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.zh-Hant.xlf (1)
1-167: Well-structured localization file for Traditional Chinese.The localization file provides comprehensive translations for all error messages and notes related to JSON serialization. This supports the NativeAOT/Trimming objectives by ensuring proper internationalization of diagnostics.
src/tests/CSharpToJsonSchema.AotTests/CSharpToJsonSchema.AotTests.csproj (3)
8-8: Good implementation of AOT testing configuration.Setting
JsonSerializerIsReflectionEnabledByDefaultto false properly ensures that the tests will use source-generated serializers rather than reflection-based ones, which is essential for validating NativeAOT compatibility.
25-27: Well-structured project references for source generator testing.Setting the Generators project reference with
OutputItemType="Analyzer" ReferenceOutputAssembly="false"correctly configures it to be used as a source generator during build without creating a runtime dependency.
4-4:✅ Verification successful
Verify .NET 9.0 compatibility in your CI pipeline.
The project targets .NET 9.0, which is currently in preview. Ensure your CI pipeline uses the appropriate SDK version to build this project.
🏁 Script executed:
#!/bin/bash # Check the .NET SDK version used in GitHub Actions echo "Checking .NET SDK version in GitHub Actions workflows" find .github -name "*.yml" -o -name "*.yaml" | xargs grep -l "dotnet" | xargs grep -i "version"Length of output: 271
CI Pipeline – .NET 9.0 Compatibility Confirmed
The GitHub Actions workflows are already configured with
dotnet-version: 9.0.x, which aligns with the project’s target of .NET 9.0 (currently in preview). No changes are needed at this time.src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.de.xlf (2)
1-167: Comprehensive German localization for JSON serialization errors.The XLIFF file provides complete German translations for all JSON serialization error messages, which is excellent for international users. The translations accurately convey technical concepts while maintaining natural German language flow.
106-107: Important AOT-specific error message for enum converters.This message correctly guides users to use the generic
JsonStringEnumConverter<TEnum>instead of the non-generic version that requires reflection, which is crucial for NativeAOT compatibility.src/tests/CSharpToJsonSchema.AotTests/Services/ComplexClass.cs (1)
5-5: Well-configured class for source generation.The class is correctly marked as
partial, which is required for the System.Text.Json source generator to extend it with generated code.src/tests/CSharpToJsonSchema.SnapshotTests/CSharpToJsonSchema.SnapshotTests.csproj (2)
22-23: Resource configuration updated for generator compatibility.The resource configuration has been updated to work correctly with H.Resources.Generator. Removing
Visible="false"and addingHResourcesGenerator_Resource="true"ensures the resources are properly processed by the generator.
43-45: New Snapshots folder for generated output.The addition of a dedicated Snapshots folder is a good practice for organizing verification test outputs, making it easier to review and manage the generated artifacts.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.pl.xlf (1)
105-107: Good addition of AOT-specific error message for JsonStringEnumConverter.The message correctly informs users that the non-generic
JsonStringEnumConverteris not supported in native AOT and suggests using the generic version instead. This aligns well with the PR's objective of enhancing NativeAOT support.src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.tr.xlf (1)
105-107: Appropriate translation of AOT-specific error message.The Turkish translation correctly conveys the warning about non-generic
JsonStringEnumConverternot being supported in native AOT and recommends using the generic alternative.src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.ko.xlf (1)
105-107: Correctly translated AOT-specific error message.The Korean translation properly informs users about the limitations of non-generic
JsonStringEnumConverterin native AOT environments and suggests the appropriate alternative.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/GeneratorAttributeSyntaxContext.cs (1)
6-47: Well-designed immutable context struct for attribute handling.The implementation uses a readonly struct with clear documentation and properly encapsulated properties. This design is appropriate for the attribute context information needed during source generation.
This struct is essential for the PR's goal of enhancing NativeAOT support by providing the necessary context for analyzing and generating JsonSerializerContext implementations.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.es.xlf (1)
1-167: LGTM! The Spanish translation file looks complete and well-structured.The XLIFF file follows proper formatting and contains all the necessary translation units for JSON serialization error messages. This will enhance the internationalization support for Spanish users.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.it.xlf (1)
1-167: LGTM! The Italian translation file is properly formatted and complete.The XLIFF translation file is correctly structured according to the XLIFF 1.2 standard and contains all required JSON serialization error message translations for Italian users.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueEqualityImmutableDictionary.cs (1)
41-56: Consider documenting the mutable operations that are delegated to the immutable dictionary.You're implementing a mutable interface (IDictionary<T,U>) on top of an immutable data structure, which may lead to unexpected behavior. Methods like Add, Remove, and Clear won't modify the wrapped ImmutableDictionary as expected.
Consider either:
- Documenting that these operations throw NotSupportedException
- Actually implementing them to return a new ValueEqualityImmutableDictionary with modified Map
- Making the implementation explicitly throw NotSupportedException
-public void Add(T key, U value) => ((IDictionary<T, U>)Map).Add(key, value); +public void Add(T key, U value) => throw new NotSupportedException("ValueEqualityImmutableDictionary is immutable");src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/SourceGeneratorHelpers.cs (1)
10-20: LGTM! The enum formatting helper is well-implemented.The method correctly handles both named enum values and numeric literals, formatting them appropriately for code generation.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.pt-BR.xlf (1)
1-167: LGTM! Good addition of Portuguese (Brazil) localization.The XLIFF file is well-structured and contains complete translations for all JSON serialization-related error messages and notes. This is a valuable addition for internationalization support in the NativeAOT context.
src/tests/CSharpToJsonSchema.IntegrationTests/VariousTypesTools.cs (1)
13-13: Method rename looks correct and consistent.The method has been renamed from
GetCurrentWeathertoGetCurrentWeather3consistently in both the interface and implementation. This change supports the NativeAOT/Trimming functionality being added.Also applies to: 37-37
src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/TypeModelHelper.cs (2)
7-30: Well-implemented traversal of generic types.The
GetAllTypeArgumentsInScopemethod effectively collects all type arguments by recursively traversing containing types. The use of a local function for traversal is efficient and keeps the code clean.
32-32: Good utility method for obtaining fully qualified names.This extension method provides a convenient way to get the fully qualified name of a type, which will be useful for JSON serialization context generation.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.ru.xlf (1)
1-167: LGTM! Good addition of Russian localization.The XLIFF file is well-structured and contains complete translations for all JSON serialization-related error messages and notes. This enhances internationalization support for the NativeAOT functionality.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/DiagnosticHelper.cs (1)
1-22: Well-designed diagnostic helper implementationThis is a clean implementation of a utility class for creating diagnostic descriptors. The help link generated follows Microsoft's standard format for System.Text.Json diagnostics.
src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonConstants.cs (1)
1-14: Clean constants implementationGood organization of JSON-related constants that will be used across the source generator. The constants follow consistent naming patterns and include the full namespaces, which is important for code generation.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.fr.xlf (1)
1-167: Comprehensive French localization fileThis XLIFF file provides French translations for JSON serialization error messages, which is essential for proper internationalization. All translation units are properly structured with source and target elements. The translation states are marked as "translated" which indicates they've been reviewed.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.cs.xlf (1)
1-167: Comprehensive Czech localization fileThis XLIFF file provides Czech translations for JSON serialization error messages, following the same well-structured format as the French version. Having localized error messages improves the developer experience for non-English speakers.
src/libs/CSharpToJsonSchema.Generators/JsonSchemaGenerator.cs (1)
1-2: Verify namespace organization.The namespaces have been reorganized with some imports moving from Core.Conversion to Models, and new JsonGen namespaces added. These changes support the NativeAOT/Trimming functionality being added in this PR.
Also applies to: 7-8
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.String.g.verified.cs (1)
1-37: Correct auto-generated serialization context.This is a properly formatted, auto-generated serialization context file that correctly implements JsonTypeInfo for string types. The nullable annotations and warning pragma directives are appropriately set.
src/libs/CSharpToJsonSchema/GenerateJsonTypeInfoAttribute.cs (1)
27-31:❓ Verification inconclusive
Commented-out property suggests incomplete implementation.
The GenerationMode property is commented out but fully documented. This could indicate it's planned for a future implementation. Consider adding a TODO comment or removing the documentation if it's not intended to be implemented yet.
🏁 Script executed:
#!/bin/bash # Check if JsonSourceGenerationMode is defined elsewhere in the project rg -A 1 -B 1 "JsonSourceGenerationMode" --type csharpLength of output: 9350
Action: Clarify the Commented-Out
GenerationModePropertyIn
src/libs/CSharpToJsonSchema/GenerateJsonTypeInfoAttribute.cs(lines 27–31), the XML documentation for theGenerationModeproperty remains even though the property itself is commented out. To prevent confusion about whether this is an incomplete implementation or an intentional omission, please either:
- Add a TODO comment above the commented-out property indicating that its implementation is pending, or
- Remove both the commented-out code and its documentation if this property is not intended to be implemented.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SequenceEqualImmutableArray.cs (2)
12-17: Well-documented wrapper for ImmutableArray with sequence equality.The struct correctly implements a wrapper for ImmutableArray that provides sequence-based equality, which is essential for proper comparison in source generation scenarios.
30-30:✅ Verification successful
Verify HashCode.SequentialValuesHash usage.
The method HashCode.SequentialValuesHash is used for computing the hash code. Make sure this method exists in the targeted framework versions to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Check if this is a standard .NET method or a custom implementation rg -A 3 -B 3 "SequentialValuesHash" --type csharpLength of output: 1607
HashCode.SequentialValuesHash Verified
I confirmed that the
HashCode.SequentialValuesHashmethod is custom-implemented and exists in our codebase atsrc/libs/CSharpToJsonSchema.Generators/JsonGen/Base/HashCode.cs. Its usage inSequenceEqualImmutableArray.csis valid, and there are no runtime compatibility concerns.src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/xlf/Strings.zh-Hans.xlf (1)
1-167: LGTM! Well-structured localization file.The XLIFF file for Simplified Chinese translations is properly formatted and contains complete translations for all JSON serialization error messages and warnings. This addition supports internationalization for Chinese-speaking developers using the library.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ISyntaxHelper.cs (1)
5-32: Good interface design for syntax analysis.The
ISyntaxHelperinterface is well-defined with clear method signatures for C# syntax analysis. The interface is cohesive, focusing on operations needed for JSON schema generation such as identifier validation, attribute handling, and namespace analysis.src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/PropertyInitializerGenerationSpec.cs (1)
6-34: Excellent use of C# record for immutability.The sealed record with clear documentation explains its design choices well. Using the record pattern provides immutability and structural equality, which is ideal for Roslyn incremental values caching as noted in the comments.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Resources/Strings.resx (1)
1-216: Well-structured resource file with clear error messages.This resource file follows standard ResX format and provides comprehensive error messages for JSON serialization scenarios. The naming convention is consistent with paired titles and message formats, and the error messages are informative and actionable.
I particularly appreciate the NativeAOT-specific error messages such as the one for non-generic
JsonStringEnumConverter(lines 174-179), which directly supports the PR's objective of enhancing NativeAOT compatibility.src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/ParameterGenerationSpec.cs (1)
1-35: Well-structured record for Roslyn incremental generator supportThis new record is well-designed for use with Roslyn's incremental generation system. The comprehensive documentation clearly explains the immutability and structural equality requirements for proper caching. The use of required properties ensures all necessary data is provided during initialization.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/ValueEqualityImmutableDictionaryHelperExtensions.cs (1)
1-13: LGTM: Extension method for converting to value equality dictionaryThis extension method provides a clean way to convert an enumerable to a value equality dictionary with a single call.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/PropertyGenerationSpec.cs (2)
1-153: Well-structured property model with thorough documentationThe
PropertyGenerationSpecrecord is well-designed with comprehensive XML documentation. The use of theDebuggerDisplayattribute improves debugging experience, and the required properties ensure all necessary data is provided during initialization.
157-194:❓ Verification inconclusive
Verify NativeAOT compatibility with reflection alternatives
The method looks well-structured with good conditional logic. However, for the context of NativeAOT/trimming support mentioned in the PR objective, runtime reflection-based property access can cause issues.
Ensure that the property access in your JsonGen implementation uses code generation rather than reflection-based approaches for the fast-path serialization. This would align with the PR's objective of enhancing NativeAOT support.
🏁 Script executed:
#!/bin/bash # Check for reflection usage in serialization code rg -l "GetProperty|PropertyInfo|MethodInfo|GetMethod" --type cs src/libs/CSharpToJsonSchema.Generators/JsonGen/Length of output: 343
Review: Verify Reflection Usage for NativeAOT Compatibility
The logic in
PropertyGenerationSpec.cs(lines 157–194) for determining fast-path inclusion is solid and does not use reflection. However, the search results indicate that reflection methods (e.g.,GetProperty,PropertyInfo, etc.) are used in:
JsonSourceGenerator.Parser.csJsonSourceGenerator.Emitter.csHelpers/RoslynExtensions.csSince the PR aims to enhance NativeAOT support, please verify that any reflection-based property access in these related components is either safely confined to compile-time code generation or refactored to use code generation techniques. This will ensure that no runtime reflection is used in the fast-path serializer that might compromise trimming and AOT compatibility.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/SourceGenerationOptionsSpec.cs (1)
13-74: Well-structured record with thorough properties.All configuration properties are clearly defined as
required, and the record is well-documented. This design is consistent with the optional nature of each setting. Please ensure that downstream code properly initializes these properties to avoid null reference issues at runtime.src/libs/CSharpToJsonSchema.Generators/Sources.Calls.cs (2)
12-12: Use of string interpolation to enable nullable contexts.Enabling
#nullableis beneficial for modern .NET projects. Confirm that all code generation paths properly handle null and non-null references to maintain consistency throughout the codebase.
63-86:❓ Verification inconclusive
Potential .NET 6 compatibility concern with
IsReflectionEnabledByDefault.The property
JsonSerializerOptions.IsReflectionEnabledByDefaultwas introduced in .NET 7. If you plan to support .NET 6, you may encounter CS0117 ("JsonSerializerOptions does not contain a definition..."). Verify that your build or runtime environment indeed supports this property.
🏁 Script executed:
#!/bin/bash # Check all references to IsReflectionEnabledByDefault and confirm no .NET 6 builds fail rg "IsReflectionEnabledByDefault" -A 3Length of output: 13903
Attention: Verify .NET Target Compatibility for IsReflectionEnabledByDefault Usage
The conditional block in
src/libs/CSharpToJsonSchema.Generators/Sources.Calls.cs(lines 63–86) utilizesJsonSerializer.IsReflectionEnabledByDefault, a property introduced in .NET 7. However, the code is enclosed within a#if NET6_0_OR_GREATERdirective—which includes .NET 6—potentially resulting in a compilation error (CS0117) if building under .NET 6.Please verify that:
- Your project explicitly targets .NET 7 (or later). If so, consider updating the preprocessor condition to
#if NET7_0_OR_GREATERto more clearly reflect the runtime requirements.- If .NET 6 support is needed, provide an alternate code path, since the property will not be available and will lead to compilation failures.
This concern appears in multiple locations (as evidenced from the search output), so ensuring the correct target framework is crucial.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/CollectionExtensions.cs (1)
1-19:✅ Verification successful
Code looks good, but a reference to SequenceEqualImmutableArray type is missing.
The extension methods are well-implemented, but the
SequenceEqualImmutableArray<T>type they return isn't defined in this file. Make sure this type is properly defined elsewhere in the codebase to avoid compilation errors.
🏁 Script executed:
#!/bin/bash # Check if SequenceEqualImmutableArray is defined in the codebase rg -p "class|struct|record\s+SequenceEqualImmutableArray\s*<" --type csLength of output: 43588
SequenceEqualImmutableArray is already defined elsewhere in the codebase
The extension methods in
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/CollectionExtensions.csare correctly returning aSequenceEqualImmutableArray<T>, which is defined insrc/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SequenceEqualImmutableArray.cs(line 12). No further changes are required.src/libs/CSharpToJsonSchema.Generators/Models/ClassData.cs (1)
5-25: LGTM - well-structured class with proper encapsulation.The
ClassDataclass is well-implemented with good property documentation and proper handling of the global namespace edge case.CSharpToJsonSchema.sln (1)
43-44: LGTM - Proper addition of new AOT test project.The new AOT test project is correctly added to the solution with appropriate configurations for both Debug and Release builds. It's also properly nested under the tests folder.
Also applies to: 75-78, 91-91
src/libs/CSharpToJsonSchema.Generators/JsonGen/System.Text.Json/JsonSourceGenerationAttribute.cs (1)
1-40: Looks great overall!The constructor logic for handling
JsonSerializerDefaults.WebvsJsonSerializerDefaults.Generalis concise and clear. There are no obvious performance, security, or concurrency issues in these lines. The default assignment of JSON options seems correct for enabling out‐of‐the‐box web-friendly serialization versus the general case.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/CSharpSyntaxUtilities.cs (2)
1-20: Well-structured utility for formatting C# literals!The approach of falling back to
defaultwhenvalueisnullis clear, and usage ofCultureInfo.InvariantCultureavoids locale issues. No immediate concerns around performance or concurrency.
27-32:❓ Verification inconclusive
Confirm valid pattern matching on special floating-point constants.
Using
case double.NegativeInfinity:andcase float.NegativeInfinity:might compile successfully in modern C#, but it could raise compatibility questions (e.g., older compilers expectwhen double.IsNegativeInfinity(d)patterns). Verify that your language version supports this usage without errors.Also applies to: 35-40
🏁 Script executed:
#!/bin/bash # Attempt to compile these lines to confirm that the 'case double.NegativeInfinity:' pattern is recognized by the current C# compiler fd -e cs --exec csc /langversion:latest {}Length of output: 4036
Action Required: Manually Verify C# Pattern Matching on Special Floating-Point Constants
The snippet below:
case double.NegativeInfinity: return "double.NegativeInfinity"; case double.PositiveInfinity: return "double.PositiveInfinity"; case double.NaN: return "double.NaN";is intended to rely on modern C# pattern matching using well-known special floating-point constants. The initial verification using the
csccommand was inconclusive because the environment did not have the C# compiler available (i.e., the commandcscwas not found). To confirm that the language version fully supports this pattern matching without errors, please manually verify the compilation of the project—ideally by running a dotnet build (or another equivalent compilation command) with the latest language version flag (e.g.,/p:LangVersion=latest) on the relevant project.This manual verification should also be applied to the similar code at lines 35–40.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.DateTime.g.verified.cs (3)
1-9: Consistent use of nullable directives and suppression flags.
All directives (#nullable enable/disable) and warning suppressions appear to be correctly placed for generated code scenarios.
14-23: Initialization of_DateTimeproperty.
The lazy initialization approach for_DateTimeis clear and concise. It properly leveragesOptions.GetTypeInfoto retrieve theJsonTypeInfo<DateTime>.
26-35: Runtime custom converter handling.
The flow for checking custom converters before creating the defaultDateTimemetadata is straightforward. Ensure thatTryGetTypeInfoForRuntimeCustomConverteris available and covered by appropriate tests to guarantee correct fallback behavior.src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/TypeGenerationSpec.cs (3)
1-29: Thorough summary and immutability remarks.
The documentation and guidelines (immutable record, structural equality, incremental caching) are well-stated and help future contributors maintain consistency.
30-86: Use of required properties.
Declaring each field with therequiredkeyword ensures all critical metadata is set during construction. This reduces the risk of partial or invalid state. No concerns here.
101-145: Logic inIsFastPathSupportedmethod.
The conditions for skipping fast-path serialization (e.g., polymorphism, special number handling, extension data property) seem comprehensive. However, consider verifying corner cases (like nested objects with numeric conversions) to ensure the method's checks remain valid.src/libs/CSharpToJsonSchema.Generators/JsonGen/Model/ContextGenerationSpec.cs (2)
1-28: Clear definition of caching constraints.
The remarks section clarifies how this type must remain immutable to satisfy the Roslyn incremental caching rules. Good job documenting the rationale.
30-39: Composition of the context specification.
The required properties (e.g.,ContextType,GeneratedTypes,ContextClassDeclarations) effectively capture the essential inputs for source generation. Consider verifying downstream usage to ensure all necessary fields are populated in the correct contexts.src/libs/CSharpToJsonSchema.Generators/CSharpToJsonSchema.Generators.csproj (1)
10-10:✅ Verification successful
Verify that unsafe code blocks are required for NativeAOT support.
Enabling
AllowUnsafeBlocksallows the use of unsafe code constructs like pointers, which can be beneficial for NativeAOT scenarios with performance-critical serialization operations. However, ensure this capability is actually needed by the implementation.
🏁 Script executed:
#!/bin/bash # Search for unsafe code blocks in the codebase to verify this setting is needed echo "Searching for 'unsafe' keyword usage in code files:" rg "unsafe\s+{|unsafe\s+static|unsafe\s+void|unsafe\s+class" --type csharpLength of output: 343
Please run the updated verification commands below, as the original regex produced an error due to an unescaped curly brace. These commands use a corrected regex pattern (and an additional simple search) to help confirm whether any unsafe code blocks are actually used in the codebase—which in turn will clarify if enabling
AllowUnsafeBlocksis truly required for NativeAOT support.Manual Verification of Unsafe Code Blocks
- The updated regex now properly escapes the curly brace.
- A simpler search for the word “unsafe” is also included as a fallback check.
Once you verify the results from these commands, please confirm whether unsafe blocks are in use. This will determine if
AllowUnsafeBlocksis necessary for your NativeAOT performance optimizations.
🏁 Script executed:
#!/bin/bash echo "Searching for 'unsafe' keyword usage in C# code with corrected regex:" rg "unsafe\\s+\\{|unsafe\\s+static|unsafe\\s+void|unsafe\\s+class" --type csharp echo "Searching for any usage of the keyword 'unsafe' with word boundary:" rg "\bunsafe\b" --type csharpLength of output: 855
Unsafe Code Verified for NativeAOT Support
After re-running the verification, we found that theunsafekeyword is used in the code (specifically in theAppendSpanmethod ofsrc/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SourceWriter.cs). This confirms that enabling<AllowUnsafeBlocks>true</AllowUnsafeBlocks>is necessary for your NativeAOT performance optimizations. No further action is needed on this setting.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Single.g.verified.cs (1)
1-37: LGTM - Well-structured serialization context for NativeAOT supportThe implementation correctly adds source-generated JSON serialization context for the
floattype, following the System.Text.Json patterns needed for NativeAOT/trimming scenarios. The lazy initialization pattern with null-coalescing operator is appropriately used here.src/libs/CSharpToJsonSchema.Generators/Conversion/SymbolGenerator.cs (1)
100-129: Good implementation of JSON converter attribute generationThe
GetConvertermethod is well-structured and correctly builds the attribute syntax for JSON string enum conversion, which will be useful for the NativeAOT/trimming support mentioned in the PR objectives.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.DateOnly.g.verified.cs (1)
1-37: LGTM - Properly implemented DateOnly serialization for NativeAOTThe implementation correctly adds source-generated JSON serialization context for the
DateOnlytype, which is an important part of ensuring NativeAOT and trimming compatibility.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Int32.g.verified.cs (1)
1-37: LGTM - Int32 serialization context looks goodThe implementation correctly adds source-generated JSON serialization context for the
inttype, consistent with the pattern used for other primitive types. This is essential for NativeAOT/trimming support.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetJsonTypeInfo.g.verified.cs (1)
1-74: Well-structured auto-generated serialization context class.This auto-generated class implements
IJsonTypeInfoResolverto provide type resolution for JSON serialization in a NativeAOT-friendly way. The implementation correctly handles various types by delegating to specific creation methods.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetJsonTypeInfo.g.verified.cs (1)
1-50: Clean implementation of JSON type resolution for weather-related types.The class properly implements both the override and interface methods for
GetTypeInfo, handling specific types likedouble,GetCurrentWeatherArgs, and others with appropriate type checks.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Double.g.verified.cs (2)
14-24: Good use of lazy initialization for type info.The property uses the null-coalescing operator to efficiently initialize the type info only when needed, which is a good performance optimization.
26-35: Proper fallback mechanism for type info resolution.The method correctly tries to get type info from a runtime custom converter first, and falls back to creating a new value info if that fails. This ensures compatibility with both custom converters and the default serialization system.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherAsyncArgs.g.verified.cs (1)
26-46: Well-structured object info configuration.The creation method properly sets up all necessary components for the serialization process, including object creator, property metadata initializer, and serialization handler.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.SetValueArgs.g.verified.cs (1)
87-87:✅ Verification successful
The
PropName_valueconstant is referenced but not definedThe line references a constant
PropName_valuewhich isn't defined in this file. This is likely a constant that should be defined in this class or a base class.
🏁 Script executed:
#!/bin/bash # Search for PropName_value definition in the codebase rg -A 1 -B 1 "PropName_value" --type csLength of output: 1712
Verified: The
PropName_valueconstant is defined and used as intendedUpon verification, the
PropName_valueconstant is indeed defined in the shared file:
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.PropertyNames.g.verified.cs
- The reference in
SetValueArgs.g.verified.csis valid since the constant is intended to be maintained in the common property names file.- No action is needed here unless the design decision to centralize these constants is reconsidered.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueArgs.g.verified.cs (1)
50-52:✅ Verification successful
Empty properties array is used for GetValueArgs
This type is configured with an empty properties array, which means it won't serialize any properties. Verify that this is the expected behavior for this type.
🏁 Script executed:
#!/bin/bash # Search for the definition of GetValueArgs to confirm if it has any properties rg -A 10 "class GetValueArgs" --type csLength of output: 1604
Verified: GetValueArgs is Designed with an Empty Properties Array
The snapshot for
GetValueArgscorrectly returns an empty array, confirming that no properties are defined for this type. Our search verified that the class is intentionally empty, so its serialization behavior (i.e., not serializing any properties) is as expected.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.SetValueAsyncArgs.g.verified.cs (1)
1-92: Auto-generated code for JSON serializationThis auto-generated file looks good. It adds a specialized JsonTypeInfo implementation for the SetValueAsyncArgs class to support NativeAOT and trimming scenarios mentioned in the PR objectives.
src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/HashCode.cs (1)
8-68: Well-structured hash combination methods.The overloaded
Combinemethods are clear and consistent. They properly account for null references by defaulting to zero for each object's hash code. These methods should work effectively for generating composite hash codes.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/DiagnosticInfo.cs (1)
21-55: Clear and concise factory methods.The
Createmethod overloads provide flexible ways of instantiatingDiagnosticInfoobjects with different parameters. The usage ofToValueEquals()for dictionaries ensures consistent value-based equality. No issues noted.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/SyntaxValueProviderExtensions_2.cs (2)
79-149: Efficient filtering of syntax trees and global aliases.The first
ForAttributeWithSimpleNamemethod effectively filters trees that contain global aliases or attributes. This reduces unnecessary processing and can help performance in large projects. The incremental approach is well-structured.
550-565: Ensure safe handling of potential null entries inImmutableArray<T>.When computing the hash code in
ImmutableArrayValueComparer<T>,EqualityComparer<T>.Default.GetHashCode(value!)may throw ifvalueis a null reference type. If nulls can appear, consider a null-safe approach:- hashCode = Hash.Combine(hashCode, EqualityComparer<T>.Default.GetHashCode(value!)); + hashCode = Hash.Combine(hashCode, value is null ? 0 : + EqualityComparer<T>.Default.GetHashCode(value));src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/RoslynExtension2.cs (2)
90-136: Confirm private accessibility logic aligns with your use case.
GetResultantVisibilitycan prematurely classify symbols asPrivateif a single private scope is detected in their chain. Ensure it matches your code generation requirements, especially if partial or nested definitions are involved.
149-155: No concerns regardingHasAttributeSuffix.The helper method is straightforward and meets typical attribute-naming conventions. Implementation is concise and correct.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.PropertyNames.g.verified.cs (1)
14-22: Auto-generated code appears valid.All fields are well-defined as read-only, matching the snapshot approach. No additional manual changes are recommended unless your snapshot or schema requirements change.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueAsyncArgs.g.verified.cs (2)
26-46: Confirm the absence of properties forGetValueAsyncArgs.
This generated method creates aJsonObjectInfoValueswithout any property metadata, suggesting thatGetValueAsyncArgsis expected to have no serializable fields. Verify whether this is correct, or if properties need to be added.
48-53: Empty property metadata might be a placeholder.
GetValueAsyncArgsPropInitreturns an empty array, meaning no properties will be serialized or deserialized. Confirm thatGetValueAsyncArgshas no relevant fields. Otherwise, consider populating the property metadata.src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/JsonHelpers.cs (2)
16-23: Potential thread-safety concern forTryAdd.
This polyfill logic matches the official .NET implementation in a single-threaded scenario, but it isn’t thread-safe if multiple threads access the dictionary. If concurrency is expected, consider using locks or thread-safe collections.
28-38: Queue concurrency caveat.
Similar toTryAdd,TryDequeueis correct under single-threaded or externally synchronized usage. For concurrent scenarios, this code is not thread-safe.src/tests/CSharpToJsonSchema.IntegrationTests/WeatherStrictTools.cs (2)
26-34: Method name changes in interface.
RenamingGetCurrentWeather→GetCurrentWeather2andGetCurrentWeatherAsync→GetCurrentWeatherAsync2is fine, but please confirm all referencing code has been updated, including calls and documentation.
39-59: Implementation rename and usage check.
The implementations inWeatherStrictServicematch the new method names. Ensure that any test code or external consumers now referenceGetCurrentWeather2andGetCurrentWeatherAsync2.src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.Roslyn4.0.cs (2)
19-23: Validate the custom attribute reference.Ensure that all references to
"CSharpToJsonSchema.GenerateJsonSchemaAttribute"throughout the solution align with the intended usage of this custom attribute. If any imports or references to the previous attribute name were overlooked, they will need to be updated accordingly.
129-138: Good use of partial methods for source emission.
AddSourceas a partial method is a standard approach to integrate with the Roslyn source generation pipeline while allowing flexible test instrumentation.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherArgs.g.verified.cs (3)
12-24: Auto-generated context class appears consistent.The partial class
WeatherToolsExtensionsJsonSerializerContextproperly holds type info fields. No immediate issues or improvements are needed, as this follows standard source-generated patterns.
48-67: Check property initialization logic for correctness.The
GetCurrentWeatherArgsPropInitmethod sets up property info arrays forLocationandUnit. This is a standard approach, but verify that additional properties—if any—are appropriately handled in future expansions.
95-116: Serialization handler logic is sound.Null checks, start-object, and end-object usage are carefully handled. The property name writing follows standard conventions for custom property names (
PropName_location, etc.). Looks correct.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/CSharpSyntaxHelper.cs (4)
9-16: Internal sealed class is appropriate.Using
internal sealedforCSharpSyntaxHelperprevents extension in external assemblies and helps keep implementation details contained.
35-56: Clear attribute target handling logic.Looping over field/event variables to append attribute targets is straightforward. This is consistent with C# attribute application rules, ensuring each declared variable is accounted for.
83-102: Efficient alias handling.Filtering for
using X = A.B.C;directives is well-structured. The decision to skip non-name-based imports (e.g., using static) is a valid simplification.
110-125: Global alias detection is correctly scoped.Limiting the search to compilation-level usings aligns with how C# global aliases are declared. The checks appear correct.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.g.verified.cs (7)
40-44: Sufficient Reflection Binding FlagsDefining a helper constant for reflection binding flags is a neat approach. It improves the readability of reflection-based operations throughout the code by centralizing the binding flags usage in a single, well-labeled constant.
45-48: Default Context InitializationProviding a static
Defaultproperty that reuses the already configureds_defaultOptionsfosters consistent serialization throughout the codebase. It’s a good practice that avoids potential configuration drift in various parts of the application.
53-53: Override of GeneratedSerializerOptionsOverriding the
GeneratedSerializerOptionsproperty ensures that any subsequent usage of this context will correctly leverage the specialized options (s_defaultOptions). This is a sound design choice for controlling serialization behavior in a uniform manner.
55-63: Constructors for FlexibilityProviding both a parameterless constructor and one accepting
JsonSerializerOptionsallows flexibility. It’s useful for scenarios where you either want to rely on a default global config or supply your own. Good approach!
65-76: TryGetTypeInfoForRuntimeCustomConverter ImplementationThis method effectively checks for an applicable custom converter using the runtime type before falling back to the normal flow. This is a common and sensible pattern, especially for dynamic scenarios where you might want to plug in specialized converters.
78-90: Runtime Converter LookupIterating through registered converters in
options.Convertersand returning the first match is standard, but be mindful of ordering if you have many converters that could potentially handle the same type. Otherwise, this is a fine approach.
92-109: ExpandConverter ValidationThe checks here (null, factory usage, etc.) effectively prevent converter misconfiguration. Throwing informative exceptions clarifies converter compatibility issues. This robust validation helps maintain correct and predictable runtime behavior.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetCurrentWeather3Args.g.verified.cs (4)
14-25: Lazy Initialization of _GetCurrentWeather3ArgsStoring the
JsonTypeInfoin a nullable field and initializing it lazily is a practical approach, ensuring you only pay for type info resolution when it's actually accessed. This pattern can save startup overhead and memory in large projects.
26-46: Create_GetCurrentWeather3Args MethodThe fallback to
CreateObjectInfowhen no custom converter is applicable is standard code generation logic. The approach properly integrates with yourTryGetTypeInfoForRuntimeCustomConvertermethod. This keeps the system flexible for dynamic or custom type handling.
48-193: Property Metadata InitializationThese property initializers correctly map the various fields (
Parameter1,Parameter2, etc.) to JSON properties with straightforward getter/setter logic. The usage of compile-time resolvers (e.g.,GetProperty("Parameter1", ...)) is typical in source-generated contexts and is well-structured for strong typing.
195-218: Customized Serialize HandlerDefining a dedicated method (
GetCurrentWeather3ArgsSerializeHandler) to write each property individually provides maximum control over the serialization. It's especially valuable for advanced scenarios, like injecting custom logic for certain fields. The fallback toJsonSerializer.Serializefor theDateproperty is a neat approach to keep logic consistent.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Calls.generated.verified.cs (6)
18-19: Introduction of Partial ClassMarking
WeatherToolsExtensionsas a partial class sets the stage for combining auto-generated code with hand-crafted extensions. This pattern helps you keep auto-generated and manually authored logic separate while still living under the same class name.
40-62: AsGetCurrentWeatherArgs Deserialization LogicThe fallback approach (
#if NET6_0_OR_GREATER) toggles between reflection-based and source-generated JSON handling. This dual-mode pattern is a clean solution to accommodate different .NET versions and reflection preferences.
73-105: AsGetCurrentWeatherAsyncArgs MirroringMirroring the same approach for the async version of
GetCurrentWeatherensures consistency. Both synchronous and asynchronous calls share the same deserialization logic, which is easier to maintain and less error-prone.
111-131: CallGetCurrentWeather with Conditional SerializationYou are correctly applying consistent logic for serializing the returned weather data. The check for
JsonSerializer.IsReflectionEnabledByDefaultensures you either use the reflection-based approach or the statically generated type info. This fosters a cohesive experience across different .NET runtimes.
141-165: CallGetCurrentWeatherAsync with Reflection-Conditional SerializationSame pattern as the synchronous variant. Ensuring an identical approach for the async call keeps your code symmetrical and eliminates confusion for maintainers. This is a good design for bridging reflection-based and source-generated serialization.
182-186: Partial WeatherToolsExtensionsJsonSerializerContextAn empty partial that extends
JsonSerializerContextis a suitable forward-looking arrangement. It allows future expansions or merges from the code generator, letting you inject or override logic in the same class name if needed.src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/HashHelpers.cs (1)
6-8: Well-designed utility class for hash table operations.This class provides essential utilities for hash table operations, with constants and methods optimized for performance.
src/libs/CSharpToJsonSchema.Generators/JsonGen/System.Text.Json/Flags.cs (3)
1-32: Ensure namespace reflects System.Text.Json.The namespace
CSharpToJsonSchema.Generators.JsonGen.System.Text.Jsonincludes "System.Text.Json" within it, which could cause confusion if this is used alongside the actual System.Text.Json namespace.Consider whether this namespace should be renamed to avoid potential ambiguity with the actual System.Text.Json namespace. This is especially important in AOT scenarios where qualified name resolution is critical.
7-32: JsonSourceGenerationMode enum design is appropriate.The enum provides clear options for controlling the generation behavior with good documentation.
149-184: Ensure consistency between JsonIgnoreCondition and JsonSerializerOptionsAttribute.Verify that the JsonIgnoreCondition enum values are correctly handled by the JsonSerializerOptionsAttribute in AOT scenarios.
The JsonIgnoreCondition enum's values (particularly WhenWritingDefault, WhenWritingNull, etc.) must be correctly processed by the source generator in AOT environments. Ensure there are tests to validate this functionality.
src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.DiagnosticDescriptors.cs (3)
1-3: Confirm appropriate licensing text.
The top-level license comments refer to the .NET Foundation license. Confirm that this matches the project’s overall licensing and that no conflicts exist with any other license in your repository.
15-62: Overall structure of static properties is clear and consistent.
EachDiagnosticDescriptorusage is appropriate, with distinct IDs, severities, and messages. No logical issues are apparent.
91-137: Naming and severity levels appear appropriate.
Your chosen IDs and default severities (Warnings vs. Errors) align well with the described issues. No immediate changes recommended.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Calls.generated.verified.cs (6)
6-15: Renaming toGetCurrentWeather3Argsis consistent with usage.
This rename clarifies that the method now handles a new or updated argument structure. The property definitions (Parameter1, Parameter2, etc.) look logically sound.
37-38: Validate the partial class approach.
DeclaringVariousTypesToolsExtensionsas partial is convenient for code generation, but ensure that your team is aware of all partial definitions to avoid collisions across the codebase.
46-48: Consistent naming of “GetCurrentWeather3” method.
Good job updating both the dictionary key and the invocation to match the new naming. This maintains consistency across the extension methods.
239-257: Return type for serialization transitions looks good.
The methodCallGetCurrentWeather3properly serializes the returned object to JSON with the same reflection vs. context approach. No correctness issues observed.
309-321: Ensure concurrency-safety if calling from multiple threads.
If the extension methods can be called concurrently, confirm thatJsonSerializerOptionsandJsonSerializerContextusage is safe. Typically,JsonSerializeritself is thread-safe, but check your context usage for any shared mutable state.
357-360: Generated partial class for serializer context.
AddingVariousTypesToolsExtensionsJsonSerializerContextis appropriate for AOT-friendly scenarios. Keep it minimal, as done here, to reduce overhead in non-reflection code paths.src/libs/CSharpToJsonSchema.Generators/JsonGen/JsonSourceGenerator.Emitter.cs (19)
1-14: Header & namespace references appear correct.
The use of .NET Foundation licensing text and the references to required namespaces are consistent with the rest of the generator’s code structure.
47-79: Global references for type names.
Usingglobal::System.*references is recommended for code generation to avoid name collisions. This practice is commendable, and everything looks consistent.
80-122: Validate dictionary usage and merging logic.
The_typeIndexdictionary is used to store and check existing specs. This is a potential concurrency hazard ifEmitis ever called in multi-threaded scenarios. Otherwise, it is fine.
123-138: Finalize cleanup to avoid residual state.
Setting_emitGetConverterForNullablePropertyMethod = falseand clearing_propertyNamesand_typeIndexis good to avoid state leaking between consecutiveEmitcalls.
198-232: Well-defined switch to handle class types.
The switch statement enumerating eachClassTypeis logically sound. Returningnullfor unsupported scenarios prevents accidental generation.
272-292: Nullable type support.
The approach usingGetNullableConvertercarefully wraps the underlying type. This is important for AOT scenarios. Good job handling the “Nullable” case distinctly.
326-416: Efficient collection generation with specialized code.
EachCreateXInfomethod is mapped in a switch expression. This is a clean approach.
417-469: Fast-path logic for enumerables.
Looping through arrays vs. other enumerables is well-handled. The local variable approach to handle each element once is also beneficial for performance.
471-498: Fast-path logic for dictionaries.
Similar to enumerables, the code for dictionaries properly iterates key-value pairs and writes them to JSON. This is a well-structured approach.
500-603: Handling parameterless vs. parameterized constructors.
Object creation strategies include parameterless or parameterized approaches. The partial references topropertyInitializersappear consistent, with fallback tonullif not relevant.
721-779: Ctor parameter metadata and property initializer synergy.
Combining constructor parameters with leftover property initializers is a robust approach for capturing all required data. Good alignment with the reflection logic.
781-919: Fast-path serialization for objects.
The code handlesIJsonOnSerializingandIJsonOnSerializedcalls, gating property checks, and gracefully writing null if needed. TheThrowPropertyNullExceptionhelper is well-located.
961-971: Primitive writer helpers.
Using specialized writer methods for numeric/string types is more efficient than a generic serialization approach. This is beneficial for many scenarios.
1002-1026: Fallback toJsonSerializer.Serializeif no specialized method.
Fallback is correct. If the code generation logic can’t fast-path it, it reverts to the built-in serializer. This is a sensible default.
1180-1291: Options initialization.
Allows applying user-defined default options, including policies and naming. If you add new configuration fields, remember to add them here.
[approve]
1321-1393: Helper methods for custom converters & nullables.
GetRuntimeConverterForTypeandExpandConverterMethodNameare refreshingly separate, which helps keep logic straightforward. The fallback toGetNullableConverteris well-handled.
1395-1433:IJsonTypeInfoResolvercompliance.
The explicit implementation ofGetTypeInfois thorough, enumerating known types. This is key to making reflection vs. codegen interplay seamless.
1437-1445: Static fields for property names.
GeneratingJsonEncodedTextonce and storing them inprivate static readonlyfields is memory-friendly and prevents re-encoding overhead. Nicely done.
1524-1532: Toggle-based generation modes.
ShouldGenerateMetadataandShouldGenerateSerializationLogicchecks keep the generator’s scope adjustable. This is a neat design, but frequently check it remains in sync with project requirements.src/libs/CSharpToJsonSchema.Generators/JsonGen/Base/DiagnosticExtensions.cs (1)
52-110: Validate location logic for multi-location diagnostics.
When selecting the first in-source location and marking the rest as additional, the approach is correct and aligns with Roslyn’s diagnostic model. However, ensure that returningLocation.Nonefor non-source locations meets your project’s requirements.src/libs/CSharpToJsonSchema.Generators/JsonGen/Helpers/RoslynExtensions.cs (2)
51-107: Check edge cases in compile-time metadata erasure.
TheEraseCompileTimeMetadatamethod handles various type forms and correctly aims to preserve only essential metadata. Consider adding unit tests for less typical scenarios like deeply nested tuples or partially unbound generics to ensure correctness.
109-119: Confirm default constructor detection logic.
The code correctly checks for a public parameterless constructor or value type. If types with internal constructors are relevant for your scenario, consider clarifying or expanding the logic to address them.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Boolean.g.verified.cs (1)
1-37: The implementation correctly adds Boolean type serialization support for NativeAOT/trimming.This auto-generated code adds proper JSON serialization support for boolean types, which is essential for NativeAOT scenarios where reflection-based serialization might not work due to trimming. The use of explicit JsonTypeInfo instances ensures the type metadata is preserved during ahead-of-time compilation.
I notice the
Create_Booleanmethod on line 26 is defined but not directly called from the property getter on line 23. This appears to be part of the design pattern where the JsonSerializerOptions.GetTypeInfo might internally use this method through a registration mechanism.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.PropertyNames.g.verified.cs (1)
1-19: Well-optimized property name encoding for JSON serialization.The code uses
JsonEncodedTextfor property names, which is a performance optimization that pre-encodes strings used in serialization. This reduces runtime overhead by avoiding repeated encoding of the same property names during serialization operations.This approach is particularly beneficial for NativeAOT scenarios where runtime performance is critical and reflection should be minimized.
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Int64.g.verified.cs (1)
1-37: The implementation correctly adds Int64 type serialization support for NativeAOT/trimming.This auto-generated code follows the same pattern as other type handlers, providing explicit JsonTypeInfo for the Int64 (long) type. The implementation correctly uses the built-in Int64Converter when no custom converter is found.
As with the Boolean implementation, the
Create_Int64method is defined but not directly called from the property getter, which appears to be part of the overall design pattern for these serialization context classes.src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Double.g.verified.cs (1)
1-37: The implementation correctly adds Double type serialization support for NativeAOT/trimming.The code for Double type serialization follows the same consistent pattern as the other type handlers, ensuring proper serialization support in NativeAOT scenarios.
This implementation contributes to the overall goal of the PR to enhance NativeAOT support by providing strongly-typed JsonTypeInfo objects rather than relying on reflection-based discovery that might be affected by trimming.
| new global::CSharpToJsonSchema.Tool | ||
| { | ||
| Name = "GetCurrentWeather", | ||
| Name = "GetCurrentWeather3", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that all references to GetCurrentWeather have been updated to GetCurrentWeather3.
The API name has been changed from "GetCurrentWeather" to "GetCurrentWeather3". This is a public-facing change that might break backward compatibility for consumers of this API.
🏁 Script executed:
#!/bin/bash
# Check if there are any remaining references to the old name that need to be updated
rg -p "GetCurrentWeather[^3]" --type csLength of output: 24545
Action Required: Inconsistent API Name Updates Detected
The snapshot in src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Tools.generated.verified.cs shows the API name updated to "GetCurrentWeather3", but multiple files (including snapshot tests under the Weather category and integration tests) still reference "GetCurrentWeather" (and its async variants). This discrepancy may introduce unexpected backward compatibility issues.
• Files to verify/update:
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Tools.generated.verified.cs(line 15 and line 42)src/tests/CSharpToJsonSchema.IntegrationTests/WeatherTools.cs(lines 26, 31, 39, 50)- Other related snapshots and generated files referencing the older API name
Please ensure that all public-facing APIs use the updated "GetCurrentWeather3" naming consistently—or explicitly document any intentional exceptions to avoid potential consumer breakage.
| //public partial class ComplexClassSerializerToolsJsonSerializerContext : JsonSerializerContext | ||
| //{ | ||
|
|
||
| //} No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment and implement the JsonSerializerContext.
The commented code for ComplexClassSerializerToolsJsonSerializerContext should be uncommented and implemented with proper JsonSerializable attributes to fully test the JsonSerializerContext generation capabilities.
-//public partial class ComplexClassSerializerToolsJsonSerializerContext : JsonSerializerContext
-//{
-
-//}
+[JsonSourceGenerationOptions]
+[JsonSerializable(typeof(ComplexClassSerializerTools))]
+public partial class ComplexClassSerializerToolsJsonSerializerContext : JsonSerializerContext
+{
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //public partial class ComplexClassSerializerToolsJsonSerializerContext : JsonSerializerContext | |
| //{ | |
| //} | |
| [JsonSourceGenerationOptions] | |
| [JsonSerializable(typeof(ComplexClassSerializerTools))] | |
| public partial class ComplexClassSerializerToolsJsonSerializerContext : JsonSerializerContext | |
| { | |
| } |
| public record struct ValueEqualityImmutableDictionary<T, U>(ImmutableDictionary<T, U> Map) : IDictionary<T, U> | ||
| { |
There was a problem hiding this comment.
Add a notnull constraint to type parameter T to match ImmutableDictionary requirements.
The static analysis is correctly flagging that ImmutableDictionary requires non-null keys, but your type parameter doesn't have this constraint.
-public record struct ValueEqualityImmutableDictionary<T, U>(ImmutableDictionary<T, U> Map) : IDictionary<T, U>
+public record struct ValueEqualityImmutableDictionary<T, U>(ImmutableDictionary<T, U> Map) : IDictionary<T, U> where T : notnull📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record struct ValueEqualityImmutableDictionary<T, U>(ImmutableDictionary<T, U> Map) : IDictionary<T, U> | |
| { | |
| public record struct ValueEqualityImmutableDictionary<T, U>(ImmutableDictionary<T, U> Map) : IDictionary<T, U> where T : notnull | |
| { |
🧰 Tools
🪛 GitHub Check: Test / Build, test and publish
[warning] 6-6:
The type 'T' cannot be used as type parameter 'TKey' in the generic type or method 'ImmutableDictionary<TKey, TValue>'. Nullability of type argument 'T' doesn't match 'notnull' constraint.
[warning] 6-6:
The type 'T' cannot be used as type parameter 'TKey' in the generic type or method 'ImmutableDictionary<TKey, TValue>'. Nullability of type argument 'T' doesn't match 'notnull' constraint.
| public override int GetHashCode() | ||
| { | ||
| if (_hashCode == 0) | ||
| { | ||
| var hashCode = 0; | ||
| foreach (var tuple in this.AliasAndSymbolNames) | ||
| hashCode = Hash.Combine(tuple.GetHashCode(), hashCode); | ||
|
|
||
| _hashCode = hashCode == 0 ? 1 : hashCode; | ||
| } | ||
|
|
||
| return _hashCode; | ||
| } |
There was a problem hiding this comment.
Missing Hash class dependency
The GetHashCode implementation references a Hash.Combine method that doesn't appear to be defined or imported in this file.
Add an appropriate using directive or define this helper method to ensure the code can compile correctly:
+using CSharpToJsonSchema.Generators.JsonGen.Base.Helpers; // Or the correct namespace where Hash is definedAlternatively, if the Hash class is missing, you could implement the combining logic directly:
- hashCode = Hash.Combine(tuple.GetHashCode(), hashCode);
+ hashCode = hashCode * 31 + tuple.GetHashCode();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public override int GetHashCode() | |
| { | |
| if (_hashCode == 0) | |
| { | |
| var hashCode = 0; | |
| foreach (var tuple in this.AliasAndSymbolNames) | |
| hashCode = Hash.Combine(tuple.GetHashCode(), hashCode); | |
| _hashCode = hashCode == 0 ? 1 : hashCode; | |
| } | |
| return _hashCode; | |
| } | |
| public override int GetHashCode() | |
| { | |
| if (_hashCode == 0) | |
| { | |
| var hashCode = 0; | |
| foreach (var tuple in this.AliasAndSymbolNames) | |
| hashCode = hashCode * 31 + tuple.GetHashCode(); | |
| _hashCode = hashCode == 0 ? 1 : hashCode; | |
| } | |
| return _hashCode; | |
| } |
| #if NET6_0_OR_GREATER | ||
| if(global::System.Text.Json.JsonSerializer.IsReflectionEnabledByDefault) | ||
| {{ | ||
| PropertyNamingPolicy = global::System.Text.Json.JsonNamingPolicy.CamelCase, | ||
| Converters = {{ new global::System.Text.Json.Serialization.JsonStringEnumConverter(global::System.Text.Json.JsonNamingPolicy.CamelCase) }}, | ||
| }}); | ||
| return global::System.Text.Json.JsonSerializer.Serialize(jsonResult, new global::System.Text.Json.JsonSerializerOptions | ||
| {{ | ||
| PropertyNamingPolicy = global::System.Text.Json.JsonNamingPolicy.CamelCase, | ||
| Converters = {{ new global::System.Text.Json.Serialization.JsonStringEnumConverter(global::System.Text.Json.JsonNamingPolicy.CamelCase) }}, | ||
| }}); | ||
| }} | ||
| else | ||
| {{ | ||
| return global::System.Text.Json.JsonSerializer.Serialize(jsonResult, global::{@interface.Namespace}.{extensionsClassName}JsonSerializerContext.Default.GetTypeInfo(jsonResult.GetType())); | ||
| }} | ||
| #else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reflection fallback looks correct but still poses the same .NET 6 compatibility question.
Both serialization blocks rely on IsReflectionEnabledByDefault. Ensure these code paths are guarded if you must support .NET 6. Otherwise, clarify in documentation that .NET 7 (or higher) is required.
Also applies to: 146-159
| }); | ||
| } | ||
|
|
||
| public async Task<ComplexClassSerializerTools> GetComplexDataType(Weather weatherName, Unit unit = Unit.Celsius, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Parameter name mismatch between interface and implementation
The interface method GetComplexDataType defines the parameter as weatherToTest, but the implementation uses weatherName. This mismatch could lead to confusion for developers.
-public async Task<ComplexClassSerializerTools> GetComplexDataType(Weather weatherName, Unit unit = Unit.Celsius, CancellationToken cancellationToken = default)
+public async Task<ComplexClassSerializerTools> GetComplexDataType(Weather weatherToTest, Unit unit = Unit.Celsius, CancellationToken cancellationToken = default)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task<ComplexClassSerializerTools> GetComplexDataType(Weather weatherName, Unit unit = Unit.Celsius, CancellationToken cancellationToken = default) | |
| public async Task<ComplexClassSerializerTools> GetComplexDataType(Weather weatherToTest, Unit unit = Unit.Celsius, CancellationToken cancellationToken = default) |
|
|
||
| public const string PropertyGetterDisallowNull = | ||
| "The property or field '{0}' on type '{1}' doesn't allow getting null values. Consider updating its nullability annotation."; | ||
| }; |
There was a problem hiding this comment.
Syntax error: Unexpected semicolon
There's a semicolon after the closing curly brace of the ExceptionMessages class, which is a syntax error in C#.
- };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }; | |
| } |
| public static int SequentialValuesHash<T>(IEnumerable<T> values) | ||
| { | ||
| int hash = 0; | ||
| foreach (var value in values) | ||
| { | ||
| hash = Hash.Combine(hash, value.GetHashCode()); | ||
| } | ||
| return hash; | ||
| } |
There was a problem hiding this comment.
Avoid potential NullReferenceException.
In SequentialValuesHash<T>, calling value.GetHashCode() can throw an exception if value is null. Consider treating null as zero or handling null values explicitly:
- hash = Hash.Combine(hash, value.GetHashCode());
+ hash = Hash.Combine(hash, value?.GetHashCode() ?? 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static int SequentialValuesHash<T>(IEnumerable<T> values) | |
| { | |
| int hash = 0; | |
| foreach (var value in values) | |
| { | |
| hash = Hash.Combine(hash, value.GetHashCode()); | |
| } | |
| return hash; | |
| } | |
| public static int SequentialValuesHash<T>(IEnumerable<T> values) | |
| { | |
| int hash = 0; | |
| foreach (var value in values) | |
| { | |
| hash = Hash.Combine(hash, value?.GetHashCode() ?? 0); | |
| } | |
| return hash; | |
| } |
3 participants
Added JsonSerializerContext Generation for NativeAOT support. I just used the codes from System.Text.Json Code generation.
Summary by CodeRabbit
New Features
Refactor
Tests