From 6d5bfbe381f6a550f8cec41284d990b97f92a051 Mon Sep 17 00:00:00 2001 From: Guy Ludvig Date: Mon, 13 Jul 2026 11:36:21 +0300 Subject: [PATCH 1/4] De-reflect + DRY the array/enumerable converters (P4) Introduce a shared CollectionTypeConverter base that builds collection results with Array.CreateInstance + indexed fill and selects the element converter by walking the concrete LinkedList (struct enumerator) rather than LINQ First. ArrayTypeConverter/EnumerableTypeConverter collapse to thin subclasses that differ only in CanConvert + element-type extraction; both now return T[] (safe: IsEnumerable() matches only IEnumerable, which a T[] satisfies). This drops the per-convert List + its backing array, the reflected Enumerable.ToArray (MakeGenericMethod + Invoke + args array), and the First predicate closure. TypeConverter.CreateNullResult swaps the Enumerable.Empty() reflection for Array.CreateInstance(t, 0), and GetConverter is now a true manual walk (matching its own comment). Also strips a stray UTF-8 BOM from the file. Proof via the new gated ConvertArrayBenchmark (isolates the hot path like Q1/Q3/Q4): 1.33 KB -> 688 B (-49%), 1,247 -> 219 ns (5.7x). The residual 688 B is the split-substrings + per-element boxing + result array shared by both the old and new code. Adds 7 collection-converter parity tests (delimited string -> int[] / string[] / IEnumerable, empty-entry removal, custom delimiter, default-array passthrough, and that the enumerable path materializes a T[]). Suite: 63 tests per TFM (was 56). Also refreshes SESSION-HANDOFF.md + FIX-PLAN.md and carries the pre-P4 post-P3 style tweaks to TypeConverter.cs / TypeExtensions.cs. --- FIX-PLAN.md | 15 +-- SESSION-HANDOFF.md | 21 ++-- .../Conversion/ArrayTypeConverter.cs | 73 +++-------- .../Conversion/CollectionTypeConverter.cs | 71 +++++++++++ .../Conversion/EnumerableTypeConverter.cs | 75 ++++------- .../Core/Reflection/TypeConverter.cs | 15 +-- .../Core/Reflection/TypeExtensions.cs | 2 +- .../Conversion/CollectionConversionTests.cs | 116 ++++++++++++++++++ .../MicroBenchmarks.cs | 29 +++++ 9 files changed, 287 insertions(+), 130 deletions(-) create mode 100644 src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs create mode 100644 src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs diff --git a/FIX-PLAN.md b/FIX-PLAN.md index cb1b0db..630d706 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -2,12 +2,12 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · performance). Every finding below was verified against source with file:line. Work items are self-contained and ordered so they can be implemented one at a time._ -## Progress (2026-07-12) +## Progress (2026-07-13) - **Done & merged:** B1, B2, B4, B5, B9 + T1, T2 (PR #8) · BindingContext test (#10) · D3 namespace typo (#11) · T3 DI integration tests (#12) · solution rename (#13) · **A2 naming → ExistForAll (#15)** · **P0 benchmark harness (#16)** · **P1 provider cache + C3 decided/implemented (#17)** · **P2 memoize `ExtractTypeProperties` + `HashSet` dedup (#18)** · **docs tutorials refresh (#20)** · **Q1–Q4 perf quick wins + M1 collision fix + micro-benchmarks (#21)**. - **Q1–Q4 proven** via isolated micro-benchmarks (macro `ScanBenchmark` can't resolve them): Q1 2.7× / 64 KB→88 B · Q3 2.65× / 152 B→0 · Q4 32× / 224 B→0. **Q5 was already resolved by B4.** **M1** (code-review finding): namespace-qualify the generated impl name in the generator only — `GetNormalizeInterfaceName` also backs the section name. Suite → **56 per TFM**. -- **Merged since:** **benchmark-tracking CI (#22)** — BDN on push/PR, gates PRs on **allocation** regressions (>10%) via github-action-benchmark on `gh-pages`; time informational. · **session-wrap docs (#23)**. -- **In flight:** **P3 — cached "settings plan"** (`SettingsPlan` per type: section name resolved once + lazily, per-property key/default/converter precomputed, `[SettingsProperty]` read **once** per property). Warm re-populate **−55–61%** allocations (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)** — after a code+perf review pass, consolidating the 3× attribute read more than paid for the plan objects. The emitted/compiled setter was built and measured but **reverted** — it regressed the gated cold scan **+25%** for **no** warm gain (net10 reflective `SetValue` is already allocation-free for the args). A new gated `PlanPopulateBenchmark` tracks the warm path. -- **Next:** P4 (de-reflect array/enumerable converters) → P5 (resolve config section once per type). +- **Merged since:** **benchmark-tracking CI (#22)** — BDN on push/PR, gates PRs on **allocation** regressions (>10%) via github-action-benchmark on `gh-pages`; time informational. · **session-wrap docs (#23)** · **P3 — cached "settings plan" (#24)** — `SettingsPlan` per type (section name once+lazy, key/default/converter precomputed, `[SettingsProperty]` read once). Warm re-populate **−55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. Reviewed by code+perf agents; emitted/compiled setter reverted (regressed the gated cold scan for **no** warm gain — net10 `SetValue` is already alloc-free). New gated `PlanPopulateBenchmark` tracks the warm path. +- **In flight:** **P4 — de-reflect + DRY the array/enumerable converters** on branch `perf/p4-dereflect-converters`, ready to PR. New shared `CollectionTypeConverter` builds results via `Array.CreateInstance` + indexed fill and selects the element converter by walking the concrete `LinkedList` (struct enumerator) — no `List`+reflected `Enumerable.ToArray`, no `First` closure. `TypeConverter.CreateNullResult` de-reflected too (`Array.CreateInstance(t,0)` for `Enumerable.Empty()`). Proven via new gated `ConvertArrayBenchmark`: **1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)**. Suite **63 per TFM** (+7 collection-converter parity tests). Branch also carries the pre-P4 doc refresh + post-P3 style tweaks to `TypeConverter.cs`/`TypeExtensions.cs`. +- **Next:** P5 (resolve config section once per type) → optional P3b (tiered/lazy compiled setter, only if set *time* shows in a profile). - **C3 — DECIDED (option 2):** cache in the provider only; Core `SettingsBuilder.GetSettings` unchanged; no reload. See #17. - **Held — do NOT delete (feature work coming):** D1 Validations (reconcile with the `validate-settings` branch) · D2 EqualityCompererCreator. - Running status lives in `SESSION-HANDOFF.md`. @@ -58,7 +58,7 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - [x] P2 · Memoize `ExtractTypeProperties` + fix O(n²) dedup · Sev High · Eff S - [x] P3 · Cached “settings plan” — hoist section (lazy) + keys, precompute/cache converters, plan per type. Reflective `SetValue` kept; compiled setter deferred (regressed the gated cold scan for no warm gain) · Sev High · Eff L - [x] Q1–Q5 · Quick wins (GetEnumerator, OrdinalIgnoreCase, env-binder, type-cache; Q5 dead ctor checks already done by B4) -- [ ] P4 · De-reflect array/enumerable converters · Sev Med · Eff M +- [x] P4 · De-reflect + DRY array/enumerable converters (shared `CollectionTypeConverter`; `Array.CreateInstance` + manual converter walk; `CreateNullResult` de-reflected) — **1.33 KB→688 B, 5.7×**; branch `perf/p4-dereflect-converters` · Sev Med · Eff M - [ ] P5 · Resolve config section once per type, not per property · Sev Med · Eff M **Phase 6 — Architecture strategy** @@ -220,8 +220,9 @@ The populate loop (`src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:36-55 - **Q4** `SettingsClassGenerator.cs:37` string-keyed assembly `GetType(...Replace("+"…))` per generate → `Dictionary` cache. - **Q5** `BindingContext.cs:31-32` dead null-checks per allocation (also covered by B4). -### P4 · De-reflect array/enumerable converters — Sev Med · Eff M -`TypeConverter.cs:22-23` (empty enumerable via `Enumerable.Empty` `MakeGenericMethod().Invoke()`), `ArrayTypeConverter.cs:40,48-52` (`Activator.CreateInstance(List<>)` + reflected `ToArray`), `EnumerableTypeConverter.cs:40`. Use `Array.Empty()` factories, `Array.CreateInstance(elementType, n)` + indexed assignment, and cache any unavoidable `MethodInfo`/generic instantiations per element type. (`ArrayTypeConverter`/`EnumerableTypeConverter` are near-duplicates — DRY them here.) +### P4 · De-reflect + DRY array/enumerable converters — Sev Med · Eff M · **DONE (branch `perf/p4-dereflect-converters`)** +Was: `TypeConverter.cs` (empty enumerable via `Enumerable.Empty` `MakeGenericMethod().Invoke()`), `ArrayTypeConverter` (`Activator.CreateInstance(List<>)` + reflected `Enumerable.ToArray` `Invoke`), `EnumerableTypeConverter` (`Activator.CreateInstance(List<>)`), both selecting the element converter with LINQ `First` (boxes the `LinkedList` enumerator + a closure). +Now: a shared `CollectionTypeConverter` base implements `Convert` once — normalize the value to an array (split delimited string / passthrough / wrap scalar), select the element converter by walking the concrete `LinkedList` (struct enumerator, no boxing/closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` are now thin subclasses differing only in `CanConvert` + element-type extraction; both return `T[]` (safe — `IsEnumerable()` matches only `IEnumerable`, which a `T[]` satisfies). `CreateNullResult` uses `Array.CreateInstance(t,0)` instead of the `Enumerable.Empty()` reflection. **Proof (`ConvertArrayBenchmark`, gated): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** 7 new parity tests in `Conversion/CollectionConversionTests.cs`. Residual 688 B = irreducible split-substrings + element boxing + result array (shared by old & new). ### P5 · Resolve config section once per type — Sev Med · Eff M `ConfigurationBinder.BindPropertySettings` (`ConfigurationBinder.cs:25-33`) calls `_configuration.GetSection(...)` **per property**; the section is constant per type. Resolve the `IConfigurationSection` once per (type, section) — pass section context via the plan (P3) or cache per section string. Touches the binder/context contract. diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index dc48ed1..e9689e0 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -1,23 +1,24 @@ # SESSION HANDOFF — SimpleSettings -_Last updated: 2026-07-12 · owner: Guy Ludvig (guy@frontegg.com)_ +_Last updated: 2026-07-13 · owner: Guy Ludvig (guy@frontegg.com)_ ## TL;DR -We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The performance track is through **P2 + quick wins Q1–Q4** (merged), plus a namespace-collision fix (**M1**) and **isolated micro-benchmarks that prove the wins** (2.7×–32× on the repeated paths, allocations eliminated). A **per-push benchmark-tracking CI** that gates PRs on allocation regressions is **merged and live** (#22). **P3 — cached "settings plan" — is done + review-hardened (this PR, branch `perf/p3-compiled-settings-plan`):** warm re-populate **−55–61%** allocations (50 props 15,681→6,816 B), gated `ScanBenchmark` **≈flat (−0.4%)**. A code+perf review pass (two agents) confirmed behavior/exception parity and drove the final refinements. Suite green — **56 tests on net10.0** locally (CI runs net8.0 + net10.0). +We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The performance track is through **P3** (merged, #24) plus quick wins Q1–Q4, a benchmark-tracking CI that gates PRs on allocation regressions (#22, live), and now **P4 — de-reflect + DRY the array/enumerable converters — IMPLEMENTED on branch `perf/p4-dereflect-converters`, green, ready to commit + PR.** A new shared `CollectionTypeConverter` builds collection results via `Array.CreateInstance` + indexed fill and picks the element converter by walking the concrete `LinkedList` (struct enumerator) — no `List` + reflected `Enumerable.ToArray`, no `First` closure; `TypeConverter.CreateNullResult` de-reflected too. **Proven via the new gated `ConvertArrayBenchmark`: 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** Suite green — **63 tests on net10.0** (+7 collection-converter parity tests; CI runs net8.0 + net10.0). -Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — keep doing breaking cleanup now. +Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — keep doing breaking cleanup now. (P4's `EnumerableTypeConverter` now returning `T[]` instead of `List` is safe regardless — `IsEnumerable()` matches only `IEnumerable`, which `T[]` satisfies.) ## Do this first (new session) -1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. -2. **Merge the P3 PR** (`perf/p3-compiled-settings-plan`) if still open + green — via the `guy-lud` identity. Confirm the benchmark check passed (scan ≈ flat / −0.4%). Then `git checkout master && git pull`. -3. Start **P4** (next perf item) on a fresh work branch. **Refresh this handoff on that branch** at wrap — never a dedicated docs branch (see Gotchas). +1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. Expect `master` @ `faa48d9` (P3, #24); branch **`perf/p4-dereflect-converters`** holds the finished-but-**uncommitted** P4 work. +2. **P4 is implemented, green (63 tests net10), and proven, but NOT yet committed or PR'd.** If that's still true, the next step is: commit on the P4 branch, then push + open the PR via the **`guy-lud`** identity (see Gotchas). The commit carries the P4 source + tests + benchmark **and** the pre-P4 doc refresh + post-P3 style tweaks (`TypeConverter.cs`/`TypeExtensions.cs`) that were already in the tree. +3. After P4 merges → **P5** (resolve config section once per type) on a fresh branch. **Refresh this handoff on the work branch** at wrap — never a dedicated docs branch (see Gotchas). ## Current state -- On branch **`perf/p3-compiled-settings-plan`** (off `master` @ `6bd5b0e`) — **P3 PR open**, this session's work; warm −56% / scan +4.7%, 56 tests green. Merge it, then `master` is the base for P4. +- On branch **`perf/p4-dereflect-converters`** (off `master` @ `faa48d9`). **P4 fully implemented + tested + benchmarked, uncommitted.** **No PRs open.** Changed: new `Conversion/CollectionTypeConverter.cs` + `Conversion/CollectionConversionTests.cs`; rewritten `ArrayTypeConverter`/`EnumerableTypeConverter`; `TypeConverter.cs` (de-reflect + BOM strip); `MicroBenchmarks.cs` (+`ConvertArrayBenchmark`); doc refresh; pre-existing `TypeExtensions.cs` tweak. - A **`gh-pages`** branch was bootstrapped to hold benchmark data (`dev/bench/`); do **not** delete it — the baseline lives there (first recorded on the #22 master run). Otherwise the remote holds only **legacy / held** branches (`validate-settings`, `version-7.x`, older pre-#8 feature branches). Deleting remote branches needs the `guy-lud` push identity. ## What shipped (recent → older) -- **P3 — cached "settings plan" (this PR).** `ValuesPopulator` builds a per-type `SettingsPlan` once (cached on the populator instance): section name resolved **once and lazily** (a no-binder scan never pays for it); `[SettingsProperty]` read **once** per property then threaded into key/default/conversion; per property a `readonly struct` `PropertyPlan`/`PropertyConversion` carrying the resolved key, default, and **precomputed converter** (chosen via a manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. New gated `PlanPopulateBenchmark` tracks the warm path. **Reflective `SetValue` kept:** the emitted `__Set`/compiled-`Action` setter was built + measured but **reverted** — it regressed the gated cold scan (+25% for `__Set`) for **zero** warm gain, since net10's `PropertyInfo.SetValue` no longer allocates an args array. **Reviewed** by the dotnet code-review + perf agents (behavior/exception parity confirmed); their fixes: single attribute read, `SettingsPropertyValueException` re-wrap at plan build, binder-array materialization, `ISettingsTypeConverter` stateless/thread-safe doc. Follow-ups in `FIX-PLAN.md`: **P3b** (tiered/lazy setter, only if set *time* matters) and the binder `CreateKey` string-concat (pre-existing, ~⅓ of the warm number). +- **P4 — de-reflect + DRY the array/enumerable converters (branch `perf/p4-dereflect-converters`, uncommitted).** New abstract `Conversion/CollectionTypeConverter` owns the shared `Convert`: normalize the incoming value to an array (split a delimited string / passthrough an existing array / wrap a scalar), select the element converter by a **manual walk over the concrete `LinkedList`** (struct enumerator — no boxed enumerator, no predicate closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` collapse to thin subclasses (only `CanConvert` + element-type extraction differ); both now return `T[]`. Gone: the `List` + its backing array, the reflected `Enumerable.ToArray` (`MakeGenericMethod`+`Invoke`+args array), and the `First` closure. `TypeConverter.CreateNullResult` swaps the `Enumerable.Empty()` reflection for `Array.CreateInstance(t,0)`, and its `GetConverter` is now a true manual walk (matching its own comment). **Proof — new gated `ConvertArrayBenchmark` (isolates the hot path like Q1/Q3/Q4): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)** (before measured by swapping master's old converter back in). Residual 688 B is the split-substrings + per-element boxing + result array shared by both versions. **7 new parity tests** (`Conversion/CollectionConversionTests.cs`): delimited→`int[]`/`string[]`/`IEnumerable`, empty-entry removal, custom delimiter, default-array passthrough, and that the enumerable path now materializes a `T[]`. Suite **63 net10** (was 56). +- **P3 — cached "settings plan" (#24, merged `faa48d9`).** `ValuesPopulator` builds a per-type `SettingsPlan` once (cached on the populator instance): section name resolved **once and lazily** (a no-binder scan never pays for it); `[SettingsProperty]` read **once** per property then threaded into key/default/conversion; per property a `readonly struct` `PropertyPlan`/`PropertyConversion` carrying the resolved key, default, and **precomputed converter** (chosen via a manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. New gated `PlanPopulateBenchmark` tracks the warm path. **Reflective `SetValue` kept:** the emitted `__Set`/compiled-`Action` setter was built + measured but **reverted** — it regressed the gated cold scan (+25% for `__Set`) for **zero** warm gain, since net10's `PropertyInfo.SetValue` no longer allocates an args array. **Reviewed** by the dotnet code-review + perf agents (behavior/exception parity confirmed); their fixes: single attribute read, `SettingsPropertyValueException` re-wrap at plan build, binder-array materialization, `ISettingsTypeConverter` stateless/thread-safe doc. Follow-ups in `FIX-PLAN.md`: **P3b** (tiered/lazy setter, only if set *time* matters) and the binder `CreateKey` string-concat (pre-existing, ~⅓ of the warm number). - **#23 — session-wrap docs.** Handoff + fix-plan refresh; gitignored BenchmarkDotNet output + the personal `.claude/settings.local.json` (SessionStart hook). Squash-merged onto `master`. - **#22 — benchmark-tracking CI merged.** (Detail below.) First master run recorded the allocation baseline on `gh-pages`. - **#21 — Perf quick wins Q1–Q4 + M1 fix + micro-benchmarks.** @@ -40,8 +41,8 @@ Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking chan - **Pre-stable window:** no `v*` stable tag (the `version-*` tags are the dead legacy package). Breaking changes free until the first `v2.0.0-beta`. ## Next priorities (ranked — detail in FIX-PLAN.md) -1. **Perf:** **P4** — de-reflect the array/enumerable converters (`Array.Empty`/`Array.CreateInstance` + indexed assignment; DRY the near-duplicate `ArrayTypeConverter`/`EnumerableTypeConverter`), then **P5** — resolve the config section once per type (pass section context via the P3 plan, which already resolves the section name lazily). Optional **P3b**: tiered/lazy compiled setter (only if set *time* shows up in a profile — allocation is already handled). -2. **Engine tests:** T4 `ValuesPopulator`, T5 `TypeConverter`, T6 converters, T7 generator concurrency stress — the unsynchronized check-then-`DefineType` in `GenerateType` is **still open** (Q4's `ConcurrentDictionary` made the cache thread-safe but did not close that race). +1. **Perf:** **P4 is done (branch, uncommitted — commit + PR it first).** Then **P5** — resolve the config section once per type (pass section context via the P3 plan, which already resolves the section name lazily). Optional **P3b**: tiered/lazy compiled setter (only if set *time* shows up in a profile — allocation is already handled). +2. **Engine tests:** T4 `ValuesPopulator`, T5 `TypeConverter`, **T6 converters — partially done in P4** (array/enumerable path covered by `CollectionConversionTests`; DateTime/Uri/Enum/Default still thin), T7 generator concurrency stress — the unsynchronized check-then-`DefineType` in `GenerateType` is **still open** (Q4's `ConcurrentDictionary` made the cache thread-safe but did not close that race). 3. **Architecture:** A1 (AOT/trim annotations — HIGH, `Reflection.Emit` lib), C1 (`List`/`IList` support), C2 (public `SimpleSettingsException` base), A3 (`Core.AspNet` public type or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM), A5 (make `SettingsHolder` internal), A6 (command-line quoted-arg parsing). 4. **README** links — the `docs/` tutorials were done in #20; the README may still have stale `existall/SimpleConfig` links. 5. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch. diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/ArrayTypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/ArrayTypeConverter.cs index 78c4d11..112dbe1 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/ArrayTypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/ArrayTypeConverter.cs @@ -1,57 +1,22 @@ -using System; -using System.Collections; -using System.Collections.Generic; -using System.Linq; -using System.Reflection; +using System; namespace ExistForAll.SimpleSettings.Conversion { - internal class ArrayTypeConverter : ISettingsTypeConverter - { - private readonly SettingsOptions _settingsOptions; - private readonly TypeConvertersCollections _converters; - - public ArrayTypeConverter(SettingsOptions settingsOptions, TypeConvertersCollections converters) - { - _settingsOptions = settingsOptions; - _converters = converters; - } - - public bool CanConvert(Type settingsType) - { - return settingsType.IsArray; - } - - public object Convert(object value, Type settingsType) - { - if (value is string stringArray) - { - value = stringArray.Split(new[] {_settingsOptions.ArraySplitDelimiter}, - StringSplitOptions.RemoveEmptyEntries) - .ToArray(); - } - - var values = value.GetType().IsArray ? (IEnumerable) value : new[] {value}; - - var elementType = settingsType.GetElementType()!; - - var settingsTypeConverter = _converters.First(x => x.CanConvert(elementType)); - - var list = (IList) Activator.CreateInstance(typeof(List<>).MakeGenericType(elementType))!; - - foreach (var item in values) - { - var convertedValue = settingsTypeConverter.Convert(item, elementType); - list.Add(convertedValue); - } - - var toArray = typeof(Enumerable).GetTypeInfo() - .GetMethod("ToArray")! - .MakeGenericMethod(elementType); - - var array = toArray.Invoke(null, new object[] {list}); - - return array!; - } - } -} \ No newline at end of file + internal class ArrayTypeConverter : CollectionTypeConverter + { + public ArrayTypeConverter(SettingsOptions settingsOptions, TypeConvertersCollections converters) + : base(settingsOptions, converters) + { + } + + public override bool CanConvert(Type settingsType) + { + return settingsType.IsArray; + } + + protected override Type GetElementType(Type settingsType) + { + return settingsType.GetElementType()!; + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs new file mode 100644 index 0000000..79817c9 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs @@ -0,0 +1,71 @@ +using System; + +namespace ExistForAll.SimpleSettings.Conversion +{ + // Shared conversion for the two collection shapes — arrays and IEnumerable properties. Both split a + // delimited string (or wrap a scalar), convert each element with the element type's converter, and + // materialize a typed array. Building the array directly (Array.CreateInstance + indexed set) avoids the + // List + reflected Enumerable.ToArray round-trip the two converters used to share, and the element + // converter is selected by walking the concrete LinkedList (struct enumerator, no boxing) rather than + // LINQ First (which boxes the enumerator and allocates a closure) — this runs on every populate. + internal abstract class CollectionTypeConverter : ISettingsTypeConverter + { + private readonly SettingsOptions _settingsOptions; + private readonly TypeConvertersCollections _converters; + + protected CollectionTypeConverter(SettingsOptions settingsOptions, TypeConvertersCollections converters) + { + _settingsOptions = settingsOptions; + _converters = converters; + } + + public abstract bool CanConvert(Type settingsType); + + // The element type to convert each item to: the array's element type, or the IEnumerable argument. + protected abstract Type GetElementType(Type settingsType); + + public object Convert(object value, Type settingsType) + { + var elementType = GetElementType(settingsType); + var source = AsArray(value); + var elementConverter = GetElementConverter(elementType); + + var result = Array.CreateInstance(elementType, source.Length); + for (var i = 0; i < source.Length; i++) + { + result.SetValue(elementConverter.Convert(source.GetValue(i)!, elementType), i); + } + + return result; + } + + // Normalize the incoming value to an array we can size and index: split a delimited string, pass an + // existing array straight through, or wrap a lone scalar. All three branches already produced an array + // in the previous per-converter code. + private Array AsArray(object value) + { + if (value is string text) + { + return text.Split(new[] { _settingsOptions.ArraySplitDelimiter }, StringSplitOptions.RemoveEmptyEntries); + } + + return value is Array array ? array : new[] { value }; + } + + private ISettingsTypeConverter GetElementConverter(Type elementType) + { + // Manual walk over the concrete LinkedList (not LINQ First) so the struct enumerator isn't boxed + // onto the heap and no predicate closure is allocated — this runs per element-typed collection on + // every populate. + foreach (var converter in _converters) + { + if (converter.CanConvert(elementType)) + return converter; + } + + // Unreachable in practice: DefaultTypeConverter.CanConvert always returns true. Kept so every path + // returns and to mirror the old First(...) which also threw when nothing matched. + throw new InvalidOperationException($"No converter found for type '{elementType}'."); + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/EnumerableTypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/EnumerableTypeConverter.cs index 539bb73..bb3e4b4 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/EnumerableTypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/EnumerableTypeConverter.cs @@ -1,51 +1,24 @@ -using System; -using System.Collections; -using System.Collections.Generic; -using System.Linq; -using System.Reflection; -using ExistForAll.SimpleSettings.Core.Reflection; - -namespace ExistForAll.SimpleSettings.Conversion -{ - internal class EnumerableTypeConverter : ISettingsTypeConverter - { - private readonly SettingsOptions _settingsOptions; - private readonly TypeConvertersCollections _converters; - - public EnumerableTypeConverter(SettingsOptions settingsOptions, TypeConvertersCollections converters) - { - _settingsOptions = settingsOptions; - _converters = converters; - } - - public bool CanConvert(Type settingsType) - { - return settingsType.IsEnumerable(); - } - - public object Convert(object value, Type settingsType) - { - if (value is string stringArray) - { - value = stringArray.Split(new[] { _settingsOptions.ArraySplitDelimiter }, StringSplitOptions.RemoveEmptyEntries) - .ToArray(); - } - - var values = value.GetType().IsArray ? (IEnumerable)value : new[] { value }; - - var elementType = settingsType.GetTypeInfo().GetGenericArguments().First(); - - var configTypeConverter = _converters.First(x => x.CanConvert(elementType)); - - var instance = (IList)Activator.CreateInstance(typeof(List<>).MakeGenericType(elementType))!; - - foreach (var item in values) - { - var convertedValue = configTypeConverter.Convert(item, elementType); - instance.Add(convertedValue); - } - - return instance; - } - } -} \ No newline at end of file +using System; +using System.Reflection; +using ExistForAll.SimpleSettings.Core.Reflection; + +namespace ExistForAll.SimpleSettings.Conversion +{ + internal class EnumerableTypeConverter : CollectionTypeConverter + { + public EnumerableTypeConverter(SettingsOptions settingsOptions, TypeConvertersCollections converters) + : base(settingsOptions, converters) + { + } + + public override bool CanConvert(Type settingsType) + { + return settingsType.IsEnumerable(); + } + + protected override Type GetElementType(Type settingsType) + { + return settingsType.GetTypeInfo().GetGenericArguments()[0]; + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs index aa25138..464661e 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs @@ -1,5 +1,4 @@ using System; -using System.Linq; using System.Reflection; using ExistForAll.SimpleSettings.Conversion; @@ -15,7 +14,7 @@ public PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsPr { var propertyType = propertyInfo.PropertyType; - var throwOnNull = attribute != null && !attribute.AllowEmpty; + var throwOnNull = attribute is { AllowEmpty: false }; var nullResult = CreateNullResult(propertyType); var strippedType = StripIfNullable(propertyType); @@ -31,9 +30,10 @@ public PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsPr if (!propertyType.IsEnumerable()) return propertyType.GetTypeInfo().IsValueType ? Activator.CreateInstance(propertyType) : null; - var genericType = propertyType.GetTypeInfo().GetGenericArguments().First(); - var method = typeof(Enumerable).GetTypeInfo().GetMethod("Empty")!.MakeGenericMethod(genericType); - return method.Invoke(null, null); + // An empty IEnumerable is just an empty T[] (arrays implement IEnumerable). Array.CreateInstance + // replaces the old Enumerable.Empty() built via GetMethod("Empty").MakeGenericMethod().Invoke(). + var elementType = propertyType.GetTypeInfo().GetGenericArguments()[0]; + return Array.CreateInstance(elementType, 0); } private static ISettingsTypeConverter GetConverter(Type strippedType, @@ -43,8 +43,9 @@ private static ISettingsTypeConverter GetConverter(Type strippedType, if (attribute?.ConverterType != null) return (ISettingsTypeConverter)Activator.CreateInstance(attribute.ConverterType)!; - // Manual walk over the concrete LinkedList (not LINQ First) so the struct enumerator isn't boxed - // onto the heap — this runs once per property at plan build. + // Manual walk over the concrete LinkedList (not LINQ First/Where) so the struct enumerator isn't + // boxed onto the heap and no predicate closure is allocated — this runs once per property at plan + // build. foreach (var converter in options.Converters) { if (converter.CanConvert(strippedType)) diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs index c07f573..d0a0b9a 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs @@ -8,7 +8,7 @@ internal static class TypeExtensions { public static string GetNormalizeInterfaceName(this Type target) { - return target.Name[0] == 'I' ? target.Name.Substring(1) : target.Name; + return target.Name[0] == 'I' ? target.Name[1..] : target.Name; } public static string GetSectionName(this Type settingsClass, SettingsOptions options) diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs new file mode 100644 index 0000000..42aa508 --- /dev/null +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs @@ -0,0 +1,116 @@ +using System.Collections.Generic; +using System.Linq; +using ExistForAll.SimpleSettings.Binder; +using ExistForAll.SimpleSettings.Binders; + +namespace ExistForAll.SimpleSettings.UnitTests.Conversion +{ + // Exercises the array + IEnumerable converter path end-to-end (bound delimited string -> collection), + // which the P4 de-reflection rewrite shares behind CollectionTypeConverter. Locks the observable behavior: + // element values, ordering, empty-entry handling, the custom delimiter, and that an IEnumerable property + // is satisfied by the materialized array. + public class CollectionConversionTests + { + [Test] + public async Task Convert_DelimitedString_ToIntArray_ParsesEachElement() + { + var result = Build("IntArray", nameof(IIntArray.Values), "1,2,3,4"); + + await Assert.That(result.Values.SequenceEqual(new[] { 1, 2, 3, 4 })).IsTrue(); + } + + [Test] + public async Task Convert_DelimitedString_ToStringArray_KeepsOrder() + { + var result = Build("StringArray", nameof(IStringArray.Values), "alpha,beta,gamma"); + + await Assert.That(result.Values.SequenceEqual(new[] { "alpha", "beta", "gamma" })).IsTrue(); + } + + [Test] + public async Task Convert_DelimitedString_ToIntEnumerable_YieldsElements() + { + var result = Build("IntEnumerable", nameof(IIntEnumerable.Values), "5,6,7"); + + await Assert.That(result.Values.Count()).IsEqualTo(3); + await Assert.That(result.Values).Contains(5); + await Assert.That(result.Values).Contains(6); + await Assert.That(result.Values).Contains(7); + } + + [Test] + public async Task Convert_ToIntEnumerable_MaterializesAnArray() + { + // The IEnumerable converter now returns a T[] (assignable to IEnumerable) instead of a + // List; this pins that deliberate P4 behavior so a regression to List is caught. + var result = Build("IntEnumerable", nameof(IIntEnumerable.Values), "5,6,7"); + + await Assert.That(result.Values is int[]).IsTrue(); + } + + [Test] + public async Task Convert_DelimitedString_RemovesEmptyEntries() + { + var result = Build("IntArray", nameof(IIntArray.Values), "1,,2,"); + + await Assert.That(result.Values.SequenceEqual(new[] { 1, 2 })).IsTrue(); + } + + [Test] + public async Task Convert_WithCustomDelimiter_SplitsOnThatDelimiter() + { + var result = Build("IntArray", nameof(IIntArray.Values), "10;20;30", delimiter: ";"); + + await Assert.That(result.Values.SequenceEqual(new[] { 10, 20, 30 })).IsTrue(); + } + + [Test] + public async Task Convert_DefaultArray_IsPassedThrough() + { + // No binder: the [SettingsProperty] default array flows straight through the converter unchanged. + var sut = SettingsBuilder.CreateBuilder(); + + var result = sut.GetSettings(); + + await Assert.That(result.Values.SequenceEqual(new[] { 7, 8, 9 })).IsTrue(); + } + + private static T Build(string section, string key, string value, string? delimiter = null) + where T : class + { + var collection = new InMemoryCollection(); + collection.Add(section, key, value); + + var builder = SettingsBuilder.CreateBuilder(x => + { + if (delimiter != null) + x.SetArraySplitDelimiter(delimiter); + + x.AddSectionBinder(new InMemoryBinder(collection)); + }); + + return builder.GetSettings(); + } + + public interface IIntArray + { + int[] Values { get; set; } + } + + public interface IStringArray + { + string[] Values { get; set; } + } + + public interface IIntEnumerable + { + IEnumerable Values { get; set; } + } + + public interface IDefaultIntArray + { + [SettingsProperty(DefaultValue = new[] { 7, 8, 9 })] + int[] Values { get; set; } + } + } +} diff --git a/src/performance/ExistForAll.SimpleSettings.Benchmark/MicroBenchmarks.cs b/src/performance/ExistForAll.SimpleSettings.Benchmark/MicroBenchmarks.cs index b6610ec..5f0d8a1 100644 --- a/src/performance/ExistForAll.SimpleSettings.Benchmark/MicroBenchmarks.cs +++ b/src/performance/ExistForAll.SimpleSettings.Benchmark/MicroBenchmarks.cs @@ -2,6 +2,7 @@ using System.Reflection; using BenchmarkDotNet.Attributes; using ExistForAll.SimpleSettings.Binders; +using ExistForAll.SimpleSettings.Conversion; using ExistForAll.SimpleSettings.Core.Reflection; namespace ExistForAll.SimpleSettings.Benchmark @@ -84,4 +85,32 @@ public void Setup() [Benchmark] public Type GenerateWarm() => _generator.GenerateType(typeof(IProps50)); } + + /// + /// P4 — ArrayTypeConverter.Convert, the collection path now shared by the array and IEnumerable<T> + /// converters. The old code selected the element converter with LINQ First (boxes the + /// LinkedList enumerator + a predicate closure), built a List<T> via + /// Activator.CreateInstance(typeof(List<>)…), then produced the array with a reflected + /// Enumerable.ToArray (MakeGenericMethod + Invoke + an args array). The new code walks + /// the concrete LinkedList (struct enumerator, no boxing) and fills an Array.CreateInstance + /// directly. A ten-element delimited string exercises split + per-element convert + build; the converter is + /// built once in setup so only Convert is measured. + /// + [MemoryDiagnoser] + public class ConvertArrayBenchmark + { + private const string Value = "1,2,3,4,5,6,7,8,9,10"; + + private ISettingsTypeConverter _converter = null!; + + [GlobalSetup] + public void Setup() + { + var options = new SettingsOptions(); + _converter = new ArrayTypeConverter(options, options.Converters); + } + + [Benchmark] + public object ConvertArray() => _converter.Convert(Value, typeof(int[])); + } } From 5b8e1c5460f9e5e630f2a99047be0c696342538a Mon Sep 17 00:00:00 2001 From: Guy Ludvig Date: Mon, 13 Jul 2026 12:01:40 +0300 Subject: [PATCH 2/4] Harden P4 converter tests per code review Adds 5 collection-converter tests the dotnet code-reviewer suggested: - CreateNullResult null path: an unbound IEnumerable with no default now yields an empty T[] -- the one line P4 changed in TypeConverter.cs that no existing test exercised (all others bind a value or supply a default). - Element-converter parity: DayOfWeek[], DateTime[], Uri[] (the shipped tests only covered int/string, both routed to DefaultTypeConverter). - Negative: a non-numeric element for an int[] surfaces as the expected SettingsPropertyValueException, pinning the exception-wrapping contract. Suite: 68 per TFM (was 63). No production changes. Refreshes SESSION-HANDOFF.md + FIX-PLAN.md (counts, PR #25, code-review outcome). --- FIX-PLAN.md | 4 +- SESSION-HANDOFF.md | 12 ++-- .../Conversion/CollectionConversionTests.cs | 68 +++++++++++++++++++ 3 files changed, 76 insertions(+), 8 deletions(-) diff --git a/FIX-PLAN.md b/FIX-PLAN.md index 630d706..f390b46 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -6,7 +6,7 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - **Done & merged:** B1, B2, B4, B5, B9 + T1, T2 (PR #8) · BindingContext test (#10) · D3 namespace typo (#11) · T3 DI integration tests (#12) · solution rename (#13) · **A2 naming → ExistForAll (#15)** · **P0 benchmark harness (#16)** · **P1 provider cache + C3 decided/implemented (#17)** · **P2 memoize `ExtractTypeProperties` + `HashSet` dedup (#18)** · **docs tutorials refresh (#20)** · **Q1–Q4 perf quick wins + M1 collision fix + micro-benchmarks (#21)**. - **Q1–Q4 proven** via isolated micro-benchmarks (macro `ScanBenchmark` can't resolve them): Q1 2.7× / 64 KB→88 B · Q3 2.65× / 152 B→0 · Q4 32× / 224 B→0. **Q5 was already resolved by B4.** **M1** (code-review finding): namespace-qualify the generated impl name in the generator only — `GetNormalizeInterfaceName` also backs the section name. Suite → **56 per TFM**. - **Merged since:** **benchmark-tracking CI (#22)** — BDN on push/PR, gates PRs on **allocation** regressions (>10%) via github-action-benchmark on `gh-pages`; time informational. · **session-wrap docs (#23)** · **P3 — cached "settings plan" (#24)** — `SettingsPlan` per type (section name once+lazy, key/default/converter precomputed, `[SettingsProperty]` read once). Warm re-populate **−55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. Reviewed by code+perf agents; emitted/compiled setter reverted (regressed the gated cold scan for **no** warm gain — net10 `SetValue` is already alloc-free). New gated `PlanPopulateBenchmark` tracks the warm path. -- **In flight:** **P4 — de-reflect + DRY the array/enumerable converters** on branch `perf/p4-dereflect-converters`, ready to PR. New shared `CollectionTypeConverter` builds results via `Array.CreateInstance` + indexed fill and selects the element converter by walking the concrete `LinkedList` (struct enumerator) — no `List`+reflected `Enumerable.ToArray`, no `First` closure. `TypeConverter.CreateNullResult` de-reflected too (`Array.CreateInstance(t,0)` for `Enumerable.Empty()`). Proven via new gated `ConvertArrayBenchmark`: **1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)**. Suite **63 per TFM** (+7 collection-converter parity tests). Branch also carries the pre-P4 doc refresh + post-P3 style tweaks to `TypeConverter.cs`/`TypeExtensions.cs`. +- **In flight:** **P4 — de-reflect + DRY the array/enumerable converters** = **PR #25 open, code-reviewed clean** (branch `perf/p4-dereflect-converters`; awaiting CI + merge). New shared `CollectionTypeConverter` builds results via `Array.CreateInstance` + indexed fill and selects the element converter by walking the concrete `LinkedList` (struct enumerator) — no `List`+reflected `Enumerable.ToArray`, no `First` closure. `TypeConverter.CreateNullResult` de-reflected too (`Array.CreateInstance(t,0)` for `Enumerable.Empty()`). Proven via new gated `ConvertArrayBenchmark`: **1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)**. Suite **68 per TFM** (+12 collection-converter parity tests). Branch also carries the pre-P4 doc refresh + post-P3 style tweaks to `TypeConverter.cs`/`TypeExtensions.cs`. - **Next:** P5 (resolve config section once per type) → optional P3b (tiered/lazy compiled setter, only if set *time* shows in a profile). - **C3 — DECIDED (option 2):** cache in the provider only; Core `SettingsBuilder.GetSettings` unchanged; no reload. See #17. - **Held — do NOT delete (feature work coming):** D1 Validations (reconcile with the `validate-settings` branch) · D2 EqualityCompererCreator. @@ -222,7 +222,7 @@ The populate loop (`src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:36-55 ### P4 · De-reflect + DRY array/enumerable converters — Sev Med · Eff M · **DONE (branch `perf/p4-dereflect-converters`)** Was: `TypeConverter.cs` (empty enumerable via `Enumerable.Empty` `MakeGenericMethod().Invoke()`), `ArrayTypeConverter` (`Activator.CreateInstance(List<>)` + reflected `Enumerable.ToArray` `Invoke`), `EnumerableTypeConverter` (`Activator.CreateInstance(List<>)`), both selecting the element converter with LINQ `First` (boxes the `LinkedList` enumerator + a closure). -Now: a shared `CollectionTypeConverter` base implements `Convert` once — normalize the value to an array (split delimited string / passthrough / wrap scalar), select the element converter by walking the concrete `LinkedList` (struct enumerator, no boxing/closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` are now thin subclasses differing only in `CanConvert` + element-type extraction; both return `T[]` (safe — `IsEnumerable()` matches only `IEnumerable`, which a `T[]` satisfies). `CreateNullResult` uses `Array.CreateInstance(t,0)` instead of the `Enumerable.Empty()` reflection. **Proof (`ConvertArrayBenchmark`, gated): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** 7 new parity tests in `Conversion/CollectionConversionTests.cs`. Residual 688 B = irreducible split-substrings + element boxing + result array (shared by old & new). +Now: a shared `CollectionTypeConverter` base implements `Convert` once — normalize the value to an array (split delimited string / passthrough / wrap scalar), select the element converter by walking the concrete `LinkedList` (struct enumerator, no boxing/closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` are now thin subclasses differing only in `CanConvert` + element-type extraction; both return `T[]` (safe — `IsEnumerable()` matches only `IEnumerable`, which a `T[]` satisfies). `CreateNullResult` uses `Array.CreateInstance(t,0)` instead of the `Enumerable.Empty()` reflection. **Proof (`ConvertArrayBenchmark`, gated): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** 12 parity tests in `Conversion/CollectionConversionTests.cs` (int/string/enum/DateTime/Uri elements, empty-entry removal, custom delimiter, default passthrough, unbound→empty `T[]`, `T[]`-not-`List` guard, bad-element negative). Residual 688 B = irreducible split-substrings + element boxing + result array (shared by old & new). **Code-reviewed clean** (dotnet code-reviewer verified all parity claims; the enum/DateTime/Uri + null-path + negative tests were its suggestions — partially closes T6). ### P5 · Resolve config section once per type — Sev Med · Eff M `ConfigurationBinder.BindPropertySettings` (`ConfigurationBinder.cs:25-33`) calls `_configuration.GetSection(...)` **per property**; the section is constant per type. Resolve the `IConfigurationSection` once per (type, section) — pass section context via the plan (P3) or cache per section string. Touches the binder/context contract. diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index e9689e0..1213f1d 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -3,21 +3,21 @@ _Last updated: 2026-07-13 · owner: Guy Ludvig (guy@frontegg.com)_ ## TL;DR -We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The performance track is through **P3** (merged, #24) plus quick wins Q1–Q4, a benchmark-tracking CI that gates PRs on allocation regressions (#22, live), and now **P4 — de-reflect + DRY the array/enumerable converters — IMPLEMENTED on branch `perf/p4-dereflect-converters`, green, ready to commit + PR.** A new shared `CollectionTypeConverter` builds collection results via `Array.CreateInstance` + indexed fill and picks the element converter by walking the concrete `LinkedList` (struct enumerator) — no `List` + reflected `Enumerable.ToArray`, no `First` closure; `TypeConverter.CreateNullResult` de-reflected too. **Proven via the new gated `ConvertArrayBenchmark`: 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** Suite green — **63 tests on net10.0** (+7 collection-converter parity tests; CI runs net8.0 + net10.0). +We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The performance track is through **P3** (merged, #24) plus quick wins Q1–Q4, a benchmark-tracking CI that gates PRs on allocation regressions (#22, live), and now **P4 — de-reflect + DRY the array/enumerable converters — SHIPPED as [PR #25](https://github.com/existall/SimpleSettings/pull/25)** (open, base `master`; **reviewed clean by the dotnet code-reviewer** — no correctness/security/concurrency/parity defects). A new shared `CollectionTypeConverter` builds collection results via `Array.CreateInstance` + indexed fill and picks the element converter by walking the concrete `LinkedList` (struct enumerator) — no `List` + reflected `Enumerable.ToArray`, no `First` closure; `TypeConverter.CreateNullResult` de-reflected too. **Proven via the new gated `ConvertArrayBenchmark`: 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** Suite green — **68 tests on net10.0** (+12 collection-converter parity tests; CI runs net8.0 + net10.0). Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — keep doing breaking cleanup now. (P4's `EnumerableTypeConverter` now returning `T[]` instead of `List` is safe regardless — `IsEnumerable()` matches only `IEnumerable`, which `T[]` satisfies.) ## Do this first (new session) -1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. Expect `master` @ `faa48d9` (P3, #24); branch **`perf/p4-dereflect-converters`** holds the finished-but-**uncommitted** P4 work. -2. **P4 is implemented, green (63 tests net10), and proven, but NOT yet committed or PR'd.** If that's still true, the next step is: commit on the P4 branch, then push + open the PR via the **`guy-lud`** identity (see Gotchas). The commit carries the P4 source + tests + benchmark **and** the pre-P4 doc refresh + post-P3 style tweaks (`TypeConverter.cs`/`TypeExtensions.cs`) that were already in the tree. -3. After P4 merges → **P5** (resolve config section once per type) on a fresh branch. **Refresh this handoff on the work branch** at wrap — never a dedicated docs branch (see Gotchas). +1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. Expect `master` @ `faa48d9` (P3, #24) and **[PR #25](https://github.com/existall/SimpleSettings/pull/25) open** (P4, branch `perf/p4-dereflect-converters`). +2. **P4 is committed, pushed, PR'd (#25), and code-reviewed clean** — the only open item is watching CI (`build-test` + the `benchmark` allocation gate) and **merging #25** once green (merge via the `guy-lud` identity — see Gotchas). If CI is already green, merge it. +3. After #25 merges → **P5** (resolve config section once per type) on a fresh branch. Per the workflow the user set (see project memory `[[dotnet-review-workflow]]`): **plan first, review the plan with the `dotnet-architect` / `performance-analyst` / `security-auditor` agents, implement, then review the diff with `code-reviewer`.** **Refresh this handoff on the work branch** at wrap — never a dedicated docs branch (see Gotchas). ## Current state -- On branch **`perf/p4-dereflect-converters`** (off `master` @ `faa48d9`). **P4 fully implemented + tested + benchmarked, uncommitted.** **No PRs open.** Changed: new `Conversion/CollectionTypeConverter.cs` + `Conversion/CollectionConversionTests.cs`; rewritten `ArrayTypeConverter`/`EnumerableTypeConverter`; `TypeConverter.cs` (de-reflect + BOM strip); `MicroBenchmarks.cs` (+`ConvertArrayBenchmark`); doc refresh; pre-existing `TypeExtensions.cs` tweak. +- On branch **`perf/p4-dereflect-converters`** (off `master` @ `faa48d9`), commit `6d5bfbe` + a follow-up test-hardening commit. **P4 pushed and open as PR #25**, reviewed clean by the code-reviewer. Files: new `Conversion/CollectionTypeConverter.cs` + `Conversion/CollectionConversionTests.cs` (12 tests); rewritten `ArrayTypeConverter`/`EnumerableTypeConverter`; `TypeConverter.cs` (de-reflect + BOM strip); `MicroBenchmarks.cs` (+`ConvertArrayBenchmark`); doc refresh; pre-existing `TypeExtensions.cs` tweak. - A **`gh-pages`** branch was bootstrapped to hold benchmark data (`dev/bench/`); do **not** delete it — the baseline lives there (first recorded on the #22 master run). Otherwise the remote holds only **legacy / held** branches (`validate-settings`, `version-7.x`, older pre-#8 feature branches). Deleting remote branches needs the `guy-lud` push identity. ## What shipped (recent → older) -- **P4 — de-reflect + DRY the array/enumerable converters (branch `perf/p4-dereflect-converters`, uncommitted).** New abstract `Conversion/CollectionTypeConverter` owns the shared `Convert`: normalize the incoming value to an array (split a delimited string / passthrough an existing array / wrap a scalar), select the element converter by a **manual walk over the concrete `LinkedList`** (struct enumerator — no boxed enumerator, no predicate closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` collapse to thin subclasses (only `CanConvert` + element-type extraction differ); both now return `T[]`. Gone: the `List` + its backing array, the reflected `Enumerable.ToArray` (`MakeGenericMethod`+`Invoke`+args array), and the `First` closure. `TypeConverter.CreateNullResult` swaps the `Enumerable.Empty()` reflection for `Array.CreateInstance(t,0)`, and its `GetConverter` is now a true manual walk (matching its own comment). **Proof — new gated `ConvertArrayBenchmark` (isolates the hot path like Q1/Q3/Q4): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)** (before measured by swapping master's old converter back in). Residual 688 B is the split-substrings + per-element boxing + result array shared by both versions. **7 new parity tests** (`Conversion/CollectionConversionTests.cs`): delimited→`int[]`/`string[]`/`IEnumerable`, empty-entry removal, custom delimiter, default-array passthrough, and that the enumerable path now materializes a `T[]`. Suite **63 net10** (was 56). +- **P4 — de-reflect + DRY the array/enumerable converters ([PR #25](https://github.com/existall/SimpleSettings/pull/25), open, code-reviewed clean).** New abstract `Conversion/CollectionTypeConverter` owns the shared `Convert`: normalize the incoming value to an array (split a delimited string / passthrough an existing array / wrap a scalar), select the element converter by a **manual walk over the concrete `LinkedList`** (struct enumerator — no boxed enumerator, no predicate closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` collapse to thin subclasses (only `CanConvert` + element-type extraction differ); both now return `T[]`. Gone: the `List` + its backing array, the reflected `Enumerable.ToArray` (`MakeGenericMethod`+`Invoke`+args array), and the `First` closure. `TypeConverter.CreateNullResult` swaps the `Enumerable.Empty()` reflection for `Array.CreateInstance(t,0)`, and its `GetConverter` is now a true manual walk (matching its own comment). **Proof — new gated `ConvertArrayBenchmark` (isolates the hot path like Q1/Q3/Q4): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)** (before measured by swapping master's old converter back in). Residual 688 B is the split-substrings + per-element boxing + result array shared by both versions. **12 parity tests** (`Conversion/CollectionConversionTests.cs`): delimited→`int[]`/`string[]`/`IEnumerable`/`DayOfWeek[]`/`DateTime[]`/`Uri[]`, empty-entry removal, custom delimiter, default-array passthrough, unbound-no-default→empty `T[]` (the `CreateNullResult` line), the enumerable-path-materializes-`T[]` guard, and a bad-element→`SettingsPropertyValueException` negative. Suite **68 net10** (was 56). **Code-reviewed** by the dotnet code-reviewer (the last 5 tests were its suggestions): all 6 adversarial parity claims verified, no defects. - **P3 — cached "settings plan" (#24, merged `faa48d9`).** `ValuesPopulator` builds a per-type `SettingsPlan` once (cached on the populator instance): section name resolved **once and lazily** (a no-binder scan never pays for it); `[SettingsProperty]` read **once** per property then threaded into key/default/conversion; per property a `readonly struct` `PropertyPlan`/`PropertyConversion` carrying the resolved key, default, and **precomputed converter** (chosen via a manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. New gated `PlanPopulateBenchmark` tracks the warm path. **Reflective `SetValue` kept:** the emitted `__Set`/compiled-`Action` setter was built + measured but **reverted** — it regressed the gated cold scan (+25% for `__Set`) for **zero** warm gain, since net10's `PropertyInfo.SetValue` no longer allocates an args array. **Reviewed** by the dotnet code-review + perf agents (behavior/exception parity confirmed); their fixes: single attribute read, `SettingsPropertyValueException` re-wrap at plan build, binder-array materialization, `ISettingsTypeConverter` stateless/thread-safe doc. Follow-ups in `FIX-PLAN.md`: **P3b** (tiered/lazy setter, only if set *time* matters) and the binder `CreateKey` string-concat (pre-existing, ~⅓ of the warm number). - **#23 — session-wrap docs.** Handoff + fix-plan refresh; gitignored BenchmarkDotNet output + the personal `.claude/settings.local.json` (SessionStart hook). Squash-merged onto `master`. - **#22 — benchmark-tracking CI merged.** (Detail below.) First master run recorded the allocation baseline on `gh-pages`. diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs index 42aa508..fb01135 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Linq; using ExistForAll.SimpleSettings.Binder; @@ -75,6 +76,58 @@ public async Task Convert_DefaultArray_IsPassedThrough() await Assert.That(result.Values.SequenceEqual(new[] { 7, 8, 9 })).IsTrue(); } + [Test] + public async Task Convert_UnboundEnumerable_NoDefault_YieldsEmptyArray() + { + // No binder and no default => the value stays null and PropertyConversion returns the precomputed + // null result. This is the exact line P4 changed in TypeConverter.CreateNullResult: an empty + // IEnumerable is now Array.CreateInstance(elementType, 0) instead of Enumerable.Empty(). + var sut = SettingsBuilder.CreateBuilder(); + + var result = sut.GetSettings(); + + await Assert.That(result.Values.Count()).IsEqualTo(0); + await Assert.That(result.Values is int[]).IsTrue(); + } + + [Test] + public async Task Convert_DelimitedString_ToEnumArray_ParsesEachElement() + { + var result = Build("DayOfWeekArray", nameof(IDayOfWeekArray.Values), "Monday,Friday"); + + await Assert.That(result.Values.SequenceEqual(new[] { DayOfWeek.Monday, DayOfWeek.Friday })).IsTrue(); + } + + [Test] + public async Task Convert_DelimitedString_ToDateTimeArray_ParsesWithConfiguredFormat() + { + // Default DateTimeFormat is "yyyy-MM-dd" (see SettingsOptions). + var result = Build("DateTimeArray", nameof(IDateTimeArray.Values), "2020-01-02,2021-03-04"); + + await Assert.That(result.Values.SequenceEqual(new[] { new DateTime(2020, 1, 2), new DateTime(2021, 3, 4) })).IsTrue(); + } + + [Test] + public async Task Convert_DelimitedString_ToUriArray_ParsesEachElement() + { + var result = Build("UriArray", nameof(IUriArray.Values), "https://a.example/,https://b.example/"); + + await Assert.That(result.Values.SequenceEqual(new[] { new Uri("https://a.example/"), new Uri("https://b.example/") })).IsTrue(); + } + + [Test] + public async Task Convert_NonConvertibleElement_ThrowsSettingsPropertyValueException() + { + // A bad element (non-numeric for an int[]) surfaces as the same wrapped exception the old converter + // produced; P4 leaves the exception-wrapping contract intact. + var collection = new InMemoryCollection(); + collection.Add("IntArray", nameof(IIntArray.Values), "1,x,3"); + + var builder = SettingsBuilder.CreateBuilder(x => x.AddSectionBinder(new InMemoryBinder(collection))); + + await Assert.That(() => builder.GetSettings()).Throws(); + } + private static T Build(string section, string key, string value, string? delimiter = null) where T : class { @@ -112,5 +165,20 @@ public interface IDefaultIntArray [SettingsProperty(DefaultValue = new[] { 7, 8, 9 })] int[] Values { get; set; } } + + public interface IDayOfWeekArray + { + DayOfWeek[] Values { get; set; } + } + + public interface IDateTimeArray + { + DateTime[] Values { get; set; } + } + + public interface IUriArray + { + Uri[] Values { get; set; } + } } } From be58d9e74667e11c3577815b89b4b665ab29b73c Mon Sep 17 00:00:00 2001 From: Guy Ludvig Date: Mon, 13 Jul 2026 13:19:13 +0300 Subject: [PATCH 3/4] making it nicer a bit --- .../CommandLineSettingsBinderOptions.cs | 4 ++-- .../SettingsBuilderOptions.cs | 2 +- .../SettingsBuilderOptionsExtensions.cs | 4 ++-- .../Conversion/CollectionTypeConverter.cs | 9 ++++----- .../Core/Reflection/EqualityCompererCreator.cs | 8 +++++--- .../Core/Reflection/PropertyCreator.cs | 3 ++- .../SettingsBuilderExtensions.cs | 4 ++-- .../Validations/ValidationResult.cs | 2 +- .../Conversion/CollectionConversionTests.cs | 8 ++++---- .../AddSimpleSettingsIntegrationTests.cs | 8 ++++---- .../NonDefaultSettingsTypesExtractorTests.cs | 2 +- .../SimpleSettings/SettingsTypesExtractorTests.cs | 2 +- 12 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/CommandLineSettingsBinderOptions.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/CommandLineSettingsBinderOptions.cs index 4a11473..34df427 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/CommandLineSettingsBinderOptions.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/CommandLineSettingsBinderOptions.cs @@ -6,8 +6,8 @@ namespace ExistForAll.SimpleSettings.Binders { public class CommandLineSettingsBinderOptions { - private readonly List _argumentPrefixes = new List(new[] {'-', '/'}); - private readonly List _delimiters = new List(new[] {":", "="}); + private readonly List _argumentPrefixes = [..new[] { '-', '/' }]; + private readonly List _delimiters = [..new[] { ":", "=" }]; public NameFormatter? NameFormatter { get; set; } diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsBuilderOptions.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsBuilderOptions.cs index c4fdde5..f01b6df 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsBuilderOptions.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsBuilderOptions.cs @@ -7,7 +7,7 @@ namespace ExistForAll.SimpleSettings.Extensions.GenericHost public class SettingsBuilderOptions : ISettingsBuilderOptions { private readonly ISettingsBuilderFactory _settingsBuilderFactory; - private readonly List _assemblies = new List(); + private readonly List _assemblies = []; public SettingsOptions Options => _settingsBuilderFactory.Options; public IEnumerable Assemblies => _assemblies; diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsBuilderOptionsExtensions.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsBuilderOptionsExtensions.cs index f6b9783..132db64 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsBuilderOptionsExtensions.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.GenericHost/SettingsBuilderOptionsExtensions.cs @@ -7,7 +7,7 @@ public static class SettingsBuilderOptionsExtensions { public static ISettingsBuilderOptions AddAssembly(this ISettingsBuilderOptions target, Assembly assembly) { - target.AddAssemblies(new []{assembly}); + target.AddAssemblies([assembly]); return target; } @@ -20,7 +20,7 @@ public static ISettingsBuilderOptions AddAssembly(this ISettingsBuilderOption public static ISettingsBuilderOptions AddAssemblies(this ISettingsBuilderOptions target, Assembly assembly, params Assembly[] assemblies) { - assemblies = assemblies == null ? new [] {assembly} : assemblies.Concat(new []{ assembly }).ToArray(); + assemblies = assemblies == null ? [assembly] : assemblies.Concat([assembly]).ToArray(); target.AddAssemblies(assemblies); return target; } diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs index 79817c9..3cce222 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs @@ -46,10 +46,10 @@ private Array AsArray(object value) { if (value is string text) { - return text.Split(new[] { _settingsOptions.ArraySplitDelimiter }, StringSplitOptions.RemoveEmptyEntries); + return text.Split([_settingsOptions.ArraySplitDelimiter], StringSplitOptions.RemoveEmptyEntries); } - return value is Array array ? array : new[] { value }; + return value as Array ?? new[] { value }; } private ISettingsTypeConverter GetElementConverter(Type elementType) @@ -57,10 +57,9 @@ private ISettingsTypeConverter GetElementConverter(Type elementType) // Manual walk over the concrete LinkedList (not LINQ First) so the struct enumerator isn't boxed // onto the heap and no predicate closure is allocated — this runs per element-typed collection on // every populate. - foreach (var converter in _converters) + foreach (var converter in _converters.Where(converter => converter.CanConvert(elementType))) { - if (converter.CanConvert(elementType)) - return converter; + return converter; } // Unreachable in practice: DefaultTypeConverter.CanConvert always returns true. Kept so every path diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/EqualityCompererCreator.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/EqualityCompererCreator.cs index 2bb47ab..c6e7957 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/EqualityCompererCreator.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/EqualityCompererCreator.cs @@ -12,7 +12,8 @@ public void CreateEqualsMethod(TypeBuilder typeBuilder, List fields) if (typeBuilder == null) throw new ArgumentNullException(nameof(typeBuilder)); if (fields == null) throw new ArgumentNullException(nameof(fields)); - var method = typeBuilder.DefineMethod("Equals", MethodAttributes.Public | MethodAttributes.ReuseSlot | MethodAttributes.Virtual | MethodAttributes.HideBySig, typeof(bool), new[] { typeof(object) }); + var method = typeBuilder.DefineMethod("Equals", MethodAttributes.Public | MethodAttributes.ReuseSlot | MethodAttributes.Virtual | MethodAttributes.HideBySig, typeof(bool), + [typeof(object)]); var methodGenerator = method.GetILGenerator(); var other = methodGenerator.DeclareLocal(typeBuilder.DeclaringType!); @@ -33,7 +34,8 @@ public void CreateEqualsMethod(TypeBuilder typeBuilder, List fields) methodGenerator.Emit(OpCodes.Ldarg_0); methodGenerator.Emit(OpCodes.Ldfld, field); methodGenerator.Emit(OpCodes.Ldloc, other); - methodGenerator.EmitCall(OpCodes.Callvirt, comparerType.GetMethod("Equals", new[] { field.FieldType, field.FieldType })!, null); + methodGenerator.EmitCall(OpCodes.Callvirt, comparerType.GetMethod("Equals", [field.FieldType, field.FieldType + ])!, null); methodGenerator.Emit(OpCodes.Brtrue_S, next); methodGenerator.Emit(OpCodes.Ldc_I4); methodGenerator.Emit(OpCodes.Ret); @@ -59,7 +61,7 @@ public void CreateGetHashCodeMethod(TypeBuilder typeBuilder, List fie methodGenerator.EmitCall(OpCodes.Call, comparerType.GetMethod("get_Default")!, null); methodGenerator.Emit(OpCodes.Ldarg_0); methodGenerator.Emit(OpCodes.Ldfld, field); - methodGenerator.EmitCall(OpCodes.Callvirt, comparerType.GetMethod("GetHashCode", new[] { field.FieldType })!, null); + methodGenerator.EmitCall(OpCodes.Callvirt, comparerType.GetMethod("GetHashCode", [field.FieldType])!, null); methodGenerator.Emit(OpCodes.Xor); } diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyCreator.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyCreator.cs index 93221b1..e933fb5 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyCreator.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyCreator.cs @@ -24,7 +24,8 @@ public void CreateAnonymousProperties(TypeBuilder typeBuilder, PropertyInfo[] pr MethodAttributes.Virtual | MethodAttributes.Final; var getter = typeBuilder.DefineMethod($"get_{property.Name}", methodAttributes, property.PropertyType, Type.EmptyTypes); - var setter = typeBuilder.DefineMethod($"set_{property.Name}", methodAttributes, null, new[] { property.PropertyType }); + var setter = typeBuilder.DefineMethod($"set_{property.Name}", methodAttributes, null, [property.PropertyType + ]); var getterGenerator = getter.GetILGenerator(); getterGenerator.Emit(OpCodes.Ldarg_0); diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsBuilderExtensions.cs b/src/Core/ExistForAll.SimpleSettings/SettingsBuilderExtensions.cs index 362503b..5d30992 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsBuilderExtensions.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsBuilderExtensions.cs @@ -12,10 +12,10 @@ public static ISettingsCollection ScanAssemblies(this SettingsBuilder target, As if (assemblies == null) { - assemblies = new Assembly[0]; + assemblies = []; } - assemblies = assemblies.Concat(new[] { assembly }).ToArray(); + assemblies = assemblies.Concat([assembly]).ToArray(); return target.ScanAssemblies(assemblies); } diff --git a/src/Core/ExistForAll.SimpleSettings/Validations/ValidationResult.cs b/src/Core/ExistForAll.SimpleSettings/Validations/ValidationResult.cs index c4d23c4..3365b0a 100644 --- a/src/Core/ExistForAll.SimpleSettings/Validations/ValidationResult.cs +++ b/src/Core/ExistForAll.SimpleSettings/Validations/ValidationResult.cs @@ -6,7 +6,7 @@ namespace ExistForAll.SimpleSettings.Validations { public class ValidationResult { - private readonly List _errors = new List(); + private readonly List _errors = []; public IEnumerable Errors => _errors; diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs index fb01135..887b0c2 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/CollectionConversionTests.cs @@ -17,7 +17,7 @@ public async Task Convert_DelimitedString_ToIntArray_ParsesEachElement() { var result = Build("IntArray", nameof(IIntArray.Values), "1,2,3,4"); - await Assert.That(result.Values.SequenceEqual(new[] { 1, 2, 3, 4 })).IsTrue(); + await Assert.That(result.Values.SequenceEqual([1, 2, 3, 4])).IsTrue(); } [Test] @@ -54,7 +54,7 @@ public async Task Convert_DelimitedString_RemovesEmptyEntries() { var result = Build("IntArray", nameof(IIntArray.Values), "1,,2,"); - await Assert.That(result.Values.SequenceEqual(new[] { 1, 2 })).IsTrue(); + await Assert.That(result.Values.SequenceEqual([1, 2])).IsTrue(); } [Test] @@ -62,7 +62,7 @@ public async Task Convert_WithCustomDelimiter_SplitsOnThatDelimiter() { var result = Build("IntArray", nameof(IIntArray.Values), "10;20;30", delimiter: ";"); - await Assert.That(result.Values.SequenceEqual(new[] { 10, 20, 30 })).IsTrue(); + await Assert.That(result.Values.SequenceEqual([10, 20, 30])).IsTrue(); } [Test] @@ -73,7 +73,7 @@ public async Task Convert_DefaultArray_IsPassedThrough() var result = sut.GetSettings(); - await Assert.That(result.Values.SequenceEqual(new[] { 7, 8, 9 })).IsTrue(); + await Assert.That(result.Values.SequenceEqual([7, 8, 9])).IsTrue(); } [Test] diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs index b27524b..470274f 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/DependencyInjection/AddSimpleSettingsIntegrationTests.cs @@ -19,7 +19,7 @@ public async Task AddSimpleSettings_ResolvesSettingsInterface_WithBoundValues() var services = new ServiceCollection(); services.AddSimpleSettings(o => { - o.AddAssemblies(new[] { typeof(IDiExampleSettings).Assembly }); + o.AddAssemblies([typeof(IDiExampleSettings).Assembly]); o.AddSectionBinder(new InMemoryBinder(collection)); }); @@ -33,7 +33,7 @@ public async Task AddSimpleSettings_ResolvesSettingsInterface_WithBoundValues() public async Task AddSimpleSettings_RegistersSettingsAsSingleton() { var services = new ServiceCollection(); - services.AddSimpleSettings(o => o.AddAssemblies(new[] { typeof(IDiExampleSettings).Assembly })); + services.AddSimpleSettings(o => o.AddAssemblies([typeof(IDiExampleSettings).Assembly])); var provider = services.BuildServiceProvider(); var first = provider.GetRequiredService(); @@ -52,7 +52,7 @@ public async Task AddSimpleSettings_RegistersSettingsProvider_ThatBindsValues() var services = new ServiceCollection(); services.AddSimpleSettings(o => { - o.AddAssemblies(new[] { typeof(IDiExampleSettings).Assembly }); + o.AddAssemblies([typeof(IDiExampleSettings).Assembly]); o.AddSectionBinder(new InMemoryBinder(collection)); }); @@ -68,7 +68,7 @@ public async Task AddSimpleSettings_RegistersSettingsProvider_ThatBindsValues() public async Task AddSimpleSettings_Provider_ReturnsTheSameInstanceAsTheContainer() { var services = new ServiceCollection(); - services.AddSimpleSettings(o => o.AddAssemblies(new[] { typeof(IDiExampleSettings).Assembly })); + services.AddSimpleSettings(o => o.AddAssemblies([typeof(IDiExampleSettings).Assembly])); var provider = services.BuildServiceProvider(); var fromContainer = provider.GetRequiredService(); diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/NonDefaultSettingsTypesExtractorTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/NonDefaultSettingsTypesExtractorTests.cs index b2488a1..97460ac 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/NonDefaultSettingsTypesExtractorTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/NonDefaultSettingsTypesExtractorTests.cs @@ -54,7 +54,7 @@ public async Task ExtractSettingsTypes_WhenTypeHasNonDefaultSuffixIndidation_Sho private IEnumerable MockAssemblies(Type returnType) { - return new Assembly[] {returnType.GetTypeInfo().Assembly}; + return [returnType.GetTypeInfo().Assembly]; } public interface INonDefaultSuffixIndicationInterfaceSomeSuffix diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsTypesExtractorTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsTypesExtractorTests.cs index db3941c..86a3994 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsTypesExtractorTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsTypesExtractorTests.cs @@ -71,7 +71,7 @@ public async Task ExtractSettingsTypes_WhenSuffixDiffersOnlyByCase_ShouldExtract private IEnumerable MockAssemblies(Type returnType) { - return new[] {returnType.GetTypeInfo().Assembly}; + return [returnType.GetTypeInfo().Assembly]; } public interface ICasingMismatchSETTINGS From 3009b8a499e71a12d2e19e9f9565d784aa6fef4d Mon Sep 17 00:00:00 2001 From: guy-lud Date: Mon, 13 Jul 2026 13:44:17 +0300 Subject: [PATCH 4/4] bring it back --- .../CommandLineSettingsBinderOptions.cs | 6 ++++-- .../Conversion/CollectionTypeConverter.cs | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/CommandLineSettingsBinderOptions.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/CommandLineSettingsBinderOptions.cs index 34df427..47c8673 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/CommandLineSettingsBinderOptions.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/CommandLineSettingsBinderOptions.cs @@ -19,13 +19,15 @@ public class CommandLineSettingsBinderOptions public void AddArgumentPrefix(char prefix) { - if (prefix <= 0) throw new ArgumentOutOfRangeException(nameof(prefix)); + if (prefix <= 0) + throw new ArgumentOutOfRangeException(nameof(prefix)); _argumentPrefixes.Add(prefix); } public void AddDelimiter(string prefix) { - if (prefix == null) throw new ArgumentNullException(nameof(prefix)); + if (prefix == null) + throw new ArgumentNullException(nameof(prefix)); _delimiters.Add(prefix); } diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs index 3cce222..32496b9 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/CollectionTypeConverter.cs @@ -57,9 +57,10 @@ private ISettingsTypeConverter GetElementConverter(Type elementType) // Manual walk over the concrete LinkedList (not LINQ First) so the struct enumerator isn't boxed // onto the heap and no predicate closure is allocated — this runs per element-typed collection on // every populate. - foreach (var converter in _converters.Where(converter => converter.CanConvert(elementType))) + foreach (var converter in _converters) { - return converter; + if (converter.CanConvert(elementType)) + return converter; } // Unreachable in practice: DefaultTypeConverter.CanConvert always returns true. Kept so every path