Improvements, refactoring and tests

Author: PawelGerr

.clinerules

@@ -10,6 +10,8 @@
     <File Path="LICENSE.md" />
     <File Path="README.md" />
     <File Path="CLAUDE.md" />
+    <File Path="src\Thinktecture.Runtime.Extensions.SourceGenerator\AnalyzerReleases.Shipped.md" />
+    <File Path="src\Thinktecture.Runtime.Extensions.SourceGenerator\AnalyzerReleases.Unshipped.md" />
   </Folder>
   <Folder Name="/assets/.github/">
     <File Path=".github\workflows\claude.yml" />

.github/copilot-instructions.md

@@ -0,0 +1,19 @@
+Errors
+- default(ArgumentName) can cause NullReferenceException:
+  - Equals(string other) calls Name.Equals(...), which throws if Name is null (default struct or null passed to Create).
+  - GetHashCode() calls Name.GetHashCode(), which throws if Name is null.
+  - ToString() returns Name; for a default instance this returns null, which is unexpected for ToString and can cause downstream issues.
+  - Remediation: Enforce non-null invariant in the private constructor (ArgumentNullException.ThrowIfNull(name)) and/or make fields private with a validating factory. Additionally, harden methods against default by using Name is null checks (e.g., return false in Equals(string), return 0 in GetHashCode, return string.Empty in ToString) or prevent default by using a non-defaultable pattern.
+
+Warnings
+- Inconsistent equality semantics:
+  - Equals(string other) uses OrdinalIgnoreCase, while Equals(ArgumentName other) and operator== use case-sensitive string comparison and include RenderAsIs in equality. This inconsistency can lead to subtle bugs where ArgumentName.Equals("name") is true but new ArgumentName("name") != new ArgumentName("Name").
+  - Remediation: Decide on a single comparison semantic. If case-insensitive is intended, update Equals(ArgumentName), operator==, and GetHashCode to use StringComparer.OrdinalIgnoreCase. If case-sensitive is intended, change Equals(string) to case-sensitive and/or remove the overload to avoid confusion.
+
+- Public API surface leakage from SourceGenerator assembly:
+  - Type is public and placed in the root Thinktecture namespace, with public readonly fields. This unnecessarily expands the public surface of the analyzer/generator assembly and risks name collisions with consumer code that references Thinktecture.* namespaces. Public fields also reduce versioning flexibility.
+  - Remediation: Make the type internal (and ideally move to an internal sub-namespace used by the source generator). Consider making fields private and exposing read-only properties if external access is ever needed.
+
+- Non-nullable field with defaultable struct:
+  - Name is declared as non-nullable string but can be null in default(ArgumentName), violating the declared nullability contract and leading to the errors noted above. This can also produce misleading nullability analysis.
+  - Remediation: Prevent default, or relax the field to string? and handle nulls, or enforce non-null invariant as described above.

SourceGenerator-Review-Plan.md

@@ -0,0 +1,35 @@
+# Review: Logging/DebugLogger.cs
+
+Path: `src/Thinktecture.Runtime.Extensions.SourceGenerator/Logging/DebugLogger.cs`
+
+## Summary
+`DebugLogger` is a sealed logger implementation that inherits `LoggerBase` and implements `ILogger`. It sets its minimum level to `LogLevel.Debug` via the base constructor and delegates Debug/Information/Error to `LoggerBase.Log`. `LogTrace` is intentionally a no-op.
+
+## API and Behavior
+- Constructor: `DebugLogger(LoggerFactory, ILoggingSink, string source)` calls `base(..., LogLevel.Debug)` which establishes the effective minimum level for this logger instance.
+- `LogTrace(string message, TypeDeclarationSyntax type)`: empty method body — trace messages are ignored by this logger.
+- `LogDebug(...)` and `LogInformation(...)`: forward to `Log(LogLevel.Debug|Information, ...)` on `LoggerBase`.
+- Generic overloads for Debug/Information accept `T : INamespaceAndName` to include symbol identity context. The calls forward to base with the same constraints.
+- `LogError(...)`: forwards to `Log(LogLevel.Error, ...)` including the `Exception?`.
+
+Interface compliance:
+- Implements all members of `ILogger`. Optional parameters on the interface are compile-time features and not required on the implementing method; the provided signatures are compatible.
+
+## Potential Issues and Warnings
+- Behavior consistency: `LogTrace` is a no-op while other levels delegate to `LoggerBase.Log`. If the intent is to completely suppress trace logs for this logger type, this is correct. If the intent is to let the base decide (e.g., to allow sinks with independent level gating), consider delegating to `base.Log(LogLevel.Trace, ...)` for consistency.
+- Unused parameter: `type` in `LogTrace` is not used. Some analyzers (e.g., IDE0060) may flag “Remove unused parameter.” Because the signature is dictated by the interface, removal is not possible. If analyzer noise is a concern:
+  - Add `_ = type;` to explicitly acknowledge the parameter; or
+  - Call into `base.Log(LogLevel.Trace, ...)` as suggested above; or
+  - Add a justification comment or suppression attribute as per project conventions.
+- Documentation: Public API lacks XML doc comments. If the project enforces documentation, consider adding short summaries clarifying that this logger deliberately suppresses trace logs.
+- Nullability: Signatures align with the interface (`TypeDeclarationSyntax?` for non-trace overloads; non-nullable for `LogTrace`). Passing through `null` where allowed is handled by `LoggerBase`.
+
+## Recommendations
+- Decide on the intended semantics for trace in `DebugLogger` and either:
+  - Keep as no-op and add a brief comment, e.g., “Debug logger intentionally ignores trace-level messages,” and optionally `_ = type;` to avoid IDE0060; or
+  - Delegate to `base.Log(LogLevel.Trace, message, type)` for uniform handling (base will filter by level anyway).
+- Consider adding minimal XML docs to clarify intended filtering.
+- No functional errors detected.
+
+## Verdict
+No errors. One minor warning around an unused parameter and a small consistency consideration for `LogTrace` handling. The implementation is otherwise correct and consistent with the logging design used across the project.

Thinktecture.Runtime.Extensions.slnx

@@ -0,0 +1,39 @@
+# Issues found in src/Thinktecture.Runtime.Extensions.SourceGenerator/Extensions/EnumExtensions.cs
+
+## Errors
+
+1) Invalid array cast for underlying values
+- Code: `public static readonly int[] Values = (int[])Enum.GetValues(typeof(T));`
+- Problem: `Enum.GetValues(typeof(T))` returns an array of the enum type (`T[]`), not an array of the underlying numeric type. Casting `T[]` to `int[]` always fails at runtime with InvalidCastException, regardless of the enum’s underlying type.
+- Impact: Any first access to `EnumHelper<T>` (e.g., calling `GetValidValue`) will throw.
+- Fix options:
+  - Use `Enum.GetValuesAsUnderlyingType(typeof(T))` (returns an `Array` of the actual underlying numeric type), and adjust comparison logic accordingly.
+  - If the API is intended to support only int-based enums, guard explicitly:
+    ```
+    if (Enum.GetUnderlyingType(typeof(T)) != typeof(int)) throw new NotSupportedException(...);
+    ```
+    and remove the invalid cast.
+
+2) `GetValidValue<T>(this int value)` incompatible with non-int underlying enums
+- Code: `public static T? GetValidValue<T>(this int value) where T : struct, Enum`
+- Problems:
+  - The parameter is `int`, but enums may use any integral underlying type (`sbyte, byte, short, ushort, int, uint, long, ulong`). For non-int underlying enums this can truncate or mis-compare values.
+  - Combined with the broken `Values` array cast, the method is currently non-functional.
+- Impact: Incorrect behavior or exceptions for enums not using `int` as underlying type.
+- Fix options:
+  - Generalize input type by comparing via `Convert.ToUInt64(value)` against enum values converted to `UInt64`, or
+  - Use `Enum.GetValuesAsUnderlyingType` and compare using typed values, or
+  - Restrict to int-based enums with a runtime check and document the limitation.
+
+## Warnings
+
+1) `IsValid<T>` does not account for [Flags] combinations
+- Code: `Array.IndexOf(EnumHelper<T>.Items, item) >= 0`
+- Problem: Returns `false` for valid bitwise combinations unless explicitly declared as named members.
+- Risk: Surprising semantics for flags enums; may be intended, but if general validity for flags is desired, this is too strict.
+- Suggestion: If flags support is needed, validate via bitwise decomposition against defined values or document current behavior clearly.
+
+2) Redundant generic constraints
+- Code: `where T : struct, Enum` (in `GetValidValue`)
+- Problem: `Enum` constraint implies value type; adding `struct` is redundant.
+- Suggestion: Simplify to `where T : Enum` for consistency with `IsValid<T>`.

review/ArgumentName.cs.md

@@ -0,0 +1,46 @@
+Issues found
+
+1) Public API surface in SourceGenerator assembly
+- Severity: Warning
+- Details: The extension class and method are public. This assembly is an analyzer/source generator; exposing helper extensions publicly increases the surface area unintentionally for consumers of the analyzer package.
+- Recommendation: Make the class and method internal to keep the public API minimal.
+
+Suggested change:
+------- PATCH
+- public static class EnumerableExtension
++ internal static class EnumerableExtensions
+
+-   public static void Enumerate<T>(this IEnumerable<T> enumerable)
++   internal static void Enumerate<T>(this IEnumerable<T> enumerable)
++++++++ END
+
+
+2) Naming inconsistency: use plural "Extensions"
+- Severity: Warning
+- Details: Other helper types follow the "Extensions" plural naming convention (e.g., ReadOnlyCollectionExtensions). This class is named "EnumerableExtension" (singular), which is inconsistent with common .NET conventions and with the project naming.
+- Recommendation: Rename the class to EnumerableExtensions and the file to EnumerableExtensions.cs.
+
+Suggested change:
+- File rename: src/Thinktecture.Runtime.Extensions.SourceGenerator/Extensions/EnumerableExtension.cs → EnumerableExtensions.cs
+- Class rename:
+------- PATCH
+- public static class EnumerableExtension
++ internal static class EnumerableExtensions
++++++++ END
+
+
+3) Potential missing using for IEnumerable<T>
+- Severity: Warning (conditional)
+- Details: The file relies on IEnumerable<T> but has no explicit using System.Collections.Generic;. If implicit or global usings are disabled for this project, this will fail to compile.
+- Recommendation: Ensure a global using exists or add an explicit using.
+
+Suggested change (if needed):
+------- PATCH
++ using System.Collections.Generic;
+ 
+ namespace Thinktecture;
++++++++ END
+
+
+Notes
+- Null-check for enumerable is not strictly required due to NRT and project guidance (nullable reference types enabled). If defensive checks are desired, add ArgumentNullException.ThrowIfNull(enumerable); at the start of the method.

review/DebugLogger.cs.md

@@ -0,0 +1,42 @@
+# Review: Logging/ErrorLogger.cs
+
+Path: `src/Thinktecture.Runtime.Extensions.SourceGenerator/Logging/ErrorLogger.cs`
+
+## Summary
+`ErrorLogger` is a sealed logger inheriting `LoggerBase` and implementing `ILogger`. It sets its minimum level to `LogLevel.Error`. It intentionally ignores Trace, Debug, and Information (no-ops) while delegating Error to `LoggerBase.Log`.
+
+## API and Behavior
+- Constructor: `ErrorLogger(LoggerFactory, ILoggingSink, string source)` calls `base(..., LogLevel.Error)`, setting the min level to Error.
+- `LogTrace(string message, TypeDeclarationSyntax type)`: no-op — trace suppressed.
+- `LogDebug(...)` (generic and non-generic): no-op — debug suppressed.
+- `LogInformation(...)` (generic and non-generic): no-op — information suppressed.
+- `LogError(...)`: delegates to `Log(LogLevel.Error, ...)` including `Exception?`.
+
+Interface compliance:
+- Implements all `ILogger` members. Optional parameters on the interface are not required on the implementation signatures. Members coming from `LoggerBase` (e.g., `IsEnabled`, `Log(LogLevel, string)`) satisfy the interface.
+
+## Potential Issues and Warnings
+- Unused parameters (analyzers like IDE0060):
+  - `type` in `LogTrace`
+  - `type`, `factory` in non-generic `LogDebug`
+  - `type`, `namespaceAndName`, `factory` in generic `LogDebug`
+  - `type`, `factory` in non-generic `LogInformation`
+  - `type`, `namespaceAndName`, `factory` in generic `LogInformation`
+  Options:
+  - Add `_ = param;` for each; or
+  - Delegate to `base.Log(LogLevel.Trace|Debug|Information, ...)` and let base perform level gating; or
+  - Suppress with justification per project conventions.
+- Consistency:
+  - Other logger variants either delegate suppressed levels to base (e.g., `DebugLogger` for Debug/Info) or no-op. If uniform routing through the base (for consistent formatting/enrichment and future-proofing) is desired, delegate and rely on base’s level filtering. If strict suppression is desired, keep no-ops and document intent.
+- Documentation:
+  - No XML docs. If the project enforces documentation, add summaries clarifying suppression of Trace/Debug/Information.
+
+## Nullability
+- Matches `ILogger`: non-nullable `TypeDeclarationSyntax` for Trace; nullable for other overloads. Delegation for Error should handle nulls in `LoggerBase`.
+
+## Recommendations
+- Either keep no-ops and add comments plus `_ =` assignments to silence analyzers, or delegate suppressed levels to `base.Log(...)` for consistency.
+- Optionally add XML docs describing behavior.
+
+## Verdict
+No functional errors. Minor warnings regarding unused parameters and a design choice on suppression vs delegation for Trace/Debug/Information.

review/EnumExtensions.cs.md

@@ -0,0 +1,24 @@
+Issues found in src/Thinktecture.Runtime.Extensions.SourceGenerator/Extensions/FieldSymbolExtensions.cs
+
+1) Warning: Implicit dependency on global usings for Roslyn types
+- Problem: The file references SyntaxToken, IFieldSymbol, and IPropertySymbol without a local using Microsoft.CodeAnalysis;. Only using Microsoft.CodeAnalysis.CSharp.Syntax; is present, which does not import SyntaxToken nor the symbol interfaces.
+- Impact: Compilation relies on a global using elsewhere. If that global using is removed or not included in certain build contexts (e.g., tests, analyzers, or consumers embedding sources), this file will fail to compile.
+- Recommendation: Add an explicit using to remove the hidden dependency.
+
+Suggested change:
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+
+namespace Thinktecture;
+...
+
+2) Warning: Backing field detection may be broader than intended
+- Problem: IsPropertyBackingField checks only field.AssociatedSymbol is IPropertySymbol. This is generally correct for compiler-generated backing fields, but if the intention is to strictly detect compiler-generated fields, it should also verify field.IsImplicitlyDeclared to avoid any edge cases where a field might be associated but not implicitly generated.
+- Impact: Potential false positives in rare/edge cases if non-implicit fields with an associated property symbol appear (uncommon, but tightening the check is safer).
+- Recommendation: Narrow the check if strictness is desired.
+
+Suggested change:
+public static bool IsPropertyBackingField(this IFieldSymbol field)
+{
+   return field.IsImplicitlyDeclared && field.AssociatedSymbol is IPropertySymbol;
+}