chore: Separate core functionalities into Linqraft.Core project #31
Conversation
|
Warning Rate limit exceeded@arika0093 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
WalkthroughExtracted Roslyn-based DTO analysis and generation utilities into a new public Linqraft.Core project, promoted multiple types from internal to public, added analysis helpers and code-generation methods, and wired Linqraft.Core into the source generator as an analyzer reference; adjusted project/lockfile targets and mappings. Changes
Sequence Diagram(s)sequenceDiagram
participant SG as SourceGenerator
participant Core as Linqraft.Core (analyzer)
participant Roslyn as Compilation/SemanticModel
Note over SG,Core: Build-time source generation flow
SG->>Roslyn: Parse invocation & semantic model
SG->>Core: Call SelectExpr analysis (types, AnalyzeExpression/AnalyzeNamedType)
Core->>Roslyn: Query symbols / types as needed
Core-->>SG: Return DtoStructure / GenerateDtoClassInfo results
SG->>SG: Emit generated DTO code (from BuildCode())
alt error
SG-->>Developer: Diagnostic / skip generation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
arika0093
force-pushed
the
feat/split-core-lib
branch
from
b39811b to
3317d70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Linqraft.Core/DtoProperty.cs (1)
33-141: ChangeHasNullableAccessto useDescendantNodesAndSelf()instead ofDescendantNodes()The bug is confirmed. At line 196,
HasNullableAccessusesDescendantNodes(), which excludes the root node. When the entire expression is aConditionalAccessExpressionSyntax(e.g.,s.Child?.Name), it fails to detect the?.operator at the top level, incorrectly leavingIsNullableasfalse. This causes generated DTO properties to be marked as non-nullable when they should be nullable.The proposed fix correctly addresses this:
- var conditionalAccesses = expression - .DescendantNodes() - .OfType<ConditionalAccessExpressionSyntax>(); + var conditionalAccesses = expression + .DescendantNodesAndSelf() + .OfType<ConditionalAccessExpressionSyntax>();The lambda-exclusion logic is preserved—nested conditional accesses inside Select lambdas will still be correctly filtered out by the
isInsideLambdacheck.
🧹 Nitpick comments (6)
src/Linqraft.Core/GenerateDtoClassInfo.cs (1)
57-141: Clarify or encapsulate namespace handling inBuildCode
BuildCodecurrently emits only classes (including nested parent classes) without anynamespaceblock, even thoughNamespaceis tracked on the model and used forFullName/type references. That’s fine if the caller is expected to wrap the result in anamespace { ... }construct, but it’s easy to misuse.Consider either:
- documenting explicitly that
BuildCodereturns code without a namespace and must be wrapped by the caller, or- adding an overload/helper (e.g.,
BuildCodeWithNamespace()) that includes the namespace wrapper, usingNamespacedirectly, to reduce call-site boilerplate and mistakes.src/Linqraft.Core/DtoStructure.cs (2)
49-113: AnalyzeNamedType skips positional constructor arguments; confirm this is intentional
AnalyzeNamedTypeonly populatesDtoStructure.Propertiesfrom:
- named ctor arguments (
arg.NameColon) and- object initializer assignments.
Positional arguments like
new MyDto(s.Id, s.Name)are ignored and result in no properties, which in turn causesGenerateSelectExprCodesto skip generation whenProperties.Count == 0. If you intend to support positional DTO constructors, you’ll need to:
- resolve the ctor
IMethodSymbolfromreturnType, and- map arguments by position to parameters, then to corresponding properties.
Otherwise, consider documenting that
SelectExprrequires named arguments or object initializer syntax for DTOs.You may want tests that cover
new MyDto(Id: s.Id, Name: s.Name),new MyDto { Id = s.Id }, andnew MyDto(s.Id, s.Name)to lock in the intended behavior.
123-157: Implicit property name inference is intentionally narrow; consider extending or documenting
AnalyzeAnonymousType+GetImplicitPropertyNameonly infer names for:
- simple member access (
s.Id) and- bare identifiers (
id),skipping any more complex expressions (e.g.,
order.Customer!.Name, casts, method calls). This keeps things predictable but means those anonymous properties are silently dropped from the DTO structure.If that’s not the desired behavior, consider:
- walking into simple unary/cast wrappers to recover the underlying member name, or
- at least logging/diagnosing dropped properties in the generator, so users understand why some properties are ignored.
It would be helpful to add tests around anonymous projections using null-forgiving (
!), simple casts, or trivial wrappers to confirm the currently intended behavior.src/Linqraft.Core/SelectExprInfo.cs (3)
116-155: Nested Select conversion is powerful but fragile; consider more targeted guards and tests
ConvertNestedSelectWithRoslyn+ExtractSelectInfoFromSyntaxcover a lot of syntax forms (conditional access,?? [], chained calls like.ToList()), which is great. A few points to watch:
- Detection of
IEnumerablevsIQueryableviaIsEnumerableInvocation()usesToDisplayString().Contains("IQueryable"/"IEnumerable")and simple interface-name checks. This can misclassify exotic or custom types whose names happen to contain those substrings.- When
ExtractSelectInfoFromSyntaxfails, you fall back tosyntax.ToString(), andGeneratePropertyAssignmentlabels this with/* CONVERSION FAILED */only when the original expression string still contains"Select". That’s helpful, but it also means certain edge cases silently skip conversion if they don’t match your current patterns.- The string‑normalization steps (regex whitespace compaction and
.Replace("?.", ".")for base expressions) are generally safe but can get brittle when new syntax patterns are introduced.Given how central this is to nested DTO generation, I’d recommend:
- adding focused tests for the shapes you intend to support (plain
.Select,?.Select,?.Select(...).ToList() ?? [], chained LINQ, etc.), and- adding at least one negative test where you deliberately fall back to the original expression to ensure the diagnostics (
/* CONVERSION FAILED */) behave as expected.You might also consider gradually moving more of the transformation to true Roslyn tree rewrites (rather than string manipulation) as coverage grows, but that can be deferred.
Also applies to: 233-307, 315-438
194-228: Nullable-access rewriting may change semantics when typeSymbol isn’t nullable; tighten defaults
GeneratePropertyAssignmentroutes nullable conditional access throughConvertNullableAccessToExplicitCheckWithRoslynwhenproperty.IsNullableand there’s aConditionalAccessExpressionSyntax. The conversion then:
- builds a chain of
x != null && x.Child != nullchecks,- casts to
typeSymbol.ToDisplayString(), and- uses
GetDefaultValueForType(typeSymbol)for the: defaultValuebranch.Potential issues:
- When
property.IsNullableis set via syntax heuristics buttypeSymbol.NullableAnnotationis not annotated (e.g., Roslyn reports non‑nullable for a syntactically nullable access you special‑case),GetDefaultValueForTypereturnsdefaultfor value types instead ofnull. That changes semantics vs. the original?.operator, which would normally yieldnullfor aNullable<T>result.- The special casing in
GetDefaultValueForTypefor strings (SpecialType.System_String => "string.Empty") is currently unreachable because all reference types (including string) exit early with"null". Either the switch or the comment should be aligned with the actual behavior.nullableTypeNameis always set to a cast oftypeSymbolValue; if that type string does not carry the?annotation when you conceptually expect a nullable result, you again risk returning non‑nullable values when nulls were expected.I’d suggest:
- ensuring that when
property.IsNullableis true because of syntax (e.g.,?.), thetypeSymbolpassed in is the nullable version, or- explicitly treating the “syntactic nullable but non‑annotated typeSymbol” case as nullable in
GetDefaultValueForTypeand when building the cast/conditional expression.Tests around
c.Child?.Idtargeting bothintandint?DTO properties (with and without??coalescing) would help confirm the rewritten expression preserves original C# semantics.Also applies to: 443-496, 520-536
498-515: Minor duplication and small polish opportunitiesTwo small, non-blocking observations:
GetImplicitPropertyNamehere duplicates the logic of the private static helper inDtoStructure. If you keep expanding support for implicit names, consider centralizing this in a shared helper to avoid divergence.GetAccessibilityStringdefaults to"public"for unexpected cases, which is fine, but you may want to treatAccessibility.NotApplicabledifferently (e.g., throw or log) if it ever shows up for symbols you expect to map to real types.These are minor and can be deferred; calling them out now may prevent subtle inconsistencies later.
If you decide to centralize implicit-name logic, a small unit test suite around that helper will help keep its behavior stable as you evolve it.
Also applies to: 541-553
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Linqraft.sln(3 hunks)src/Linqraft.Core/DtoProperty.cs(2 hunks)src/Linqraft.Core/DtoStructure.cs(3 hunks)src/Linqraft.Core/GenerateDtoClassInfo.cs(2 hunks)src/Linqraft.Core/Linqraft.Core.csproj(1 hunks)src/Linqraft.Core/SelectExprInfo.cs(10 hunks)src/Linqraft.Core/SelectExprInfoAnonymous.cs(1 hunks)src/Linqraft.Core/SelectExprInfoExplicitDto.cs(6 hunks)src/Linqraft.Core/SelectExprInfoNamed.cs(1 hunks)src/Linqraft.Core/packages.lock.json(1 hunks)src/Linqraft.SourceGenerator/Linqraft.SourceGenerator.csproj(2 hunks)src/Linqraft.SourceGenerator/SelectExprGenerator.cs(1 hunks)src/Linqraft.SourceGenerator/SelectExprGroups.cs(1 hunks)src/Linqraft.SourceGenerator/packages.lock.json(1 hunks)src/Linqraft/Linqraft.csproj(0 hunks)src/Linqraft/packages.lock.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/Linqraft/Linqraft.csproj
🧰 Additional context used
🧬 Code graph analysis (7)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (3)
src/Linqraft.SourceGenerator/SelectExprInfo.cs (2)
SelectExprInfo(14-694)List(23-23)tests/Linqraft.Tests/IEnumerableCaseTest.cs (1)
IEnumerableCaseTest(6-89)src/Linqraft.SourceGenerator/SelectExprInfoExplicitDto.cs (1)
SelectExprInfoExplicitDto(15-203)
src/Linqraft.Core/GenerateDtoClassInfo.cs (5)
src/Linqraft.Core/DtoStructure.cs (2)
DtoStructure(49-114)DtoStructure(123-157)src/Linqraft.Core/SelectExprInfo.cs (1)
DtoStructure(50-50)src/Linqraft.Core/SelectExprInfoAnonymous.cs (1)
DtoStructure(28-31)src/Linqraft.Core/SelectExprInfoExplicitDto.cs (1)
DtoStructure(124-127)src/Linqraft.Core/SelectExprInfoNamed.cs (1)
DtoStructure(28-31)
src/Linqraft.Core/SelectExprInfoNamed.cs (4)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (2)
SelectExprInfoNamed(185-224)SelectExprInfo(87-127)src/Linqraft.Core/SelectExprInfo.cs (6)
List(45-45)List(87-103)DtoStructure(50-50)GetClassName(55-55)GetParentDtoClassName(60-60)GetDtoNamespace(65-65)src/Linqraft.Core/SelectExprInfoAnonymous.cs (5)
List(23-23)DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-41)GetDtoNamespace(47-47)src/Linqraft.Core/DtoStructure.cs (2)
DtoStructure(49-114)DtoStructure(123-157)
src/Linqraft.Core/SelectExprInfo.cs (8)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (1)
SelectExprInfo(87-127)src/Linqraft.Core/DtoProperty.cs (1)
InvocationExpressionSyntax(218-274)src/Linqraft.Core/SelectExprInfoAnonymous.cs (6)
List(23-23)DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-41)GetDtoNamespace(47-47)GenerateSelectExprMethod(52-89)src/Linqraft.Core/SelectExprInfoExplicitDto.cs (9)
List(45-50)List(52-101)List(106-119)DtoStructure(124-127)GetClassName(132-133)GetParentDtoClassName(138-138)GetDtoNamespace(143-143)GetNestedDtoFullName(148-157)GenerateSelectExprMethod(180-232)src/Linqraft.Core/SelectExprInfoNamed.cs (6)
List(23-23)DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-42)GetDtoNamespace(48-49)GenerateSelectExprMethod(54-95)src/Linqraft.Core/GenerateDtoClassInfo.cs (1)
GenerateDtoClassInfo(11-142)src/Linqraft.Core/DtoStructure.cs (3)
DtoStructure(49-114)DtoStructure(123-157)GetUniqueId(28-40)src/Linqraft.SourceGenerator/SelectExprGroups.cs (1)
GetUniqueId(33-40)
src/Linqraft.Core/DtoStructure.cs (5)
src/Linqraft.Core/SelectExprInfo.cs (3)
DtoStructure(50-50)List(45-45)List(87-103)src/Linqraft.Core/SelectExprInfoAnonymous.cs (2)
DtoStructure(28-31)List(23-23)src/Linqraft.Core/SelectExprInfoExplicitDto.cs (4)
DtoStructure(124-127)List(45-50)List(52-101)List(106-119)src/Linqraft.Core/SelectExprInfoNamed.cs (2)
DtoStructure(28-31)List(23-23)src/Linqraft.Core/DtoProperty.cs (1)
DtoProperty(33-142)
src/Linqraft.Core/SelectExprInfoAnonymous.cs (3)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (2)
SelectExprInfoAnonymous(144-183)SelectExprInfo(87-127)src/Linqraft.Core/SelectExprInfo.cs (6)
List(45-45)List(87-103)DtoStructure(50-50)GetClassName(55-55)GetParentDtoClassName(60-60)GetDtoNamespace(65-65)src/Linqraft.Core/SelectExprInfoNamed.cs (5)
List(23-23)DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-42)GetDtoNamespace(48-49)
src/Linqraft.Core/SelectExprInfoExplicitDto.cs (6)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (2)
SelectExprInfoExplicitDto(226-292)SelectExprInfo(87-127)src/Linqraft.Core/DtoStructure.cs (3)
DtoStructure(49-114)DtoStructure(123-157)GetUniqueId(28-40)src/Linqraft.Core/SelectExprInfo.cs (5)
DtoStructure(50-50)GetClassName(55-55)GetUniqueId(160-165)GetParentDtoClassName(60-60)GetDtoNamespace(65-65)src/Linqraft.Core/SelectExprInfoAnonymous.cs (4)
DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-41)GetDtoNamespace(47-47)src/Linqraft.Core/SelectExprInfoNamed.cs (4)
DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-42)GetDtoNamespace(48-49)src/Linqraft.SourceGenerator/SelectExprGroups.cs (1)
GetUniqueId(33-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest)
🔇 Additional comments (21)
src/Linqraft.Core/Linqraft.Core.csproj (1)
1-9: Minimal core project definition looks consistentThe SDK project file is structurally correct and keeps Linqraft.Core’s package surface minimal, which fits the new core-library role.
src/Linqraft.Core/DtoProperty.cs (1)
8-23: PublicDtoProperty+TypeNamehelper are a solid abstractionMaking
DtoPropertypublic inLinqraft.Coreand exposingTypeNamebased onTypeSymbol.ToDisplayStringis a clean way to surface type metadata for downstream generators; the API shape here looks good.src/Linqraft.Core/GenerateDtoClassInfo.cs (1)
8-37: DTO metadata model andFullNamecomputation are coherentThe combination of
Structure,Accessibility,ClassName,Namespace,ParentClasses, andNestedClasses, plus theFullNameproperty, gives a clear, composable model for DTO generation and nested-type resolution. This should make consumers ofLinqraft.Corestraightforward to implement.Also applies to: 49-55
src/Linqraft.Core/packages.lock.json (1)
1-132: Lock file aligns with the new Core project’s dependency graphThe
.NETStandard,Version=v2.0lock file looks consistent with introducingLinqraft.Coreas a Roslyn-based core library and should help keep restores deterministic.src/Linqraft/packages.lock.json (1)
118-130: Project dependency graph updates reflect the Core/SourceGenerator splitAdding the
linqraft.coreproject entry and wiringlinqraft.sourcegeneratorto depend on it matches the new architecture and keeps the NuGet graph coherent.src/Linqraft.SourceGenerator/SelectExprGroups.cs (1)
7-7: LGTM!The addition of the
using Linqraft.Core;directive correctly enables access to types that have been moved to the new Linqraft.Core project.src/Linqraft.SourceGenerator/SelectExprGenerator.cs (1)
7-7: LGTM!The addition of the
using Linqraft.Core;directive correctly enables access to the SelectExprInfo types that have been relocated to the Linqraft.Core project.src/Linqraft.SourceGenerator/packages.lock.json (1)
130-136: LGTM!The lockfile correctly reflects the new project dependency on Linqraft.Core, including its transitive dependency on Microsoft.CodeAnalysis.CSharp version 4.13.0 or higher.
src/Linqraft.SourceGenerator/Linqraft.SourceGenerator.csproj (2)
14-19: LGTM!The change from wildcard
$(OutputPath)\*.dllto the specific$(OutputPath)\Linqraft.SourceGenerator.dllis more precise and prevents unintended DLLs from being packaged.
38-40: LGTM!The ProjectReference to Linqraft.Core with
OutputItemType="Analyzer"follows the correct pattern for Roslyn analyzer dependencies, ensuring Linqraft.Core.dll is properly included in the analyzer package.Linqraft.sln (3)
37-38: LGTM!The new Linqraft.Core project is correctly added to the solution with a unique GUID.
145-156: LGTM!All necessary build configuration mappings are present for the Linqraft.Core project across all platforms and configurations.
170-170: LGTM!The Linqraft.Core project is correctly nested under the src solution folder.
src/Linqraft.Core/SelectExprInfoExplicitDto.cs (2)
10-10: Namespace migration aligns with PR objectives.The namespace change from
LinqrafttoLinqraft.Corecorrectly reflects the move to the new core project.
15-40: Public API exposure established.The type is now public with well-documented required properties. This creates a public API surface that should be maintained with care, considering semantic versioning for any future changes.
src/Linqraft.Core/SelectExprInfoAnonymous.cs (2)
8-8: Namespace migration aligns with PR objectives.The namespace change to
Linqraft.Corecorrectly reflects the move to the new core project.
13-18: Public API exposure established with clear documentation.The type is now public with a well-documented required property. The documentation clearly indicates this is for anonymous type Select expressions.
src/Linqraft.Core/SelectExprInfoNamed.cs (2)
8-8: Namespace migration aligns with PR objectives.The namespace change to
Linqraft.Corecorrectly reflects the move to the new core project.
13-18: Public API exposure established with clear documentation.The type is now public with a well-documented required property. The documentation clearly indicates this is for named (predefined DTO) Select expressions.
src/Linqraft.Core/DtoStructure.cs (1)
12-40: DtoStructure metadata and ID generation look coherent with DTO pipelineThe public record shape (SourceType, Properties) and the helpers
SourceTypeName,SourceTypeFullName, andGetUniqueId()line up with howGenerateDtoClassInfoand the variousSelectExprInfo*classes consume DTO metadata; the ID is stable and incorporates both source type and property structure. No issues from a correctness standpoint.It’s still worth adding a couple of tests ensuring
GetUniqueId()stays stable across runs and changes only when properties or source type change (e.g., reordering properties vs. renaming them).src/Linqraft.Core/SelectExprInfo.cs (1)
15-46: Public SelectExprInfo surface and SelectExpr code generation flow look consistentThe required-init properties (
SourceType,SemanticModel,Invocation,LambdaParameterName,CallerNamespace) and theGenerateSelectExprCodesworkflow (buildDtoStructure, short‑circuit on empty properties, then delegate toGenerateSelectExprMethod) align cleanly with the concreteSelectExprInfo*implementations and the generator usage. No correctness issues stand out.If you later notice
GenerateDtoStructure()being called frequently for the same invocation (e.g., via bothGenerateSelectExprCodesandGetUniqueId()), consider caching the structure in a private field for reuse, but that’s an optimization rather than a blocker.Worth adding a benchmark or at least a regression test that exercises the full SelectExpr pipeline on a large projection to ensure any recomputation here remains acceptable.
Also applies to: 84-103
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Linqraft.Core/SelectExprInfoNamed.cs (1)
57-98: I need to understand if the generated code properly handles potential null values from failed casts.Add null validation after the type cast to prevent NullReferenceException.
The double-casting pattern (
as object as TargetType) is consistent across SelectExprInfoNamed, SelectExprInfoAnonymous, and SelectExprInfoExplicitDto classes. While this pattern is intentional for the interceptor mechanism, theasoperator returns null if the conversion isn't possible and never throws an exception.The generated code does not validate that the cast succeeded before calling
.Select()on line 79:
- Line 77:
var matchedQuery = query as object as {returnTypePrefix}<{querySourceTypeFullName}>;- Line 79:
var converted = matchedQuery.Select(...)— unsafe if matchedQuery is nullAdd a null check after line 77 to handle cases where the runtime type doesn't match the expected type, preventing NullReferenceException at runtime. For example:
var matchedQuery = query as object as {returnTypePrefix}<{querySourceTypeFullName}>; if (matchedQuery == null) throw new InvalidCastException($"Query type is not compatible with {returnTypePrefix}<{querySourceTypeFullName}>");
🧹 Nitpick comments (3)
src/Linqraft.Core/DtoProperty.cs (1)
11-18: Consider usingConvertedTypeas a fallback inAnalyzeExpression
AnalyzeExpressioncurrently returnsnullwhensemanticModel.GetTypeInfo(expression).Typeisnull, even thoughConvertedTypemay still be available (e.g., for some conversions and lambdas). UsingtypeInfo.Type ?? typeInfo.ConvertedTypewould make this analysis more robust and avoid silently dropping properties in those cases.Everything else in the nullable and nested-
Selecthandling looks internally consistent withDtoStructureandSelectExprInfousage.- var typeInfo = semanticModel.GetTypeInfo(expression); - if (typeInfo.Type is null) - return null; - - var propertyType = typeInfo.Type; + var typeInfo = semanticModel.GetTypeInfo(expression); + var resolvedType = typeInfo.Type ?? typeInfo.ConvertedType; + if (resolvedType is null) + return null; + + var propertyType = resolvedType;Also applies to: 20-24, 25-41, 44-56, 58-71, 73-132, 134-141
src/Linqraft.Core/SelectExprInfo.cs (1)
15-21: Optional: cacheDtoStructureinstead of recomputing it
GenerateSelectExprCodesandGetUniqueIdeach callGenerateDtoStructure(), which in turn analyzes Roslyn syntax (potentially non-trivial). If these methods are called multiple times per expression, you might want to cache the resultingDtoStructurein the instance to avoid repeated analysis. Not required for correctness, but it could reduce analyzer work in large projects.Also applies to: 42-51, 67-69, 85-104, 158-167
src/Linqraft.Core/DtoStructure.cs (1)
42-114: Optional: deduplicate implicit property-name extraction logic
AnalyzeAnonymousTypeuses a privateGetImplicitPropertyNamehelper that mirrors the protectedGetImplicitPropertyNamealready present onSelectExprInfo. While this isn’t incorrect, it does duplicate logic for deriving implicit property names from expressions (member access vs identifier). If you find yourself changing this heuristic later, consider centralizing it in one place to avoid divergence.Also applies to: 116-157, 159-175
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Linqraft.sln(3 hunks)src/Linqraft.Core/DtoProperty.cs(2 hunks)src/Linqraft.Core/DtoStructure.cs(3 hunks)src/Linqraft.Core/GenerateDtoClassInfo.cs(2 hunks)src/Linqraft.Core/Linqraft.Core.csproj(1 hunks)src/Linqraft.Core/SelectExprInfo.cs(11 hunks)src/Linqraft.Core/SelectExprInfoAnonymous.cs(1 hunks)src/Linqraft.Core/SelectExprInfoExplicitDto.cs(6 hunks)src/Linqraft.Core/SelectExprInfoNamed.cs(1 hunks)src/Linqraft.Core/packages.lock.json(1 hunks)src/Linqraft.SourceGenerator/Linqraft.SourceGenerator.csproj(2 hunks)src/Linqraft.SourceGenerator/SelectExprGenerator.cs(1 hunks)src/Linqraft.SourceGenerator/SelectExprGroups.cs(1 hunks)src/Linqraft.SourceGenerator/packages.lock.json(1 hunks)src/Linqraft/Linqraft.csproj(0 hunks)src/Linqraft/packages.lock.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/Linqraft/Linqraft.csproj
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Linqraft.Core/Linqraft.Core.csproj
- src/Linqraft.SourceGenerator/SelectExprGenerator.cs
- src/Linqraft.SourceGenerator/SelectExprGroups.cs
- src/Linqraft.SourceGenerator/Linqraft.SourceGenerator.csproj
- src/Linqraft.Core/GenerateDtoClassInfo.cs
- src/Linqraft.Core/packages.lock.json
- src/Linqraft.SourceGenerator/packages.lock.json
🧰 Additional context used
🧬 Code graph analysis (5)
src/Linqraft.Core/DtoStructure.cs (5)
src/Linqraft.Core/SelectExprInfo.cs (3)
DtoStructure(50-50)List(45-45)List(88-104)src/Linqraft.Core/SelectExprInfoAnonymous.cs (2)
DtoStructure(28-31)List(23-23)src/Linqraft.Core/SelectExprInfoExplicitDto.cs (4)
DtoStructure(124-127)List(45-50)List(52-101)List(106-119)src/Linqraft.Core/SelectExprInfoNamed.cs (2)
DtoStructure(28-31)List(23-23)src/Linqraft.Core/DtoProperty.cs (1)
DtoProperty(33-142)
src/Linqraft.Core/SelectExprInfoExplicitDto.cs (6)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (2)
SelectExprInfoExplicitDto(226-292)SelectExprInfo(87-127)src/Linqraft.Core/DtoStructure.cs (3)
DtoStructure(49-114)DtoStructure(123-157)GetUniqueId(28-40)src/Linqraft.Core/SelectExprInfo.cs (4)
DtoStructure(50-50)GetClassName(55-55)GetUniqueId(161-166)GetParentDtoClassName(60-60)src/Linqraft.Core/SelectExprInfoAnonymous.cs (3)
DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-41)src/Linqraft.Core/SelectExprInfoNamed.cs (3)
DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-42)src/Linqraft.SourceGenerator/SelectExprGroups.cs (1)
GetUniqueId(33-40)
src/Linqraft.Core/SelectExprInfoAnonymous.cs (5)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (2)
SelectExprInfoAnonymous(144-183)SelectExprInfo(87-127)src/Linqraft.Core/SelectExprInfo.cs (5)
List(45-45)List(88-104)DtoStructure(50-50)GetClassName(55-55)GetParentDtoClassName(60-60)src/Linqraft.Core/SelectExprInfoExplicitDto.cs (6)
List(45-50)List(52-101)List(106-119)DtoStructure(124-127)GetClassName(132-133)GetParentDtoClassName(138-138)src/Linqraft.Core/SelectExprInfoNamed.cs (4)
List(23-23)DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-42)src/Linqraft.Core/DtoStructure.cs (2)
DtoStructure(49-114)DtoStructure(123-157)
src/Linqraft.Core/SelectExprInfo.cs (7)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (1)
SelectExprInfo(87-127)src/Linqraft.Core/DtoProperty.cs (1)
InvocationExpressionSyntax(218-274)src/Linqraft.Core/SelectExprInfoAnonymous.cs (5)
List(23-23)DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-41)GenerateSelectExprMethod(50-87)src/Linqraft.Core/SelectExprInfoExplicitDto.cs (7)
List(45-50)List(52-101)List(106-119)DtoStructure(124-127)GetClassName(132-133)GetParentDtoClassName(138-138)GenerateSelectExprMethod(181-229)src/Linqraft.Core/SelectExprInfoNamed.cs (5)
List(23-23)DtoStructure(28-31)GetClassName(36-36)GetParentDtoClassName(41-42)GenerateSelectExprMethod(57-98)src/Linqraft.Core/GenerateDtoClassInfo.cs (1)
GenerateDtoClassInfo(11-142)src/Linqraft.Core/DtoStructure.cs (3)
DtoStructure(49-114)DtoStructure(123-157)GetUniqueId(28-40)
src/Linqraft.Core/SelectExprInfoNamed.cs (3)
src/Linqraft.SourceGenerator/SelectExprGenerator.cs (2)
SelectExprInfoNamed(185-224)SelectExprInfo(87-127)src/Linqraft.Core/SelectExprInfo.cs (7)
List(45-45)List(88-104)DtoStructure(50-50)GetClassName(55-55)GetParentDtoClassName(60-60)GetDtoNamespace(65-65)GetExprTypeString(68-68)src/Linqraft.Core/DtoStructure.cs (2)
DtoStructure(49-114)DtoStructure(123-157)
🪛 GitHub Actions: .NET Build and Test
src/Linqraft.Core/SelectExprInfoAnonymous.cs
[error] 13-13: 'SelectExprInfoAnonymous' does not implement inherited abstract member 'SelectExprInfo.GetExprTypeString()'
🪛 GitHub Check: build (ubuntu-latest)
src/Linqraft.Core/SelectExprInfoAnonymous.cs
[failure] 13-13:
'SelectExprInfoAnonymous' does not implement inherited abstract member 'SelectExprInfo.GetExprTypeString()'
[failure] 13-13:
'SelectExprInfoAnonymous' does not implement inherited abstract member 'SelectExprInfo.GetExprTypeString()'
🔇 Additional comments (13)
Linqraft.sln (1)
37-38: Linqraft.Core project wiring in solution looks consistentProject entry, configuration mappings for all platforms, and nesting under the
srcsolution folder are internally consistent and match the new GUID. No issues from the solution file side.Also applies to: 145-156, 170-170
src/Linqraft/packages.lock.json (1)
118-123: Lock file changes align with the new Core project; ensure they’re tool-generatedThe added
linqraft.coreproject node and the newLinqraft.Coredependency forlinqraft.sourcegeneratorare consistent with the new project layout. Please just confirm this lock file was regenerated viadotnet restore(or equivalent) so it stays in sync with the project files rather than being manually edited.Also applies to: 124-130
src/Linqraft.Core/SelectExprInfo.cs (1)
198-233: NestedSelectand null-conditional conversion logic looks coherentThe new
GeneratePropertyAssignment,ConvertNestedSelectWithRoslyn,ExtractSelectInfoFromSyntax, andConvertNullableAccessToExplicitCheckWithRoslynpaths are internally consistent: they only attempt Roslyn-based rewrites when appropriate and fall back tosyntax.ToString()when extraction fails. The safety check that annotates failed nested-Selectconversions with a comment is also helpful for debugging generated code.Also applies to: 237-311, 313-442, 444-500
src/Linqraft.Core/SelectExprInfoExplicitDto.cs (1)
15-31: Explicit DTO SelectExpr handling is consistent with the new core abstractionsThe public
SelectExprInfoExplicitDtorecord cleanly wires explicit DTO scenarios into the sharedSelectExprInfoinfrastructure: it derives structures from the anonymous projection, generates DTO metadata (including parent class/accessibility info), resolves the actual target namespace, and emits aSelectExpr_{id}method constrained onTResult : <explicit DTO>. Nothing stands out as problematic here.Also applies to: 42-101, 103-119, 121-139, 140-147, 148-158, 160-176, 178-229
src/Linqraft.Core/DtoStructure.cs (1)
12-24: DTO structure identity and source-type helpers look good
SourceTypeName,SourceTypeFullName, and the SHA256-basedGetUniqueIdare consistent with howSelectExprInfoandGenerateDtoClassInfoconsume them and should give stable DTO identifiers across runs.Also applies to: 25-40
src/Linqraft.Core/SelectExprInfoNamed.cs (8)
8-8: LGTM: Namespace change aligns with PR objectives.The namespace change from
LinqrafttoLinqraft.Corecorrectly reflects the relocation of core functionalities into the new project.
10-13: LGTM: Public API exposure is appropriate for core library.Promoting
SelectExprInfoNamedto public visibility with clear XML documentation is appropriate for a core library component that may be consumed by external code.
15-18: LGTM: ObjectCreation property is well-designed.The required init-only property correctly stores the object creation syntax node needed for DTO structure analysis, with appropriate documentation.
20-23: LGTM: Correct behavior for predefined types.Returning an empty list is correct for named types since they don't require DTO class generation—the types already exist.
33-36: LGTM: Straightforward class name resolution.Using the source type name from the structure is the correct approach for named/predefined types.
38-42: LGTM: Correct delegation for flat structure.For named types without nested DTOs, delegating
GetParentDtoClassNametoGetClassNameis the correct approach.
44-49: LGTM: Proper namespace resolution with fallback.The logic correctly retrieves the DTO's containing namespace with an appropriate fallback to
CallerNamespacewhen the namespace cannot be determined.
51-52: LGTM: Appropriate expression type identifier.Returning "predefined" correctly identifies this as a named/predefined type expression for documentation purposes.
| protected override DtoStructure GenerateDtoStructure() | ||
| { | ||
| return DtoStructure.AnalyzeNamedType(ObjectCreation, SemanticModel, SourceType)!; | ||
| } |
There was a problem hiding this comment.
Handle potential null return from AnalyzeNamedType.
The null-forgiving operator ! suppresses the nullable warning, but DtoStructure.AnalyzeNamedType can legitimately return null if analysis fails (e.g., invalid syntax or type information). This could cause a NullReferenceException when the structure is accessed.
Consider adding null handling:
protected override DtoStructure GenerateDtoStructure()
{
- return DtoStructure.AnalyzeNamedType(ObjectCreation, SemanticModel, SourceType)!;
+ var structure = DtoStructure.AnalyzeNamedType(ObjectCreation, SemanticModel, SourceType);
+ if (structure is null)
+ {
+ // Return empty structure or throw appropriate exception
+ return new DtoStructure(SourceType, new List<DtoProperty>());
+ }
+ return structure;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Linqraft.Core/SelectExprInfoNamed.cs around lines 28 to 31, the call to
DtoStructure.AnalyzeNamedType currently uses the null-forgiving operator and can
return null; replace the direct return with a null-check: call AnalyzeNamedType
into a local variable, and if it is null either throw a clear
InvalidOperationException (including context such as SourceType or
ObjectCreation location) or return a sensible fallback default DtoStructure;
ensure the method no longer uses the null-forgiving operator and provides a
descriptive error message when analysis fails.
arika0093
force-pushed
the
feat/split-core-lib
branch
from
eea4383 to
40f8ee1
Compare
feat: add workflow to verify NuGet package installation and functionality (#29) docs: update C# version requirements and remove Polysharp references in README files docs: update C# version requirements and add details on language features in README files Enhance method header generation to include location type (#30) * feat: enhance method header generation to include location type in SelectExpr records * feat: add expression type string for documentation in SelectExpr records chore: remove branch restrictions from NuGet package verification workflow fix: update descendant node retrieval to include the current node in HasNullableAccess method fix: enable locked mode for dotnet restore in workflow files refactor: streamline NuGet package verification workflow by consolidating directory creation and file copying steps refactor: remove unnecessary TargetFrameworks from project files refactor: remove Windows from build matrix due to performance issues chore: Separate core functionalities into Linqraft.Core project (#31) * chore: Separate core functionalities from SourceGenerator project into Linqraft.Core project * feat: add GetExprTypeString method for documentation of anonymous expression types * chore: disable documentation file generation in Directory.Build.props fix: add missing comma and restore additionalVersions in devcontainer.json fix: add missing PropertyGroup for DevelopmentDependency in Linqraft.csproj close #35 fix: remove PolySharp package reference from Directory.Build.props Revert "fix: add missing PropertyGroup for DevelopmentDependency in Linqraft.csproj" This reverts commit 7916999. Revert "fix: add missing comma and restore additionalVersions in devcontainer.json" This reverts commit 8a386ab. Reapply "fix: add missing comma and restore additionalVersions in devcontainer.json" This reverts commit 1743adc. Reapply "fix: add missing PropertyGroup for DevelopmentDependency in Linqraft.csproj" This reverts commit ad3f14a. Revert "fix: remove PolySharp package reference from Directory.Build.props" This reverts commit 5b1a189. fix: add PolySharp package reference and update LangVersion instructions in README files fix: update generic type parameters in SelectExpr methods for consistency fix: update build instructions to ensure a clean build without incremental compilation fix: add clean-test.sh script for easier test cleanup and build process fix: update SelectExpr method signatures for consistency across implementations close #33 fix: change Issue33_SealedPatternTest class to partial for extensibility fix: refactor DummyExpression class and update SelectExpr method summaries for clarity (#38)
Refactor the codebase by moving core functionalities from the SourceGenerator project to a new Linqraft.Core project, enhancing modularity and maintainability. This change includes updates to project references and dependencies.
Summary by CodeRabbit
New Features
Refactor
Chores