Conversation
📝 WalkthroughWalkthroughAdds centralized MSBuild metadata, registers four new devtool projects in the solution, introduces a comprehensive C# code-generation DSL and source-generator scaffolding, updates many project PackageId/Description properties, and adds/refactors reflection and utility wrappers plus several behavioral tweaks (math, subprocess handling, file-save logging). Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Factory as FileCodeBlockFactory (CodeFactory.File)
participant FileBlock as FileCodeBlock
participant Generator as GeneratorBase<T>
participant Roslyn as IncrementalContext
Dev->>Factory: Create(usings, namespace, class, builder)
Factory->>FileBlock: Create ClassCodeBlock + NamespaceCodeBlock
Dev->>FileBlock: builder adds fields/properties/methods
FileBlock->>FileBlock: Compose code via CodeBlockBase.GetCode(...)
Dev->>Generator: Register generator
Generator->>Roslyn: GetValueProvider(...) & GetProjectMetadata
Roslyn->>Generator: Provide types / metadata
Generator->>Dev: GenerateFiles(generation) -> file contents
Generator->>Roslyn: AddSource(fileName.g.cs, SourceText)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Communication/Shintio.Communication.SubProcess.Server/Utils/ClientRunner.cs (1)
56-74: Runtime crash:BeginRequest()andSend()will fail withNullReferenceException.The public methods
BeginRequest()andSend()depend onProcessbeing non-null. Given thatProcessis never assigned inStartClient(string path)(due to commented-out code), these methods will crash at runtime when clients attempt to send requests or receive responses.This method pair is unusable in its current state. Fix the process initialization (line 43–51) or refactor these methods to handle a null
Processgracefully.
🧹 Nitpick comments (29)
src/Misc/Shintio.ReflectionBomb/Types/ProcessWrapper.cs (1)
20-21: Remove commented-out code instead of leaving it in place.The commented blocks should be deleted entirely rather than left as comments. Version control maintains the history if you need to reference the old implementation later.
Note: The commented
SetStartInfomethod (lines 44-47) also contains a bug—it incorrectly passesthisinstead of_processtoSetValue.Apply this diff to remove the commented code:
- // private static readonly MethodInfo StaticStartMethod = - // ProcessType.GetMethod("Start", new Type[] { typeof(ProcessStartInfo) })!; -- // public void SetStartInfo(ProcessStartInfo startInfo) - // { - // StartInfoProperty.SetValue(this, startInfo); - // } -- // public static ProcessWrapper Start(ProcessStartInfo startInfo) - // { - // return new ProcessWrapper(StaticStartMethod.Invoke(null, new object[] { startInfo })); - // }Also applies to: 44-47, 64-67
src/Misc/Shintio.Math/Common/Vector3.cs (1)
338-341: Consider optimizing withDistanceSquaredto avoid square root calculation.The implementation is correct, but you can improve performance by using squared distance comparison, which avoids the expensive square root operation in the
Distancemethod.Apply this diff to optimize the performance:
public static bool IsPointWithinSphere(Vector3 point, Vector3 center, float radius) { - return Distance(point, center) <= radius; + return DistanceSquared(point, center) <= radius * radius; }src/Misc/Shintio.ReflectionBomb/Types/MemoryStreamWrapper.cs (1)
8-13: Avoid hard‑codingSystem.Private.CoreLibfor non‑NETCOREAPP targetsThe
#elsebranch assumes the presence of an assembly with"System.Private.CoreLib"in its full name. That’s true for .NET Core / .NET 5+, but not for all possible runtimes where this code might compile (e.g., full .NET Framework, whereMemoryStreamlives inmscorlib). In such casesGetAssembly("System.Private.CoreLib")would returnnulland the null‑forgiving!would mask this until a runtimeNullReferenceException.Since
TypesHelper.GetType(TypesHelper.TypeFromSystem, "System", "IO", "MemoryStream")already resolves the type from the assembly containingList<>without relying on assembly names, you could simplify and make this more robust by using the same mechanism for all TFMs and dropping the#ifentirely:- public static readonly Type MemoryStreamType = -#if NETCOREAPP3_0_OR_GREATER - TypesHelper.GetType(TypesHelper.TypeFromSystem, "System", "IO", "MemoryStream")!; -#else - AppDomainWrapper.GetAssembly("System." + "Private." + "CoreLib")!.GetNativeType("System." + "IO" + ".MemoryStream")!; -#endif + public static readonly Type MemoryStreamType = + TypesHelper.GetType(TypesHelper.TypeFromSystem, "System", "IO", "MemoryStream")!;This keeps the “no static reference to
MemoryStream” property while avoiding runtime coupling to a specific BCL assembly name. Please double‑check against the exact target frameworks for this project to ensure there’s no TFM whereTypesHelper.TypeFromSystem’s assembly doesn’t exposeSystem.IO.MemoryStream.src/Misc/Shintio.Web/Shintio.Web.csproj (1)
9-12: Add more descriptive package metadata.The Description value "Web" is minimal and provides limited context for users discovering this package on NuGet. Consider expanding it to better reflect the package's purpose.
src/Database/Shintio.Database/Shintio.Database.csproj (1)
9-12: Consider expanding the package description.Similar to other projects in this PR, the Description "Database" is generic. A more detailed description (e.g., "Database layer and ORM integration" or similar) would better inform users of this package's scope.
src/Json/Shintio.Json.System/Shintio.Json.System.csproj (1)
10-13: Metadata addition looks good; document description guidance.The PackageId and Description metadata are consistent with project conventions. As part of this metadata standardization effort, consider documenting a description style guide to ensure consistency across all projects (e.g., target audience, key capabilities).
src/DevTools/Shintio.CodeGenerator/GeneratorRunner.cs (1)
40-74: Logging saved files is useful; consider making output deterministicThe new
savedFilestracking and per-file logging is a nice usability improvement. To make logs easier to scan and diff, you might want to sort the list before printing:- stopwatch.Stop(); - Console.WriteLine($"Saved {savedFiles.Count} files in {stopwatch.Elapsed}:"); - foreach (var file in savedFiles) + stopwatch.Stop(); + savedFiles.Sort(StringComparer.OrdinalIgnoreCase); + Console.WriteLine($"Saved {savedFiles.Count} files in {stopwatch.Elapsed}:"); + foreach (var file in savedFiles) { Console.WriteLine($" - {file}"); }Purely optional; current implementation is functionally fine.
src/DevTools/Shintio.SourceGenerators/Extensions/SourceProductionContextExtensions.cs (1)
18-25: Refine DiagnosticDescriptor construction.Using the generator name as both the diagnostic ID and category, and the message as both title and messageFormat limits diagnostic filtering capabilities. Consider using a more structured approach with distinct IDs per severity level and a brief title.
Apply this diff to improve the diagnostic descriptor:
var descriptor = new DiagnosticDescriptor( - generator, - message, + $"{generator}_{level}", + $"{generator} {level}", message, generator, level, isEnabledByDefault: true );src/DevTools/Shintio.SourceGenerators/Extensions/IncrementalGeneratorInitializationContextExtensions.cs (2)
47-61: Consider handling missing project directory more explicitly.Returning an empty string when the project directory cannot be determined may cause subtle bugs in path operations. Consider documenting this behavior or throwing an exception if the directory is required.
Optional: Add validation or documentation:
return context.AnalyzerConfigOptionsProvider.Select((options, _) => { var projectDirectory = string.Empty; foreach (var key in ProjectDirectoryKeys) { if (options.GlobalOptions.TryGetValue(key, out var value)) { projectDirectory = value; break; } } - + // Note: Returns empty string if project directory cannot be determined return projectDirectory; });
68-92: Similar concern: empty string returns for metadata.Like
GetProjectDirectoryProvider, this method returns empty strings when metadata cannot be determined, which may cause issues downstream. Consider documenting this behavior or validating in consuming code.src/DevTools/Shintio.CodeBuilder.CSharp/CSharpBuilder.cs (1)
1-1: File contains only a namespace declaration with no types.This file appears to be a stub or placeholder. A file with only a namespace declaration serves no functional purpose in C#.
Would you like me to help implement this file or should it be removed from the PR?
src/DevTools/Shintio.CodeBuilder.CSharp/Utils/Formatter.cs (1)
3-6: Empty utility class serves no current purpose.The
Formatterclass is currently empty and provides no functionality. Consider either implementing the formatting utilities or deferring this file until the implementation is ready.Would you like me to help draft a formatting utility implementation or open an issue to track this placeholder?
src/DevTools/Shintio.CodeBuilder.CSharp/Extensions/CodeBlockBaseExtensions.cs (1)
8-20: Properties are unconditionally set, potentially overwriting existing values.The
PostProcessingmethod setsPrefix,Postfix, andPostProcessoreven when the parameters arenull. This means callingcodeBlock.PostProcessing()without arguments will reset all three properties tonull, potentially losing previously configured values.Consider either:
- Only setting properties when the corresponding parameter is non-null
- Documenting that this method completely replaces any existing post-processing configuration
Example of conditional assignment:
public static T PostProcessing<T>( this T codeBlock, string? prefix = null, string? postfix = null, Func<string, string>? postProcessor = null ) where T : CodeBlockBase { - codeBlock.Prefix = prefix; - codeBlock.Postfix = postfix; - codeBlock.PostProcessor = postProcessor; + if (prefix is not null) codeBlock.Prefix = prefix; + if (postfix is not null) codeBlock.Postfix = postfix; + if (postProcessor is not null) codeBlock.PostProcessor = postProcessor; return codeBlock; }src/DevTools/Shintio.CodeBuilder.CSharp/CodeBlocks/AutoPropertyGetterCodeBlock.cs (1)
5-10: Simple auto-property getter implementation is correctReturning
"get;"is sufficient for representing a basic auto-property getter and integrates cleanly with the existingCodeBlockBasepipeline. If you ever need customized getter modifiers (e.g.,private get;), mirroring theAutoPropertySetterCodeBlockpattern with aModifierproperty would be a natural extension, but it isn’t required for the current use.src/DevTools/Shintio.CodeBuilder.CSharp/CodeBlocks/CommentCodeBlock.cs (1)
8-22: Comment rendering is correct; consider guarding against null inputThe constructor’s split/join logic correctly turns each input line into a
//-prefixed comment and plays nicely withCodeBlockBase’s indentation logic.One small robustness improvement: if
textcan ever benull,text.Split(...)will throw. If you’d prefer this to be tolerant rather than throwing, you could normalize to an empty string:- public CommentCodeBlock(string text) + public CommentCodeBlock(string text) { - Value = string.Join( + Value = string.Join( Environment.NewLine, - text.Split([Environment.NewLine], StringSplitOptions.None) + (text ?? string.Empty).Split([Environment.NewLine], StringSplitOptions.None) .Select(l => $"// {l}") ); }If
nullis never expected and you’d rather fail fast, this can be left as-is.src/DevTools/Shintio.CodeBuilder.CSharp/Class1.cs (1)
13-19: Clean up unused members in the demo
- The instance field
public string? test;(line 13) is never used.constructorParameters(line 17) is created but not consumed.If this class is kept purely as a DSL demo, consider either removing these or wiring them into the example to avoid dead code and make the sample easier to follow.
src/DevTools/Shintio.CodeBuilder.CSharp/CodeBlocks/FileCodeBlock.cs (1)
3-18: File/namespace wiring looks good; consider immutability for consistency
FileCodeBlocknicely encapsulates aNamespaceCodeBlock+ClassCodeBlockand automatically registers the class intoNamespace.Classesin the constructor. That’s a solid composition.Because
ClassandNamespaceare publicly settable, later reassignments won’t updateNamespace.Classes, andGetCodeInternal()always renders the originalNamespaceinstance. If you don’t need those properties to be mutable, consider making theminit-only or get-only to avoid inconsistent states.src/DevTools/Shintio.CodeBuilder.CSharp/Factories/FieldCodeBlockFactory.cs (1)
15-23: Reflection-based factory ignoresstatic/readonlysemanticsThe
Create(FieldInfo)andCreate(PropertyInfo)overloads correctly derive access and type, but they leaveIsStaticandIsReadonlyat their defaultfalsevalues onFieldCodeBlock. SinceFieldInfoexposesIsStaticandIsInitOnlyproperties, and these flags are settable on the code block, consider:
- Set
IsStaticwhenfield.IsStatic.- Set
IsReadonlywhenfield.IsInitOnly.This would make generated fields more faithful to the source metadata.
src/DevTools/Shintio.CodeBuilder.CSharp/CodeBlocks/CodeBlockBase.cs (1)
7-59: Collection expression syntax is fine;skipFirstLinecleanup remains optionalThe project explicitly sets
LangVersiontolatestin the .csproj file, which fully supports C# 12 collection expressions. The[Environment.NewLine]syntax at line 38 is valid and requires no changes.The
skipFirstLineparameter remains unused—the only call toAddIndents(line 18) passes only two arguments. Removing this dead parameter would simplify the method signature if you want to clean it up, but it's optional.src/DevTools/Shintio.CodeBuilder.CSharp/Extensions/ParameterCodeBlockExtensions.cs (1)
7-22: Extensions look good; only minor naming/style nitThe helpers are straightforward and correctly rely on
ParameterCodeBlock.GetCode(0)/.Name. If you want to reduce potential confusion with LINQ’s ownJoinoperator and better convey intent, you could optionally renameJointo something likeToParameterListorToSignaturein a future cleanup, but the current shape is perfectly workable.src/DevTools/Shintio.CodeBuilder.CSharp/CodeBlocks/AutoPropertySetterCodeBlock.cs (1)
5-19: Auto-property setter implementation is correct; consider normalizing modifier inputThe generated
" set;"/" private set;"shape looks right and matches the rest of the code-gen API. If you want to avoid ever storingnullinModifier, you could coalesce the argument:public string GetCode(int indent, string modifier) { - Modifier = modifier; + Modifier = modifier ?? string.Empty; return GetCode(indent); }Not required for correctness since
string.IsNullOrEmpty(Modifier)already guards it, but it keeps the internal state a bit cleaner.src/DevTools/Shintio.CodeBuilder.CSharp/CodeBlocks/BackingPropertySetterCodeBlock.cs (1)
5-37: Backing setter generation is correct; consider minor modifier normalizationThe backing setter block correctly produces:
- An optional blank line (likely to separate from the getter),
- A
setline optionally prefixed with the modifier,- A body assigning
FieldName = value;.If you want to mirror the slightly safer pattern suggested for
AutoPropertySetterCodeBlock, you could normalize the modifier input:public string GetCode(int indent, string modifier) { - Modifier = modifier; + Modifier = modifier ?? string.Empty; return GetCode(indent); }That’s purely a small robustness/style tweak; the current implementation is functionally fine.
src/DevTools/Shintio.CodeBuilder.CSharp/Utils/CodeFactory.cs (1)
5-12: Prefer read-only properties (orreadonlyfields) for shared factory instancesExposing these as mutable
public staticfields makes it easy for consumers to accidentally reassign the factories. Given they’re effectively singletons, you can make the API safer without changing call sites by switching to get-only properties:-public static class CodeFactory -{ - public static FileCodeBlockFactory File = new(); - public static ClassCodeBlockFactory Class = new(); - public static FieldCodeBlockFactory Field = new(); - public static PropertyCodeBlockFactory Property = new(); - public static ConstructorCodeBlockFactory Constructor = new(); - public static MethodCodeBlockFactory Method = new(); -} +public static class CodeFactory +{ + public static FileCodeBlockFactory File { get; } = new(); + public static ClassCodeBlockFactory Class { get; } = new(); + public static FieldCodeBlockFactory Field { get; } = new(); + public static PropertyCodeBlockFactory Property { get; } = new(); + public static ConstructorCodeBlockFactory Constructor { get; } = new(); + public static MethodCodeBlockFactory Method { get; } = new(); +}This keeps usage like
CodeFactory.File.Create(...)unchanged while preventing reassignment.src/DevTools/Shintio.CodeBuilder.CSharp/CodeBlocks/ParameterCodeBlock.cs (1)
29-34: Remove or activate commented-out constructor.The commented constructor for accepting
ICodeBlockas a default value is unused. Since theDefaultValueproperty is already typed asICodeBlock?, callers can set custom default values after construction.If this constructor is not needed, remove it:
- // public ParameterCodeBlock(TypeInfo type, string name, ICodeBlock defaultValue) - // { - // Type = type; - // Name = name; - // DefaultValue = defaultValue; - // }If you anticipate needing it for constructor-based initialization, uncomment and document when to use it versus the object-initializer approach or the literal-value constructor.
src/DevTools/Shintio.CodeBuilder.CSharp/Factories/PropertyCodeBlockFactory.cs (1)
13-40: Be explicit thatCreate(FieldInfo/PropertyInfo)produce metadata-only properties, and simplifyCreateAuto(FieldInfo)Lines 25–40:
Create(FieldInfo)andCreate(PropertyInfo)only populate the property metadata (access modifier, type, name).GetterandSetterremainnull, so if callers don’t explicitly assign accessors afterwards, the generated property will compile to:public int SomeProperty { }which is invalid C#. That’s fine if these overloads are meant as “raw metadata” factories, but it’s easy to misuse compared to the
CreateAuto(...)/CreateBacking(...)helpers. Consider either:
- Documenting these as “metadata-only” entry points, or
- Redirecting most usages to
CreateAuto(...)orCreateBacking(...)if you don’t actually need custom getter/setter blocks.Lines 69–75:
CreateAuto(FieldInfo field)recomputes and reassigns the same access modifier:return new AutoPropertyCodeBlock(AccessModifier.From(field), field, field.Name, true) { AccessModifier = AccessModifier.From(field), };Since
AutoPropertyCodeBlock(viaPropertyCodeBlock) already storesaccessModifierfrom the constructor, the initializer is redundant andAccessModifier.From(field)is called twice. You can simplify to:public AutoPropertyCodeBlock CreateAuto(FieldInfo field) { - return new AutoPropertyCodeBlock(AccessModifier.From(field), field, field.Name, true) - { - AccessModifier = AccessModifier.From(field), - }; + var access = AccessModifier.From(field); + return new AutoPropertyCodeBlock(access, field, field.Name, true); }Also applies to: 69-88
src/DevTools/Shintio.CodeBuilder.CSharp/Components/TypeInfo.cs (1)
72-77: Clarify or implement theIsPrimitive/IsString/IsBool/IsFloat/IsEnumflagsThese boolean properties are defined but never set anywhere (including
FromTypeand the string-based constructor), so they will always befalseand may mislead callers.Either wire them up (e.g., in
FromTypeand/orTypeInfo(string fullName)based on known types), or remove them until they’re needed to avoid dead-but-public API surface.src/DevTools/Shintio.CodeBuilder.CSharp/Extensions/ClassCodeBlockExtensions.cs (1)
61-77: MakeAddBackingPropertyWithFieldmore defensive for generated field names
fieldNameis synthesized as:var fieldName = $"_{name[0].ToString().ToLowerInvariant()}{name.Substring(1)}";This will throw if
nameis ever empty. While callers should not pass invalid identifiers, this is a library API used in code generation and can be hardened cheaply.You could guard and fall back to a simple
_valuename, or at least assert:- var fieldName = $"_{name[0].ToString().ToLowerInvariant()}{name.Substring(1)}"; + if (string.IsNullOrWhiteSpace(name)) + { + throw new ArgumentException("Property name must be non-empty", nameof(name)); + } + + var fieldName = $"_{char.ToLowerInvariant(name[0])}{name.Substring(1)}";(Using
char.ToLowerInvariantalso avoids the temporary string allocation.)src/Misc/Shintio.ReflectionBomb/Types/AssemblyLoadContextWrapper.cs (2)
11-19: Be more defensive when resolvingAssemblyLoadContextTypeandLoadFromStream.
TypesHelper.GetType(TypesHelper.TypeFromSystem, "System", "Runtime", "Loader", "AssemblyLoadContext")!assumesAssemblyLoadContextis in the same assembly asList<>, and theLoadFromStreamlookup assumes a single 1‑parameter overload. If either assumption fails on a target runtime, the type initializer will blow up with aNullReferenceExceptioninstead of a clear error.Consider making the resolution explicit and failing with a descriptive exception, e.g.:
- public static readonly Type AssemblyLoadContextType = - TypesHelper.GetType(TypesHelper.TypeFromSystem, "System", "Runtime", "Loader", "AssemblyLoadContext")!; + public static readonly Type AssemblyLoadContextType = + Type.GetType("System.Runtime.Loader.AssemblyLoadContext", throwOnError: true)!;and similarly check
LoadFromStreamMethodfor null and throwNotSupportedExceptionif not found, so misconfigurations are easier to diagnose.
48-75: TightenSubscribeToAssembly’s contract and consider an unsubscribe path.Two concerns here:
handleris allowed to be null and to return anyobject, but the generated delegate blindly casts the result toAssembly. Ifhandleris null or returns something non‑castable, you get less obvious runtime exceptions from the expression machinery instead of a clear argument/contract failure.- The subscription is permanent for the lifetime of the
AssemblyLoadContext; there’s no way to detach, so handlers (and any captured state) can leak.You could improve robustness with minimal changes:
- public void SubscribeToAssembly(Func<object, AssemblyName, object> handler) - { + public void SubscribeToAssembly(Func<object, AssemblyName, object> handler) + { + if (handler == null) throw new ArgumentNullException(nameof(handler)); @@ - var cast = Expression.Convert(call, asmType); + var cast = Expression.Convert(call, asmType); // assumes handler returns Assembly or null @@ - eventInfo.AddEventHandler(AssemblyLoadContext, compiled); + eventInfo.AddEventHandler(AssemblyLoadContext, compiled);And, if you want to go further, consider returning an
IDisposablethat callseventInfo.RemoveEventHandler(AssemblyLoadContext, compiled)so callers can unsubscribe explicitly.
| <!-- <ItemGroup>--> | ||
| <!-- <PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="$(MicrosoftCodeAnalysisNetAnalyzersPackageVersion)" PrivateAssets="All" />--> | ||
| <!-- <PackageReference Include="Microsoft.SourceLink.GitHub" Version="$(MicrosoftSourceLinkGitHubPackageVersion)" PrivateAssets="All" />--> | ||
| <!-- </ItemGroup>--> |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify why analyzer and source-link references are disabled.
The Microsoft.CodeAnalysis.NetAnalyzers and Microsoft.SourceLink.GitHub PackageReferences are commented out without explanation. Removing these can degrade code quality checks and source traceability.
If this change is intentional, add an inline comment explaining the rationale (e.g., build performance, compatibility issue, etc.). If temporary, consider restoring them or opening a tracking issue.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the full context of the .csproj file around lines 52-55
cat libs/Newtonsoft.Json/Newtonsoft.Json.csproj | head -60 | tail -20
Length of output: 2728
🏁 Script executed:
#!/bin/bash
# Check git history for this specific file to understand when/why changes were made
git log --oneline -n 20 -- libs/Newtonsoft.Json/Newtonsoft.Json.csprojLength of output: 178
🏁 Script executed:
#!/bin/bash
# Search for any inline comments or patterns related to analyzers being disabled
rg -n "NetAnalyzers|SourceLink" libs/Newtonsoft.Json/Newtonsoft.Json.csproj -C 3Length of output: 626
🏁 Script executed:
#!/bin/bash
# Check if similar PackageReferences exist in other .csproj files
fd -e csproj -x rg -l "Microsoft.CodeAnalysis.NetAnalyzers|Microsoft.SourceLink.GitHub" {}Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check the entire Newtonsoft.Json.csproj file for structure and any other commented sections
cat libs/Newtonsoft.Json/Newtonsoft.Json.csprojLength of output: 13738
🏁 Script executed:
#!/bin/bash
# Look for any TODO or FIXME comments near the disabled references
rg -n "TODO|FIXME|XXX|HACK" libs/Newtonsoft.Json/Newtonsoft.Json.csproj -C 5Length of output: 0
Add explanation for disabling analyzer and source-link package references.
These references are commented out without documentation. The recent commit "Fix newtonsoft for dotnet 10" suggests this is intentional, likely due to compatibility issues. However, the reason should be documented inline.
Either uncomment and resolve any .NET 10 compatibility issues, or add an inline comment explaining why they're disabled (e.g., <!-- Disabled due to incompatibility with .NET 10; see issue #XXX -->).
🤖 Prompt for AI Agents
In libs/Newtonsoft.Json/Newtonsoft.Json.csproj around lines 52 to 55, the
PackageReference entries for Microsoft.CodeAnalysis.NetAnalyzers and
Microsoft.SourceLink.GitHub are commented out with no explanation; add an inline
XML comment explaining why they are disabled (e.g., compatibility issue with
.NET 10 and reference to the tracking issue or PR number) or alternatively
re-enable them by uncommenting and fixing any .NET 10 compatibility problems;
ensure the comment includes the reason and a reference (issue/PR) so future
maintainers know whether to restore them.
| public void StartClient(string path) | ||
| { | ||
| Process = ProcessWrapper.Start(new ProcessStartInfo | ||
| { | ||
| FileName = path, | ||
| ArgumentList = { Name }, | ||
| UseShellExecute = false, | ||
| CreateNoWindow = true, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardInput = true, | ||
| }); | ||
| // Process = ProcessWrapper.Start(new ProcessStartInfo | ||
| // { | ||
| // FileName = path, | ||
| // ArgumentList = { Name }, | ||
| // UseShellExecute = false, | ||
| // CreateNoWindow = true, | ||
| // RedirectStandardOutput = true, | ||
| // RedirectStandardInput = true, | ||
| // }); | ||
|
|
||
| InitWorkers(); | ||
| } |
There was a problem hiding this comment.
Critical: Process remains null after StartClient(string path) exits.
Lines 43–51 are commented out, so ProcessWrapper.Start() is never called and the Process property is never assigned. This causes:
InitWorkers()(called on line 53) immediately returns because of the null check at lines 78–81.- No background message-reading or ping tasks are started.
- Subsequent calls to
BeginRequest()(lines 56–59) andSend()(lines 70–74) will attempt to dereference a nullProcessand throwNullReferenceExceptionat runtime.
This appears to be incomplete or debug code left in place. Either:
- Uncomment lines 43–51 to restore process creation, or
- Remove/rework this method to clarify the intended behavior if process management is being replaced.
🤖 Prompt for AI Agents
In
src/Communication/Shintio.Communication.SubProcess.Server/Utils/ClientRunner.cs
around lines 41 to 54, the code that starts and assigns the child process has
been commented out so the Process property remains null; restore process
creation and assignment so InitWorkers() can run correctly. Re-enable the
ProcessWrapper.Start(...) call (or otherwise create and assign Process with the
correct ProcessStartInfo including FileName=path, ArgumentList.Add(Name),
UseShellExecute=false, CreateNoWindow=true, RedirectStandardOutput=true,
RedirectStandardInput=true) before calling InitWorkers(), and ensure any
exceptions from starting the process are handled/logged and Process is not left
null.
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.CodeAnalysis" PrivateAssets="all" /> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.4.0" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
🧩 Analysis chain
Specify an explicit version for Microsoft.CodeAnalysis.
The PackageReference for Microsoft.CodeAnalysis on Line 10 lacks a Version attribute, while Microsoft.CodeAnalysis.CSharp explicitly specifies v4.4.0. Omitting the version can lead to non-deterministic builds and unexpected package updates.
Add an explicit version constraint to match your repository's dependency management approach:
- <PackageReference Include="Microsoft.CodeAnalysis" PrivateAssets="all" />
+ <PackageReference Include="Microsoft.CodeAnalysis" Version="4.4.0" PrivateAssets="all" />
</web_search>
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all .csproj files to check version pinning patterns
fd -t f '\.csproj$' -x cat {} \; | grep -A2 -B2 'PackageReference'
Length of output: 5817
🏁 Script executed:
#!/bin/bash
# Check if the specific source generator project file exists and inspect it
find . -name "Shintio.CodeBuilder.CSharp.SourceGenerators.csproj" -type f -exec cat {} \;Length of output: 560
🌐 Web query:
Microsoft.CodeAnalysis version compatibility 4.4.0 source generators
💡 Result:
Short answer
- Roslyn/Microsoft.CodeAnalysis 4.4.0 introduced APIs (e.g. newer IncrementalGenerator helpers) that source generators may want to use, but a generator that references Microsoft.CodeAnalysis 4.4.0 will fail to load on older compiler/SDK hosts that provide an older Microsoft.CodeAnalysis assembly (you’ll see CS8032 / "Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.4.0.0'"). [1][2][4]
What to do
- If you need the 4.4.0 APIs: multi-target your generator (produce builds that reference older Microsoft.CodeAnalysis versions and one that references 4.4.0) or ship a 4.4-targeted build alongside older-targeted builds (this is the approach used by Microsoft-built generators). This lets the generator run on both newer and older SDKs. [2][5][3]
- If you want widest compatibility and can avoid new APIs: target the lowest Microsoft.CodeAnalysis version that provides the APIs you actually need (so the generator will load on older SDKs). [2][4]
References
- Microsoft.CodeAnalysis 4.4.0 NuGet package. [1]
- Andrew Lock — "Supporting multiple .NET SDK versions in analyzers and source generators" (walkthrough, CS8032 example and multi-targeting approach). [2]
- Microsoft Docs — Roslyn/source generators overview (general guidance on generators and host interaction). [3]
- StackOverflow example showing CS8032 when analyzer references a newer Microsoft.CodeAnalysis. [4]
- Discussion/summary of compatibility trade-offs for Roslyn components (multi-targeting explanation). [5]
If you want, I can show a minimal csproj multi-targeting example that supports both older Roslyn and 4.4.0.
Specify an explicit version for Microsoft.CodeAnalysis.
The PackageReference for Microsoft.CodeAnalysis on line 10 lacks a Version attribute, while Microsoft.CodeAnalysis.CSharp explicitly specifies v4.4.0. This inconsistency creates two problems: (1) unversioned packages float to the latest version, causing non-deterministic builds, and (2) version mismatches between Microsoft.CodeAnalysis and Microsoft.CodeAnalysis.CSharp can trigger CS8032 errors at runtime when the source generator cannot load on older SDK hosts. All other PackageReferences in the repository explicitly pin versions.
Add an explicit version constraint matching Microsoft.CodeAnalysis.CSharp:
- <PackageReference Include="Microsoft.CodeAnalysis" PrivateAssets="all" />
+ <PackageReference Include="Microsoft.CodeAnalysis" Version="4.4.0" PrivateAssets="all" />📝 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.
| <ItemGroup> | |
| <PackageReference Include="Microsoft.CodeAnalysis" PrivateAssets="all" /> | |
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.4.0" /> | |
| </ItemGroup> | |
| <ItemGroup> | |
| <PackageReference Include="Microsoft.CodeAnalysis" Version="4.4.0" PrivateAssets="all" /> | |
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.4.0" /> | |
| </ItemGroup> |
🤖 Prompt for AI Agents
In
src/DevTools/Shintio.CodeBuilder.CSharp.SourceGenerators/Shintio.CodeBuilder.CSharp.SourceGenerators.csproj
around lines 9 to 12, the PackageReference for Microsoft.CodeAnalysis is missing
a Version attribute which can lead to floating versions and runtime CS8032
errors; update that PackageReference to explicitly pin Version="4.4.0" (matching
Microsoft.CodeAnalysis.CSharp), keep PrivateAssets="all", and ensure the project
file uses the same version for both references to produce deterministic builds
and avoid runtime mismatches.
| var file = CodeFactory.File.Create( | ||
| [ | ||
| "System", | ||
| "System.Collections.Generic", | ||
| ], | ||
| "Shintio.TestNamespace.Scenes", | ||
| builder => builder | ||
| .AddFields(names, name => | ||
| new FieldCodeBlock("public", "SceneInfo", name) | ||
| .Static() | ||
| .Readonly()) | ||
| .AddAdditionalBlock( | ||
| new ClassCodeBlock("SceneInfo") | ||
| .AddField("public", "string", "Id") | ||
| .AddField(CodeFactory.Field.Create(AccessModifier.Public, TypeInfo.String, "Id").Readonly()) | ||
| .AddProperty(CodeFactory.Property.CreateAuto(AccessModifier.Public, TypeInfo.String, "Id", true, | ||
| defaultValue: "qwe")) | ||
| .AddRaw("public readonly bool Enabled;") | ||
| .AddConstructor( | ||
| "public", | ||
| new ParameterCodeBlock(TypeInfo.Int, "id"), | ||
| new ParameterCodeBlock("string", "name"), | ||
| new ParameterCodeBlock("bool", "enabled") | ||
| ) | ||
| .WithBody(""" | ||
| Id = id; | ||
| Name = name; | ||
| Enabled = enabled; | ||
| """) |
There was a problem hiding this comment.
🧩 Analysis chain
Generated SceneInfo code is currently invalid (duplicate members and type mismatch)
The builder chain creating SceneInfo will generate uncompilable C#:
- Line 36 and line 37 both introduce a field named
Id, and line 38 introduces a property namedIdas well. That’s at least two, likely three, members with the same identifier in the same class. - The auto property is created as
TypeInfo.Stringfor"Id", but the constructor parameteridisTypeInfo.Int(line 43), and the body assignsId = id;(line 48), which is a type mismatch (stringvsint). - The constructor body assigns
Name = name;(line 49), but there is noNamefield or property defined in the builder.
If Test() is intended as a realistic sample of the DSL, I’d suggest:
- Decide whether
Idshould beintorstringand make the property and constructor parameter consistent. - Remove the duplicate
Idfield declaration(s) so you only have one backing representation. - Add a corresponding
Namemember (FieldorProperty) or adjust the constructor body accordingly.
🏁 Script executed:
#!/bin/bash
# Optional: quickly inspect the generated output to confirm diagnostics.
dotnet script -q <<'CS'
#r "nuget: Scriban, 5.9.0" // placeholder to allow quick C# execution if scripting is configured
// If you can run your code here, call Class1.Test() and inspect result.cs, then build it.
CSLength of output: 44
🏁 Script executed:
#!/bin/bash
# Inspect the exact file and line range mentioned in the review
cat -n src/DevTools/Shintio.CodeBuilder.CSharp/Class1.cs | sed -n '20,55p'Length of output: 1294
🏁 Script executed:
#!/bin/bash
# Check if there are any other files that document or test the CodeFactory DSL
fd -e cs -e md | xargs rg -l "CodeFactory\." | head -20Length of output: 128
🏁 Script executed:
#!/bin/bash
# Search for any documentation or examples of the CodeFactory.Property.CreateAuto API
rg -nC5 "CreateAuto" --type=csLength of output: 6632
Fix duplicate Id members, type mismatch, and missing Name member in SceneInfo builder
The builder chain at lines 36–50 contains three independent issues that will prevent the generated SceneInfo class from compiling:
- Duplicate
Idmembers: Lines 36, 37, and 38 each add a member namedIdto the class—a field (line 36), another field viaCodeFactory.Field.Create(line 37), and an auto property (line 38). Only one should remain. - Type mismatch: The auto property on line 38 is declared as
TypeInfo.String, but the constructor parameteridon line 43 isTypeInfo.Int, and line 48 assignsId = id;(assigninginttostring). - Missing
Namemember: The constructor body assignsName = name;on line 49, but noNamefield or property is defined in the builder chain.
Action required:
- Decide the correct type for
Id(likelyintto match the constructor parameter on line 43). - Remove the redundant
Idfield declarations on lines 36 and/or 37, keeping only one backing representation. - Add a
Namefield or property to the builder, or adjust the constructor body.
🤖 Prompt for AI Agents
In src/DevTools/Shintio.CodeBuilder.CSharp/Class1.cs around lines 23 to 51, the
SceneInfo builder currently creates duplicate Id members, mismatches Id types,
and omits a Name member; fix by making Id a single member of type int (to match
the constructor parameter) — remove the redundant Id field(s) or the
auto-property so only one Id remains as either a readonly int field or an int
auto-property with a getter, ensure the constructor parameter stays int, and add
a Name member (public string Name field or property) so the constructor
assignment Name = name; is valid; also remove the string default value for Id
and align all types accordingly.
| public AccessModifier AccessModifier { get; set; } = AccessModifier.Internal; | ||
| public string Name { get; set; } | ||
|
|
||
| public bool IsPartial { get; set; } = false; | ||
| public bool IsStatic { get; set; } = false; | ||
| public bool IsAbstract { get; set; } = false; | ||
| public CommentCodeBlock? Comment { get; set; } = null; | ||
|
|
||
| public List<ConstructorCodeBlock> Constructors { get; } = []; | ||
| public List<FieldCodeBlock> Fields { get; } = []; | ||
| public List<PropertyCodeBlock> Properties { get; } = []; | ||
| public List<MethodCodeBlock> Methods { get; } = []; | ||
|
|
||
| public List<ICodeBlock> AdditionalBlocks { get; } = []; |
There was a problem hiding this comment.
AccessModifier property is ignored; all classes are emitted as public
ClassCodeBlock exposes an AccessModifier property (Line 16) with a default of AccessModifier.Internal, but GetCodeInternal currently hardcodes public:
return $$"""
{{Comment.GetCode()}}public {{@abstract}}{{@static}}{{@partial}}class {{Name}}
...
""";That makes the AccessModifier API misleading and prevents callers from generating internal, protected, or file classes through this block.
You can fix this by using AccessModifier in the template instead:
- return $$"""
- {{Comment.GetCode()}}public {{@abstract}}{{@static}}{{@partial}}class {{Name}}
+ return $$"""
+ {{Comment.GetCode()}}{{AccessModifier}} {{@abstract}}{{@static}}{{@partial}}class {{Name}}
{{{(string.IsNullOrWhiteSpace(body) ? string.Empty : Environment.NewLine + body)}}
}
""";You may also want to double‑check whether the default of AccessModifier.Internal is intentional now that it will take effect.
Also applies to: 90-103
🤖 Prompt for AI Agents
In src/DevTools/Shintio.CodeBuilder.CSharp/CodeBlocks/ClassCodeBlock.cs around
lines 16–29 (and also check lines 90–103), the AccessModifier property is
declared but ignored because the template hardcodes "public"; update the code
generation template to emit the actual AccessModifier value instead of the
literal "public" (e.g., map the AccessModifier enum to its string representation
and inject it into the interpolated template), and verify the default
AccessModifier.Internal is still the intended default now that it will be
honored.
| { | ||
| const float oneRad = 180 / MathF.PI; | ||
|
|
||
| direction = direction.GetNormalized(); |
There was a problem hiding this comment.
Guard against zero-vector input to prevent division by zero.
Calling GetNormalized() on a zero vector will cause a division by zero crash, as the length will be 0.
Apply this diff to add a guard:
public static Vector3 GetRotation(Vector3 direction)
{
const float oneRad = 180 / MathF.PI;
+ var length = direction.Length();
+ if (length < Epsilon)
+ {
+ return Vector3.Zero;
+ }
+
direction = direction.GetNormalized();Alternatively, check the length before normalization and return a sensible default (like Vector3.Zero or throw an ArgumentException).
🤖 Prompt for AI Agents
In src/Misc/Shintio.Math/Utils/Mathf.cs around line 629, calling
direction.GetNormalized() can divide by zero for a zero vector; guard by
checking the vector's length (or squared length) before normalizing and handle
the zero case by returning a sensible default (e.g., Vector3.Zero) or throwing
an ArgumentException; replace the direct GetNormalized() call with a conditional
that checks length > epsilon and only normalizes when safe otherwise returns the
chosen fallback.
| public static AssemblyLoadContextWrapper? GetLoadContext(AssemblyWrapper assembly) | ||
| { | ||
| return new AssemblyLoadContextWrapper(GetLoadContextMethod.Invoke(null, new[] { assembly.Assembly })!); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Handle AssemblyLoadContext.GetLoadContext returning null instead of wrapping it.
AssemblyLoadContext.GetLoadContext(Assembly) can legally return null, but GetLoadContext here always constructs a wrapper via the null‑forgiving operator. That means the API advertises AssemblyLoadContextWrapper? but will actually produce an instance with _assemblyLoadContext == null, leading to runtime failures when calling instance methods/events later.
Propagate the null instead of wrapping it:
- public static AssemblyLoadContextWrapper? GetLoadContext(AssemblyWrapper assembly)
- {
- return new AssemblyLoadContextWrapper(GetLoadContextMethod.Invoke(null, new[] { assembly.Assembly })!);
- }
+ public static AssemblyLoadContextWrapper? GetLoadContext(AssemblyWrapper assembly)
+ {
+ var context = GetLoadContextMethod.Invoke(null, new[] { assembly.Assembly });
+ return context != null ? new AssemblyLoadContextWrapper(context) : null;
+ }This matches the nullable return type and avoids hidden TargetException/null dereferences later.
I'll verify the review comment by examining the codebase and confirming the API behavior.
🏁 Script executed:
#!/bin/bash
# Inspect the current implementation of GetLoadContext method
cat src/Misc/Shintio.ReflectionBomb/Types/AssemblyLoadContextWrapper.csLength of output: 2507
🏁 Script executed:
#!/bin/bash
# Search for usages of GetLoadContext in the codebase
rg -n "GetLoadContext" --type=cs -C3Length of output: 2041
🏁 Script executed:
#!/bin/bash
# Examine AssemblyLoadContextWrapper class structure to understand _assemblyLoadContext field
ast-grep --pattern 'class AssemblyLoadContextWrapper {
$$$
$_* _assemblyLoadContext
$$$
}'Length of output: 0
🌐 Web query:
AssemblyLoadContext.GetLoadContext() return null behavior .NET
💡 Result:
Short answer
- AssemblyLoadContext.GetLoadContext(assembly) can return null when the Assembly argument isn't a runtime (RuntimeAssembly) instance — for example certain dynamic/reflection-only or otherwise non-runtime-built assemblies. For normal runtime assemblies the method either returns the specific AssemblyLoadContext that owns the assembly or AssemblyLoadContext.Default (the runtime default) for TPA-bound framework/app assemblies. [1][2]
Why (technical summary)
- The runtime implementation first maps the provided System.Reflection.Assembly to an internal RuntimeAssembly; if that mapping fails the API returns null. If the mapping succeeds but the native lookup returns a zero pointer (TPA/default binder), the method returns the Default load context instead of null. [2][3]
Practical implications
- Expect null for assemblies that are not runtime assemblies (e.g., some dynamic/reflection-only cases). For regular loaded assemblies you will get either the specific ALC or AssemblyLoadContext.Default. If you see unexpected nulls, check whether the Assembly is a RuntimeAssembly (not a dynamic/emit-only one). [2][3]
Sources
- Microsoft docs: AssemblyLoadContext overview and behavior. [1][3]
- .NET Core runtime source (AssemblyLoadContext.GetLoadContext implementation). [2]
References
[1] Microsoft Learn — How to use and debug assembly unloadability in .NET.
[2] source.dot.net — AssemblyLoadContext.CoreCLR.cs (GetLoadContext implementation).
[3] Microsoft Learn — About AssemblyLoadContext / dependency-loading.
Fix GetLoadContext to propagate null instead of wrapping it.
Verification confirms that AssemblyLoadContext.GetLoadContext(assembly) can return null when the Assembly argument isn't a runtime (RuntimeAssembly) instance. The current implementation at lines 29–32 uses the null-forgiving operator (!) on a potentially null result, then always constructs a wrapper instance. This means _assemblyLoadContext can be null at runtime, causing failures later when LoadFromStream (line 35) or SubscribeToAssembly (line 44) invoke methods on it.
The return type AssemblyLoadContextWrapper? advertises nullability but the implementation never actually returns null—only instances that wrap null internally. Apply the suggested fix to propagate null:
public static AssemblyLoadContextWrapper? GetLoadContext(AssemblyWrapper assembly)
{
- return new AssemblyLoadContextWrapper(GetLoadContextMethod.Invoke(null, new[] { assembly.Assembly })!);
+ var context = GetLoadContextMethod.Invoke(null, new[] { assembly.Assembly });
+ return context != null ? new AssemblyLoadContextWrapper(context) : null;
}This matches the nullable return type and prevents null dereferences.
🤖 Prompt for AI Agents
In src/Misc/Shintio.ReflectionBomb/Types/AssemblyLoadContextWrapper.cs around
lines 29–32, the code unconditionally wraps the result of
GetLoadContextMethod.Invoke(...) using the null-forgiving operator and always
returns a new AssemblyLoadContextWrapper even when the underlying value is null;
update the method to capture the Invoke result into a local variable, check if
it is null and return null in that case, otherwise construct and return a new
AssemblyLoadContextWrapper from the non-null value (remove the null-forgiving
operator and propagate null instead of wrapping it).
| // public static void CreateDirectoryViaCmd(string path) | ||
| // { | ||
| // var wrapper = ProcessWrapper.Start(new ProcessStartInfo | ||
| // { | ||
| // FileName = "cmd.exe", | ||
| // Arguments = $"/C mkdir \"{path}\"", | ||
| // UseShellExecute = false, | ||
| // CreateNoWindow = true, | ||
| // }); | ||
| // wrapper.Start(); | ||
| // wrapper.WaitForExit(); | ||
| // } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify removal of public API CreateDirectoryViaCmd.
The public method CreateDirectoryViaCmd has been commented out, which is a breaking change for any consumers of this library.
Run this script to check if the method is referenced elsewhere:
If the method is unused and being permanently removed, delete the commented code entirely rather than leaving it in place. If it's being temporarily disabled, add a comment explaining why and when it will be restored.
🏁 Script executed:
#!/bin/bash
# Search for usages of CreateDirectoryViaCmd in the codebase
rg -n 'CreateDirectoryViaCmd' --type=cs -g '!DirectoryWrapper.cs'Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for package metadata and project configuration
echo "=== Checking for package metadata ==="
fd -e "\.nuspec$" -o -e "\.csproj$" | xargs grep -l "PackageId\|Package\|Version" 2>/dev/null | head -5
echo "=== Checking DirectoryWrapper.cs full content around the method ==="
cat "src/Misc/Shintio.ReflectionBomb/Types/DirectoryWrapper.cs" | head -60
echo "=== Searching for any deprecation or breaking change comments ==="
rg -n "breaking|deprecated|remove|obsolete" --type=cs -i "DirectoryWrapper.cs"
Length of output: 1912
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the project file for this assembly
echo "=== Locating project file ==="
find . -name "*.csproj" -path "*/Shintio.ReflectionBomb/*" -type f
echo "=== Checking project file content ==="
cat "src/Misc/Shintio.ReflectionBomb/Shintio.ReflectionBomb.csproj" 2>/dev/null | grep -E "PackageId|Version|Description" | head -10
echo "=== Looking for CHANGELOG or breaking change documentation ==="
find . -type f \( -name "CHANGELOG*" -o -name "BREAKING*" -o -name "RELEASE*" \) | head -5
Length of output: 327
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine full project file
echo "=== Full .csproj content ==="
cat "src/Misc/Shintio.ReflectionBomb/Shintio.ReflectionBomb.csproj"
echo -e "\n=== Searching for any test or example usage ==="
rg -n 'CreateDirectoryViaCmd' --type=cs
echo -e "\n=== Checking if DirectoryWrapper is used elsewhere ==="
rg -n 'DirectoryWrapper' --type=cs -g '!DirectoryWrapper.cs' | head -20
Length of output: 635
Remove commented-out code for repository cleanliness.
The commented CreateDirectoryViaCmd method (lines 40-51) is completely unused—no internal references exist in the codebase, and the project is not configured as a published NuGet package (no PackageId or Version in the project file). Since this is unused internal code, delete the commented block entirely rather than keeping dead code.
🤖 Prompt for AI Agents
In src/Misc/Shintio.ReflectionBomb/Types/DirectoryWrapper.cs around lines 40 to
51, remove the entire commented-out CreateDirectoryViaCmd method block (the //
public static void ... to the closing // }), deleting the dead/commented code so
the file no longer contains that unused snippet and commit the cleaned file.
| // public static string GetTempPathViaCmd() | ||
| // { | ||
| // var temp = "System" + "." + "IO" + "." + "Path"; | ||
| // return CliHelper.GetOutput($"[{temp}]::GetTempPath()", CliInterpreter.Powershell) | ||
| // .GetAwaiter() | ||
| // .GetResult(); | ||
| // } |
There was a problem hiding this comment.
🧩 Analysis chain
Remove commented-out code entirely.
Commented-out code should be removed rather than kept in the codebase, as version control preserves the history. If GetTempPathViaCmd() is no longer needed, delete these lines completely.
Additionally, verify that no callers depend on this method, as removing it constitutes a breaking change to the public API.
Apply this diff to remove the commented code:
- // public static string GetTempPathViaCmd()
- // {
- // var temp = "System" + "." + "IO" + "." + "Path";
- // return CliHelper.GetOutput($"[{temp}]::GetTempPath()", CliInterpreter.Powershell)
- // .GetAwaiter()
- // .GetResult();
- // }Run the following script to verify no usages remain:
🏁 Script executed:
#!/bin/bash
# Description: Search for any references to GetTempPathViaCmd across the codebase.
# Search for method calls
rg -n -C3 'GetTempPathViaCmd' --type=csLength of output: 660
Remove the commented-out GetTempPathViaCmd() method.
Verification confirms this method is not referenced anywhere in the codebase, so it is safe to remove entirely. Commented-out code should not be kept as version control preserves history.
Delete lines 28-34 from src/Misc/Shintio.ReflectionBomb/Types/PathWrapper.cs.
🤖 Prompt for AI Agents
In src/Misc/Shintio.ReflectionBomb/Types/PathWrapper.cs around lines 28 to 34,
remove the entire commented-out GetTempPathViaCmd() method (lines 28-34) since
it's unused; delete those commented lines so the file no longer contains the
dead, commented code and commit the change.
| // public static async Task<string> GetOutput(string command, CliInterpreter interpreter = CliInterpreter.Cmd) | ||
| // { | ||
| // var process = ProcessWrapper.Start(new ProcessStartInfo | ||
| // { | ||
| // FileName = GetFileName(interpreter), | ||
| // Arguments = FormatArguments(command, interpreter), | ||
| // UseShellExecute = false, | ||
| // CreateNoWindow = true, | ||
| // RedirectStandardOutput = true, | ||
| // }); | ||
| // | ||
| // process.Start(); | ||
| // | ||
| // return await process.StandardOutput.ReadToEndAsync(); | ||
| // } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify removal of public API GetOutput.
The public async method GetOutput has been commented out, which is a breaking change for any consumers of this library.
Run this script to check if the method is referenced elsewhere:
If the method is unused and being permanently removed, delete the commented code entirely. If it's being temporarily disabled, add a comment explaining why. Also note that the using System.Threading.Tasks directive at line 1 may no longer be needed since the only async method is now commented out.
🏁 Script executed:
#!/bin/bash
# Search for usages of GetOutput in the codebase
rg -n 'GetOutput' --type=cs -g '!CliHelper.cs'Length of output: 143
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if GetOutput has any active (non-commented) usages in the entire codebase
rg -n '^[^/]*GetOutput' --type=cs -g '!CliHelper.cs' 2>/dev/null || echo "No active (non-commented) usages found"
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check CliHelper.cs for remaining methods and using directives
cat src/Misc/Shintio.ReflectionBomb/Utils/CliHelper.csLength of output: 1370
🏁 Script executed:
#!/bin/bash
# Check PathWrapper.cs context around line 31
sed -n '25,37p' src/Misc/Shintio.ReflectionBomb/Types/PathWrapper.csLength of output: 322
Delete commented code and remove unused import.
The public API GetOutput has no active consumers in the codebase—the only reference is itself commented out in PathWrapper.cs:31. Rather than leaving dead code, either delete the method entirely or add documentation explaining its preservation.
Additionally, using System.Threading.Tasks (line 1 of CliHelper.cs) is now unused since all async methods are commented out. Remove it.
🤖 Prompt for AI Agents
In src/Misc/Shintio.ReflectionBomb/Utils/CliHelper.cs around lines 9 to 23,
remove the commented-out GetOutput method and clean up the file by deleting the
unused using System.Threading.Tasks import at the top; if you prefer to keep the
method for future use, replace the commented block with a brief XML doc comment
explaining why it’s retained, otherwise delete the commented code and the unused
using to eliminate dead code and unused imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Misc/Shintio.ReflectionBomb/Types/AssemblyLoadContextWrapper.cs`:
- Around line 48-75: The SubscribeToAssembly method is building a delegate type
Func<object, AssemblyName, Assembly> that doesn't match the Resolving event's
exact delegate type and will cause ArgumentException at AddEventHandler; update
SubscribeToAssembly to use eventInfo.EventHandlerType (obtained from
AssemblyLoadContextType.GetEvent("Resolving", ...).EventHandlerType) when
constructing the Expression.Lambda and compiled delegate so the runtime delegate
type exactly matches the event (cast or convert parameters as needed but create
the lambda with eventInfo.EventHandlerType); apply the same fix to
SubscribeToAssemblyUnload by using the eventInfo.EventHandlerType for the
Unloading event instead of constructing Action<object> so the compiled delegate
matches Action<AssemblyLoadContext>.
🧹 Nitpick comments (2)
src/DevTools/Shintio.CodeGenerator/Utils/ReflectionHelper.cs (1)
74-90: Exact duplicates ofShintio.Essentials/Utils/ReflectionHelper.csmethods.Both
GetChildrenTypesandGetChildrenInterfacesare verbatim copies of the same methods insrc/Shintio.Essentials/Utils/ReflectionHelper.cs(lines 33–49). Consider sharing a common implementation or delegating to the existing helper to avoid divergence over time.Also,
Enumerable.Where()never returnsnull, so the?? Array.Empty<Type>()on lines 82 and 89 is dead code and gives a false impression that nullability is a concern here.♻️ Remove the unnecessary null-coalescing
public static IEnumerable<Type> GetChildrenTypes(Type type, bool allowAbstract = false) { return type.Assembly .GetTypes() .Where(child => child.IsClass && (allowAbstract || !child.IsAbstract) && child.IsSubclassOf(type) - ) ?? Array.Empty<Type>(); + ); } public static IEnumerable<Type> GetChildrenInterfaces(Type type) { return type.Assembly .GetTypes() - .Where(t => type.IsAssignableFrom(t) && t != type) ?? Array.Empty<Type>(); + .Where(t => type.IsAssignableFrom(t) && t != type); }src/Misc/Shintio.ReflectionBomb/Types/AssemblyLoadContextWrapper.cs (1)
17-18: PreferFirst()overFirstOrDefault()!for a clearer failure mode.If no matching
LoadFromStreamoverload is found,FirstOrDefaultreturnsnulland the!operator silently suppresses the warning. A subsequent call toLoadFromStreamMethod.Invoke(...)would then throw aNullReferenceExceptionwith no indication of the root cause. UsingFirst()would throw anInvalidOperationExceptionwith a meaningful message at initialization time.Suggested fix
- private static readonly MethodInfo LoadFromStreamMethod = AssemblyLoadContextType.GetMethods() - .FirstOrDefault(m => m.Name == "LoadFromStream" && m.GetParameters().Length == 1)!; + private static readonly MethodInfo LoadFromStreamMethod = AssemblyLoadContextType.GetMethods() + .First(m => m.Name == "LoadFromStream" && m.GetParameters().Length == 1);
| public void SubscribeToAssembly(Func<object, AssemblyName, object> handler) | ||
| { | ||
| var eventInfo = AssemblyLoadContextType.GetEvent("Resolving", BindingFlags.Instance | BindingFlags.Public)!; | ||
|
|
||
| var asmNameType = typeof(AssemblyName); | ||
| var asmType = TypesHelper.GetType(TypesHelper.TypeFromSystem, "System", "Reflection", "Assembly")!; | ||
|
|
||
| var ctxParam = Expression.Parameter(typeof(object), "ctx"); | ||
| var nameParam = Expression.Parameter(asmNameType, "name"); | ||
|
|
||
| var handlerConst = Expression.Constant(handler); | ||
|
|
||
| var call = Expression.Call( | ||
| handlerConst, | ||
| handler.GetType().GetMethod("Invoke")!, | ||
| ctxParam, | ||
| nameParam | ||
| ); | ||
|
|
||
| var cast = Expression.Convert(call, asmType); | ||
|
|
||
| var lambdaType = typeof(Func<,,>).MakeGenericType(typeof(object), asmNameType, asmType); | ||
| var lambda = Expression.Lambda(lambdaType, cast, ctxParam, nameParam); | ||
|
|
||
| var compiled = lambda.Compile(); | ||
|
|
||
| eventInfo.AddEventHandler(AssemblyLoadContext, compiled); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AssemblyLoadContext Resolving event delegate type signature .NET
💡 Result:
AssemblyLoadContext.Resolving is an event whose handler must match this delegate signature:
Func<AssemblyLoadContext, AssemblyName, Assembly?>So a handler method typically looks like:
Assembly? OnResolving(AssemblyLoadContext context, AssemblyName name)
{
// return a loaded Assembly, or null if you can't resolve it
}Microsoft docs show the event type as Func<AssemblyLoadContext, AssemblyName, Assembly> (but handlers are expected to return null when they can’t resolve, so Assembly? is the practical signature in C# with nullable enabled). [1]
Source(s): Microsoft Learn — AssemblyLoadContext.Resolving event type and remarks. [1]
🏁 Script executed:
# Check the SubscribeToAssemblyUnload method mentioned in the review
cd /tmp && git clone https://github.com/coderabbit-ai/shintio-reflection-bomb 2>/dev/null || true
# Actually, let me search the provided repo for the relevant code
rg "SubscribeToAssemblyUnload" -A 20Repository: axeok/Shintio.Net
Length of output: 209
🌐 Web query:
.NET Delegate.Combine variance contravariance covariance exact type match
💡 Result:
Delegate.Combine (and the + / += operators for multicast delegates) does not perform variance conversions. It requires that the delegates being combined are instances of the exact same delegate runtime type, otherwise it throws ArgumentException (“not instances of the same delegate type”).[1]
Why variance “works” for assignment but not for combination
Variance (contra-/co-variance) lets you treat one delegate type as another for assignment/invocation compatibility (per the delegate compatibility rules).[2][3] But combining delegates is stricter: it’s effectively concatenating invocation lists, and the runtime demands identical delegate types for the resulting multicast delegate.[1]
The common “gotcha”: compile-time type matches, runtime type doesn’t
You can write code where both operands appear to be the same type (because of variance), but one operand is actually a different delegate type object at runtime, so the combine fails. This is the classic example (covariance): combining two Func<object> variables can still throw because one is really a Func<string> object.[4]
Why your Action<Base> / Action<Derived> example may appear to combine
In expressions like baction + daction, the compiler can insert an implicit conversion so both operands are statically Action<Derived>. Whether this succeeds then depends on what objects you end up with at runtime:
- If the conversion produces a delegate object whose runtime type becomes
Action<Derived>, combining can succeed. - If you end up with one operand still being an
Action<Base>object (just viewed through a variant reference), combining will fail at runtime (per the exact-type rule).[1][4]
Practical rule of thumb
- Variance is for assignment and method binding.
- Combination requires exact delegate type identity at runtime (same constructed generic type, same signature and same delegate type object kind).[1]
Sources: Microsoft API docs for Delegate.Combine and its variance note[1], C# delegate variance guidance[3], C# spec delegate compatibility (variance) rules[2], and Eric Lippert’s explanation of the runtime-type mismatch pitfall[4].
🏁 Script executed:
# Find all files in the project
git ls-files | head -20Repository: axeok/Shintio.Net
Length of output: 709
🏁 Script executed:
# Look for any test files or usage of SubscribeToAssembly
rg "SubscribeToAssembly\|SubscribeToAssemblyUnload" -B 2 -A 2Repository: axeok/Shintio.Net
Length of output: 43
🏁 Script executed:
# Check the full AssemblyLoadContextWrapper file to understand context
find . -name "AssemblyLoadContextWrapper.cs" -exec head -150 {} \;Repository: axeok/Shintio.Net
Length of output: 3218
Fix delegate type mismatch in event handler subscription.
The compiled delegate type Func<object, AssemblyName, Assembly> does not match the Resolving event's expected type Func<AssemblyLoadContext, AssemblyName, Assembly?>. AddEventHandler internally uses Delegate.Combine, which requires exact delegate type matching at runtime and does not honor variance. This will cause an ArgumentException when the handler is registered.
Use eventInfo.EventHandlerType instead of manually constructing the lambda type:
Fix for SubscribeToAssembly
- var lambdaType = typeof(Func<,,>).MakeGenericType(typeof(object), asmNameType, asmType);
+ var lambdaType = eventInfo.EventHandlerType!;The same issue exists in SubscribeToAssemblyUnload (line 91), which constructs Action<object> but the Unloading event expects Action<AssemblyLoadContext>. Apply the same fix there.
🤖 Prompt for AI Agents
In `@src/Misc/Shintio.ReflectionBomb/Types/AssemblyLoadContextWrapper.cs` around
lines 48 - 75, The SubscribeToAssembly method is building a delegate type
Func<object, AssemblyName, Assembly> that doesn't match the Resolving event's
exact delegate type and will cause ArgumentException at AddEventHandler; update
SubscribeToAssembly to use eventInfo.EventHandlerType (obtained from
AssemblyLoadContextType.GetEvent("Resolving", ...).EventHandlerType) when
constructing the Expression.Lambda and compiled delegate so the runtime delegate
type exactly matches the event (cast or convert parameters as needed but create
the lambda with eventInfo.EventHandlerType); apply the same fix to
SubscribeToAssemblyUnload by using the eventInfo.EventHandlerType for the
Unloading event instead of constructing Action<object> so the compiled delegate
matches Action<AssemblyLoadContext>.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes & Updates
Chores