fix: added benchmarks for AutoMapper, Mapperly, Facet and Mapster#2
Merged
fix: added benchmarks for AutoMapper, Mapperly, Facet and Mapster#2
Conversation
FreakyAli
changed the title
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a BenchmarkDotNet project and many benchmarks; extends the source generator with collection-projection, null-safe nested/collection access, init-only handling and collection materialization for immutable/read-only types; introduces StrictMapping diagnostics and converter-signature validation in the analyzer; expands attributes and broad test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as "Developer / CI"
participant Generator as "ForgeGenerator"
participant Analyzer as "ForgeAnalyzer"
participant Diagnostics as "ForgeDiagnostics"
participant Tests as "Tests / Benchmarks"
Dev->>Generator: Build / run source generator
Generator->>Generator: Classify method (Create/Update/CollectionProject), use sourceParamName
Generator->>Generator: Emit null-safe nested/flattening and collection projections, handle init-only and immutable/read-only materialization
Generator->>Analyzer: Provide symbol/attribute context for analysis
Analyzer->>Diagnostics: Validate converters, honor ForgeIgnore.Side, evaluate StrictMapping -> emit FKF100/FKF101 or FKF110/FKF111 and FKF221
Dev->>Tests: Run unit/integration tests and BenchmarkDotNet
Tests->>Generator: Assert generated code (null-guards, collection projection, init-only)
Tests->>Analyzer: Assert diagnostics (strict mapping, converter signatures, ignore-side)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
FreakyAli
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs (1)
35-59:⚠️ Potential issue | 🟠 MajorThese “does not suppress” tests still don’t exercise
ForgeIgnoreSide.Line [35] and Line [89] scenarios still never apply
[ForgeIgnore(Side = ...)]to the member under test, so they can pass without validating side-specific suppression behavior.✅ Suggested test adjustment
public void ForgeIgnore_Side_Source_DoesNotSuppressFKF100() { @@ public class Dest { public string Name { get; set; } = ""; - // Not ignored on dest side — will emit FKF100 since there's no source match + // Source-side ignore must not suppress destination-side FKF100 + [ForgeIgnore(Side = ForgeIgnoreSide.Source)] public int Unmatched { get; set; } } @@ public void ForgeIgnore_Side_Destination_DoesNotSuppressFKF101() { @@ public class Source { public string Name { get; set; } = ""; + [ForgeIgnore(Side = ForgeIgnoreSide.Destination)] public string Extra { get; set; } = ""; }Also applies to: 89-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs` around lines 35 - 59, The tests named ForgeIgnore_Side_Source_DoesNotSuppressFKF100 (and the similar case around lines 89–112) never actually apply the ForgeIgnore attribute to the member under test, so they don't validate side-specific suppression; update the test source string for ForgeIgnore_Side_Source_DoesNotSuppressFKF100 to add [ForgeIgnore(Side = Side.Source)] on the Dest.Unmatched member (and in the other test add the opposite side, e.g., [ForgeIgnore(Side = Side.Dest)] on the Source member) so the member under test carries the side-specific attribute, then keep the AssertContainsDiagnostic(...) expectations (FKF100 should still fire when the attribute is on the opposite side). Ensure you reference the test methods ForgeIgnore_Side_Source_DoesNotSuppressFKF100 (and the corresponding test around lines 89–112) when making the changes.
🧹 Nitpick comments (9)
src/FreakyKit.Forge/Attributes/ForgeMapAttribute.cs (1)
5-10: XML documentation should mention parameter applicability.The
AttributeUsagenow includesAttributeTargets.Parameter, but the XML summary at lines 5-9 still only mentions "property or field." Consider updating the documentation to reflect that the attribute can also be applied to constructor parameters.📝 Suggested documentation update
/// <summary> -/// Maps a property or field to a differently-named member on the counterpart type. +/// Maps a property, field, or constructor parameter to a differently-named member on the counterpart type. /// When applied to a source member, the value specifies the destination member name. /// When applied to a destination member, the value specifies the source member name. +/// When applied to a constructor parameter, the value specifies the source member name to use for that parameter. /// </summary>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FreakyKit.Forge/Attributes/ForgeMapAttribute.cs` around lines 5 - 10, Update the XML documentation on ForgeMapAttribute to reflect that it applies to parameters as well as properties and fields: modify the summary for the ForgeMapAttribute class (the attribute with AttributeUsage including AttributeTargets.Parameter) to mention "property, field, or constructor/parameter" and/or state that when applied to a constructor parameter the value specifies the counterpart member name, mirroring the existing descriptions for source/destination usage.tests/FreakyKit.Forge.Analyzers.Tests/StrictMappingTests.cs (1)
91-94: Assert warning severity explicitly in non-strict mode tests.These tests validate IDs but not that FKF100/FKF101 remain warnings. Add severity checks to guard against accidental severity regressions.
Proposed assertions
AssertContainsDiagnostic(source, "FKF100"); + AssertDiagnosticSeverity(source, "FKF100", DiagnosticSeverity.Warning); AssertNotContainsDiagnostic(source, "FKF110");AssertContainsDiagnostic(source, "FKF101"); + AssertDiagnosticSeverity(source, "FKF101", DiagnosticSeverity.Warning); AssertNotContainsDiagnostic(source, "FKF111");Also applies to: 113-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FreakyKit.Forge.Analyzers.Tests/StrictMappingTests.cs` around lines 91 - 94, The test currently asserts presence/absence of diagnostic IDs but not the severity; update the non-strict-mode assertions in StrictMappingTests.cs to explicitly assert diagnostic severity for the relevant IDs (use AssertContainsDiagnostic/AssertDiagnosticSeverity or the project's helper to verify FKF100 and FKF101 are reported as Warning and that FKF110 is not reported as Error), and add equivalent severity assertions in the other affected test block (the one that currently checks FKF100/FKF110) so the tests fail if severity changes accidentally.tests/FreakyKit.Forge.Generator.Tests/ForgeIgnoreSideGeneratorTests.cs (1)
140-142: Tighten the assertion to the concrete assignment.
Assert.Contains("InternalId", generated)is too broad and can pass on unrelated occurrences. Assert the exact mapping line instead.Proposed test assertion hardening
- Assert.Contains("source.AltId", generated); - Assert.Contains("InternalId", generated); + Assert.Contains("__result.InternalId = source.AltId", generated); + Assert.DoesNotContain("__result.InternalId = source.InternalId", generated);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FreakyKit.Forge.Generator.Tests/ForgeIgnoreSideGeneratorTests.cs` around lines 140 - 142, The test currently uses a broad Assert.Contains("InternalId", generated) which can match unrelated occurrences; update the assertion in ForgeIgnoreSideGeneratorTests to check the concrete mapping line in the generated string (use the `generated` variable) — assert the exact assignment text for InternalId (e.g., the exact mapping "InternalId = source.InternalId") so the test only passes when the proper mapping is emitted.tests/FreakyKit.Forge.Generator.Tests/CollectionProjectGeneratorTests.cs (1)
57-59: Use a stricter assertion thanreturn source.This check can false-pass on unrelated methods. Assert the projected/materialized form you actually require.
Example tightening
- // The projection method body should be a single return — no construction - Assert.Contains("return source", generated); + // Projection should be direct and materialized, without extra object construction + Assert.Contains("return source.Select(x => ToDto(x)).ToList();", generated);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FreakyKit.Forge.Generator.Tests/CollectionProjectGeneratorTests.cs` around lines 57 - 59, Replace the weak substring check Assert.Contains("return source", generated) in CollectionProjectGeneratorTests with a stricter assertion that matches the exact projection/materialization syntax you expect (e.g., the precise Select(...) and ToList()/ToArray() form). Update the assertion to inspect the 'generated' string for the full projection expression (or use Assert.Equal/Assert.Contains with the full expected snippet) so it only passes when the method body returns the exact projected result rather than any unrelated "return source".benchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/ThroughputBenchmark.cs (1)
99-107: Facet benchmark returns different type than other benchmarks.The
FacetBenchmethod returnsMediumFacetDto[]while all other benchmarks returnMediumDestination[]. This is likely intentional due to Facet's type generation requirements, but it may affect comparison fairness ifMediumFacetDtohas a different structure. Consider adding a comment explaining this discrepancy for clarity.💡 Suggested documentation
[Benchmark(Description = "Facet")] [BenchmarkCategory("Throughput")] + // Facet requires its own DTO type (MediumFacetDto) due to its facet generation approach public MediumFacetDto[] FacetBench()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/ThroughputBenchmark.cs` around lines 99 - 107, The FacetBench method returns MediumFacetDto[] while other benchmarks return MediumDestination[]; add a brief inline comment above the FacetBench method (or inside the method) explaining why it returns MediumFacetDto (e.g., Facet generates a different DTO type required for the facet mapping and is intentionally different for fair/expected comparisons) so readers and reviewers understand the discrepancy and its intent; reference the FacetBench method and the types MediumFacetDto and MediumDestination in the comment for clarity.benchmarks/FreakyKit.Forge.Benchmarks/Forges/HandWrittenMappers.cs (1)
90-99: Consider adding null checks for nested property access inMapFlattening.The
MapFlatteningmethod directly accessessource.HomeAddress.Streetetc. without null guards. Ifsource.HomeAddressis null, this will throw aNullReferenceException. The Forge-generated code uses null-conditional access (source.HomeAddress?.Street), so the hand-written baseline may behave differently when source contains nulls.If the benchmark fixtures always provide non-null
HomeAddress, this is fine. Otherwise, consider aligning null-handling with the generated code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/FreakyKit.Forge.Benchmarks/Forges/HandWrittenMappers.cs` around lines 90 - 99, MapFlattening currently dereferences source.HomeAddress directly which can throw if HomeAddress is null; update MapFlattening to guard nested access (e.g., use null checks or null-conditional semantics when reading HomeAddress.Street/City/State/ZipCode) and ensure it assigns nulls to FlatteningDestination fields when HomeAddress is null so behavior matches the Forge-generated mapping; check FlatteningSource, HomeAddress, and FlatteningDestination property names when applying the guards.src/FreakyKit.Forge.Generator/ForgeGenerator.cs (1)
990-994: Init-only properties withIgnoreIfNullare silently skipped.Lines 990-993 filter out init-only assignments that have
IgnoreIfNullset, as object initializers cannot use conditional assignment. This is correct behavior, but it may surprise users who expect the property to be conditionally set.Consider emitting a diagnostic (info-level) when an init-only property with
IgnoreIfNull=trueis encountered, so users understand why their conditional mapping isn't being applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FreakyKit.Forge.Generator/ForgeGenerator.cs` around lines 990 - 994, When computing init-only assignments you currently filter out those with IgnoreIfNull into skippedInitOnly; instead, after building skippedInitOnly (from method.Assignments where a.IsInitOnly && a.IgnoreIfNull) emit an informational diagnostic for each skipped assignment so users know the init-only property was intentionally not mapped; use the generator's diagnostic/logging API (same place other diagnostics are produced) and include the target member name and that IgnoreIfNull prevented object-initializer assignment, referencing the variables/members initOnlyAssignments, skippedInitOnly, method.Assignments and the IgnoreIfNull flag to locate where to add the diagnostic emission.src/FreakyKit.Forge.Analyzers/ForgeAnalyzer.cs (2)
958-959: DefaultKeyValuePaircheck may not work as expected.
FirstOrDefaultonNamedArgumentsreturns a defaultKeyValuePair<string, TypedConstant>when not found, whereKeyisnull(not an empty string). However, the logic is inverted—whenSideisn't specified, you wantside = 0(both sides). IfKeyisnull, the ternary defaults to0, which is correct.That said, checking
sideArg.Key != nullis fragile. Consider using a more explicit pattern:♻️ Suggested refinement for clarity
- var sideArg = attr.NamedArguments.FirstOrDefault(a => a.Key == "Side"); - int side = sideArg.Key != null && sideArg.Value.Value is int sv ? sv : 0; + int side = 0; + foreach (var arg in attr.NamedArguments) + { + if (arg.Key == "Side" && arg.Value.Value is int sv) + { + side = sv; + break; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FreakyKit.Forge.Analyzers/ForgeAnalyzer.cs` around lines 958 - 959, The current check uses FirstOrDefault and then tests sideArg.Key != null which is fragile; instead find the "Side" entry explicitly and only accept its value if the found Key equals "Side". Update the attr.NamedArguments lookup so that you get sideArg via attr.NamedArguments.FirstOrDefault(a => a.Key == "Side") and then set int side = (sideArg.Key == "Side" && sideArg.Value.Value is int sv) ? sv : 0; ensuring you reference the variables sideArg, attr.NamedArguments and side to locate and fix the logic.
162-168: Consider validating the attribute's containing namespace.The check
a.AttributeClass?.Name == "ForgeConverterAttribute"matches by name only. If another library defines aForgeConverterAttribute, this could produce false positives. Consider also verifying the containing namespace matchesFreakyKit.Forge.♻️ Suggested refinement
- bool hasConverterAttr = member.GetAttributes() - .Any(a => a.AttributeClass?.Name == "ForgeConverterAttribute" || - a.AttributeClass?.Name == "ForgeConverter"); + bool hasConverterAttr = member.GetAttributes() + .Any(a => (a.AttributeClass?.Name == "ForgeConverterAttribute" || + a.AttributeClass?.Name == "ForgeConverter") && + a.AttributeClass?.ContainingNamespace?.ToDisplayString() == "FreakyKit.Forge");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FreakyKit.Forge.Analyzers/ForgeAnalyzer.cs` around lines 162 - 168, The current attribute check in the loop over type.GetMembers().OfType<IMethodSymbol>() uses only a.Name == "ForgeConverterAttribute" or "ForgeConverter", which can collide with similarly named attributes; update the predicate on member.GetAttributes() to also verify a.AttributeClass?.ContainingNamespace?.ToDisplayString() (or walk ContainingNamespace) equals "FreakyKit.Forge" (or starts with that namespace if sub-namespaces are allowed), and keep the null-safety checks; ensure the code that uses hasConverterAttr still references the same symbol names (ForgeConverterAttribute / ForgeConverter) but only sets hasConverterAttr true when both name and namespace match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/FlatteningMapBenchmark.cs`:
- Around line 13-14: The benchmark suite incorrectly groups Facet (the
non-flattening mapper) under the "Flattening" category; update the
BenchmarkCategory attributes so that any benchmark methods or the
FlatteningMapBenchmark class referencing Facet are moved out of "Flattening"
into a new category (e.g., "NonFlattening" or "Facet") and ensure the methods
that actually perform flattened mappings remain under "Flattening"; locate
references to Facet in FlatteningMapBenchmark (and the other occurrences around
the same area noted near lines 64-67) and change their BenchmarkCategory
annotation accordingly so rankings don't mix non-equivalent categories.
In `@benchmarks/FreakyKit.Forge.Benchmarks/Forges/MapsterConfig.cs`:
- Around line 7-12: The Configure() method reads/writes the static _configured
flag without synchronization; make it thread-safe by implementing double-checked
locking: declare a private static readonly object (e.g., _configureLock) and
mark _configured as volatile, then in Configure() first if (_configured) return;
lock(_configureLock) { if (_configured) return; perform the
TypeAdapterConfig.GlobalSettings setup and call
TypeAdapterConfig.GlobalSettings.Compile(); set _configured = true; } so the
configuration block runs exactly once even under concurrent BenchmarkDotNet
GlobalSetup calls.
In `@docs/future-plans.md`:
- Line 336: Change the sentence "Update reverse: Should reverse also support
update methods (void return, 2 params)?" to the more natural wording "Should
reverse support update methods as well?" — locate the line containing the
"Update reverse" bullet/question and replace the phrasing accordingly (preserve
the parenthetical detail "(void return, 2 params)" if desired).
In `@docs/generator.md`:
- Around line 41-43: The documentation example shows a signature "public static
partial List<PersonDto> ToDtos(List<Person> source)" but the generated example
returns null, causing a mismatch; update the example to use a nullable return
and/or parameter to align nullability (e.g., make the return type
List<PersonDto>? and/or parameter List<Person>? as appropriate) so the signature
of ToDtos matches the illustrated behavior of returning null when source is
null; ensure the unique symbols ToDtos, List<PersonDto> and List<Person> are
updated consistently in the doc example.
In `@README.md`:
- Line 89: Update the diagnostics count in the README for the Analyzers package:
change the text "34 diagnostics" to "35 diagnostics" so it matches the rest of
the document and the diagnostics table (which now includes FKF221); locate the
occurrence of the string "34 diagnostics" in the README and replace it with "35
diagnostics" to resolve the inconsistency.
In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs`:
- Around line 115-139: The test ForgeIgnore_Side_Both_SuppressesBothWarnings
currently only exercises a source-side unmatched member and thus never triggers
FKF100; update the test's source string to add an unmatched destination member
on class Dest decorated with [ForgeIgnore(Side = ForgeIgnoreSide.Both)] (so
both-source-and-dest ignore is exercised), keeping the existing
AssertNotContainsDiagnostic checks for "FKF101" and "FKF100" to verify both
warnings are suppressed; refer to the ForgeIgnore attribute and
ForgeIgnoreSide.Both, the Dest class and the
ForgeIgnore_Side_Both_SuppressesBothWarnings test to locate where to add the
unmatched destination property.
In `@tests/FreakyKit.Forge.Generator.Tests/ForgeMapCtorGeneratorTests.cs`:
- Around line 63-89: The test ForgeMap_OnCtorParam_NullableSourceMember
currently asserts the generator emitted a direct .Value access
("source.Score.Value") which forces a null-unsafe mapping; update the assertion
to expect a null-safe access (e.g. "source.Score.GetValueOrDefault()" or the
equivalent null-coalescing expression) for the generated output held in variable
generated, and remove the negative assertion about diagnostic "FKF501" (or
alternatively replace both assertions with an AssertContains for the diagnostic
"FKF501" if you choose to require a diagnostic instead); reference the test
method name ForgeMap_OnCtorParam_NullableSourceMember and the generated variable
to locate where to change the assertions.
---
Duplicate comments:
In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs`:
- Around line 35-59: The tests named
ForgeIgnore_Side_Source_DoesNotSuppressFKF100 (and the similar case around lines
89–112) never actually apply the ForgeIgnore attribute to the member under test,
so they don't validate side-specific suppression; update the test source string
for ForgeIgnore_Side_Source_DoesNotSuppressFKF100 to add [ForgeIgnore(Side =
Side.Source)] on the Dest.Unmatched member (and in the other test add the
opposite side, e.g., [ForgeIgnore(Side = Side.Dest)] on the Source member) so
the member under test carries the side-specific attribute, then keep the
AssertContainsDiagnostic(...) expectations (FKF100 should still fire when the
attribute is on the opposite side). Ensure you reference the test methods
ForgeIgnore_Side_Source_DoesNotSuppressFKF100 (and the corresponding test around
lines 89–112) when making the changes.
---
Nitpick comments:
In `@benchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/ThroughputBenchmark.cs`:
- Around line 99-107: The FacetBench method returns MediumFacetDto[] while other
benchmarks return MediumDestination[]; add a brief inline comment above the
FacetBench method (or inside the method) explaining why it returns
MediumFacetDto (e.g., Facet generates a different DTO type required for the
facet mapping and is intentionally different for fair/expected comparisons) so
readers and reviewers understand the discrepancy and its intent; reference the
FacetBench method and the types MediumFacetDto and MediumDestination in the
comment for clarity.
In `@benchmarks/FreakyKit.Forge.Benchmarks/Forges/HandWrittenMappers.cs`:
- Around line 90-99: MapFlattening currently dereferences source.HomeAddress
directly which can throw if HomeAddress is null; update MapFlattening to guard
nested access (e.g., use null checks or null-conditional semantics when reading
HomeAddress.Street/City/State/ZipCode) and ensure it assigns nulls to
FlatteningDestination fields when HomeAddress is null so behavior matches the
Forge-generated mapping; check FlatteningSource, HomeAddress, and
FlatteningDestination property names when applying the guards.
In `@src/FreakyKit.Forge.Analyzers/ForgeAnalyzer.cs`:
- Around line 958-959: The current check uses FirstOrDefault and then tests
sideArg.Key != null which is fragile; instead find the "Side" entry explicitly
and only accept its value if the found Key equals "Side". Update the
attr.NamedArguments lookup so that you get sideArg via
attr.NamedArguments.FirstOrDefault(a => a.Key == "Side") and then set int side =
(sideArg.Key == "Side" && sideArg.Value.Value is int sv) ? sv : 0; ensuring you
reference the variables sideArg, attr.NamedArguments and side to locate and fix
the logic.
- Around line 162-168: The current attribute check in the loop over
type.GetMembers().OfType<IMethodSymbol>() uses only a.Name ==
"ForgeConverterAttribute" or "ForgeConverter", which can collide with similarly
named attributes; update the predicate on member.GetAttributes() to also verify
a.AttributeClass?.ContainingNamespace?.ToDisplayString() (or walk
ContainingNamespace) equals "FreakyKit.Forge" (or starts with that namespace if
sub-namespaces are allowed), and keep the null-safety checks; ensure the code
that uses hasConverterAttr still references the same symbol names
(ForgeConverterAttribute / ForgeConverter) but only sets hasConverterAttr true
when both name and namespace match.
In `@src/FreakyKit.Forge.Generator/ForgeGenerator.cs`:
- Around line 990-994: When computing init-only assignments you currently filter
out those with IgnoreIfNull into skippedInitOnly; instead, after building
skippedInitOnly (from method.Assignments where a.IsInitOnly && a.IgnoreIfNull)
emit an informational diagnostic for each skipped assignment so users know the
init-only property was intentionally not mapped; use the generator's
diagnostic/logging API (same place other diagnostics are produced) and include
the target member name and that IgnoreIfNull prevented object-initializer
assignment, referencing the variables/members initOnlyAssignments,
skippedInitOnly, method.Assignments and the IgnoreIfNull flag to locate where to
add the diagnostic emission.
In `@src/FreakyKit.Forge/Attributes/ForgeMapAttribute.cs`:
- Around line 5-10: Update the XML documentation on ForgeMapAttribute to reflect
that it applies to parameters as well as properties and fields: modify the
summary for the ForgeMapAttribute class (the attribute with AttributeUsage
including AttributeTargets.Parameter) to mention "property, field, or
constructor/parameter" and/or state that when applied to a constructor parameter
the value specifies the counterpart member name, mirroring the existing
descriptions for source/destination usage.
In `@tests/FreakyKit.Forge.Analyzers.Tests/StrictMappingTests.cs`:
- Around line 91-94: The test currently asserts presence/absence of diagnostic
IDs but not the severity; update the non-strict-mode assertions in
StrictMappingTests.cs to explicitly assert diagnostic severity for the relevant
IDs (use AssertContainsDiagnostic/AssertDiagnosticSeverity or the project's
helper to verify FKF100 and FKF101 are reported as Warning and that FKF110 is
not reported as Error), and add equivalent severity assertions in the other
affected test block (the one that currently checks FKF100/FKF110) so the tests
fail if severity changes accidentally.
In `@tests/FreakyKit.Forge.Generator.Tests/CollectionProjectGeneratorTests.cs`:
- Around line 57-59: Replace the weak substring check Assert.Contains("return
source", generated) in CollectionProjectGeneratorTests with a stricter assertion
that matches the exact projection/materialization syntax you expect (e.g., the
precise Select(...) and ToList()/ToArray() form). Update the assertion to
inspect the 'generated' string for the full projection expression (or use
Assert.Equal/Assert.Contains with the full expected snippet) so it only passes
when the method body returns the exact projected result rather than any
unrelated "return source".
In `@tests/FreakyKit.Forge.Generator.Tests/ForgeIgnoreSideGeneratorTests.cs`:
- Around line 140-142: The test currently uses a broad
Assert.Contains("InternalId", generated) which can match unrelated occurrences;
update the assertion in ForgeIgnoreSideGeneratorTests to check the concrete
mapping line in the generated string (use the `generated` variable) — assert the
exact assignment text for InternalId (e.g., the exact mapping "InternalId =
source.InternalId") so the test only passes when the proper mapping is emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e24c3912-a2b2-4ba0-964b-de24239d8bb0
📒 Files selected for processing (59)
README.mdbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/CollectionMapBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/DeepGraphMapBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/EcommerceOrderBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/FlatteningMapBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/MediumMapBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/NestedMapBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/NullableEntityBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/SimpleMapBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/ThroughputBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/UpdateMapBenchmark.csbenchmarks/FreakyKit.Forge.Benchmarks/Forges/AutoMapperConfig.csbenchmarks/FreakyKit.Forge.Benchmarks/Forges/BenchmarkForges.csbenchmarks/FreakyKit.Forge.Benchmarks/Forges/FacetConfig.csbenchmarks/FreakyKit.Forge.Benchmarks/Forges/HandWrittenMappers.csbenchmarks/FreakyKit.Forge.Benchmarks/Forges/MapperlyMappers.csbenchmarks/FreakyKit.Forge.Benchmarks/Forges/MapsterConfig.csbenchmarks/FreakyKit.Forge.Benchmarks/FreakyKit.Forge.Benchmarks.csprojbenchmarks/FreakyKit.Forge.Benchmarks/Models/Destination.csbenchmarks/FreakyKit.Forge.Benchmarks/Models/Source.csbenchmarks/FreakyKit.Forge.Benchmarks/Program.csdocs/attributes.mddocs/benchmarks.mddocs/diagnostics.mddocs/future-plans.mddocs/generator.mdsrc/FreakyKit.Forge.Analyzers/ForgeAnalyzer.cssrc/FreakyKit.Forge.Diagnostics/AnalyzerReleases.Unshipped.mdsrc/FreakyKit.Forge.Diagnostics/ForgeDiagnostics.cssrc/FreakyKit.Forge.Generator/ForgeGenerator.cssrc/FreakyKit.Forge.Generator/Models/ForgeMethodKind.cssrc/FreakyKit.Forge.Generator/Models/ForgeMethodModel.cssrc/FreakyKit.Forge.Generator/Models/MemberAssignmentModel.cssrc/FreakyKit.Forge/Attributes/ForgeIgnoreAttribute.cssrc/FreakyKit.Forge/Attributes/ForgeMapAttribute.cssrc/FreakyKit.Forge/Attributes/ForgeMethodAttribute.cstests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cstests/FreakyKit.Forge.Analyzers.Tests/InvalidConverterTests.cstests/FreakyKit.Forge.Analyzers.Tests/StrictMappingTests.cstests/FreakyKit.Forge.Generator.Tests/CollectionGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/CollectionProjectGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/ConverterGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/CrossFeatureGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/EnumGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/ExplicitModeGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/ExtendedCollectionGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/FlatteningGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/ForgeIgnoreSideGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/ForgeMapCtorGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/ForgeMapGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/GeneratorSuccessTests.cstests/FreakyKit.Forge.Generator.Tests/GeneratorTestBase.cstests/FreakyKit.Forge.Generator.Tests/HooksGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/InitOnlyGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/NullSafeNestedGeneratorTests.cstests/FreakyKit.Forge.Generator.Tests/NullableGeneratorTests.cstests/FreakyKit.Forge.Integration.Tests/CollectionIntegrationTests.cstests/FreakyKit.Forge.Integration.Tests/FlatteningIntegrationTests.cstests/FreakyKit.Forge.Integration.Tests/IntegrationTestBase.cs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs (2)
34-59:⚠️ Potential issue | 🟠 MajorTest does not exercise
ForgeIgnoreSide— passes even if side-filter logic is broken.
Unmatchedhas no[ForgeIgnore]attribute, so this test merely confirms that an unignored member emits FKF100. To actually verify thatForgeIgnoreSide.Sourcedoes not suppress FKF100, apply[ForgeIgnore(Side = ForgeIgnoreSide.Source)]toUnmatchedand assert FKF100 still appears.✅ Suggested fix
public class Dest { public string Name { get; set; } = ""; - // Not ignored on dest side — will emit FKF100 since there's no source match + // Source-side ignore must NOT suppress destination-side FKF100 + [ForgeIgnore(Side = ForgeIgnoreSide.Source)] public int Unmatched { get; set; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs` around lines 34 - 59, The test ForgeIgnore_Side_Source_DoesNotSuppressFKF100 currently doesn't exercise ForgeIgnoreSide because the Unmatched property lacks a ForgeIgnore attribute; update the test source so the Unmatched property is annotated with [ForgeIgnore(Side = ForgeIgnoreSide.Source)] and keep the assertion AssertContainsDiagnostic(source, "FKF100") so the test verifies that a Source-side ignore does not suppress the FKF100 diagnostic on the destination-only member.
88-112:⚠️ Potential issue | 🟠 MajorTest does not exercise
ForgeIgnoreSide— passes even if side-filter logic is broken.
Extrahas no[ForgeIgnore]attribute, so this test only confirms that an unignored source member emits FKF101. To verify thatForgeIgnoreSide.Destinationdoes not suppress FKF101, apply[ForgeIgnore(Side = ForgeIgnoreSide.Destination)]toExtraand assert FKF101 still appears.✅ Suggested fix
public class Source { public string Name { get; set; } = ""; + [ForgeIgnore(Side = ForgeIgnoreSide.Destination)] public string Extra { get; set; } = ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs` around lines 88 - 112, Update the test ForgeIgnore_Side_Destination_DoesNotSuppressFKF101 so it actually exercises ForgeIgnoreSide by annotating the Source.Extra property with [ForgeIgnore(Side = ForgeIgnoreSide.Destination)]; keep the rest of the test the same and continue to assert FKF101 via AssertContainsDiagnostic(source, "FKF101") to confirm Destination-side ignores do not suppress the missing-destination diagnostic.
🧹 Nitpick comments (3)
tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs (2)
61-86: Test only verifies suppression, not that the other diagnostic is unaffected.Similar to the first test — the name claims
NotFKF101but only asserts FKF100 absence. Consider adding a source member without a destination match to verify FKF101 still fires.🧪 Suggested enhancement
- public class Source { public string Name { get; set; } = ""; } + public class Source + { + public string Name { get; set; } = ""; + public string Extra { get; set; } = ""; // No dest match → FKF101 should still fire + } public class Dest { public string Name { get; set; } = ""; [ForgeIgnore(Side = ForgeIgnoreSide.Destination)] public int ComputedScore { get; set; } } """; // Dest side ignored → no FKF100 for ComputedScore AssertNotContainsDiagnostic(source, "FKF100"); + // FKF101 should still fire for unmatched source member + AssertContainsDiagnostic(source, "FKF101");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs` around lines 61 - 86, The test ForgeIgnore_Side_Destination_SuppressesFKF100_NotFKF101 currently only asserts FKF100 is absent; to validate FKF101 is unaffected add an unmapped source member (e.g., in class Source add a property that has no matching property in Dest) and then assert the analyzer emits FKF101 using AssertContainsDiagnostic(source, "FKF101") while keeping the existing AssertNotContainsDiagnostic for FKF100; this ensures the test still references the same types (Source, Dest, ComputedScore) and the same test method name ForgeIgnore_Side_Destination_SuppressesFKF100_NotFKF101.
7-32: Test only verifies suppression, not that the other diagnostic is unaffected.The test name claims
NotFKF100but only asserts that FKF101 is not present. Consider adding a positive assertion that FKF100 is still emitted for a different unmatched destination member to fully validate the side-filtering behavior.🧪 Suggested enhancement
public class Source { public string Name { get; set; } = ""; [ForgeIgnore(Side = ForgeIgnoreSide.Source)] public string InternalId { get; set; } = ""; } - public class Dest { public string Name { get; set; } = ""; } + public class Dest + { + public string Name { get; set; } = ""; + public int Unmatched { get; set; } // No source match → FKF100 should still fire + } """; // Source side ignored → no FKF101 for InternalId AssertNotContainsDiagnostic(source, "FKF101"); + // FKF100 should still fire for unmatched dest member + AssertContainsDiagnostic(source, "FKF100");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs` around lines 7 - 32, The test ForgeIgnore_Side_Source_SuppressesFKF101_NotFKF100 currently only asserts FKF101 is absent; update it to also assert FKF100 is still emitted for an unmatched destination member to validate side-filtering. After the existing AssertNotContainsDiagnostic(source, "FKF101"), add a positive assertion using AssertContainsDiagnostic or the test helper used elsewhere to check that "FKF100" is present for the mapping (e.g., targeting the Dest member mismatch), referencing the test's source string and keeping the test method name ForgeIgnore_Side_Source_SuppressesFKF101_NotFKF100 and existing class names (Source, Dest) so the test ensures FKF101 is suppressed while FKF100 still occurs.README.md (1)
280-280: Minor grammar: sentence fragment missing subject."Can be placed on properties, fields, or constructor parameters" is a sentence fragment. Consider adding a subject for completeness.
📝 Suggested fix
-Use `[ForgeMap]` to map members with different names. Can be placed on properties, fields, or constructor parameters: +Use `[ForgeMap]` to map members with different names. It can be placed on properties, fields, or constructor parameters:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 280, The sentence fragment "Can be placed on properties, fields, or constructor parameters" should include a subject; update the README text near the [ForgeMap] description so it reads something like "It can be placed on properties, fields, or constructor parameters" or "The [ForgeMap] attribute can be placed on properties, fields, or constructor parameters" to make a complete sentence and preserve the existing meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs`:
- Around line 34-59: The test ForgeIgnore_Side_Source_DoesNotSuppressFKF100
currently doesn't exercise ForgeIgnoreSide because the Unmatched property lacks
a ForgeIgnore attribute; update the test source so the Unmatched property is
annotated with [ForgeIgnore(Side = ForgeIgnoreSide.Source)] and keep the
assertion AssertContainsDiagnostic(source, "FKF100") so the test verifies that a
Source-side ignore does not suppress the FKF100 diagnostic on the
destination-only member.
- Around line 88-112: Update the test
ForgeIgnore_Side_Destination_DoesNotSuppressFKF101 so it actually exercises
ForgeIgnoreSide by annotating the Source.Extra property with [ForgeIgnore(Side =
ForgeIgnoreSide.Destination)]; keep the rest of the test the same and continue
to assert FKF101 via AssertContainsDiagnostic(source, "FKF101") to confirm
Destination-side ignores do not suppress the missing-destination diagnostic.
---
Nitpick comments:
In `@README.md`:
- Line 280: The sentence fragment "Can be placed on properties, fields, or
constructor parameters" should include a subject; update the README text near
the [ForgeMap] description so it reads something like "It can be placed on
properties, fields, or constructor parameters" or "The [ForgeMap] attribute can
be placed on properties, fields, or constructor parameters" to make a complete
sentence and preserve the existing meaning.
In `@tests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs`:
- Around line 61-86: The test
ForgeIgnore_Side_Destination_SuppressesFKF100_NotFKF101 currently only asserts
FKF100 is absent; to validate FKF101 is unaffected add an unmapped source member
(e.g., in class Source add a property that has no matching property in Dest) and
then assert the analyzer emits FKF101 using AssertContainsDiagnostic(source,
"FKF101") while keeping the existing AssertNotContainsDiagnostic for FKF100;
this ensures the test still references the same types (Source, Dest,
ComputedScore) and the same test method name
ForgeIgnore_Side_Destination_SuppressesFKF100_NotFKF101.
- Around line 7-32: The test ForgeIgnore_Side_Source_SuppressesFKF101_NotFKF100
currently only asserts FKF101 is absent; update it to also assert FKF100 is
still emitted for an unmatched destination member to validate side-filtering.
After the existing AssertNotContainsDiagnostic(source, "FKF101"), add a positive
assertion using AssertContainsDiagnostic or the test helper used elsewhere to
check that "FKF100" is present for the mapping (e.g., targeting the Dest member
mismatch), referencing the test's source string and keeping the test method name
ForgeIgnore_Side_Source_SuppressesFKF101_NotFKF100 and existing class names
(Source, Dest) so the test ensures FKF101 is suppressed while FKF100 still
occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b830c68d-f0e0-402b-97bf-bcf084e1a54a
📒 Files selected for processing (4)
README.mdbenchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/FlatteningMapBenchmark.csdocs/generator.mdtests/FreakyKit.Forge.Analyzers.Tests/ForgeIgnoreSideTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- benchmarks/FreakyKit.Forge.Benchmarks/Benchmarks/FlatteningMapBenchmark.cs
- docs/generator.md
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
New Features
Benchmarks
Diagnostics
Tests