Address review comments

src/EFCore.Design/Properties/DesignStrings.resx

@@ -150,6 +150,9 @@
   <data name="CircularBaseClassDependency" xml:space="preserve">
     <value>You cannot add a migration with the name 'Migration'.</value>
   </data>
+  <data name="CompilationMustBeLoaded" xml:space="preserve">
+    <value>A compilation must be loaded.</value>
+  </data>
   <data name="CompiledModelConstructorBinding" xml:space="preserve">
     <value>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.</value>
   </data>

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<string, Type>(GetTypeTypeParameters(methodSymbol.ContainingType));
 
-            IEnumerable<KeyValuePair<string, Type>> 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<string, Type>(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<KeyValuePair<string, Type>> 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<string, Type>(typeParamSymbol.Name, typeParamType);
+            }
+        }
     }
 
     /// <summary>
@@ -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}'");
             }

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<ExpressionSyntax>(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;
     }

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) { }
     }

src/EFCore/Properties/CoreStrings.resx

@@ -1340,7 +1340,7 @@
     <value>'OnConfiguring' cannot be used to modify DbContextOptions when DbContext pooling is enabled.</value>
   </data>
   <data name="PrecompiledQueryNotSupported" xml:space="preserve">
-    <value>Precompiled queries aren't supported by your EF provider.</value>
+    <value>Precompiled queries aren't supported by the current provider.</value>
   </data>
   <data name="PrimaryKeyAttributeOnDerivedEntity" xml:space="preserve">
     <value>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'.</value>

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: