Apply code review suggestions

Author: fabien.menager

src/EntityFrameworkCore.Projectables.CodeFixes/FactoryMethodTransformationHelper.cs

@@ -139,6 +139,13 @@ async static internal Task<Solution> ConvertToConstructorAndUpdateCallersAsync(
                 else
                 {
                     // Method group: Class.Method → p1 => new ReturnType(p1)
+                    // Skip nameof(Class.Method): SymbolFinder returns these locations but
+                    // replacing them with a lambda would produce invalid C#.
+                    if (IsInsideNameOf(methodRefExpr))
+                    {
+                        continue;
+                    }
+
                     if (annotationByMethodGroup.ContainsKey(methodRefExpr))
                     {
                         continue;
@@ -305,6 +312,13 @@ async static internal Task<Solution> ConvertToConstructorAndUpdateCallersAsync(
                 else
                 {
                     // Method group: Class.Method → p => new ReturnType(p)
+                    // Skip nameof(Class.Method): SymbolFinder returns these locations but
+                    // replacing them with a lambda would produce invalid C#.
+                    if (IsInsideNameOf(methodRefExpr))
+                    {
+                        continue;
+                    }
+
                     var lambda = BuildMethodGroupLambda(methodParamNames, returnTypeSyntax)
                         .WithLeadingTrivia(methodRefExpr.GetLeadingTrivia())
                         .WithTrailingTrivia(methodRefExpr.GetTrailingTrivia());
@@ -329,7 +343,7 @@ private static SyntaxNode BuildRootWithConstructor(
         MethodDeclarationSyntax method,
         TypeDeclarationSyntax containingType)
     {
-        var creation = (ObjectCreationExpressionSyntax)method.ExpressionBody!.Expression;
+        var creation = (BaseObjectCreationExpressionSyntax)method.ExpressionBody!.Expression;
         var initializer = creation.Initializer!;
 
         // Only support simple object-initializer assignments (Prop = value). If there are
@@ -341,10 +355,32 @@ private static SyntaxNode BuildRootWithConstructor(
         }
 
         // Convert each object-initializer assignment (Prop = value) to a statement (Prop = value;).
-        var statements = initializer.Expressions
-            .OfType<AssignmentExpressionSyntax>()
-            .Select(a => (StatementSyntax)SyntaxFactory.ExpressionStatement(a))
-            .ToArray();
+        // Two trivia sources must be preserved:
+        //   1. The expression's own trailing trivia (e.g. an inline "// comment") must move onto
+        //      the semicolon token so it stays on the same line as the statement terminator.
+        //   2. The separator comma's trailing trivia (e.g. "\r\n    ") carries the newline and
+        //      indentation that separates adjacent items in the initializer list.  That trivia
+        //      is lost when we extract only the expressions, so we append it to the semicolon as
+        //      well so the next statement starts on its own correctly-indented line.
+        static StatementSyntax ToStatement(AssignmentExpressionSyntax a, SyntaxTriviaList separatorTrailing)
+        {
+            var exprTrailing = a.GetTrailingTrivia();
+            var semicolonTrailing = exprTrailing.AddRange(separatorTrailing);
+            return SyntaxFactory.ExpressionStatement(
+                a.WithoutTrailingTrivia(),
+                SyntaxFactory.Token(SyntaxKind.SemicolonToken).WithTrailingTrivia(semicolonTrailing));
+        }
+
+        var exprs = initializer.Expressions;
+        var statements = new StatementSyntax[exprs.Count];
+        for (var i = 0; i < exprs.Count; i++)
+        {
+            var a = (AssignmentExpressionSyntax)exprs[i];
+            var sepTrivia = i < exprs.SeparatorCount
+                ? exprs.GetSeparator(i).TrailingTrivia
+                : default;
+            statements[i] = ToStatement(a, sepTrivia);
+        }
 
         var ctorModifiers = GetConstructorModifiers(method);
 
@@ -361,17 +397,24 @@ private static SyntaxNode BuildRootWithConstructor(
         var newMembers = containingType.Members.RemoveAt(methodIndex);
         newMembers = newMembers.Insert(Math.Min(methodIndex, newMembers.Count), ctor);
 
-        // Add an explicit parameterless constructor if the class has none, to avoid breaking
-        // code that relied on the implicit default constructor.
-        var hasParamlessCtor = containingType.Members
-            .OfType<ConstructorDeclarationSyntax>()
-            .Any(c => c.ParameterList.Parameters.Count == 0);
+        // Add an explicit parameterless constructor only when the class originally had NO
+        // explicit constructors at all.  The C# compiler emits an implicit parameterless
+        // constructor solely in that case (C# spec §10.11.4), and it is always declared public.
+        // If the class already had other user-declared constructors the implicit default was
+        // already suppressed, so we must not introduce a new public overload.
+        var existingCtors = containingType.Members.OfType<ConstructorDeclarationSyntax>().ToArray();
+        var hasParamlessCtor = existingCtors.Any(c => c.ParameterList.Parameters.Count == 0);
+        var hadNoExplicitCtors = existingCtors.Length == 0;
 
-        if (!hasParamlessCtor)
+        if (!hasParamlessCtor && hadNoExplicitCtors)
         {
+            // The implicit default ctor is always public (C# spec §10.11.4) regardless of the
+            // factory method's own accessibility, so hard-code public here.
             var paramlessCtor = SyntaxFactory
                 .ConstructorDeclaration(containingType.Identifier.WithoutTrivia())
-                .WithModifiers(ctorModifiers)
+                .WithModifiers(SyntaxFactory.TokenList(
+                    SyntaxFactory.Token(SyntaxKind.PublicKeyword)
+                        .WithTrailingTrivia(SyntaxFactory.Space)))
                 .WithParameterList(SyntaxFactory.ParameterList())
                 .WithBody(SyntaxFactory.Block())
                 .WithAdditionalAnnotations(Formatter.Annotation)
@@ -397,6 +440,29 @@ private static SyntaxTokenList GetConstructorModifiers(MethodDeclarationSyntax m
         return SyntaxFactory.TokenList(filteredModifiers);
     }
 
+    /// <summary>
+    /// Returns <see langword="true"/> when <paramref name="expr"/> is the argument of a
+    /// <c>nameof(…)</c> expression.
+    /// <para>
+    /// Roslyn parses <c>nameof(X.Y)</c> as an <see cref="InvocationExpressionSyntax"/> whose
+    /// callee is an <see cref="IdentifierNameSyntax"/> with the text <c>nameof</c>.
+    /// <see cref="Microsoft.CodeAnalysis.FindSymbols.SymbolFinder"/> still returns such
+    /// locations, but replacing them with a lambda or object-creation expression would produce
+    /// invalid C# — they must be skipped.
+    /// </para>
+    /// </summary>
+    private static bool IsInsideNameOf(ExpressionSyntax expr) =>
+        expr.Parent is ArgumentSyntax
+        {
+            Parent: ArgumentListSyntax
+            {
+                Parent: InvocationExpressionSyntax
+                {
+                    Expression: IdentifierNameSyntax { Identifier.Text: "nameof" }
+                }
+            }
+        };
+
     /// <summary>
     /// Builds a lambda expression that wraps a constructor call, for use when a factory
     /// method is referenced as a method group (e.g. <c>Select(MyType.Create)</c>).

src/EntityFrameworkCore.Projectables.CodeFixes/SyntaxHelpers.cs

@@ -1,4 +1,6 @@
-using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
 
 namespace EntityFrameworkCore.Projectables.CodeFixes;
 
@@ -8,11 +10,16 @@ static internal class SyntaxHelpers
     /// Returns <see langword="true"/> when <paramref name="method"/> matches the
     /// factory-method pattern:
     /// <list type="bullet">
-    ///   <item><description>Decorated with <c>[Projectable]</c>.</description></item>
     ///   <item><description>Expression body of the form <c>=> new ContainingType { … }</c>
     ///       (object initializer only, no constructor arguments in the <c>new</c>
     ///       expression).</description></item>
+    ///   <item><description>Static method.</description></item>
     ///   <item><description>Return type simple name equals the containing class name.</description></item>
+    ///   <item><description>For explicit <c>new T { … }</c>, <c>T</c> must be an unqualified
+    ///       identifier that matches the containing class name.  Qualified names such as
+    ///       <c>new Other.MyObj { … }</c> or <c>new global::Other.MyObj { … }</c> are
+    ///       rejected because they cannot be confirmed as the same type without a semantic
+    ///       model.</description></item>
     /// </list>
     /// </summary>
     static internal bool TryGetFactoryMethodPattern(
@@ -47,6 +54,33 @@ static internal bool TryGetFactoryMethodPattern(
             return false;
         }
 
+        // Only allow static factory methods, to keep the code fix simpler
+        if (!method.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)))
+        {
+            return false;
+        }
+
+        // For explicit new T { … }, verify the type is an unqualified name matching the
+        // containing class.  Qualified names (new Other.MyObj { }, new global::Other.MyObj { })
+        // are rejected: without a semantic model we cannot confirm they resolve to the same
+        // type, so this is the conservative safe choice.
+        // For implicit new() { }, the compiler infers the type from the method's return type,
+        // which is already validated below, so no additional check is needed here.
+        if (creation is ObjectCreationExpressionSyntax { Type: var createdType })
+        {
+            var createdTypeName = createdType switch
+            {
+                IdentifierNameSyntax id => id.Identifier.Text,
+                GenericNameSyntax generic => generic.Identifier.Text,
+                _ => null  // QualifiedNameSyntax, AliasQualifiedNameSyntax, etc. — reject
+            };
+
+            if (createdTypeName is null || createdTypeName != parentType.Identifier.Text)
+            {
+                return false;
+            }
+        }
+
         // The method's return type must match the containing type (syntax-level name comparison).
         var returnTypeName = method.ReturnType switch
         {

tests/EntityFrameworkCore.Projectables.CodeFixes.Tests/FactoryMethodToCtorCodeFixProviderTests.CodeFix_DoesNotAddParameterlessConstructor_WhenOtherExplicitCtorExists.verified.txt

@@ -0,0 +1,13 @@
+
+namespace Foo {
+    class Input { public int Value { get; set; } }
+    class Output {
+        public int Value { get; set; }
+        public Output(string name) { }
+        [Projectable]
+        public Output(Input i)
+        {
+            Value = i.Value;
+        }
+    }
+}

tests/EntityFrameworkCore.Projectables.CodeFixes.Tests/FactoryMethodToCtorCodeFixProviderTests.CodeFix_ImplicitObjectCreation.verified.txt

@@ -0,0 +1,16 @@
+
+namespace Foo {
+    class OtherObj { public string Prop1 { get; set; } }
+    class MyObj {
+        public MyObj()
+        {
+        }
+
+        public string Prop1 { get; set; }
+        [Projectable]
+        public MyObj(OtherObj obj)
+        {
+            Prop1 = obj.Prop1;
+        }
+    }
+}

tests/EntityFrameworkCore.Projectables.CodeFixes.Tests/FactoryMethodToCtorCodeFixProviderTests.CodeFix_InsertedParameterlessCtorIsAlwaysPublic.verified.txt

@@ -0,0 +1,15 @@
+
+namespace Foo {
+    class Input { }
+    class Output {
+        public Output()
+        {
+        }
+
+        public int Value { get; set; }
+        [Projectable]
+        internal Output(Input i)
+        {
+        }
+    }
+}

tests/EntityFrameworkCore.Projectables.CodeFixes.Tests/FactoryMethodToCtorCodeFixProviderTests.cs

@@ -96,6 +96,77 @@ public Output() { }
                 _provider,
                 actionIndex: 0));
 
+    /// <summary>
+    /// When the class already has at least one explicit constructor (but no parameterless one),
+    /// the C# compiler did NOT generate an implicit default constructor — so the transformation
+    /// must NOT insert one, which would unintentionally widen the public surface area.
+    /// </summary>
+    [Fact]
+    public Task CodeFix_DoesNotAddParameterlessConstructor_WhenOtherExplicitCtorExists() =>
+        Verifier.Verify(
+            ApplyCodeFixAsync(
+                @"
+namespace Foo {
+    class Input { public int Value { get; set; } }
+    class Output {
+        public int Value { get; set; }
+        public Output(string name) { }
+        [Projectable]
+        public static Output Create(Input i) => new Output { Value = i.Value };
+    }
+}",
+                "EFP0012",
+                FirstMethodIdentifierSpan,
+                _provider,
+                actionIndex: 0));
+
+    /// <summary>
+    /// The implicit default constructor is always public (C# spec §10.11.4) regardless of the
+    /// factory method's accessibility.  The inserted explicit parameterless ctor must therefore
+    /// be public too, even when the factory method is internal or protected.
+    /// </summary>
+    [Fact]
+    public Task CodeFix_InsertedParameterlessCtorIsAlwaysPublic() =>
+        Verifier.Verify(
+            ApplyCodeFixAsync(
+                @"
+namespace Foo {
+    class Input { }
+    class Output {
+        public int Value { get; set; }
+        [Projectable]
+        internal static Output Create(Input i) => new Output { };
+    }
+}",
+                "EFP0012",
+                FirstMethodIdentifierSpan,
+                _provider,
+                actionIndex: 0));
+
+    /// <summary>
+    /// Regression test: implicit object creation (<c>new() { … }</c>) must not throw
+    /// an <see cref="InvalidCastException"/> — <see cref="FactoryMethodTransformationHelper"/>
+    /// must treat it as <see cref="Microsoft.CodeAnalysis.CSharp.Syntax.BaseObjectCreationExpressionSyntax"/>
+    /// rather than casting to the explicit <c>ObjectCreationExpressionSyntax</c>.
+    /// </summary>
+    [Fact]
+    public Task CodeFix_ImplicitObjectCreation() =>
+        Verifier.Verify(
+            ApplyCodeFixAsync(
+                @"
+namespace Foo {
+    class OtherObj { public string Prop1 { get; set; } }
+    class MyObj {
+        public string Prop1 { get; set; }
+        [Projectable]
+        public static MyObj Create(OtherObj obj) => new() { Prop1 = obj.Prop1 };
+    }
+}",
+                "EFP0012",
+                FirstMethodIdentifierSpan,
+                _provider,
+                actionIndex: 0));
+
     [Fact]
     public async Task TwoCodeFixActionsAreOffered()
     {

tests/EntityFrameworkCore.Projectables.CodeFixes.Tests/FactoryMethodToCtorCodeRefProviderTests.ConvertToConstructor_DoesNotAddParameterlessConstructor_WhenOtherExplicitCtorExists.verified.txt

@@ -0,0 +1,13 @@
+
+namespace Foo {
+    class Input { public int Value { get; set; } }
+    class Output {
+        public int Value { get; set; }
+        public Output(string name) { }
+        [Projectable]
+        public Output(Input i)
+        {
+            Value = i.Value;
+        }
+    }
+}

tests/EntityFrameworkCore.Projectables.CodeFixes.Tests/FactoryMethodToCtorCodeRefProviderTests.ConvertToConstructor_ImplicitObjectCreation.verified.txt

@@ -0,0 +1,16 @@
+
+namespace Foo {
+    class OtherObj { public string Prop1 { get; set; } }
+    class MyObj {
+        public MyObj()
+        {
+        }
+
+        public string Prop1 { get; set; }
+        [Projectable]
+        public MyObj(OtherObj obj)
+        {
+            Prop1 = obj.Prop1;
+        }
+    }
+}

tests/EntityFrameworkCore.Projectables.CodeFixes.Tests/FactoryMethodToCtorCodeRefProviderTests.ConvertToConstructor_InsertedParameterlessCtorIsAlwaysPublic.verified.txt

@@ -0,0 +1,15 @@
+
+namespace Foo {
+    class Input { }
+    class Output {
+        public Output()
+        {
+        }
+
+        public int Value { get; set; }
+        [Projectable]
+        internal Output(Input i)
+        {
+        }
+    }
+}

tests/EntityFrameworkCore.Projectables.CodeFixes.Tests/FactoryMethodToCtorCodeRefProviderTests.ConvertToConstructor_MultipleInitializerAssignments.verified.txt

@@ -11,8 +11,7 @@ namespace Foo {
         [Projectable]
         public Dest(Src src)
         {
-            A = src.A;
-            B = src.B;
+            A = src.A; B = src.B;
         }
     }
 }