Sanitize expression variable names when generating precompiled query code - #39062
philcarbone wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Parameter/catch names and reserved keywords can still produce invalid generated C# code.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Sanitizes expression-tree variable names before generating precompiled C# query code.
Changes:
- Moves identifier sanitization into
LinqToCSharpSyntaxTranslator. - Reuses it for runtime constant names.
- Adds coverage for invalid block-variable names.
| File | Summary | Review comments |
|---|---|---|
test/EFCore.Design.Tests/Query/LinqToCSharpSyntaxTranslatorTest.cs |
Tests invalid block-variable sanitization. | — |
src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs |
Reuses the shared sanitizer. | — |
src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs |
Adds sanitization and integrates it with name uniquification. | Moderate: lambda and catch parameter names can bypass sanitization (3 votes). Moderate: reserved keywords remain valid identifiers (1 vote). |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
4459a7c to
443d8e4
Compare
443d8e4 to
edba99f
Compare
edba99f to
5b1fa41
Compare
|
On the "reserved keywords" point from the second review summary: Roslyn would emit a keyword-named variable as a verbatim identifier ( |
5b1fa41 to
1f3260b
Compare
1f3260b to
3293518
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two unresolved moderate issues affect identifier collision safety and translator state cleanup.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
3293518 to
75d2660
Compare
|
To close this out systematically rather than one finding at a time, I went through every place the translator turns a runtime string into an identifier. Everything that can carry a name from an expression tree now goes through |
a3761bb to
447775f
Compare
447775f to
855ee58
Compare
855ee58 to
f46f3cf
Compare
f46f3cf to
c34cf3b
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The per-query processor loses expression-based cross-query runtime-constant deduplication, potentially emitting duplicate static fields.
Review effort: Balanced
Findings: 1
Resolved since last review (1)
c34cf3b to
aca0cf8
Compare
|
A note on the churn here, since this PR has taken an unusual number of rounds. An AI assistant was working this PR, including the replies to the review findings. For several rounds it fixed each finding in isolation rather than the rule underneath them, which is why the same class of problem kept reappearing one case at a time: a lambda parameter, then a lifted temporary, then a captured variable. It also twice claimed the area was fully audited when it was not. I interjected and redirected it to derive the invariant first. That is what the current shape is: the translator collects the names the expression references but does not declare, before generating anything, and nothing it generates may take one of them; runtime constant fields live in a namespace of their own; a failed query leaves nothing of itself behind. Each finding since was tested before it was answered, including the ones that did not reproduce, and the evidence for those is in the threads rather than another patch. Apologies for the notification noise. |
aca0cf8 to
59a7d38
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Executor-owned names remain unreserved during final translation, and cumulative snapshots introduce quadratic generation overhead.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (4)
Resolved since last review (1)
…code - Block variable and lambda parameter names in expression trees are arbitrary strings; the shaper derives some of them from user-configured JSON property names, which need not be valid C# identifiers. LinqToCSharpSyntaxTranslator only uniquified those names, so the generated interceptor failed to compile with CS1001 for such a name. - Move the identifier sanitizer that PrecompiledQueryCodeGenerator already applied to runtime constant names into the translator and apply it to every variable name it emits; the generator now calls the shared method. Names that are reserved keywords are prefixed so that generated identifiers never have to be verbatim; contextual keywords are left as they are. Unicode formatting characters become underscores as well, since C# ignores them when comparing identifiers, so two names differing only by one would otherwise be one identifier declared twice; captured variables are compared against generated names without them for the same reason. - Lambda parameters that do not clash with a constant replacement, catch-block variables and goto labels were emitted from their raw names as well; they now go through the same sanitizer. A lambda parameter that already had the same name as a variable in scope still shadows it, but a collision that only sanitization creates is uniquified, as is any collision with a lifted variable, since lifted declarations are emitted inside the lambda body where a parameter of the same name does not compile. - Register the catch variable in its own stack frame, so that its declaration and the references to it agree on the name, and reject the same ParameterExpression being declared by an enclosing block and by a catch clause, as VisitBlock does. - Collect the names the expression references but does not declare before translating it. Those belong to the caller and are emitted as they are, so nothing generated afterwards may take one of them, in either order: a name chosen later would rebind an earlier reference, and a reference appearing later would resolve to the generated variable. A lifted invocation argument is registered by identity rather than matched by name, so that it is not mistaken for one of them. - Reset the scope stack and lifted state when a translation throws, and restore the member access replacements of RuntimeModelLinqToCSharpSyntaxTranslator in a finally: the translator is reused for later queries after PrecompiledQueryCodeGenerator records a failure, and neither may leak into them. - Sanitize the names of lifted constants in PrecompiledQueryCodeGenerator as well. Those come from whoever called ILiftableConstantFactory.CreateLiftableConstant, so a provider can supply a name that is not a valid C# identifier, and the generator emitted it verbatim. The parameter is renamed rather than just its declaration, since the same name is what the translated executor emits for every reference to that constant. - Reserve every identifier the generated executor declares for itself. They are now constants used both to emit those declarations and to build the reservation set, so a lifted constant named after one of them, such as queryContext or DbContext, no longer collides with the executor's own parameters; the same set is passed to the translator as the names declared in the enclosing scope, so a shaper variable named after one is uniquified instead of shadowing it. - Prefix the names of runtime constant fields with an underscore. A runtime constant becomes a field of the generated interceptors class under a name the model or a provider chose, and everything else that class refers to by simple name starts with a letter: its own members (interceptors, executor fields and generators, unsafe accessors, the class itself), the types the translated code uses, and the executor's parameters. A provider constant named Query1_Executor, EntityFrameworkCoreInterceptors or Encoding made the file uncompilable, and a JSON property named after the unsafe accessor of a Bytes-suffixed setter did so from the model alone; the prefix keeps such a field out of the generated members and the executor's identifiers at once rather than reserving them one family at a time, and the prefixed name is what gets sanitized, since the prefix can complete a reserved token such as __arglist. A type can start with an underscore as well, so the translator writes any type whose simple name is in use in the scope it emits into, as a constant replacement, a declared name or a variable, fully qualified with global:: rather than by its simple name. - Generate each query's code into a builder of its own and add it to the file only once the query has succeeded, record the runtime constants a query declares so that exactly those are taken back when it fails to precompile, and give each query its own runtime constant processor so that a rolled-back query cannot leave a deduplication entry behind for a later query that reuses the same constant. Constants shared between queries still share one field: the generator matches them by initializer as well as by value, since each shaper evaluates a JSON property name into a byte array of its own, and rolls that map back too. The region and the operator interceptors are written before the executor is generated, so a failure while generating the executor left interceptors calling an executor that was never emitted, and a half-written executor method, making the whole file uncompilable for the queries that did succeed. - Add translator unit tests for each of these, and precompiled query tests for a lifted constant named after a C# keyword, after the executor's own identifiers and after another constant's sanitized name, for a runtime constant named after the executor field, the interceptors class, a type the generated code uses, a name that is reserved once prefixed, a type with a leading underscore, and an unsafe accessor, for a query that fails to precompile alongside one that succeeds and for one that fails between two that succeed, for a lifted constant named like a runtime constant field, for one followed by a query reusing its runtime constant, and for two queries sharing a runtime constant. The existing RuntimeConstantExpression test expects the prefixed field name. The precompiled query test helper now compiles what was generated even when a test expects a precompilation error, which is what let the partial output go unnoticed. Fixes dotnet#39061
Fixes #39061
LinqToCSharpSyntaxTranslatorturns EF's shaper expression trees into C# for precompiled queries. The names of variables, lambda parameters, catch variables and labels in those trees are arbitrary strings, and the relational shaper derives some of them from user-configured JSON property names, which need not be valid C# identifiers. The translator uniquified those names but emitted them verbatim, so a precompiled query whose shaper contained such a name produced an interceptor that failed to compile with CS1001 (found while reviewing #39014, where a block variable named after a JSON property called1!NOT VALID COLLECTION;produced invalid code).Changes
PrecompiledQueryCodeGeneratoralready applied to runtime constant names moves into the translator asSanitizeIdentifierName, and the generator calls it there. Invalid characters become_, and so do Unicode formatting characters (category Cf), which are valid inside an identifier but ignored when identifiers are compared, so two names differing only by one would otherwise declare the same identifier twice; a leading digit gets a_prefix, and a reserved keyword gets one too, so generated identifiers are always plain identifiers rather than verbatim ones (_class). Captured variables, which are emitted as the caller wrote them, are compared against generated names without their formatting characters for the same reason. Contextual keywords are valid identifiers in these positions and are left alone,valueamong them, since generated property setters use it.UniquifyVariableName), lambda parameters, catch variables and goto labels. Reflection-provided names (types, members, methods, enum members) are already valid identifiers, and captured variables inVisitParameterare still emitted verbatim on purpose, because they have to match the declaration in the enclosing generated code.ParameterExpressionin an enclosing block and as a catch variable is now rejected the wayVisitBlockrejects it, instead of silently producing two unrelated locals.NotSupportedException(the generator records the query and moves on) rather than wrong code. Names reserved for the caller's own declarations, the constant replacements, are unaffected, since they are exactly what such a reference is meant to resolve to.PrecompiledQueryCodeGeneratoremittedvar {name} = ...straight from the lifted constant's parameter name, and that name is whatever the caller passed toILiftableConstantFactory.CreateLiftableConstant, so a provider can supply one that is not a valid C# identifier. A constant namedClassgeneratedvar class = .... The parameter is renamed rather than just its declaration, since the same name is what the translated executor emits for every reference to that constant.queryContextorDbContextcollided with the executor's own parameters. The names are constants used both to emit the declarations and to build the reservation set, so the two cannot drift apart, and the set is handed to the translator for the executor and the lifted initializers as names declared in the enclosing scope, so a shaper variable nameddbContextcomes out asdbContext0rather than shadowing the parameter, and the executor lambda's ownqueryContextparameter no longer shadows the method's either.private static readonlyfield of the generated interceptors class, under a name that comes from the model (a JSON property name plusBytes) or from a provider (RuntimeConstantExpressionaccepts any name and uppercases its first character). The class's own members (Query{n}_…interceptors, executor fields and executor generators,UnsafeAccessor_…methods, the class itself) and the executor's parameters all start with a letter, and the translated code also refers to types by their simple names (Encoding,Snapshot, …). A provider constant namedQuery1_Executorproduced CS0102, one namedEntityFrameworkCoreInterceptorsCS0542, and one namedEncodingbroke every runtime constant initializer, since those areEncoding.UTF8.GetBytes(...)(CS1061). The model alone can reach the first kind, with a JSON property named after the unsafe accessor of a…Bytesproperty that materialization sets through its private setter. Prefixing the field with_puts it outside the generated members and the executor's identifiers at once, instead of reserving them one family at a time. The prefixed name is what gets sanitized, since the prefix can complete a reserved token:_arglistis an identifier and__arglistis not. A type can start with an underscore too, and no convention on the field side can rule that out, so the translator now writes any type whose simple name is in use in the scope it emits into, as a constant replacement, a declared field or a variable, fully qualified withglobal::. A provider constant namedFoowhose initializer read a static member of a type_Fooproduced CS1061 before. Lifted constants are already uniquified against these fields, and cannot shadow a type or a generated member themselves, sinceLiftableConstantExpressionlowercases their first character.TranslateCoreresets the scope stack and the lifted state when a translation throws, andRuntimeModelLinqToCSharpSyntaxTranslatorrestores its member-access replacements in afinally. Without the latter, a query that failed while replacements were set left them applied to the runtime-constant initializers generated afterwards.Tests
New
LinqToCSharpSyntaxTranslatorTestcases, each of which fails without the corresponding change: an invalid block variable, lambda parameter, catch variable and label name; two variables differing only by a formatting character, and a generated local equal to a captured variable up to one; reserved keyword names, andvar,asyncandvalue, which must stay as they are; a lambda parameter colliding with a lifted variable under differing and identical raw names, and with an outer variable after sanitization; a lambda parameter, a block variable and an invocation-lifted local whose names would otherwise collide with a captured variable, in both orders, with a captured variable naming a constant replacement as the counter-case; a lifted constant named after a C# keyword, after each of the executor's own identifiers and after another constant's sanitized name; shaper variables named after all five executor identifiers; a runtime constant named after the executor field, after the interceptors class, after a type the generated code uses, after a name that becomes a reserved token once prefixed, after a type with a leading underscore, and after an unsafe accessor, the last through model configuration alone; two JSON property names differing only by a formatting character; translator cases for a type, a generic type and a type shadowed by a local being written out fully qualified; a query that fails to precompile next to one that succeeds, one followed by a query reusing its runtime constant, and two queries sharing a runtime constant that must produce one field, all as precompiled query tests that compile and run the generated code on SQLite and SQL Server; the duplicate catch declaration; and a translation reused after a failure. The existingRuntimeConstantExpressiontest now expects the field as_NumberBytes; it was the only test that pinned the old name.A lambda parameter that already had the same name as a variable in scope still shadows it, which
Variable_with_same_name_in_lambda_does_not_get_renamedandSame_parameter_instance_is_used_twice_in_nested_lambdaspin down; this PR does not change that.EFCore.Design.Testspasses locally, as do the SQLite precompiled-query and compiled-model functional classes end to end, the latter with no baseline changes, so generated compiled models are unaffected.Two things I noticed nearby and did not touch, both pre-existing: in the shapes where a lambda parameter meets a lifted variable, the translator moves the pending lifted declaration (
var i = 8;) into the lambda body, so it is evaluated on every invocation rather than once where the expression tree evaluates it; and a catch body that is an expression with lifted statements loses them, since only the expression is wrapped in a block. The new tests assert today's output for the first. Happy to open issues for either if they aren't already known.No public API change: the new method is on a type in an
.Internalnamespace and the Design API baseline is unaffected.