Add LQRS010 code fix for extracting projections into LinqraftMapping declarations#346
Conversation
Agent-Logs-Url: https://github.com/arika0093/Linqraft/sessions/5cb38fa0-17fe-4725-8ea3-edb6a758576a Co-authored-by: arika0093 <4524647+arika0093@users.noreply.github.com>
Agent-Logs-Url: https://github.com/arika0093/Linqraft/sessions/5cb38fa0-17fe-4725-8ea3-edb6a758576a Co-authored-by: arika0093 <4524647+arika0093@users.noreply.github.com>
Agent-Logs-Url: https://github.com/arika0093/Linqraft/sessions/5cb38fa0-17fe-4725-8ea3-edb6a758576a Co-authored-by: arika0093 <4524647+arika0093@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds the missing LQRS010 diagnostic + code fix to promote eligible Linqraft projections into reusable [LinqraftMapping] declarations, along with docs and smoke coverage to validate the new behavior end-to-end.
Changes:
- Introduces hidden design diagnostic
LQRS010for supported projection forms and suppresses it inside[LinqraftMapping]methods / untyped anonymous fluent projections. - Implements
LQRS010code fix to rewrite call sites toProjectTo...(...)and add a new*MappingExtensions.csdeclaration file, including capture-to-parameter lifting. - Adds documentation and analyzer/code-fix smoke tests covering positive/negative cases (including capture and
GroupByExpr).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Linqraft.Tests.Analyzer/AnalyzerSmokeTests.cs | Adds LQRS010 analyzer smoke tests and updates expectations to include/exclude the new diagnostic. |
| tests/Linqraft.Tests.Analyzer/AnalyzerCodeFixSmokeTests.cs | Adds code-fix smoke tests validating call-site rewrite + new mapping file emission (including capture/group-by cases). |
| src/Linqraft.Analyzer/LinqraftCompositeCodeFixProvider.cs | Registers LQRS010 fix and implements projection→mapping conversion, mapping file generation, and capture/outer-ref parameterization. |
| src/Linqraft.Analyzer/LinqraftCompositeAnalyzer.cs | Adds analyzer pass that reports LQRS010 for eligible projections and suppresses inside mapping methods/untyped anonymous projections. |
| src/Linqraft.Analyzer/DiagnosticDescriptors.cs | Declares the new LQRS010 DiagnosticDescriptor and includes it in the exported descriptor list. |
| src/Linqraft.Analyzer/AnalyzerHelpers.cs | Adds helper utilities for extracting projection lambdas and detecting explicit projection result typing. |
| docs/analyzers/LQRS010.md | Documents when LQRS010 triggers, when it doesn’t, and what the code fix generates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| diagnostics.Select(diagnostic => diagnostic.Id).ShouldBe(["LQRS010"]); | ||
| } |
There was a problem hiding this comment.
This assertion now expects LQRS010, but the test name above still says "does_not_report_diagnostics". Please rename the test to match the new expected behavior so failures are easier to interpret.
| if ( | ||
| invocation.Expression is not MemberAccessExpressionSyntax memberAccess | ||
| ) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| if ( | ||
| memberAccess.Expression is InvocationExpressionSyntax useLinqraftInvocation | ||
| && AnalyzerHelpers.GetInvocationName(useLinqraftInvocation.Expression) == "UseLinqraft" | ||
| ) | ||
| { | ||
| return (useLinqraftInvocation.Expression as MemberAccessExpressionSyntax)?.Expression; | ||
| } | ||
|
|
||
| return memberAccess.Expression; |
There was a problem hiding this comment.
GetProjectionSourceExpression only handles member-access invocations and returns null otherwise. However, the analyzer can report LQRS010 for non-member-access forms (e.g., using static ...; SelectExpr<TIn,TDto>(source, ...) or SomeExtensions.SelectExpr(source, ...)), which would make the code fix either no-op or generate an invalid caller (SomeExtensions.ProjectToXxx(...)). Consider deriving the source from the first invocation argument when the call isn’t an instance/member-access form, and ensure CreateMappingDeclarationInvocation can rewrite those shapes too.
| if ( | |
| invocation.Expression is not MemberAccessExpressionSyntax memberAccess | |
| ) | |
| { | |
| return null; | |
| } | |
| if ( | |
| memberAccess.Expression is InvocationExpressionSyntax useLinqraftInvocation | |
| && AnalyzerHelpers.GetInvocationName(useLinqraftInvocation.Expression) == "UseLinqraft" | |
| ) | |
| { | |
| return (useLinqraftInvocation.Expression as MemberAccessExpressionSyntax)?.Expression; | |
| } | |
| return memberAccess.Expression; | |
| // Instance / member-access form: source.SelectExpr(...) | |
| if (invocation.Expression is MemberAccessExpressionSyntax memberAccess) | |
| { | |
| if ( | |
| memberAccess.Expression is InvocationExpressionSyntax useLinqraftInvocation | |
| && AnalyzerHelpers.GetInvocationName(useLinqraftInvocation.Expression) == "UseLinqraft" | |
| ) | |
| { | |
| return (useLinqraftInvocation.Expression as MemberAccessExpressionSyntax)?.Expression; | |
| } | |
| return memberAccess.Expression; | |
| } | |
| // Non-member-access forms, e.g.: | |
| // using static ...; | |
| // SelectExpr<TIn, TDto>(source, ...) | |
| // In these cases, the projection source is the first invocation argument. | |
| if (invocation.ArgumentList.Arguments.Count == 0) | |
| { | |
| return null; | |
| } | |
| var firstArgumentExpression = invocation.ArgumentList.Arguments[0].Expression; | |
| if ( | |
| firstArgumentExpression is InvocationExpressionSyntax useLinqraftArgumentInvocation | |
| && AnalyzerHelpers.GetInvocationName(useLinqraftArgumentInvocation.Expression) == "UseLinqraft" | |
| ) | |
| { | |
| return (useLinqraftArgumentInvocation.Expression as MemberAccessExpressionSyntax)?.Expression; | |
| } | |
| return UnwrapParentheses(firstArgumentExpression); |
| var namespaceName = semanticModel.GetEnclosingSymbol( | ||
| invocation.SpanStart, | ||
| CancellationToken.None | ||
| )?.ContainingNamespace; |
There was a problem hiding this comment.
CreateMappingDocumentSource uses CancellationToken.None when calling semanticModel.GetEnclosingSymbol(...). Please thread through the provided cancellationToken from the code fix to keep the operation cancellable (especially for larger solutions) and consistent with the rest of the code.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d9fd14f0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expression = UnwrapParentheses(expression); | ||
| var symbol = semanticModel.GetSymbolInfo(expression, cancellationToken).Symbol; | ||
| return symbol is not null && localSymbols.Contains(symbol); |
There was a problem hiding this comment.
Detect local roots before capturing member accesses
IsLocalAccess only checks the symbol of the immediate expression, so chains like entity.Child.Name are treated as non-local because entity.Child resolves to a property symbol rather than the lambda parameter. In CollectOuterReferenceCandidates this causes nested member accesses rooted in lambda locals to be promoted into mapping parameters, and the caller rewrite then emits arguments such as entity.Child.Name outside the lambda scope, which does not compile. This will break common projections that access nested properties.
Useful? React with 👍 / 👎.
| semanticModel, | ||
| cancellationToken | ||
| ); | ||
| var mappingMethodName = $"ProjectTo{dtoSimpleName}"; |
There was a problem hiding this comment.
Ensure generated ProjectTo method names are unique
The generated mapping method name is derived only from the DTO simple name (ProjectTo{dtoSimpleName}), so applying the fix to multiple projections with the same source/DTO pair will generate duplicate extension signatures (e.g., two ProjectToEntityDto(this LinqraftMapper<Entity> source) methods). GetUniqueDocumentName avoids file-name collisions but does not prevent method-signature collisions, leading to compile-time duplicate-member errors.
Useful? React with 👍 / 👎.
arika0093
left a comment
There was a problem hiding this comment.
- The inference of local variables is incorrect. For example, the variable in
s => s.child3.Idshould not be captured, but it is actually captured asid. - The generated content below
=> query.Select<TDto>(...)also needs to be properly formatted and line-broken. It should not be output all in one line.
|
@copilot address PR #346 all reviews |
Agent-Logs-Url: https://github.com/arika0093/Linqraft/sessions/2a5050be-6aee-44b6-b7d4-cd8be1f9fe6b Co-authored-by: arika0093 <4524647+arika0093@users.noreply.github.com>
Addressed in 3f0c236. Member-access chains rooted in the lambda source are no longer inferred as captures, so cases like Done in 3f0c236. I addressed the requested follow-ups from the review set that apply here: fixed the capture inference issue, threaded the cancellation token through the enclosing-symbol lookup, renamed the mismatched analyzer test, and hardened the code-fix source-rewriting path while adding regression tests and revalidating the full solution. |
|
@copilot The indentation should be improved a bit more. current: [global::Linqraft.LinqraftMapping]
internal static global::System.Linq.IQueryable<global::Linqraft.ApiSample.OrderDto> ProjectToOrderDto(this global::Linqraft.LinqraftMapper<global::Linqraft.ApiSample.Order> source)
=>
source.Select<OrderDto>(s => new
{
Id = s.Id,
CustomerName = s.Customer?.Name,
CustomerCountry = s.Customer?.Address?.Country?.Name,
CustomerCity = s.Customer?.Address?.City?.Name,
Items = s.OrderItems.Select(oi => new
{
ProductName = oi.Product?.Name,
Quantity = oi.Quantity,
}),
});should be: [global::Linqraft.LinqraftMapping]
internal static global::System.Linq.IQueryable<global::Linqraft.ApiSample.OrderDto> ProjectToOrderDto(this global::Linqraft.LinqraftMapper<global::Linqraft.ApiSample.Order> source)
=> source.Select<OrderDto>(s => new {
Id = s.Id,
CustomerName = s.Customer?.Name,
CustomerCountry = s.Customer?.Address?.Country?.Name,
CustomerCity = s.Customer?.Address?.City?.Name,
Items = s.OrderItems.Select(oi => new
{
ProductName = oi.Product?.Name,
Quantity = oi.Quantity,
}),
}); |
Agent-Logs-Url: https://github.com/arika0093/Linqraft/sessions/92407d98-303b-4ffb-b0c4-ab878d8f0212 Co-authored-by: arika0093 <4524647+arika0093@users.noreply.github.com>
Agent-Logs-Url: https://github.com/arika0093/Linqraft/sessions/92407d98-303b-4ffb-b0c4-ab878d8f0212 Co-authored-by: arika0093 <4524647+arika0093@users.noreply.github.com>
Agent-Logs-Url: https://github.com/arika0093/Linqraft/sessions/92407d98-303b-4ffb-b0c4-ab878d8f0212 Co-authored-by: arika0093 <4524647+arika0093@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static bool IsCaptureEligible(ISymbol? symbol, ISet<ISymbol> localSymbols) | ||
| { | ||
| if (symbol is null || localSymbols.Contains(symbol)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (symbol is ILocalSymbol local && local.IsConst) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (symbol is IFieldSymbol field && field.IsConst) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return symbol.Kind | ||
| is SymbolKind.Local | ||
| or SymbolKind.Parameter | ||
| or SymbolKind.Field | ||
| or SymbolKind.Property; | ||
| } |
There was a problem hiding this comment.
IsCaptureEligible treats any field/property symbol as capture-eligible, including static fields/properties. The code fix will then turn things like DateTime.Now, Environment.MachineName, or unqualified static members into mapping parameters, which can change semantics and query translation (e.g., server-side GETDATE() becomes a client-evaluated constant). Consider excluding IFieldSymbol/IPropertySymbol where IsStatic == true (and possibly other non-value symbols) from capture eligibility.
| foreach (var memberAccess in lambda.DescendantNodes().OfType<MemberAccessExpressionSyntax>()) | ||
| { | ||
| var symbol = semanticModel.GetSymbolInfo(memberAccess, cancellationToken).Symbol; | ||
| if ( | ||
| symbol is not IFieldSymbol and not IPropertySymbol | ||
| || IsRootedInLocalAccess( | ||
| memberAccess, | ||
| semanticModel, | ||
| cancellationToken, | ||
| localSymbols | ||
| ) | ||
| || !seenSymbols.Add(symbol) | ||
| ) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| yield return new OuterReferenceCandidate( | ||
| AnalyzerHelpers.GetCaptureMemberName(memberAccess), | ||
| memberAccess, | ||
| symbol | ||
| ); | ||
| } |
There was a problem hiding this comment.
CollectOuterReferenceCandidates captures every MemberAccessExpressionSyntax whose symbol is a field/property and not rooted in a local. This will also pick up intermediate receiver accesses in a chain (e.g., both this.User and this.User.Id), producing redundant/unused mapping parameters and extra caller arguments. Consider skipping member-access nodes that are the receiver of a larger member access (i.e., node.Parent is MemberAccessExpressionSyntax parent && parent.Expression == node) so only the full access actually used as a value is captured.
- Exclude static fields and properties from capture eligibility in IsCaptureEligible to prevent DateTime.Now, Environment.MachineName and similar static members from being turned into mapping parameters. - Skip intermediate receiver nodes in CollectOuterReferenceCandidates: member-access expressions that are the .Expression of a parent member-access (e.g. this.User in this.User.Id) are now skipped so only the outermost / full access is captured as a parameter. - Ensure unique mapping method and class names when GetUniqueDocumentName must append a suffix to avoid file-name collisions. The same numeric suffix is now applied to both mappingClassName and mappingMethodName before the caller invocation and mapping document are built, preventing duplicate extension-method signatures when the code fix is applied multiple times for the same DTO type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This adds the missing
LQRS010code fix for Linqraft projections that should be promoted into reusable mapping declarations. It covers typed fluent/Expr projection forms and generates the corresponding[LinqraftMapping]template plus caller-sideProjectTo...(...)usage.Analyzer
LQRS010as a hidden design diagnostic.UseLinqraft().Select<TDto>(...)SelectExpr<TSource, TDto>(...)SelectManyExpr(...)GroupByExpr(...)new MyDto { ... }[LinqraftMapping]method or for untyped anonymous fluent projections.Code fix
query.UseLinqraft().Select<MyDto>(...)→query.ProjectToMyDto(...)MyDtoMappingExtensions.cs[LinqraftMapping]template method usingLinqraftMapper<TSource>and mapper-surface projection calls.capture:transport into explicit mapping method parameters.Projection shape handling
SelectExpr/SelectManyExpr/GroupByExprbodies into the corresponding mapper API shape.Documentation and coverage
LQRS010.GroupByExprscenarios.Example:
becomes:
with generated declaration source like:
⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.