From 4eaafe501ae977994e4899d5d020c200c24b7ca0 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 29 Apr 2024 20:30:50 +0200 Subject: [PATCH] Address review comments --- .../Properties/DesignStrings.Designer.cs | 6 ++ .../Properties/DesignStrings.resx | 3 + .../Query/Internal/CSharpToLinqTranslator.cs | 70 ++++++++----------- .../Internal/LinqToCSharpSyntaxTranslator.cs | 10 +-- .../Internal/PrecompiledQueryCodeGenerator.cs | 2 +- src/EFCore/Properties/CoreStrings.Designer.cs | 2 +- src/EFCore/Properties/CoreStrings.resx | 2 +- src/EFCore/Query/ExpressionPrinter.cs | 2 +- 8 files changed, 46 insertions(+), 51 deletions(-) diff --git a/src/EFCore.Design/Properties/DesignStrings.Designer.cs b/src/EFCore.Design/Properties/DesignStrings.Designer.cs index 7522c1e7190..1054ed2b67a 100644 --- a/src/EFCore.Design/Properties/DesignStrings.Designer.cs +++ b/src/EFCore.Design/Properties/DesignStrings.Designer.cs @@ -101,6 +101,12 @@ public static string CannotGenerateTypeQualifiedMethodCall public static string CircularBaseClassDependency => GetString("CircularBaseClassDependency"); + /// + /// A compilation must be loaded. + /// + public static string CompilationMustBeLoaded + => GetString("CompilationMustBeLoaded"); + /// /// The entity type '{entityType}' has a custom constructor binding. Compiled model can't be generated, because custom constructor bindings are not supported. Configure the custom constructor binding in '{customize}' in a partial '{className}' class instead. /// diff --git a/src/EFCore.Design/Properties/DesignStrings.resx b/src/EFCore.Design/Properties/DesignStrings.resx index 9ab2c25b1d2..902e4b9807f 100644 --- a/src/EFCore.Design/Properties/DesignStrings.resx +++ b/src/EFCore.Design/Properties/DesignStrings.resx @@ -150,6 +150,9 @@ You cannot add a migration with the name 'Migration'. + + A compilation must be loaded. + The entity type '{entityType}' has a custom constructor binding. Compiled model can't be generated, because custom constructor bindings are not supported. Configure the custom constructor binding in '{customize}' in a partial '{className}' class instead. diff --git a/src/EFCore.Design/Query/Internal/CSharpToLinqTranslator.cs b/src/EFCore.Design/Query/Internal/CSharpToLinqTranslator.cs index 284d611a950..9c88522fb05 100644 --- a/src/EFCore.Design/Query/Internal/CSharpToLinqTranslator.cs +++ b/src/EFCore.Design/Query/Internal/CSharpToLinqTranslator.cs @@ -91,7 +91,7 @@ public virtual Expression Translate(SyntaxNode node, SemanticModel semanticModel { if (_compilation is null) { - throw new InvalidOperationException("A compilation must be loaded."); + throw new InvalidOperationException(DesignStrings.CompilationMustBeLoaded); } Check.DebugAssert( @@ -109,9 +109,6 @@ public virtual Expression Translate(SyntaxNode node, SemanticModel semanticModel var result = Visit(node); - // TODO: Sanity check: make sure all captured variables in _capturedVariables have non-null values - // (i.e. have been encountered and referenced) - Debug.Assert(_parameterStack.Count == 1); return result; } @@ -250,7 +247,7 @@ public override Expression VisitBinaryExpression(BinaryExpressionSyntax binary) SyntaxKind.LogicalOrExpression => OrElse(left, right), SyntaxKind.LogicalAndExpression => AndAlso(left, right), - // For bitwise operations over enums, we the enum to its underlying type before the bitwise operation, and then back to the + // For bitwise operations over enums, we cast the enum to its underlying type before the bitwise operation, and then back to the // enum afterwards (this is corresponds to the LINQ expression tree that the compiler generates) SyntaxKind.BitwiseOrExpression when left.Type.IsEnum || right.Type.IsEnum => Convert(Or(Convert(left, left.Type.GetEnumUnderlyingType()), Convert(right, right.Type.GetEnumUnderlyingType())), left.Type), @@ -334,10 +331,7 @@ public override Expression VisitElementAccessExpression(ElementAccessExpressionS .Select(t => t.Property) .FirstOrDefault(); - if (property?.GetMethod is null) - { - throw new UnreachableException("No matching property found for ElementAccessExpressionSyntax"); - } + Check.DebugAssert(property?.GetMethod is not null, "No matching property found for ElementAccessExpressionSyntax"); return Call(visitedExpression, property.GetMethod, arguments.Select(a => Visit(a.Expression))); @@ -382,7 +376,7 @@ public override Expression VisitIdentifierName(IdentifierNameSyntax identifierNa case null: throw new InvalidOperationException($"Identifier without symbol: {identifierName}"); default: - throw new NotImplementedException($"IdentifierName of type {symbol.GetType().Name}: {identifierName}"); + throw new UnreachableException($"IdentifierName of type {symbol.GetType().Name}: {identifierName}"); } // TODO: Separate out EF Core-specific logic (EF Core would extend this visitor) @@ -402,7 +396,6 @@ public override Expression VisitIdentifierName(IdentifierNameSyntax identifierNa // The Translate entry point into the translator uses Roslyn's data flow analysis to locate all captured variables, and populates // the _capturedVariable dictionary with them (with null values). - // TODO: Test closure over class member (not local variable) if (symbol is ILocalSymbol localSymbol && _capturedVariables.TryGetValue(localSymbol, out var memberExpression)) { // The first time we see a captured variable, we create MemberExpression for it and cache it in _capturedVariables. @@ -542,31 +535,6 @@ public override Expression VisitInvocationExpression(InvocationExpressionSyntax // containing type (and recursively, its containing types) var typeTypeParameterMap = new Dictionary(GetTypeTypeParameters(methodSymbol.ContainingType)); - IEnumerable> GetTypeTypeParameters(INamedTypeSymbol typeSymbol) - { - // TODO: We match Roslyn type parameters by name, not sure that's right; also for the method's generic type parameters - - if (typeSymbol.ContainingType is INamedTypeSymbol containingTypeSymbol) - { - foreach (var kvp in GetTypeTypeParameters(containingTypeSymbol)) - { - yield return kvp; - } - } - - var type = ResolveType(typeSymbol); - var genericArguments = type.GetGenericArguments(); - - Check.DebugAssert( - genericArguments.Length == typeSymbol.TypeParameters.Length, - "genericArguments.Length == typeSymbol.TypeParameters.Length"); - - foreach (var (typeParamSymbol, typeParamType) in typeSymbol.TypeParameters.Zip(genericArguments)) - { - yield return new KeyValuePair(typeParamSymbol.Name, typeParamType); - } - } - var definitionMethodInfos = declaringType.GetMethods() .Where(m => { @@ -703,8 +671,32 @@ public override Expression VisitInvocationExpression(InvocationExpressionSyntax Check.DebugAssert(destArguments.All(a => a is not null), "arguments.All(a => a is not null)"); - // TODO: Generic type arguments return Call(instance, methodInfo, destArguments!); + + IEnumerable> GetTypeTypeParameters(INamedTypeSymbol typeSymbol) + { + // TODO: We match Roslyn type parameters by name, not sure that's right; also for the method's generic type parameters + + if (typeSymbol.ContainingType is INamedTypeSymbol containingTypeSymbol) + { + foreach (var kvp in GetTypeTypeParameters(containingTypeSymbol)) + { + yield return kvp; + } + } + + var type = ResolveType(typeSymbol); + var genericArguments = type.GetGenericArguments(); + + Check.DebugAssert( + genericArguments.Length == typeSymbol.TypeParameters.Length, + "genericArguments.Length == typeSymbol.TypeParameters.Length"); + + foreach (var (typeParamSymbol, typeParamType) in typeSymbol.TypeParameters.Zip(genericArguments)) + { + yield return new KeyValuePair(typeParamSymbol.Name, typeParamType); + } + } } /// @@ -846,7 +838,7 @@ public override Expression VisitObjectCreationExpression(ObjectCreationExpressio default: // Find the correct Add() method on the collection type // TODO: This doesn't work if there are multiple Add() methods (contrived). Complete solution would be to find the base - // Type for all initializer expressions and find an Add overload of that type (or a superclass thereof) + // TODO: type for all initializer expressions and find an Add overload of that type (or a superclass thereof) var addMethod = type.GetMethods().SingleOrDefault(m => m.Name == "Add" && m.GetParameters().Length == 1); if (addMethod is null) { @@ -1175,8 +1167,6 @@ Type GetClrTypeFromAssembly(IAssemblySymbol? assemblySymbol, string name) // If we can't find the assembly, use the assembly where the user's DbContext type lives; this is primarily to support // testing, where user code is in an assembly that's built as part of the the test and loaded into a specific // AssemblyLoadContext (which gets unloaded later). - - // TODO: Strings assembly = _additionalAssembly ?? throw new InvalidOperationException($"Could not load assembly for IAssemblySymbol '{assemblySymbol.Name}'"); } diff --git a/src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs b/src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs index 4dbdd20f9a2..bf996806d5f 100644 --- a/src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs +++ b/src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs @@ -1579,9 +1579,8 @@ when constantExpression.Type.Attributes.HasFlag(TypeAttributes.NestedPrivate) // Static member { Expression: null } => Generate(member.Member.DeclaringType!), - // If the member isn't declared on the same type as the expression, (e.g. explicit interface implementation), add - // a cast up to the declaring type. - _ when member.Member.DeclaringType is Type declaringType && declaringType != member.Expression.Type + // If the member is declared on an interface, add a cast up to it, to handle explicit interface implementation. + _ when member.Member.DeclaringType is { IsInterface: true } => ParenthesizedExpression( CastExpression(Generate(member.Member.DeclaringType), Translate(member.Expression))), @@ -2055,10 +2054,7 @@ protected override Expression VisitNew(NewExpression node) initializer: null); } - if (node.Type.Namespace is not null) - { - AddNamespace(node.Type); - } + AddNamespace(node.Type); return node; } diff --git a/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs b/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs index c531aa54e0a..8dd4997c91c 100644 --- a/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs +++ b/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs @@ -282,7 +282,7 @@ public PrecompiledQueryCodeGenerator() namespace System.Runtime.CompilerServices { [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)] - sealed class InterceptsLocationAttribute : Attribute + file sealed class InterceptsLocationAttribute : Attribute { public InterceptsLocationAttribute(string filePath, int line, int column) { } } diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index 3ce10a3ac67..317fe454ffa 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -2354,7 +2354,7 @@ public static string PoolingOptionsModified => GetString("PoolingOptionsModified"); /// - /// Precompiled queries aren't supported by your EF provider. + /// Precompiled queries aren't supported by the current provider. /// public static string PrecompiledQueryNotSupported => GetString("PrecompiledQueryNotSupported"); diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 5e3e94fa50f..eb2d6b4c5ac 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -1340,7 +1340,7 @@ 'OnConfiguring' cannot be used to modify DbContextOptions when DbContext pooling is enabled. - Precompiled queries aren't supported by your EF provider. + Precompiled queries aren't supported by the current provider. The derived type '{derivedType}' cannot have the [PrimaryKey] attribute since primary keys may only be declared on the root type. Move the attribute to '{rootType}', or remove '{rootType}' from the model by using [NotMapped] attribute or calling 'EntityTypeBuilder.Ignore' on the base type in 'OnModelCreating'. diff --git a/src/EFCore/Query/ExpressionPrinter.cs b/src/EFCore/Query/ExpressionPrinter.cs index 8f0bc7fc140..d13448a2a90 100644 --- a/src/EFCore/Query/ExpressionPrinter.cs +++ b/src/EFCore/Query/ExpressionPrinter.cs @@ -468,7 +468,7 @@ protected override Expression VisitConstant(ConstantExpression constantExpressio break; case IQueryable queryable: - _stringBuilder.Append(Print(queryable.Expression)); + Visit(queryable.Expression); break; default: