Additional extract method cleanup. by CyrusNajmabadi · Pull Request #76611 · dotnet/roslyn

CyrusNajmabadi · 2025-01-03T17:28:17Z

No description provided.

CyrusNajmabadi · 2025-01-03T17:28:40Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs

-        public override bool ContainsNonReturnExitPointsStatements(ImmutableArray<SyntaxNode> jumpsOutOfRegion)
-            => jumpsOutOfRegion.Any(n => n is not ReturnStatementSyntax);
+        public override bool ContainsNonReturnExitPointsStatements(ImmutableArray<SyntaxNode> exitPoints)
+            => exitPoints.Any(n => n is not ReturnStatementSyntax);


renaming to 'exitPoints' for consistency.

CyrusNajmabadi · 2025-01-03T17:30:53Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs

-                .CastArray<StatementSyntax>();
-        }
+        public override ImmutableArray<StatementSyntax> GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray<SyntaxNode> exitPoints)
+            => exitPoints.OfType<ReturnStatementSyntax>().ToImmutableArray().CastArray<StatementSyntax>();


so the prior logic was just completely unneccessary. it seems like it was written thinking that it would be passed all return-like statements in a span (for example, a return-statement inside an inner lambda), versus only the return statements out of that span. As such, it did all this complex work to filter down to only the actual return-statements returning from the span. Except that's what 'exit points' already gives you (plus things like goto/break/continue). So this can just be written to filter down to ReturnStatements and be done.

CyrusNajmabadi · 2025-01-03T17:35:08Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs

-            // make sure this method doesn't have return type.
-            return method.ReturnType is PredefinedTypeSyntax p &&
-                p.Keyword.Kind() == SyntaxKind.VoidKeyword;
+            return true;


none of hte above logic was relevant. we already ensured that all return-statements were not returning an expression (meaning they're all of the form return;). In which case one of two things are true:

we're in a non-expr-returning function-like construct, which is fine.

we're in a expr-returning function, and hte code is in error, which is also fine, we can proceed just fine, since we go into 'best effort mode' anyways and have already warned in that case.

In other words, we want to warn when the user's code was correct, but we don't know how to extract properly.

CyrusNajmabadi · 2025-01-03T18:45:30Z

@ToddGrun @JoeRobich ptal.

CyrusNajmabadi · 2025-01-03T18:46:25Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.Analyzer.cs

-                ISymbol symbol, ITypeSymbol type, VariableStyle style, bool variableDeclared)
-            {
-                return CreateFromSymbolCommon(symbol, type, style);
-            }


this was a virtual to allow VB to specialize behavior. but that specialization could be pulled up to the caller.

CyrusNajmabadi · 2025-01-03T18:46:47Z

src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs

                }
            }

-            protected override async Task<GeneratedCode> CreateGeneratedCodeAsync(


this type existed to hold onto a document and SyntaxAnnotations. but the syntax annotations could be static-readonlys. so no need for this wrapper.

CyrusNajmabadi · 2025-01-03T18:47:34Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.ExpressionResult.cs


-            // let's see whether this interface has coclass attribute
-            return info.ConvertedType.HasAttribute(coclassSymbol);
+            public override SyntaxNode GetOutermostCallSiteContainerToProcess(CancellationToken cancellationToken)


this wasa method in the base class that specialized itself for the expr vs statement case. but we have thse subclasses for that purpose. so i broke the method up and moved each part to the right subclass.

CyrusNajmabadi · 2025-01-03T18:47:50Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs

-                    ParenthesizedLambdaExpressionSyntax lambda => lambda.AsyncKeyword.Kind() == SyntaxKind.AsyncKeyword,
-                    SimpleLambdaExpressionSyntax lambda => lambda.AsyncKeyword.Kind() == SyntaxKind.AsyncKeyword,
-                    AnonymousMethodExpressionSyntax anonymous => anonymous.AsyncKeyword.Kind() == SyntaxKind.AsyncKeyword,
+                    LocalFunctionStatementSyntax localFunction => localFunction.Modifiers.Any(SyntaxKind.AsyncKeyword),


added this for completeness. figuring out how to add a test for this.

Just making sure that the lambda syntaxes were intentionally changed to always return false.

lambdas are subclasses of AnonymousFunctionExpressionSyntax .

CyrusNajmabadi · 2025-01-03T18:48:00Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.cs

        }

-        protected override ISyntaxFacts SyntaxFacts
-            => CSharpSyntaxFacts.Instance;


never used. removed.

CyrusNajmabadi · 2025-01-03T18:48:21Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs

+                   ISymbol symbol,
+                   ITypeSymbol type,
+                   VariableStyle style)
+                {


moved from protected helper below to private local funciton here.

CyrusNajmabadi · 2025-01-03T18:48:36Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs

-                var visited = new HashSet<ITypeSymbol>();
-                var candidates = SpecializedCollections.EmptyEnumerable<ITypeParameterSymbol>();
+                using var _1 = PooledHashSet<ITypeSymbol>.GetInstance(out var visited);
+                using var _2 = PooledHashSet<ITypeParameterSymbol>.GetInstance(out var candidates);


switched to pooling, sets, and no longer concatting ienumerables.

CyrusNajmabadi · 2025-01-04T01:39:00Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs

                    n is AccessorDeclarationSyntax or
                         LocalFunctionStatementSyntax or
                         BaseMethodDeclarationSyntax or
-                         AccessorDeclarationSyntax or


duplicate in list.

CyrusNajmabadi · 2025-01-04T01:39:50Z

src/Features/Core/Portable/ExtractMethod/InsertionPoint.cs

-internal sealed class InsertionPoint
-{
-    private readonly SyntaxAnnotation _annotation;
-    private readonly Lazy<SyntaxNode?> _context;


unneeded type that existed to just hang onto an object and an annotation. like other places here and in previous prs, the annotation becomes static as there is only one of them ever.

CyrusNajmabadi · 2025-01-04T01:40:38Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.AnalyzerResult.cs

            public ImmutableArray<ITypeParameterSymbol> MethodTypeParametersInDeclaration { get; } = typeParametersInDeclaration;
            public ImmutableArray<ITypeParameterSymbol> MethodTypeParametersInConstraintList { get; } = typeParametersInConstraintList;
-            public ImmutableArray<VariableInfo> VariablesToUseAsReturnValue { get; } = variablesToUseAsReturnValue;
+            public ImmutableArray<VariableInfo> VariablesToUseAsReturnValue { get; } = variables.WhereAsArray(v => v.UseAsReturnValue);


instead of making the callers do this, it is done once at this final position.

ToddGrun · 2025-01-04T02:28:45Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs

+                MultiDictionary<ISymbol, SyntaxToken> symbolMap, ISymbol symbol, bool writtenInside)
            {
-                if (!symbolMap.TryGetValue(symbol, out var tokens))
+                var tokens = symbolMap[symbol];


var tokens = symbolMap[symbol];

Guess I haven't used MultiDictionary enough, threw me for a quick loop that the indexer doesn't throw if the key isn't present.

fair. but yeah, that is the semantics. it maps 'not there' to 'empty'

ToddGrun · 2025-01-04T02:36:11Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.CodeGenerator.cs

                SyntaxNode InsertNormalMethod()
                {
-                    var syntaxKinds = document.GetLanguageService<ISyntaxKindsService>();
+                    var syntaxKinds = document.GetRequiredLanguageService<ISyntaxKindsService>();


GetRequiredLanguageService

I would have expected this to have been done in response to removing the nullability pragma at the beginning of the file.

yeah, i think i started removing nullability, then added back as it was too large.

ToddGrun · 2025-01-04T02:37:43Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs

        ExtractMethodGenerationOptions options,
        bool localFunction)
    {
+        public static readonly SyntaxAnnotation InsertionPointAnnotation = new();


InsertionPointAnnotation

Might be worth making a PR to remove the nullable pragmas if this code is all getting cleaned up anyways.

sure. can do in followup.

ToddGrun · 2025-01-04T02:39:12Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.cs

        }

-        private static async Task<(SemanticDocument analyzedDocument, InsertionPoint insertionPoint)> GetAnnotatedDocumentAndInsertionPointAsync(
+        private static async Task<SemanticDocument> GetAnnotatedDocumentAndInsertionPointAsync(


GetAnnotatedDocumentAndInsertionPointAsync

Maybe rename the method?

sure. can do in followup.

ToddGrun · 2025-01-04T02:47:50Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs

+            protected abstract int CompareTo(TVariableSymbol right);

-            public int CompareTo(ParameterVariableSymbol other)
+            protected static int DefaultCompareTo(ISymbol left, ISymbol right)


DefaultCompareTo

Why not have CompareTo be virtual and have this implementation (with ParameterVariableSymbol.CompareTo calling into the base instead of DefaultCompareTo)?

i prefered forcing the subclasses to have to override, and decide what behavior they want :)

ToddGrun · 2025-01-04T02:52:49Z

src/Features/Core/Portable/ExtractMethod/MethodExtractor.VariableSymbol.cs

-                {
-                    return 0;
-                }
+                var locationLeft = left.Locations.First();


.First()

nit: I like [0] better, makes it obvious that it's not needing to start an enumeration as the type of Locations isn't visible.

ToddGrun · 2025-01-04T02:56:50Z

...rvices/SymbolDisplayService/AbstractSymbolDisplayService.AbstractSymbolDescriptionBuilder.cs

+            var typeParameters = symbol.GetAllTypeParameters();

-            for (var i = 0; i < typeArguments.Count; i++)
+            for (var i = 0; i < typeArguments.Length; i++)


for (var i = 0; i < typeArguments.Length; i++)

nit: preference for me is to have the return false inside the condition. Always looks odd to me when a loop has the initial appearance of not looping more than once.

ToddGrun · 2025-01-04T03:10:46Z

src/Features/CSharp/Portable/ExtractMethod/CSharpSelectionResult.StatementResult.cs

+                    return firstStatement.GetRequiredParent();
+                }
+
+                if (this.IsExtractMethodOnMultipleStatements)


if (this.IsExtractMethodOnMultipleStatements)

personal preference: Contract.ThrowIfFalse and remove the if and Unreachable exception

can do in followup. tnx!

ToddGrun

CyrusNajmabadi added 4 commits January 3, 2025 09:01

Simplify

5fb756d

Simplify

969f3c5

Simplify

5802cbd

Simplify

40eb46d

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 3, 2025

dotnet-policy-service bot added the VSCode label Jan 3, 2025

CyrusNajmabadi commented Jan 3, 2025

View reviewed changes

CyrusNajmabadi added 2 commits January 3, 2025 09:48

Share code

db220c8

Simplify

be145e5

CyrusNajmabadi requested review from JoeRobich and ToddGrun January 3, 2025 18:12

CyrusNajmabadi marked this pull request as ready for review January 3, 2025 18:12

CyrusNajmabadi requested a review from a team as a code owner January 3, 2025 18:12

CyrusNajmabadi added 5 commits January 3, 2025 10:22

Simplify

f1a16f3

Move specific code to subclass

e3c94b7

Remove unused prop

281c3eb

Simplify generics

04c78ec

Simplify annnotations

f5cc86a

delete file

0fabbe3

CyrusNajmabadi commented Jan 3, 2025

View reviewed changes

CyrusNajmabadi added 5 commits January 3, 2025 12:33

in progress

1c3acff

move comptuation

033dab6

be more consistent

d4c5063

be more consistent

8c4ea43

Remove another unnecessary annotation container

bba3545

CyrusNajmabadi commented Jan 4, 2025

View reviewed changes

CyrusNajmabadi added 3 commits January 3, 2025 17:41

delete

cf2eaf2

delete

48d5703

Siplify

a5fe5d1

ToddGrun reviewed Jan 4, 2025

View reviewed changes

ToddGrun approved these changes Jan 4, 2025

View reviewed changes

CyrusNajmabadi merged commit 77df282 into dotnet:main Jan 4, 2025
25 checks passed

CyrusNajmabadi deleted the extractMethodCleanup6 branch January 4, 2025 03:26

dotnet-policy-service bot added this to the Next milestone Jan 4, 2025

canton7 mentioned this pull request Jan 6, 2025

Autocompletion menu functionality is broken, text is deleted instead of inserted ashmind/SharpLab#1316

Open

dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025

Conversation

CyrusNajmabadi commented Jan 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jan 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddGrun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants