Clean up {SqlServer,Sqlite}SqlTranslatingExpressionVisitor #37450
Conversation
Especially around NativeAOT-friendly patterns.
roji
force-pushed
the
TranslationCleanup
branch
from
39cfa7a to
f7dda44
Compare
| // https://learn.microsoft.com/dotnet/api/system.string.startswith#system-string-startswith(system-char) | ||
| // https://learn.microsoft.com/dotnet/api/system.string.endswith#system-string-endswith(system-string) | ||
| // https://learn.microsoft.com/dotnet/api/system.string.endswith#system-string-endswith(system-char) | ||
| if (method.Name is nameof(string.StartsWith) or nameof(string.EndsWith) |
There was a problem hiding this comment.
FYI this would be the new, NativeAOT-friendly pattern for matching methods/members in translation. We're currently using reflection to look up MethodInfos, which causes the (trimming) linker the preserve all these methods regardless of whether users actually use them or not (increasing size). In many cases we're even generating closed generic methods via MakeGenericMethod, which isn't allowed at all.
Instead, we pattern match based on the method name, declaring type, and precise parameter list. This allows for accurate pattern matching, is fully NativeAOT-compatible without needlessly preserving any method that isn't used by users, and IMHO even easier-to-read, with the full, idiomatic match in one place.
There was a problem hiding this comment.
Pull request overview
This PR modernizes three expression visitor classes to use NativeAOT-friendly patterns, primarily converting traditional constructors to primary constructors and refactoring method implementations for better code clarity.
- Converts constructors to C# 12 primary constructor syntax across all three visitor classes
- Removes static
MethodInfofield caching in favor of runtime pattern matching on method names and signatures - Refactors control flow in
VisitBinary,VisitUnary, andVisitMethodCallto be more consistent and maintainable
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/EFCore/Query/QueryableMethodTranslatingExpressionVisitor.cs | Converts to primary constructor with inline field initialization; moves XML documentation to constructor parameters |
| src/EFCore.Sqlite.Core/Query/Internal/SqliteSqlTranslatingExpressionVisitor.cs | Converts to primary constructor; removes static MethodInfo fields; refactors VisitUnary to handle translations before custom logic; refactors VisitBinary to use switch expressions; simplifies VisitMethodCall local function signature |
| src/EFCore.SqlServer/Query/Internal/SqlServerSqlTranslatingExpressionVisitor.cs | Converts to primary constructor; removes six static MethodInfo fields and DateTimeDataTypes HashSet; refactors all three Visit methods to call base first then handle special cases; converts VisitMethodCall to use switch statement on method names; removes unused System.Diagnostics.CodeAnalysis using |
src/EFCore.SqlServer/Query/Internal/SqlServerSqlTranslatingExpressionVisitor.cs
Show resolved
Hide resolved
2 participants
Especially around NativeAOT-friendly patterns.