From f847f3222853fd7d799aec38c470bc1c58ce3d1a Mon Sep 17 00:00:00 2001 From: Phil Carbone Date: Tue, 22 Sep 2026 19:34:31 -0400 Subject: [PATCH] Sanitize expression variable names when generating precompiled query 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 #39061 --- .../Internal/LinqToCSharpSyntaxTranslator.cs | 346 ++++++++-- .../Internal/PrecompiledQueryCodeGenerator.cs | 218 +++++-- ...untimeModelLinqToCSharpSyntaxTranslator.cs | 28 +- .../Query/LinqToCSharpSyntaxTranslatorTest.cs | 462 ++++++++++++- ...AdHocPrecompiledQueryRelationalTestBase.cs | 605 ++++++++++++++++++ .../PrecompiledQueryRelationalTestBase.cs | 8 +- .../PrecompiledQueryTestHelpers.cs | 22 +- .../AdHocPrecompiledQuerySqlServerTest.cs | 200 ++++++ 8 files changed, 1762 insertions(+), 127 deletions(-) diff --git a/src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs b/src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs index eab610eeaec..cb7ab9fadb9 100644 --- a/src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs +++ b/src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs @@ -89,6 +89,8 @@ internal LiftedState CreateChild() private bool _onLastLambdaLine; private readonly HashSet _capturedVariables = []; + private readonly HashSet _capturedVariableNames = []; + private IReadOnlySet? _declaredNames; private ISet _collectedNamespaces = null!; private readonly Dictionary _methodUnsafeAccessors = []; private readonly Dictionary<(FieldInfo Field, bool ForWrite), MethodDeclarationSyntax> _fieldUnsafeAccessors = []; @@ -128,8 +130,9 @@ public virtual SyntaxNode TranslateStatement( Expression node, IReadOnlyDictionary? constantReplacements, ISet collectedNamespaces, - ISet unsafeAccessors) - => TranslateCore(node, constantReplacements, collectedNamespaces, unsafeAccessors, statementContext: true); + ISet unsafeAccessors, + IReadOnlySet? declaredNames = null) + => TranslateCore(node, constantReplacements, declaredNames, collectedNamespaces, unsafeAccessors, statementContext: true); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -141,8 +144,9 @@ public virtual SyntaxNode TranslateExpression( Expression node, IReadOnlyDictionary? constantReplacements, ISet collectedNamespaces, - ISet unsafeAccessors) - => TranslateCore(node, constantReplacements, collectedNamespaces, unsafeAccessors, statementContext: false); + ISet unsafeAccessors, + IReadOnlySet? declaredNames = null) + => TranslateCore(node, constantReplacements, declaredNames, collectedNamespaces, unsafeAccessors, statementContext: false); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -153,12 +157,20 @@ public virtual SyntaxNode TranslateExpression( protected virtual SyntaxNode TranslateCore( Expression node, IReadOnlyDictionary? constantReplacements, + IReadOnlySet? declaredNames, ISet collectedNamespaces, ISet unsafeAccessors, bool statementContext) { _capturedVariables.Clear(); + // Names the expression references without declaring belong to the caller and are emitted as-is, so nothing generated here + // may take one of them; collecting them up front means a name chosen later cannot rebind an earlier reference. + _capturedVariableNames.Clear(); + FreeVariableNameCollector.Collect(node, _capturedVariableNames); _constantReplacements = constantReplacements; + // Other names the caller declares in the scope the result is emitted into, such as the fields for the runtime constants when + // one of their initializers is being translated; treated like the constant replacement names below. + _declaredNames = declaredNames; _collectedNamespaces = collectedNamespaces; _unnamedParameterCounter = 0; _context = statementContext ? ExpressionContext.Statement : ExpressionContext.Expression; @@ -177,32 +189,61 @@ protected virtual SyntaxNode TranslateCore( } } - try + if (declaredNames != null) { - Visit(node); + foreach (var name in declaredNames) + { + rootFrame.VariableNames.Add(name); + } } - finally + + try { - if (constantReplacements != null) + try { - foreach (var name in constantReplacements.Values) + Visit(node); + } + finally + { + if (constantReplacements != null) { - rootFrame.VariableNames.Remove(name); + foreach (var name in constantReplacements.Values) + { + rootFrame.VariableNames.Remove(name); + } + } + + if (declaredNames != null) + { + foreach (var name in declaredNames) + { + rootFrame.VariableNames.Remove(name); + } } } - } - if (_liftedState.Statements.Count > 0 - && _context == ExpressionContext.Expression) - { - throw new NotSupportedException("Lifted expressions remaining at top-level in expression context"); + if (_liftedState.Statements.Count > 0 + && _context == ExpressionContext.Expression) + { + throw new NotSupportedException("Lifted expressions remaining at top-level in expression context"); + } + + Check.DebugAssert(_stack.Count == 1, "_parameterStack.Count == 1"); + Check.DebugAssert(_stack.Peek().Variables.Count == 0, "_stack.Peek().Parameters.Count == 0"); + Check.DebugAssert(_stack.Peek().VariableNames.Count == 0, "_stack.Peek().ParameterNames.Count == 0"); + Check.DebugAssert(_stack.Peek().Labels.Count == 0); + Check.DebugAssert(_stack.Peek().UniqueLabelNames.Count == 0); } + catch + { + // The translator is reused for later translations (PrecompiledQueryCodeGenerator records a failed query and moves on to + // the next one), so the scopes and lifted statements a failed translation left behind must not leak into them + _stack.Clear(); + _stack.Push(new StackFrame([], [], [], [])); + _liftedState = new LiftedState(); - Check.DebugAssert(_stack.Count == 1, "_parameterStack.Count == 1"); - Check.DebugAssert(_stack.Peek().Variables.Count == 0, "_stack.Peek().Parameters.Count == 0"); - Check.DebugAssert(_stack.Peek().VariableNames.Count == 0, "_stack.Peek().ParameterNames.Count == 0"); - Check.DebugAssert(_stack.Peek().Labels.Count == 0); - Check.DebugAssert(_stack.Peek().UniqueLabelNames.Count == 0); + throw; + } foreach (var unsafeAccessor in _fieldUnsafeAccessors.Values.Concat(_methodUnsafeAccessors.Values)) { @@ -765,8 +806,8 @@ void PreprocessLabels() var (_, _, labels, uniqueLabelNames) = stackFrame; - // Generate names for unnamed label targets and uniquify (all label names) - identifier = label.Target.Name ?? "unnamedLabel"; + // Generate names for unnamed label targets, sanitize named ones, and uniquify (all label names) + identifier = label.Target.Name is null ? "unnamedLabel" : SanitizeIdentifierName(label.Target.Name); var identifierBase = identifier; for (var i = 0; uniqueLabelNames.Contains(identifier); i++) { @@ -795,34 +836,47 @@ protected override CatchBlock VisitCatchBlock(CatchBlock catchBlock) /// protected virtual SyntaxNode TranslateCatchBlock(CatchBlock catchBlock, bool noType = false) { - var translatedBody = Translate(catchBlock.Body) switch + if (catchBlock.Variable is { Name: null }) { - BlockSyntax b => b, - StatementSyntax s => Block(s), - ExpressionSyntax e => Block(ExpressionStatement(e)), - _ => throw new ArgumentOutOfRangeException() - }; + throw new NotSupportedException("TranslateCatchBlock: unnamed parameter as catch variable"); + } var catchDeclaration = noType ? null : CatchDeclaration(Generate(catchBlock.Test)); + // The catch variable gets its own scope, so that its declaration and the references to it agree on the (sanitized) name + var stackFrame = PushNewStackFrame(); + if (catchBlock.Variable is not null) { Check.DebugAssert(catchDeclaration is not null); - if (catchBlock.Variable.Name is null) + var name = UniquifyVariableName(catchBlock.Variable.Name!); + if (!stackFrame.Variables.TryAdd(catchBlock.Variable, name)) { - throw new NotSupportedException("TranslateCatchBlock: unnamed parameter as catch variable"); + throw new InvalidOperationException( + DesignStrings.SameParameterExpressionDeclaredAsVariableInNestedBlocks(catchBlock.Variable.Name)); } - catchDeclaration = catchDeclaration.WithIdentifier(Identifier(catchBlock.Variable.Name)); + stackFrame.VariableNames.Add(name); + catchDeclaration = catchDeclaration.WithIdentifier(Identifier(name)); } - return CatchClause( - catchDeclaration, - catchBlock.Filter is null ? null : CatchFilterClause(Translate(catchBlock.Filter)), - translatedBody); + var translatedBody = Translate(catchBlock.Body) switch + { + BlockSyntax b => b, + StatementSyntax s => Block(s), + ExpressionSyntax e => Block(ExpressionStatement(e)), + _ => throw new ArgumentOutOfRangeException() + }; + + var filter = catchBlock.Filter is null ? null : CatchFilterClause(Translate(catchBlock.Filter)); + + var popped = _stack.Pop(); + Check.DebugAssert(popped.Equals(stackFrame)); + + return CatchClause(catchDeclaration, filter, translatedBody); } /// @@ -1262,6 +1316,9 @@ protected override Expression VisitInvocation(InvocationExpression invocation) var parameter = Expression.Parameter(argument.Type, name); _liftedState.Statements.Add(GenerateVarDeclaration(name, Translate(argument))); _liftedState.VariableNames.Add(name); + // The body references this parameter in place of the lambda's; registering it means those references resolve to + // the lifted local by identity rather than by its name happening to match + _liftedState.Variables[parameter] = name; arguments[i] = parameter; } @@ -1487,6 +1544,12 @@ protected virtual bool TryGenerate(Type type, [NotNullWhen(true)] out TypeSyntax return true; } + if (IsNameInScope(type.Name)) + { + result = GloballyQualified(type.Namespace, IdentifierName(type.Name)); + return true; + } + if (type.Namespace != null) { _collectedNamespaces.Add(type.Namespace); @@ -1500,13 +1563,19 @@ NameSyntax GenerateGenericType(Type type, List genericArguments, int var offset = type.DeclaringType != null ? type.DeclaringType.GetGenericArguments().Length : 0; var genericPartIndex = type.Name.IndexOf('`'); + var simpleName = genericPartIndex <= 0 ? type.Name : type.Name[..genericPartIndex]; SimpleNameSyntax nameSyntax = genericPartIndex <= 0 ? IdentifierName(type.Name) : GenericName( - Identifier(type.Name[..genericPartIndex]), + Identifier(simpleName), TypeArgumentList(SeparatedList(genericArguments.Skip(offset).Take(length - offset)))); if (type.DeclaringType == null) { + if (IsNameInScope(simpleName)) + { + return GloballyQualified(type.Namespace, nameSyntax); + } + AddNamespace(type); return nameSyntax; @@ -1529,13 +1598,27 @@ protected override Expression VisitLambda(Expression lambda) var stackFrame = PushNewStackFrame(); var localUnnamedParameterCounter = 0; + var lambdaParameterNames = new HashSet(); foreach (var parameter in lambda.Parameters) { - var name = parameter.Name ?? ("unnamed" + (++localUnnamedParameterCounter)); - - if (_constantReplacements?.Values.Contains(name) == true) + var name = parameter.Name is null + ? "unnamed" + (++localUnnamedParameterCounter) + : SanitizeIdentifierName(parameter.Name); + + // A parameter that already had the same name as a variable in scope keeps shadowing it, as before + // (Variable_with_same_name_in_lambda_does_not_get_renamed). What must not happen is a collision that sanitization + // itself creates, or one with a lifted variable, whose declaration is emitted inside the lambda body: either would + // rebind references to that variable. The same goes for the caller's own declarations and for captured variables, + // which the lambda body refers to by name. + if (IsCallerDeclaredName(name) + || !lambdaParameterNames.Add(name) + || _liftedState.VariableNames.Contains(name) + || _capturedVariableNames.Contains(name) + || (stackFrame.VariableNames.Contains(name) + && !stackFrame.Variables.Any(v => v.Value == name && v.Key.Name == parameter.Name))) { name = UniquifyVariableName(name); + lambdaParameterNames.Add(name); } stackFrame.Variables[parameter] = name; @@ -2199,6 +2282,19 @@ protected override Expression VisitParameter(ParameterExpression parameter) throw new NotSupportedException("Unnamed captured variable"); } + // Since the name is emitted as-is, anything we generated under that same name would capture the reference instead. That + // includes generated locals, which exist in these scopes by name only. Constant replacement names are excluded: they are + // the caller's own declarations, and referencing them is exactly what a captured variable of that name means to do. + var comparableName = WithoutFormattingCharacters(parameter.Name); + if ((_stack.Peek().VariableNames.Contains(comparableName) || _liftedState.VariableNames.Contains(comparableName)) + && !IsCallerDeclaredName(comparableName)) + { + throw new NotSupportedException( + $"Captured variable '{parameter.Name}' is shadowed by a variable generated for the translated expression"); + } + + _capturedVariableNames.Add(comparableName); + Result = IdentifierName(parameter.Name); return parameter; } @@ -2822,21 +2918,104 @@ private string LookupVariableName(ParameterExpression parameter) ? name : _liftedState.Variables[parameter]; + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + [return: NotNullIfNotNull(nameof(name))] + public static string? SanitizeIdentifierName(string? name) + { + if (name == null) + { + return null; + } + + if (string.IsNullOrWhiteSpace(name)) + { + return "_"; + } + + // Formatting characters (Unicode category Cf, such as a zero-width joiner) are valid inside an identifier but ignored when + // identifiers are compared, so two names that differ only by one would declare the same identifier twice. They become + // underscores like any other unusable character, so comparing sanitized names as strings is exact. + var result = new string( + [.. name.Select(c => SyntaxFacts.IsIdentifierPartCharacter(c) && !IsFormattingCharacter(c) ? c : '_')]); + + // Reserved keywords are lexically valid identifiers; prefix them so that generated identifiers never need to be verbatim + // (@class). Contextual keywords such as var or async are valid identifiers where generated names are used, so they are kept. + if (!SyntaxFacts.IsValidIdentifier(result) || SyntaxFacts.GetKeywordKind(result) != SyntaxKind.None) + { + result = "_" + result; + } + + Check.DebugAssert(SyntaxFacts.IsValidIdentifier(result)); + + return result; + } + + // A type is referred to by its simple name unless that name is in use in the scope the code is emitted into, as a name the + // caller declares or as a variable, where the simple name would resolve to that declaration instead of the type. The type is + // then written out fully qualified. Any identifier can name a type, so no naming convention on the caller's side can rule + // this out; qualifying the reference can. + private bool IsNameInScope(string name) + => IsCallerDeclaredName(name) + || _capturedVariableNames.Contains(name) + || _liftedState.VariableNames.Contains(name) + || _stack.Any(frame => frame.VariableNames.Contains(name)); + + private static NameSyntax GloballyQualified(string? @namespace, SimpleNameSyntax name) + { + var globalAlias = IdentifierName(Token(SyntaxKind.GlobalKeyword)); + if (string.IsNullOrEmpty(@namespace)) + { + return AliasQualifiedName(globalAlias, name); + } + + var parts = @namespace.Split('.'); + NameSyntax qualified = AliasQualifiedName(globalAlias, IdentifierName(parts[0])); + for (var i = 1; i < parts.Length; i++) + { + qualified = QualifiedName(qualified, IdentifierName(parts[i])); + } + + return QualifiedName(qualified, name); + } + + private static bool IsFormattingCharacter(char c) + => CharUnicodeInfo.GetUnicodeCategory(c) == UnicodeCategory.Format; + + // A captured variable's name is emitted as the caller wrote it, formatting characters included, since the reference has to match + // the caller's declaration. For comparing it against names generated here, which never contain one, it is recorded without them. + private static string WithoutFormattingCharacters(string name) + => name.Any(IsFormattingCharacter) ? new string([.. name.Where(c => !IsFormattingCharacter(c))]) : name; + + // The names the caller declares in the scope the translation is emitted into: the constant replacements and whatever else it + // says is declared there. References to them are the caller's business; nothing generated here may take one of them. + private bool IsCallerDeclaredName(string name) + => ConstantReplacementsContain(name) || _declaredNames?.Contains(name) == true; + + private bool ConstantReplacementsContain(string name) + => _constantReplacements is BidirectionalDictionary bidirectionalDictionary + ? bidirectionalDictionary.ContainsValue(name) + : _constantReplacements?.Values.Contains(name) == true; + private string UniquifyVariableName(string? name) { var isUnnamed = name is null; - name ??= "unnamed"; + // Expression tree variable names are arbitrary strings (e.g. derived from JSON property names) and must become valid identifiers. + name = name is null ? "unnamed" : SanitizeIdentifierName(name); var parameterNames = _stack.Peek().VariableNames; - Func constantReplacementsContainsName = - _constantReplacements is BidirectionalDictionary bidirectionalDictionary - ? bidirectionalDictionary.ContainsValue - : n => _constantReplacements?.Values.Contains(n) == true; - var baseName = name; for (var j = isUnnamed ? _unnamedParameterCounter++ : 0; - parameterNames.Contains(name) || _liftedState.VariableNames.Contains(name) || constantReplacementsContainsName(name); + parameterNames.Contains(name) + || _liftedState.VariableNames.Contains(name) + || IsCallerDeclaredName(name) + // A captured variable already emitted under this name would be rebound by a local we declare now + || _capturedVariableNames.Contains(name); j++) { name = baseName + j; @@ -2845,6 +3024,81 @@ _constantReplacements is BidirectionalDictionary bidirectionalDi return name; } + /// + /// Collects the names of the variables an expression references without declaring, which belong to the code the translation + /// is emitted into. + /// + private sealed class FreeVariableNameCollector(HashSet names) : ExpressionVisitor + { + private readonly Dictionary _declared = new(ReferenceEqualityComparer.Instance); + + public static void Collect(Expression node, HashSet names) + => new FreeVariableNameCollector(names).Visit(node); + + protected override Expression VisitParameter(ParameterExpression node) + { + if (node.Name is not null && !_declared.ContainsKey(node)) + { + names.Add(WithoutFormattingCharacters(node.Name)); + } + + return node; + } + + protected override Expression VisitBlock(BlockExpression node) + { + Declare(node.Variables); + base.VisitBlock(node); + Undeclare(node.Variables); + + return node; + } + + protected override Expression VisitLambda(Expression node) + { + Declare(node.Parameters); + base.VisitLambda(node); + Undeclare(node.Parameters); + + return node; + } + + protected override CatchBlock VisitCatchBlock(CatchBlock node) + { + ParameterExpression[] variable = node.Variable is null ? [] : [node.Variable]; + + Declare(variable); + base.VisitCatchBlock(node); + Undeclare(variable); + + return node; + } + + private void Declare(IReadOnlyList variables) + { + foreach (var variable in variables) + { + _declared[variable] = _declared.TryGetValue(variable, out var count) ? count + 1 : 1; + } + } + + private void Undeclare(IReadOnlyList variables) + { + foreach (var variable in variables) + { + var count = _declared[variable]; + if (count == 1) + { + _declared.Remove(variable); + } + else + { + _declared[variable] = count - 1; + } + } + } + } + private static LocalDeclarationStatementSyntax GenerateVarDeclaration(string variableIdentifier, ExpressionSyntax initializer) => LocalDeclarationStatement( VariableDeclaration( diff --git a/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs b/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs index 6efa90df9a4..0855c5fc28e 100644 --- a/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs +++ b/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections; -using System.Diagnostics.CodeAnalysis; using System.Runtime.ExceptionServices; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -30,11 +29,35 @@ public class PrecompiledQueryCodeGenerator : IPrecompiledQueryCodeGenerator private ExpressionTreeFuncletizer _funcletizer = null!; private RuntimeModelLinqToCSharpSyntaxTranslator _linqToCSharpTranslator = null!; private LiftableConstantProcessor _liftableConstantProcessor = null!; - private RuntimeConstantProcessor _runtimeConstantProcessor = null!; + + // The identifiers the generated executor declares for itself. They are used both to emit those declarations and to keep + // anything named by the model or by a provider from taking one of them, so the two cannot drift apart. + private const string DbContextVariable = "dbContext"; + private const string QueryContextVariable = "queryContext"; + private const string RelationalModelVariable = "relationalModel"; + private const string RelationalTypeMappingSourceVariable = "relationalTypeMappingSource"; + private const string MaterializerLiftableConstantContextVariable = "materializerLiftableConstantContext"; + + private static readonly string[] GeneratedExecutorVariableNames = + [ + DbContextVariable, + QueryContextVariable, + RelationalModelVariable, + RelationalTypeMappingSourceVariable, + MaterializerLiftableConstantContextVariable + ]; private readonly Dictionary _runtimeConstants = []; private readonly BidirectionalDictionary _constantReplacements = []; + // Declared runtime constants by initializer, for sharing a field between queries whose constants are equal by initializer but + // not by value; see GenerateQueryExecutor. + private readonly Dictionary _runtimeConstantsByInitializer = + new(ExpressionEqualityComparer.Instance); + + // The runtime constants the query being generated has declared so far, so that a query which fails can take exactly those back + private readonly List _runtimeConstantsOfCurrentQuery = []; + private Symbols _symbols; private readonly HashSet _namespaces = []; @@ -85,7 +108,7 @@ public virtual IReadOnlyList GeneratePrecompiledQueries( _liftableConstantProcessor = new LiftableConstantProcessor(null!); _constantReplacements.Clear(); _runtimeConstants.Clear(); - _runtimeConstantProcessor = new RuntimeConstantProcessor(); + _runtimeConstantsByInitializer.Clear(); _queryCompiler = dbContext.GetService(); _unsafeAccessors.Clear(); var contextType = dbContext.GetType(); @@ -159,6 +182,18 @@ public virtual IReadOnlyList GeneratePrecompiledQueries( { var querySyntax = locatedQueries[queryNum]; + // A query that fails must leave nothing of itself behind: its interceptors reference an executor that will never be + // generated, and the executor method itself may be half-written, so the file would not compile for the queries that + // did succeed. Its code is therefore generated into a builder of its own and added to the file only once the query has + // succeeded, and the runtime constants it declares are recorded so that exactly those can be taken back. + var queryCode = new IndentedStringBuilder(); + for (var i = 0; i < _code.IndentCount; i++) + { + queryCode.IncrementIndent(); + } + + _runtimeConstantsOfCurrentQuery.Clear(); + try { // We have a query lambda, as a Roslyn syntax tree. Translate to LINQ expression tree. @@ -199,7 +234,7 @@ public virtual IReadOnlyList GeneratePrecompiledQueries( // The query has been compiled successfully by the EF query pipeline. // Now go over each LINQ operator, generating an interceptor for it. - _code.AppendLine($"#region Query{queryNum + 1}").AppendLine(); + queryCode.AppendLine($"#region Query{queryNum + 1}").AppendLine(); try { @@ -213,18 +248,18 @@ public virtual IReadOnlyList GeneratePrecompiledQueries( // Generate interceptors for all LINQ operators in the query, starting from the root up until the penultimate. // Then generate the interceptor for the terminating operator, and finally the query's executor. GenerateOperatorInterceptorsRecursively( - _code, penultimateOperator, penultimateOperatorSyntax, semanticModel, queryNum + 1, out var operatorNum, + queryCode, penultimateOperator, penultimateOperatorSyntax, semanticModel, queryNum + 1, out var operatorNum, cancellationToken: cancellationToken); GenerateOperatorInterceptor( - _code, terminatingOperator, querySyntax, semanticModel, queryNum + 1, operatorNum + 1, isTerminatingOperator: true, - cancellationToken); + queryCode, terminatingOperator, querySyntax, semanticModel, queryNum + 1, operatorNum + 1, + isTerminatingOperator: true, cancellationToken); - GenerateQueryExecutor(_code, queryNum + 1, queryExecutor, _namespaces, _unsafeAccessors); + GenerateQueryExecutor(queryCode, queryNum + 1, queryExecutor, _namespaces, _unsafeAccessors); } finally { - _code + queryCode .AppendLine() .AppendLine($"#endregion Query{queryNum + 1}"); } @@ -232,10 +267,31 @@ public virtual IReadOnlyList GeneratePrecompiledQueries( catch (Exception e) { precompilationErrors.Add(new QueryPrecompilationError(querySyntax, e)); + + foreach (var constantName in _runtimeConstantsOfCurrentQuery) + { + var constant = _runtimeConstants[constantName]; + _runtimeConstants.Remove(constantName); + _constantReplacements.Remove(constant.Value); + if (_runtimeConstantsByInitializer.TryGetValue(constant.InitializeExpression, out var declared) + && ReferenceEquals(declared, constant)) + { + _runtimeConstantsByInitializer.Remove(constant.InitializeExpression); + } + } + continue; } - // We're done generating the interceptors for the query's LINQ operators. + // We're done generating the interceptors for the query's LINQ operators. The query's code already carries the file's + // indentation, so it is added as it is, and the trailing line break is re-added through the builder to keep its own + // indentation state right for what follows. + using (_code.SuspendIndent()) + { + _code.Append(queryCode.ToString().TrimEnd('\r', '\n')); + } + + _code.AppendLine(); queriesPrecompiledInFile++; } @@ -259,12 +315,15 @@ public virtual IReadOnlyList GeneratePrecompiledQueries( _code.AppendLine("#endregion Unsafe accessors"); } + // The fields are in scope for their own initializers, so a type an initializer refers to by a name one of them has taken + // has to be written out fully qualified; the translator does that for the names it is told are declared here. + var runtimeConstantNames = _runtimeConstants.Keys.ToHashSet(); foreach (var (fieldName, constant) in _runtimeConstants.OrderBy(x => x.Key)) { var typeSymbol = GetTypeSymbol(semanticModel.Compilation, constant.Type); var syntax = _linqToCSharpTranslator.TranslateExpression( - constant.InitializeExpression, constantReplacements: null, _namespaces, _unsafeAccessors); + constant.InitializeExpression, constantReplacements: null, _namespaces, _unsafeAccessors, runtimeConstantNames); _code.AppendLine( $"private static readonly {typeSymbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat)} {fieldName} = {syntax.NormalizeWhitespace().ToFullString()};"); } @@ -944,20 +1003,41 @@ private void GenerateQueryExecutor( // the real strongly-typed signature inside the interceptor, where the return value is represented as a generic type parameter // (which can be an anonymous type). code - .AppendLine($"private static object Query{queryNum}_GenerateExecutor(DbContext dbContext, QueryContext queryContext)") + .AppendLine( + $"private static object Query{queryNum}_GenerateExecutor(DbContext {DbContextVariable}, QueryContext {QueryContextVariable})") .AppendLine("{") .IncrementIndent() - .AppendLine("var relationalModel = dbContext.Model.GetRelationalModel();") - .AppendLine("var relationalTypeMappingSource = dbContext.GetService();") - .AppendLine("var materializerLiftableConstantContext = new RelationalMaterializerLiftableConstantContext(") - .AppendLine(" dbContext.GetService(),") - .AppendLine(" dbContext.GetService(),") - .AppendLine(" dbContext.GetService());"); - - queryExecutor = _runtimeConstantProcessor.Process(queryExecutor); - foreach (var runtimeConstant in _runtimeConstantProcessor.LastProcessFoundRuntimeConstants) + .AppendLine($"var {RelationalModelVariable} = {DbContextVariable}.Model.GetRelationalModel();") + .AppendLine($"var {RelationalTypeMappingSourceVariable} = {DbContextVariable}.GetService();") + .AppendLine($"var {MaterializerLiftableConstantContextVariable} = new RelationalMaterializerLiftableConstantContext(") + .AppendLine($" {DbContextVariable}.GetService(),") + .AppendLine($" {DbContextVariable}.GetService(),") + .AppendLine($" {DbContextVariable}.GetService());"); + + // A processor per query: its deduplication cache would otherwise outlive a query that failed and was rolled back, so a + // later query reusing the same constant would be told it is already registered and end up with no field to reference. + // Constants shared between queries are deduplicated here instead, against what has actually been declared. Equality by + // value is not enough for that: the shaper evaluates a runtime constant's initializer afresh for every query, so the same + // JSON property name in two queries is two byte arrays. A constant whose initializer matches a declared one is replaced by + // that constant's value, which the translator emits as the existing field. + queryExecutor = new DeclaredRuntimeConstantReplacingExpressionVisitor(_runtimeConstantsByInitializer).Visit(queryExecutor); + var runtimeConstantProcessor = new RuntimeConstantProcessor(); + queryExecutor = runtimeConstantProcessor.Process(queryExecutor); + foreach (var runtimeConstant in runtimeConstantProcessor.LastProcessFoundRuntimeConstants) { - var name = SanitizeIdentifierName(runtimeConstant.Name); + if (_constantReplacements.ContainsKey(runtimeConstant.Value)) + { + continue; + } + + // A runtime constant becomes a field of the interceptors class under a name that came from the model or from a provider. + // The members the class declares for itself and the executor's parameters all start with a letter, so a field name that + // starts with an underscore is none of them, and nothing has to be reserved family by family. A type can start with one, + // which is why the translator writes out any type whose simple name is in use in the emitted scope fully qualified. The + // lifted constants, which can start with an underscore as well, are uniquified against these names below. The prefixed + // name is what gets sanitized, since the prefix can itself complete a reserved token: _arglist is an identifier, + // __arglist is not. + var name = LinqToCSharpSyntaxTranslator.SanitizeIdentifierName("_" + runtimeConstant.Name); var baseName = name; for (var j = 0; _runtimeConstants.ContainsKey(name); j++) { @@ -966,25 +1046,68 @@ private void GenerateQueryExecutor( _runtimeConstants.Add(name, runtimeConstant); _constantReplacements.Add(runtimeConstant.Value, name); + _runtimeConstantsByInitializer.TryAdd(runtimeConstant.InitializeExpression, runtimeConstant); + _runtimeConstantsOfCurrentQuery.Add(name); } - HashSet variableNames = - [.. _constantReplacements.Values, "relationalModel", "relationalTypeMappingSource", "materializerLiftableConstantContext"]; + // Everything the generated executor declares for itself, so that nothing named by the model or by a provider can take + // one of these names. Keep in step with the declarations emitted below. + HashSet variableNames = [.. _constantReplacements.Values, .. GeneratedExecutorVariableNames]; var materializerLiftableConstantContext = - Expression.Parameter(typeof(RelationalMaterializerLiftableConstantContext), "materializerLiftableConstantContext"); + Expression.Parameter(typeof(RelationalMaterializerLiftableConstantContext), MaterializerLiftableConstantContextVariable); // The materializer expression tree contains LiftedConstantExpression nodes, which contain instructions on how to resolve // constant values which need to be lifted. var queryExecutorAfterLiftingExpression = _liftableConstantProcessor.LiftConstants(queryExecutor, materializerLiftableConstantContext, variableNames); + // The name of a lifted constant is whatever the caller of ILiftableConstantFactory.CreateLiftableConstant passed, so it + // need not be a valid C# identifier. The parameter itself is renamed rather than just its declaration, because the same + // name is what the translated executor emits for every reference to that constant. + var liftedConstantOriginals = new List(); + var liftedConstantReplacements = new List(); + foreach (var liftedConstant in _liftableConstantProcessor.LiftedConstants) + { + var name = LinqToCSharpSyntaxTranslator.SanitizeIdentifierName(liftedConstant.Parameter.Name)!; + if (name == liftedConstant.Parameter.Name) + { + continue; + } + + var baseName = name; + for (var j = 0; variableNames.Contains(name); j++) + { + name = baseName + j; + } + + variableNames.Add(name); + liftedConstantOriginals.Add(liftedConstant.Parameter); + liftedConstantReplacements.Add(Expression.Parameter(liftedConstant.Parameter.Type, name)); + } + foreach (var liftedConstant in _liftableConstantProcessor.LiftedConstants) { + var parameter = liftedConstant.Parameter; + var expression = liftedConstant.Expression; + + if (liftedConstantOriginals.Count > 0) + { + var replacer = new ReplacingExpressionVisitor(liftedConstantOriginals, liftedConstantReplacements); + parameter = (ParameterExpression)replacer.Visit(parameter); + expression = replacer.Visit(expression); + } + var variableValueSyntax = _linqToCSharpTranslator.TranslateExpression( - liftedConstant.Expression, _constantReplacements, _memberAccessReplacements, namespaces, unsafeAccessors); - // code.AppendLine($"{liftedConstant.Parameter.Type.Name} {liftedConstant.Parameter.Name} = {variableValueSyntax.NormalizeWhitespace().ToFullString()};"); - code.AppendLine($"var {liftedConstant.Parameter.Name} = {variableValueSyntax.NormalizeWhitespace().ToFullString()};"); + expression, _constantReplacements, _memberAccessReplacements, namespaces, unsafeAccessors, variableNames); + code.AppendLine($"var {parameter.Name} = {variableValueSyntax.NormalizeWhitespace().ToFullString()};"); + } + + if (liftedConstantOriginals.Count > 0) + { + queryExecutorAfterLiftingExpression = + new ReplacingExpressionVisitor(liftedConstantOriginals, liftedConstantReplacements) + .Visit(queryExecutorAfterLiftingExpression); } var queryExecutorSyntaxTree = @@ -993,7 +1116,8 @@ private void GenerateQueryExecutor( _constantReplacements, _memberAccessReplacements, namespaces, - unsafeAccessors); + unsafeAccessors, + variableNames); code .AppendLine($"return {queryExecutorSyntaxTree.NormalizeWhitespace().ToFullString()};") @@ -1256,6 +1380,16 @@ private static NewArrayExpression ProcessExecuteUpdate(MethodCallExpression exec return Expression.NewArrayInit(settersArray.Type.GetElementType()!, settersArray.Expressions.Reverse()); } + private sealed class DeclaredRuntimeConstantReplacingExpressionVisitor(Dictionary declared) + : ExpressionVisitor + { + protected override Expression VisitExtension(Expression node) + => node is RuntimeConstantExpression runtimeConstant + && declared.TryGetValue(runtimeConstant.InitializeExpression, out var existing) + ? Expression.Constant(existing.Value, runtimeConstant.Type) + : base.VisitExtension(node); + } + private static ITypeSymbol GetTypeSymbol(Compilation compilation, Type type) { if (type.IsByRef || type.IsPointer || type.IsGenericParameter || type.IsByRefLike) @@ -1288,32 +1422,6 @@ private static ITypeSymbol GetTypeSymbol(Compilation compilation, Type type) : typeSymbol; } - [return: NotNullIfNotNull(nameof(name))] - private static string? SanitizeIdentifierName(string? name) - { - if (name == null) - { - return null; - } - - if (string.IsNullOrWhiteSpace(name)) - { - return "_"; - } - - var result = new string( - [.. name.Select(c => SyntaxFacts.IsIdentifierPartCharacter(c) ? c : '_')]); - - if (!SyntaxFacts.IsValidIdentifier(result)) - { - result = "_" + result; - } - - Debug.Assert(SyntaxFacts.IsValidIdentifier(result)); - - return result; - } - /// /// Contains information on a failure to precompile a specific query in the user's source code. /// Includes information about the query, its location, and the exception that occurred. diff --git a/src/EFCore.Design/Query/Internal/RuntimeModelLinqToCSharpSyntaxTranslator.cs b/src/EFCore.Design/Query/Internal/RuntimeModelLinqToCSharpSyntaxTranslator.cs index 24d1b28f962..c2d95e78521 100644 --- a/src/EFCore.Design/Query/Internal/RuntimeModelLinqToCSharpSyntaxTranslator.cs +++ b/src/EFCore.Design/Query/Internal/RuntimeModelLinqToCSharpSyntaxTranslator.cs @@ -55,12 +55,18 @@ public virtual SyntaxNode TranslateStatement( IReadOnlyDictionary? constantReplacements, IReadOnlyDictionary? memberAccessReplacements, ISet collectedNamespaces, - ISet unsafeAccessors) + ISet unsafeAccessors, + IReadOnlySet? declaredNames = null) { _memberAccessReplacements = memberAccessReplacements; - var result = TranslateStatement(node, constantReplacements, collectedNamespaces, unsafeAccessors); - _memberAccessReplacements = null; - return result; + try + { + return TranslateStatement(node, constantReplacements, collectedNamespaces, unsafeAccessors, declaredNames); + } + finally + { + _memberAccessReplacements = null; + } } /// @@ -74,12 +80,18 @@ public virtual SyntaxNode TranslateExpression( IReadOnlyDictionary? constantReplacements, IReadOnlyDictionary? memberAccessReplacements, ISet collectedNamespaces, - ISet unsafeAccessors) + ISet unsafeAccessors, + IReadOnlySet? declaredNames = null) { _memberAccessReplacements = memberAccessReplacements; - var result = TranslateExpression(node, constantReplacements, collectedNamespaces, unsafeAccessors); - _memberAccessReplacements = null; - return result; + try + { + return TranslateExpression(node, constantReplacements, collectedNamespaces, unsafeAccessors, declaredNames); + } + finally + { + _memberAccessReplacements = null; + } } /// diff --git a/test/EFCore.Design.Tests/Query/LinqToCSharpSyntaxTranslatorTest.cs b/test/EFCore.Design.Tests/Query/LinqToCSharpSyntaxTranslatorTest.cs index 07fe7e8428f..d307fd3f3aa 100644 --- a/test/EFCore.Design.Tests/Query/LinqToCSharpSyntaxTranslatorTest.cs +++ b/test/EFCore.Design.Tests/Query/LinqToCSharpSyntaxTranslatorTest.cs @@ -496,11 +496,11 @@ public void Instantiation_with_required_properties_and_parameterless_constructor Assert.Single(unsafeAccessors), ignoreLineEndingDifferences: true)); -// => AssertExpression( -// New(typeof(BlogWithRequiredProperties).GetConstructor([])!), -// """ -// Activator.CreateInstance() -// """); + // => AssertExpression( + // New(typeof(BlogWithRequiredProperties).GetConstructor([])!), + // """ + // Activator.CreateInstance() + // """); [Fact] public void Instantiation_with_required_properties_and_non_parameterless_constructor() @@ -1179,6 +1179,439 @@ public void Block_with_non_standalone_expression_as_statement() } """); + [Fact] + public void Block_variable_with_invalid_identifier_name_is_sanitized() + { + var i = Parameter(typeof(int), "1!not valid;"); + + AssertStatement( + Block(variables: [i], Assign(i, Constant(3))), + """ +{ + var _1_not_valid_ = 3; +} +"""); + } + + [Theory] + [InlineData("class")] + [InlineData("int")] + [InlineData("__arglist")] + public void Variable_named_like_a_reserved_keyword_is_sanitized(string name) + { + var i = Parameter(typeof(int), name); + + AssertStatement( + Block(variables: [i], Assign(i, Constant(3))), + $$""" +{ + var _{{name}} = 3; +} +"""); + } + + [Theory] + [InlineData("var")] + [InlineData("async")] + [InlineData("value")] + public void Variable_whose_name_is_not_a_reserved_keyword_is_not_renamed(string name) + { + // Contextual keywords are valid identifiers here, and "value" is what generated property setters name their parameter; + // renaming either would change generated code that compiles today (every compiled model, in the case of "value") + var i = Parameter(typeof(int), name); + + AssertStatement( + Block(variables: [i], Assign(i, Constant(3))), + $$""" +{ + var {{name}} = 3; +} +"""); + } + + [Fact] + public void Lambda_parameter_with_invalid_identifier_name_is_sanitized() + { + var i = Parameter(typeof(int), "1!not valid;"); + + AssertExpression( + Lambda>(Add(i, Constant(1)), i), + "int (int _1_not_valid_) => _1_not_valid_ + 1"); + } + + [Fact] + public void Lambda_parameter_whose_sanitized_name_collides_with_an_outer_variable_is_uniquified() + { + var f = Parameter(typeof(Func), "f"); + var outer = Parameter(typeof(int), "_1_not_valid_"); + var inner = Parameter(typeof(int), "1!not valid;"); + + // Same raw name would be intentional shadowing; a collision created by sanitization alone must not rebind the outer variable + AssertStatement( + Block( + variables: [outer], + Assign(outer, Constant(8)), + Assign(f, Lambda>(Add(inner, outer), inner))), + """ +{ + var _1_not_valid_ = 8; + f = int (int _1_not_valid_0) => _1_not_valid_0 + _1_not_valid_; +} +"""); + } + + [Fact] + public void Lambda_parameter_whose_sanitized_name_collides_with_a_lifted_variable_is_uniquified() + { + var f = Parameter(typeof(Func), "f"); + var lifted = Parameter(typeof(int), "_1_not_valid_"); + var inner = Parameter(typeof(int), "1!not valid;"); + + // The lifted variable lives in the lifted state rather than the stack frame while the lambda is translated, and ends up + // declared in the lambda body next to the parameter, so a collision would not even compile + AssertStatement( + Block( + variables: [f], + Assign( + f, + Block( + variables: [lifted], + Assign(lifted, Constant(8)), + Lambda>(Add(inner, lifted), inner)))), + """ +{ + var f = int (int _1_not_valid_0) => + { + var _1_not_valid_ = 8; + return _1_not_valid_0 + _1_not_valid_; + }; +} +"""); + } + + [Fact] + public void Lambda_parameter_with_the_same_name_as_a_lifted_variable_is_uniquified() + { + var f = Parameter(typeof(Func), "f"); + var lifted = Parameter(typeof(int), "i"); + var inner = Parameter(typeof(int), "i"); + + // The lifted variable is declared in the lambda body next to the parameter, so the same name would not compile (CS0136) + // even though the original names match exactly + AssertStatement( + Block( + variables: [f], + Assign( + f, + Block( + variables: [lifted], + Assign(lifted, Constant(8)), + Lambda>(Add(inner, lifted), inner)))), + """ +{ + var f = int (int i0) => + { + var i = 8; + return i0 + i; + }; +} +"""); + } + + [Fact] + public void Scope_state_is_restored_when_a_translation_fails() + { + var (translator, _) = CreateTranslator(); + var e = Parameter(typeof(InvalidOperationException), "e"); + var i = Parameter(typeof(int), "i"); + var x = Parameter(typeof(int), "x"); + + // RuntimeVariables is not supported, so this throws after a catch scope and a block scope were entered and after the + // invocation argument was lifted into a local named "x" + Assert.Throws( + () => translator.TranslateStatement( + TryCatch( + Call(FooMethod), + Catch( + e, + Block( + variables: [i], + Assign(i, Invoke(Lambda>(x, x), Call(FooMethod))), + RuntimeVariables(e), + Constant(1)))), + constantReplacements: null, + new HashSet(), + new HashSet())); + + // The translator is reused across queries, so the next translation must see neither the abandoned scopes nor the lifted name + var result = translator.TranslateStatement( + Block(variables: [x], Assign(x, Constant(1))), + constantReplacements: null, + new HashSet(), + new HashSet()); + + Assert.Equal( + """ +{ + var x = 1; +} +""", + result.NormalizeWhitespace().ToFullString(), + ignoreLineEndingDifferences: true); + } + + [Fact] + public void Generated_local_does_not_take_the_name_of_a_captured_variable() + { + var captured = Parameter(typeof(int), "x"); + var lambdaParameter = Parameter(typeof(int), "x"); + var i = Parameter(typeof(int), "i"); + + // The capture is emitted before the invocation lifts its argument into a local, so the local is the one that has to move + AssertStatement( + Block( + variables: [i], + Call(ReturnsIntWithParamMethod, captured), + Assign( + i, + Invoke(Lambda>(Add(lambdaParameter, Constant(1)), lambdaParameter), Call(FooMethod)))), + """ +{ + LinqToCSharpSyntaxTranslatorTest.ReturnsIntWithParam(x); + var x0 = LinqToCSharpSyntaxTranslatorTest.Foo(); + var i = x0 + 1; +} +"""); + } + + [Fact] + public void Lambda_parameter_whose_sanitized_name_collides_with_a_captured_variable_is_uniquified() + { + var captured = Parameter(typeof(int), "_1_not_valid_"); + var inner = Parameter(typeof(int), "1!not valid;"); + + // A captured variable is a local of the enclosing generated method, which a lambda parameter may not shadow + AssertStatement( + Block(Lambda>(Add(inner, captured), inner)), + """ +{ + _ = (int (int _1_not_valid_0) => _1_not_valid_0 + _1_not_valid_); +} +"""); + } + + [Fact] + public void Block_variable_whose_sanitized_name_collides_with_a_captured_variable_is_uniquified() + { + var captured = Parameter(typeof(int), "_1_not_valid_"); + var i = Parameter(typeof(int), "1!not valid;"); + + AssertStatement( + Block(variables: [i], Assign(i, Constant(1)), Call(ReturnsIntWithParamMethod, Add(i, captured))), + """ +{ + var _1_not_valid_0 = 1; + LinqToCSharpSyntaxTranslatorTest.ReturnsIntWithParam(_1_not_valid_0 + _1_not_valid_); +} +"""); + } + + [Fact] + public void Generated_local_does_not_take_the_name_of_a_captured_variable_referenced_later() + { + var f = Parameter(typeof(Func), "f"); + var captured = Parameter(typeof(int), "x"); + var lambdaParameter = Parameter(typeof(int), "x"); + + // The capture is referenced after the invocation lifts its argument, so the name has to be reserved before translating + AssertStatement( + Block( + Assign(f, Invoke(Lambda>>(Lambda>(lambdaParameter, lambdaParameter), lambdaParameter), Call(FooMethod))), + Call(ReturnsIntWithParamMethod, captured)), + """ +{ + f = int (int x00) => + { + var x0 = LinqToCSharpSyntaxTranslatorTest.Foo(); + return x00; + }; + LinqToCSharpSyntaxTranslatorTest.ReturnsIntWithParam(x); +} +"""); + } + + [Fact] + public void Captured_variable_with_the_name_of_a_constant_replacement_is_emitted_as_is() + { + var captured = Parameter(typeof(int), "constant"); + + // Constant replacements are declared by the generated code that calls into the translated expression, so a captured + // variable naming one of them is the intended reference rather than a collision + AssertStatement( + Block(Call(ReturnsIntWithParamMethod, captured)), + """ +{ + LinqToCSharpSyntaxTranslatorTest.ReturnsIntWithParam(constant); +} +""", + constantReplacements: new Dictionary { { new object(), "constant" } }); + } + + [Fact] + public void Variable_name_with_a_formatting_character_is_sanitized() + { + // U+200C is a Unicode formatting character: valid inside a C# identifier, but ignored when identifiers are compared, so + // these two names would declare the same local twice + var plain = Parameter(typeof(int), "ab"); + var formatted = Parameter(typeof(int), "a\u200Cb"); + + AssertStatement( + Block(variables: [plain, formatted], Assign(plain, Constant(1)), Assign(formatted, Constant(2))), + """ +{ + var ab = 1; + var a_b = 2; +} +"""); + } + + [Fact] + public void Generated_name_equal_to_a_captured_variable_up_to_formatting_characters_is_uniquified() + { + var captured = Parameter(typeof(int), "a\u200Cb"); + var generated = Parameter(typeof(int), "ab"); + + // The captured variable is the caller's and is emitted as it is; the generated local must not become the same identifier + AssertStatement( + Block(variables: [generated], Assign(generated, Constant(1)), Call(ReturnsIntWithParamMethod, captured)), + $$""" +{ + var ab0 = 1; + LinqToCSharpSyntaxTranslatorTest.ReturnsIntWithParam(a{{"\u200C"}}b); +} +"""); + } + + [Fact] + public void Type_whose_simple_name_is_a_constant_replacement_is_fully_qualified() + // The constant replacements are declared in the scope the translation is emitted into, so a type referred to by that simple + // name would resolve to the declaration instead of the type + => AssertExpression( + Property(null, typeof(System.Text.Encoding), nameof(System.Text.Encoding.UTF8)), + "global::System.Text.Encoding.UTF8", + constantReplacements: new Dictionary { { new object(), "Encoding" } }); + + [Fact] + public void Generic_type_whose_simple_name_is_a_constant_replacement_is_fully_qualified() + => AssertExpression( + New(typeof(List)), + "new global::System.Collections.Generic.List()", + constantReplacements: new Dictionary { { new object(), "List" } }); + + [Fact] + public void Type_whose_simple_name_is_a_captured_variable_is_fully_qualified() + { + var captured = Parameter(typeof(int), "Encoding"); + var utf8 = Parameter(typeof(System.Text.Encoding), "utf8"); + + // The captured variable is emitted under its own name, so within this scope that name is the caller's variable, not the type + AssertStatement( + Block( + variables: [utf8], + Assign(utf8, Property(null, typeof(System.Text.Encoding), nameof(System.Text.Encoding.UTF8))), + Call(ReturnsIntWithParamMethod, captured)), + """ +{ + var utf8 = global::System.Text.Encoding.UTF8; + LinqToCSharpSyntaxTranslatorTest.ReturnsIntWithParam(Encoding); +} +"""); + } + + [Fact] + public void Type_whose_simple_name_is_a_lambda_parameter_is_fully_qualified() + { + var encoding = Parameter(typeof(int), "Encoding"); + var f = Parameter(typeof(Func), "f"); + + AssertStatement( + Block( + Assign( + f, Lambda>( + Property( + Property(null, typeof(System.Text.Encoding), nameof(System.Text.Encoding.UTF8)), + nameof(System.Text.Encoding.CodePage)), + encoding))), + """ +{ + f = int (int Encoding) => global::System.Text.Encoding.UTF8.CodePage; +} +"""); + } + + [Fact] + public void Nested_type_whose_declaring_type_name_is_a_constant_replacement_is_fully_qualified() + // The nested type is written through its declaring type, which is what the name in scope would capture + => AssertExpression( + New(typeof(Blog)), + "new global::Microsoft.EntityFrameworkCore.Query.LinqToCSharpSyntaxTranslatorTest.Blog()", + constantReplacements: new Dictionary { { new object(), nameof(LinqToCSharpSyntaxTranslatorTest) } }); + + [Fact] + public void Type_whose_simple_name_is_a_variable_in_scope_is_fully_qualified() + { + var encoding = Parameter(typeof(int), "Encoding"); + var utf8 = Parameter(typeof(System.Text.Encoding), "utf8"); + + AssertStatement( + Block( + variables: [encoding, utf8], + Assign(encoding, Constant(1)), + Assign(utf8, Property(null, typeof(System.Text.Encoding), nameof(System.Text.Encoding.UTF8)))), + """ +{ + var Encoding = 1; + var utf8 = global::System.Text.Encoding.UTF8; +} +"""); + } + + [Fact] + public void Catch_variable_already_declared_by_an_enclosing_block_throws() + { + var (translator, _) = CreateTranslator(); + var e = Parameter(typeof(InvalidOperationException), "e"); + + // The same ParameterExpression declared twice would silently become two unrelated C# locals + var exception = Assert.Throws( + () => translator.TranslateStatement( + Block(variables: [e], TryCatch(Call(FooMethod), Catch(e, Call(BarMethod)))), + constantReplacements: null, + new HashSet(), + new HashSet())); + + Assert.Contains("'e'", exception.Message); + } + + [Fact] + public void Catch_variable_with_invalid_identifier_name_is_sanitized() + { + var e = Parameter(typeof(InvalidOperationException), "1!not valid;"); + + AssertStatement( + TryCatch(Call(FooMethod), Catch(e, Throw(e, typeof(int)))), + """ +try +{ + LinqToCSharpSyntaxTranslatorTest.Foo(); +} +catch (InvalidOperationException _1_not_valid_) +{ + throw _1_not_valid_; +} +"""); + } + [Fact] public void Lift_block_in_assignment_context() { @@ -1694,6 +2127,25 @@ public void Goto_with_named_label() """); } + [Fact] + public void Label_with_invalid_identifier_name_is_sanitized() + { + var labelTarget = Label("1!not valid;"); + + AssertStatement( + Block( + Goto(labelTarget), + Label(labelTarget), + Call(FooMethod)), + """ +{ + goto _1_not_valid_; + _1_not_valid_: + LinqToCSharpSyntaxTranslatorTest.Foo(); +} +"""); + } + [Fact] public void Goto_with_label_on_last_line() { diff --git a/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs index a36e2612871..46356a0a9e7 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/AdHocPrecompiledQueryRelationalTestBase.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; using Microsoft.EntityFrameworkCore.Query.Internal; using static Microsoft.EntityFrameworkCore.TestUtilities.PrecompiledQueryTestHelpers; @@ -417,6 +418,603 @@ public class InvalidShadowNameEntity public Guid Id { get; set; } } +#pragma warning disable EF9100 + [Fact] + public virtual async Task Query_reusing_a_runtime_constant_of_a_query_that_failed_to_precompile() + { + var contextFactory = await InitializeNonSharedTest( + addServices: s => s.AddSingleton()); + var options = contextFactory.GetOptions(); + + // Both queries read the same JSON property, whose name is emitted as a runtime constant, so the second one depends on + // state the first one registered before it failed. + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.SharedRuntimeConstantContext(dbContextOptions); +if (Environment.GetEnvironmentVariable("EF_TEST_NEVER_SET") is not null) +{ + var first = await context.Entities.ToListAsync(); +} + +var second = await context.Entities.OrderBy(e => e.Id).ToListAsync(); +""", + typeof(SharedRuntimeConstantContext), + options, + precompilationErrorAsserter: errors => Assert.Single(errors)); + } + + [Fact] + public virtual async Task Runtime_constants_differing_only_by_a_formatting_character_get_distinct_fields() + { + var contextFactory = await InitializeNonSharedTest(); + var options = contextFactory.GetOptions(); + + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.FormattingCharacterContext(dbContextOptions); +var entities = await context.Entities.ToListAsync(); +""", + typeof(FormattingCharacterContext), + options, + interceptorCodeAsserter: code => + { + Assert.Contains("_NameBytes", code); + Assert.Contains("_Na_meBytes", code); + }); + } + + // Two JSON property names that differ only by U+200C, a formatting character, which C# ignores when comparing identifiers: + // emitted as they are, the two runtime constant fields would be one duplicate declaration + public class FormattingCharacterContext(DbContextOptions options) : DbContext(options) + { + public DbSet Entities { get; set; } = null!; + + protected override void OnModelCreating(ModelBuilder modelBuilder) + => modelBuilder.Entity().ComplexProperty( + x => x.Nested, b => + { + b.ToJson(); + b.Property(x => x.Name).HasJsonPropertyName("Name"); + b.Property(x => x.Other).HasJsonPropertyName("Na\u200Cme"); + }); + } + + public class FormattingCharacterEntity + { + public Guid Id { get; set; } + public FormattingCharacterNested Nested { get; set; } = new(); + } + + public class FormattingCharacterNested + { + public string Name { get; set; } = ""; + public string Other { get; set; } = ""; + } + + [Fact] + public virtual async Task Liftable_constant_named_like_a_runtime_constant_field() + { + var contextFactory = await InitializeNonSharedTest( + addServices: s => s.AddSingleton()); + var options = contextFactory.GetOptions(); + + // Lifted constants start lowercase and runtime constant fields start with an underscore, but a lifted name can start with an + // underscore too, so the field's name is the one place the two schemes can meet + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.SharedRuntimeConstantContext(dbContextOptions); +var entities = await context.Entities.ToListAsync(); +""", + typeof(SharedRuntimeConstantContext), + options, + interceptorCodeAsserter: code => + { + Assert.Contains("_NameBytes = ", code); + Assert.Contains("var _NameBytes0 = ", code); + }); + } + + public class RuntimeConstantFieldLiftableConstantFactory(LiftableConstantExpressionDependencies dependencies) + : RenamingLiftableConstantFactory(dependencies) + { + protected override string Name + => "_NameBytes"; + } + + [Fact] + public virtual async Task Runtime_constant_shared_by_two_queries_is_emitted_once() + { + var contextFactory = await InitializeNonSharedTest(); + var options = contextFactory.GetOptions(); + + // Each query's shaper evaluates the JSON property name into a byte array of its own, so the two constants are equal by + // initializer but not by value, and must still share one field. + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.SharedRuntimeConstantContext(dbContextOptions); +var first = await context.Entities.ToListAsync(); +var second = await context.Entities.OrderBy(e => e.Id).ToListAsync(); +""", + typeof(SharedRuntimeConstantContext), + options, + interceptorCodeAsserter: code => + { + Assert.Equal(1, Regex.Matches(code, @"\bprivate\s+static\s+readonly\b[^;=]*\b_NameBytes\s*=").Count); + Assert.DoesNotContain("_NameBytes0", code); + }); + } + + public class SharedRuntimeConstantContext(DbContextOptions options) : DbContext(options) + { + public DbSet Entities { get; set; } = null!; + + protected override void OnModelCreating(ModelBuilder modelBuilder) + => modelBuilder.Entity() + .ComplexProperty(x => x.Nested, b => b.ToJson()); + } + + public class SharedRuntimeConstantEntity + { + public Guid Id { get; set; } + public SharedRuntimeConstantNested Nested { get; set; } = new(); + } + + public class SharedRuntimeConstantNested + { + public string Name { get; set; } = ""; + } + + // Poisons only the first liftable constant, so the first query fails while generating its executor and the second does not + public class PoisonFirstLiftableConstantFactory(LiftableConstantExpressionDependencies dependencies) + : LiftableConstantFactory(dependencies) + { + private bool _poisoned; + + public override Expression CreateLiftableConstant( + object? originalValue, + Expression> resolverExpression, + string variableName, + Type type) + { + if (!_poisoned) + { + _poisoned = true; + resolverExpression = Expression.Lambda>( + Expression.Parameter(typeof(object)), resolverExpression.Parameters); + } + + return base.CreateLiftableConstant(originalValue, resolverExpression, variableName, type); + } + } + + [Fact] + public virtual async Task Query_that_fails_to_precompile_leaves_the_other_queries_compilable() + { + var contextFactory = await InitializeNonSharedTest( + addServices: s => s.AddSingleton()); + var options = contextFactory.GetOptions(); + + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.PartialOutputContext(dbContextOptions); +if (Environment.GetEnvironmentVariable("EF_TEST_NEVER_SET") is not null) +{ + var firsts = await context.Firsts.ToListAsync(); +} + +var seconds = await context.Seconds.ToListAsync(); +""", + typeof(PartialOutputContext), + options, + interceptorCodeAsserter: code => Assert.Contains("UnsafeAccessor", code), + precompilationErrorAsserter: errors => Assert.Single(errors)); + } + + [Fact] + public virtual async Task Query_that_fails_to_precompile_between_two_that_succeed_leaves_both_compilable() + { + var contextFactory = await InitializeNonSharedTest( + addServices: s => s.AddSingleton()); + var options = contextFactory.GetOptions(); + + // Code is added to the file before and after the failed query, so a splice that went wrong would corrupt either neighbour + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.PartialOutputContext(dbContextOptions); +var seconds = await context.Seconds.ToListAsync(); +if (Environment.GetEnvironmentVariable("EF_TEST_NEVER_SET") is not null) +{ + var firsts = await context.Firsts.ToListAsync(); +} + +var orderedSeconds = await context.Seconds.OrderBy(s => s.Id).ToListAsync(); +""", + typeof(PartialOutputContext), + options, + interceptorCodeAsserter: code => + { + Assert.Contains("#region Query1", code); + Assert.DoesNotContain("#region Query2", code); + Assert.Contains("#region Query3", code); + }, + precompilationErrorAsserter: errors => Assert.Single(errors)); + } + + public class PartialOutputContext(DbContextOptions options) : DbContext(options) + { + public DbSet Firsts { get; set; } = null!; + public DbSet Seconds { get; set; } = null!; + } + + public class PartialOutputFirst + { + // A private setter makes the generated code reach this through an unsafe accessor, which the translator caches and + // re-adds on every later translation, so the failed query's accessor has to stay compilable + public Guid Id { get; private set; } + } + + public class PartialOutputSecond + { + public Guid Id { get; set; } + } + + // Gives the first query's entity type a resolver the C# translator cannot emit, so that query fails while generating its + // executor, after its interceptors have already been written. That is what leaves partial output behind. + public class UntranslatableLiftableConstantFactory(LiftableConstantExpressionDependencies dependencies) + : LiftableConstantFactory(dependencies) + { + public override Expression CreateLiftableConstant( + object? originalValue, + Expression> resolverExpression, + string variableName, + Type type) + { + if (originalValue is IEntityType entityType && entityType.ClrType == typeof(PartialOutputFirst)) + { + resolverExpression = Expression.Lambda>( + Expression.Parameter(typeof(object)), resolverExpression.Parameters); + } + + return base.CreateLiftableConstant(originalValue, resolverExpression, variableName, type); + } + } + + [Fact] + public virtual Task Liftable_constant_named_like_a_keyword() + => TestLiftableConstantName(); + + [Fact] + public virtual Task Liftable_constant_named_like_the_query_context_parameter() + => TestLiftableConstantName(); + + [Fact] + public virtual Task Liftable_constant_named_like_the_db_context_parameter() + => TestLiftableConstantName(); + + [Fact] + public virtual Task Liftable_constant_whose_sanitized_name_is_another_constants_name() + => TestLiftableConstantName(); + + // Every constant is given the same name, so the name under test is certain to reach the generated file whichever constants + // the optimizer keeps, and the duplicates exercise uniquification at the same time. + private async Task TestLiftableConstantName([CallerMemberName] string callerName = "") + where TFactory : class, ILiftableConstantFactory + { + var contextFactory = await InitializeNonSharedTest( + addServices: s => s.AddSingleton()); + + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.KeywordLiftableConstantContext(dbContextOptions); +var entities = await context.Entities.ToListAsync(); +""", + typeof(KeywordLiftableConstantContext), + contextFactory.GetOptions(), + callerName: callerName); + } + + public class KeywordLiftableConstantContext(DbContextOptions options) : DbContext(options) + { + public DbSet Entities { get; set; } = null!; + } + + public class KeywordLiftableConstantEntity + { + public Guid Id { get; set; } + public string Name { get; set; } = ""; + } + + // The variable name is whatever the caller passes, and providers call this API, so it can be a C# keyword or one of the + // identifiers the generated executor declares for itself. + public abstract class RenamingLiftableConstantFactory(LiftableConstantExpressionDependencies dependencies) + : LiftableConstantFactory(dependencies) + { + protected abstract string Name { get; } + + public override Expression CreateLiftableConstant( + object? originalValue, + Expression> resolverExpression, + string variableName, + Type type) + => base.CreateLiftableConstant(originalValue, resolverExpression, Name, type); + } + + public class KeywordLiftableConstantFactory(LiftableConstantExpressionDependencies dependencies) + : RenamingLiftableConstantFactory(dependencies) + { + protected override string Name + => "Class"; + } + + public class QueryContextLiftableConstantFactory(LiftableConstantExpressionDependencies dependencies) + : RenamingLiftableConstantFactory(dependencies) + { + protected override string Name + => "queryContext"; + } + + public class DbContextLiftableConstantFactory(LiftableConstantExpressionDependencies dependencies) + : RenamingLiftableConstantFactory(dependencies) + { + protected override string Name + => "DbContext"; + } + + // One name sanitizes into the other, so the two must not end up sharing a declaration + public class CollidingLiftableConstantFactory(LiftableConstantExpressionDependencies dependencies) + : LiftableConstantFactory(dependencies) + { + public override Expression CreateLiftableConstant( + object? originalValue, + Expression> resolverExpression, + string variableName, + Type type) + => base.CreateLiftableConstant( + originalValue, resolverExpression, originalValue is IEntityType ? "Class" : "_class", type); + } + + [Fact] + public virtual Task Runtime_constant_named_like_the_executor_field() + => TestRuntimeConstantName(); + + [Fact] + public virtual Task Runtime_constant_named_like_the_interceptors_class() + => TestRuntimeConstantName(); + + [Fact] + public virtual Task Runtime_constant_named_like_a_type_the_generated_code_uses() + => TestRuntimeConstantName(); + + [Fact] + public virtual Task Runtime_constant_named_like_a_reserved_token_once_prefixed() + => TestRuntimeConstantName(); + + [Fact] + public virtual Task Runtime_constant_named_like_a_type_with_a_leading_underscore() + => TestRuntimeConstantName(); + + [Fact] + public virtual async Task Shaper_variables_named_like_the_executor_identifiers_are_uniquified() + { + var contextFactory = await InitializeNonSharedTest( + addServices: s => s.AddScoped()); + + // The shaper's variables end up declared inside the executor lambda, which the generated executor method wraps in its own + // parameters and locals of these names + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.RuntimeConstantNameContext(dbContextOptions); +var entities = await context.Entities.ToListAsync(); +""", + typeof(RuntimeConstantNameContext), + contextFactory.GetOptions(), + interceptorCodeAsserter: code => + { + foreach (var name in ExecutorNamedVariablesVisitorFactory.Names) + { + Assert.DoesNotContain($"var {name} = 1;", code); + Assert.Matches($@"\bvar {name}\d+ = 1;", code); + } + }); + } + + // A shaper that declares variables under the names the generated executor method uses for its own parameters and locals + public class ExecutorNamedVariablesVisitorFactory( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies) + : RelationalShapedQueryCompilingExpressionVisitorFactory(dependencies, relationalDependencies) + { + public static readonly string[] Names = + [ + "dbContext", "queryContext", "relationalModel", "relationalTypeMappingSource", "materializerLiftableConstantContext" + ]; + + public override ShapedQueryCompilingExpressionVisitor Create(QueryCompilationContext queryCompilationContext) + => new ExecutorNamedVariablesVisitor(Dependencies, RelationalDependencies, queryCompilationContext); + + private sealed class ExecutorNamedVariablesVisitor( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies, + QueryCompilationContext queryCompilationContext) + : RelationalShapedQueryCompilingExpressionVisitor(dependencies, relationalDependencies, queryCompilationContext) + { + protected override Expression VisitShapedQuery(ShapedQueryExpression shapedQueryExpression) + { + var variables = Names.Select(name => Expression.Variable(typeof(int), name)).ToList(); + + return Expression.Block( + variables, + variables.Select(variable => (Expression)Expression.Assign(variable, Expression.Constant(1))) + .Append(base.VisitShapedQuery(shapedQueryExpression))); + } + } + } + + // A runtime constant becomes a field of the generated interceptors class, and its name is whatever the shaper that created it + // chose, so a provider's shaper can hand it the name of a member the generator emits for itself, or of a type the generated + // code refers to by its simple name. + private async Task TestRuntimeConstantName([CallerMemberName] string callerName = "") + where TFactory : class, IShapedQueryCompilingExpressionVisitorFactory + { + var contextFactory = await InitializeNonSharedTest( + addServices: s => s.AddScoped()); + + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.RuntimeConstantNameContext(dbContextOptions); +var entities = await context.Entities.ToListAsync(); +""", + typeof(RuntimeConstantNameContext), + contextFactory.GetOptions(), + callerName: callerName); + } + + public class RuntimeConstantNameContext(DbContextOptions options) : DbContext(options) + { + public DbSet Entities { get; set; } = null!; + } + + public class RuntimeConstantNameEntity + { + public Guid Id { get; set; } + } + + // Emits a runtime constant under the given name, the way the relational shaper emits a JSON property name + public abstract class NamingRuntimeConstantVisitorFactory( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies) + : RelationalShapedQueryCompilingExpressionVisitorFactory(dependencies, relationalDependencies) + { + protected abstract string Name { get; } + + // The relational shaper's own initializer for a JSON property name + protected virtual Expression CreateInitializer(string name) + => Expression.Call( + Expression.Property(null, typeof(Encoding), nameof(Encoding.UTF8)), + typeof(Encoding).GetMethod(nameof(Encoding.GetBytes), [typeof(string)])!, + Expression.Constant(name)); + + public override ShapedQueryCompilingExpressionVisitor Create(QueryCompilationContext queryCompilationContext) + => new NamingRuntimeConstantVisitor( + Dependencies, RelationalDependencies, queryCompilationContext, Name, CreateInitializer(Name)); + + private sealed class NamingRuntimeConstantVisitor( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies, + QueryCompilationContext queryCompilationContext, + string name, + Expression initializer) + : RelationalShapedQueryCompilingExpressionVisitor(dependencies, relationalDependencies, queryCompilationContext) + { + protected override Expression VisitShapedQuery(ShapedQueryExpression shapedQueryExpression) + => Expression.Block( + Expression.Call(typeof(GC).GetMethod(nameof(GC.KeepAlive))!, new RuntimeConstantExpression(name, initializer)), + base.VisitShapedQuery(shapedQueryExpression)); + } + } + + public class ExecutorFieldRuntimeConstantVisitorFactory( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies) + : NamingRuntimeConstantVisitorFactory(dependencies, relationalDependencies) + { + protected override string Name + => "Query1_Executor"; + } + + public class InterceptorsClassRuntimeConstantVisitorFactory( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies) + : NamingRuntimeConstantVisitorFactory(dependencies, relationalDependencies) + { + protected override string Name + => "EntityFrameworkCoreInterceptors"; + } + + // The field's own initializer is Encoding.UTF8.GetBytes(...), as it is for every JSON property name + public class TypeNameRuntimeConstantVisitorFactory( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies) + : NamingRuntimeConstantVisitorFactory(dependencies, relationalDependencies) + { + protected override string Name + => "Encoding"; + } + + // A valid identifier on its own, but a reserved token once the field prefix is in front of it + public class ReservedTokenRuntimeConstantVisitorFactory( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies) + : NamingRuntimeConstantVisitorFactory(dependencies, relationalDependencies) + { + protected override string Name + => "_arglist"; + } + + // The prefixed field name is exactly the name of the type the initializer reads a static member from + public class UnderscoreTypeRuntimeConstantVisitorFactory( + ShapedQueryCompilingExpressionVisitorDependencies dependencies, + RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies) + : NamingRuntimeConstantVisitorFactory(dependencies, relationalDependencies) + { + protected override string Name + => "RuntimeConstantSource"; + + protected override Expression CreateInitializer(string name) + => Expression.Property(null, typeof(_RuntimeConstantSource), nameof(_RuntimeConstantSource.Value)); + } + + [Fact] + public virtual async Task Runtime_constant_named_like_an_unsafe_accessor() + { + var contextFactory = await InitializeNonSharedTest(); + + await Test( + """ +await using var context = new AdHocPrecompiledQueryRelationalTestBase.AccessorNameContext(dbContextOptions); +var entities = await context.Entities.ToListAsync(); +""", + typeof(AccessorNameContext), + contextFactory.GetOptions(), + interceptorCodeAsserter: code => Assert.Contains( + "UnsafeAccessor_Microsoft_EntityFrameworkCore_Query_AccessorNameEntity_set_NameBytes(", code)); + } + + // The model alone can make a runtime constant take a generated name: a JSON property name is emitted as a runtime constant with + // a Bytes suffix, and a private setter that materialization goes through is reached by an unsafe accessor named after its type + // and the setter method. + public class AccessorNameContext(DbContextOptions options) : DbContext(options) + { + public DbSet Entities { get; set; } = null!; + + protected override void OnModelCreating(ModelBuilder modelBuilder) + => modelBuilder.Entity( + b => + { + b.Property(x => x.NameBytes).UsePropertyAccessMode(PropertyAccessMode.Property); + b.ComplexProperty( + x => x.Nested, nb => + { + nb.ToJson(); + nb.Property(x => x.Name) + .HasJsonPropertyName("UnsafeAccessor_Microsoft_EntityFrameworkCore_Query_AccessorNameEntity_set_Name"); + }); + }); + } + + public class AccessorNameEntity + { + public Guid Id { get; set; } + public byte[]? NameBytes { get; private set; } + public AccessorNameNested Nested { get; set; } = new(); + } + + public class AccessorNameNested + { + public string Name { get; set; } = ""; + } +#pragma warning restore EF9100 + #endregion protected TestSqlLoggerFactory TestSqlLoggerFactory @@ -452,3 +1050,10 @@ protected override IServiceCollection AddServices(IServiceCollection serviceColl protected override string NonSharedStoreName => "AdHocPrecompiledQueryTest"; } + +// Outside the test class so that the generated code refers to it by its simple name, which a runtime constant field can take +public static class _RuntimeConstantSource +{ + public static byte[] Value + => [1]; +} diff --git a/test/EFCore.Relational.Specification.Tests/Query/PrecompiledQueryRelationalTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/PrecompiledQueryRelationalTestBase.cs index 91b273873c6..241da6ee47a 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/PrecompiledQueryRelationalTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/PrecompiledQueryRelationalTestBase.cs @@ -210,11 +210,11 @@ public virtual Task RuntimeConstantExpression() interceptorCodeAsserter: code => { Assert.Matches( - @"\bprivate\s+static\s+readonly\b(?=[^;]*\bNumberBytes\b)[^;=]*\bNumberBytes\s*=\s*[^;]+;", - code); // Expected a private static readonly field named NumberBytes with an initializer. + @"\bprivate\s+static\s+readonly\b(?=[^;]*\b_NumberBytes\b)[^;=]*\b_NumberBytes\s*=\s*[^;]+;", + code); // Expected a private static readonly field named _NumberBytes with an initializer. Assert.True( - Regex.Matches(code, @"\bNumberBytes\b").Count > 1, - "Expected at least 1 reference to NumberBytes excluding the initializer."); + Regex.Matches(code, @"\b_NumberBytes\b").Count > 1, + "Expected at least 1 reference to _NumberBytes excluding the initializer."); }); #endregion Expression types diff --git a/test/EFCore.Relational.Specification.Tests/TestUtilities/PrecompiledQueryTestHelpers.cs b/test/EFCore.Relational.Specification.Tests/TestUtilities/PrecompiledQueryTestHelpers.cs index 6cfd60b610d..f4d854f7511 100644 --- a/test/EFCore.Relational.Specification.Tests/TestUtilities/PrecompiledQueryTestHelpers.cs +++ b/test/EFCore.Relational.Specification.Tests/TestUtilities/PrecompiledQueryTestHelpers.cs @@ -95,7 +95,7 @@ public async Task FullSourceTest( _metadataReferences, new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary, nullableContextOptions: NullableContextOptions.Enable)); - IReadOnlyList? generatedFiles = null; + IReadOnlyList generatedFiles = []; try { @@ -127,7 +127,14 @@ public async Task FullSourceTest( else { errorAsserter(precompilationErrors); - return; + + // Carry on to compile what was generated: a query that failed to precompile must not leave the queries that + // succeeded in a file that does not compile. A file in which every query failed is not generated at all, so + // when every located query failed there is nothing to compile. + if (generatedFiles.Count == 0) + { + return; + } } interceptorCodeAsserter?.Invoke(generatedFiles.Single().Code); @@ -171,14 +178,11 @@ public async Task FullSourceTest( void PrintGeneratedSources() { - if (generatedFiles is not null) + foreach (var generatedFile in generatedFiles) { - foreach (var generatedFile in generatedFiles) - { - testOutputHelper.WriteLine($"Generated file {generatedFile.Path}: "); - testOutputHelper.WriteLine(""); - testOutputHelper.WriteLine(generatedFile.Code); - } + testOutputHelper.WriteLine($"Generated file {generatedFile.Path}: "); + testOutputHelper.WriteLine(""); + testOutputHelper.WriteLine(generatedFile.Code); } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/AdHocPrecompiledQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/AdHocPrecompiledQuerySqlServerTest.cs index ce8b23b63cf..2b9ebd0fd84 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/AdHocPrecompiledQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/AdHocPrecompiledQuerySqlServerTest.cs @@ -124,6 +124,73 @@ FROM [Entities] AS [e] """); } + public override async Task Query_reusing_a_runtime_constant_of_a_query_that_failed_to_precompile() + { + await base.Query_reusing_a_runtime_constant_of_a_query_that_failed_to_precompile(); + + AssertSql( + """ +SELECT [e].[Id], [e].[Nested] +FROM [Entities] AS [e] +ORDER BY [e].[Id] +"""); + } + + public override async Task Query_that_fails_to_precompile_leaves_the_other_queries_compilable() + { + await base.Query_that_fails_to_precompile_leaves_the_other_queries_compilable(); + + AssertSql( + """ +SELECT [s].[Id] +FROM [Seconds] AS [s] +"""); + } + + public override async Task Liftable_constant_named_like_the_query_context_parameter() + { + await base.Liftable_constant_named_like_the_query_context_parameter(); + + AssertSql( + """ +SELECT [e].[Id], [e].[Name] +FROM [Entities] AS [e] +"""); + } + + public override async Task Liftable_constant_named_like_the_db_context_parameter() + { + await base.Liftable_constant_named_like_the_db_context_parameter(); + + AssertSql( + """ +SELECT [e].[Id], [e].[Name] +FROM [Entities] AS [e] +"""); + } + + public override async Task Liftable_constant_whose_sanitized_name_is_another_constants_name() + { + await base.Liftable_constant_whose_sanitized_name_is_another_constants_name(); + + AssertSql( + """ +SELECT [e].[Id], [e].[Name] +FROM [Entities] AS [e] +"""); + } + + public override async Task Liftable_constant_named_like_a_keyword() + { + await base.Liftable_constant_named_like_a_keyword(); + + AssertSql( + """ +SELECT [e].[Id], [e].[Name] +FROM [Entities] AS [e] +"""); + } + public override async Task Invalid_identifier_shadow_property_name() { await base.Invalid_identifier_shadow_property_name(); @@ -142,6 +209,139 @@ public virtual void Check_all_tests_overridden() protected override ITestStoreFactory NonSharedTestStoreFactory => SqlServerTestStoreFactory.Instance; + public override async Task Runtime_constant_named_like_the_executor_field() + { + await base.Runtime_constant_named_like_the_executor_field(); + + AssertSql( + """ +SELECT [e].[Id] +FROM [Entities] AS [e] +"""); + } + + public override async Task Runtime_constant_named_like_the_interceptors_class() + { + await base.Runtime_constant_named_like_the_interceptors_class(); + + AssertSql( + """ +SELECT [e].[Id] +FROM [Entities] AS [e] +"""); + } + + public override async Task Runtime_constant_named_like_a_type_the_generated_code_uses() + { + await base.Runtime_constant_named_like_a_type_the_generated_code_uses(); + + AssertSql( + """ +SELECT [e].[Id] +FROM [Entities] AS [e] +"""); + } + + public override async Task Runtime_constant_named_like_a_reserved_token_once_prefixed() + { + await base.Runtime_constant_named_like_a_reserved_token_once_prefixed(); + + AssertSql( + """ +SELECT [e].[Id] +FROM [Entities] AS [e] +"""); + } + + public override async Task Runtime_constants_differing_only_by_a_formatting_character_get_distinct_fields() + { + await base.Runtime_constants_differing_only_by_a_formatting_character_get_distinct_fields(); + + AssertSql( + """ +SELECT [e].[Id], [e].[Nested] +FROM [Entities] AS [e] +"""); + } + + public override async Task Query_that_fails_to_precompile_between_two_that_succeed_leaves_both_compilable() + { + await base.Query_that_fails_to_precompile_between_two_that_succeed_leaves_both_compilable(); + + AssertSql( + """ +SELECT [s].[Id] +FROM [Seconds] AS [s] +""", + // + """ +SELECT [s].[Id] +FROM [Seconds] AS [s] +ORDER BY [s].[Id] +"""); + } + + public override async Task Liftable_constant_named_like_a_runtime_constant_field() + { + await base.Liftable_constant_named_like_a_runtime_constant_field(); + + AssertSql( + """ +SELECT [e].[Id], [e].[Nested] +FROM [Entities] AS [e] +"""); + } + + public override async Task Runtime_constant_shared_by_two_queries_is_emitted_once() + { + await base.Runtime_constant_shared_by_two_queries_is_emitted_once(); + + AssertSql( + """ +SELECT [e].[Id], [e].[Nested] +FROM [Entities] AS [e] +""", + // + """ +SELECT [e].[Id], [e].[Nested] +FROM [Entities] AS [e] +ORDER BY [e].[Id] +"""); + } + + public override async Task Runtime_constant_named_like_a_type_with_a_leading_underscore() + { + await base.Runtime_constant_named_like_a_type_with_a_leading_underscore(); + + AssertSql( + """ +SELECT [e].[Id] +FROM [Entities] AS [e] +"""); + } + + public override async Task Shaper_variables_named_like_the_executor_identifiers_are_uniquified() + { + await base.Shaper_variables_named_like_the_executor_identifiers_are_uniquified(); + + AssertSql( + """ +SELECT [e].[Id] +FROM [Entities] AS [e] +"""); + } + + public override async Task Runtime_constant_named_like_an_unsafe_accessor() + { + await base.Runtime_constant_named_like_an_unsafe_accessor(); + + AssertSql( + """ +SELECT [e].[Id], [e].[NameBytes], [e].[Nested] +FROM [Entities] AS [e] +"""); + } + protected override PrecompiledQueryTestHelpers PrecompiledQueryTestHelpers => SqlServerPrecompiledQueryTestHelpers.Instance;