Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional extract method cleanup. #76611

Merged
merged 32 commits into from
Jan 4, 2025
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
5fb756d
Simplify
CyrusNajmabadi Jan 3, 2025
969f3c5
Simplify
CyrusNajmabadi Jan 3, 2025
5802cbd
Simplify
CyrusNajmabadi Jan 3, 2025
40eb46d
Simplify
CyrusNajmabadi Jan 3, 2025
db220c8
Share code
CyrusNajmabadi Jan 3, 2025
be145e5
Simplify
CyrusNajmabadi Jan 3, 2025
f1a16f3
Simplify
CyrusNajmabadi Jan 3, 2025
e3c94b7
Move specific code to subclass
CyrusNajmabadi Jan 3, 2025
281c3eb
Remove unused prop
CyrusNajmabadi Jan 3, 2025
04c78ec
Simplify generics
CyrusNajmabadi Jan 3, 2025
f5cc86a
Simplify annnotations
CyrusNajmabadi Jan 3, 2025
0fabbe3
delete file
CyrusNajmabadi Jan 3, 2025
ea10e13
Avoid allocs
CyrusNajmabadi Jan 3, 2025
78c90d3
Inline method
CyrusNajmabadi Jan 3, 2025
4fc8842
Simplify code
CyrusNajmabadi Jan 3, 2025
2ae4777
Simplify code
CyrusNajmabadi Jan 3, 2025
c5b653e
Simplify code
CyrusNajmabadi Jan 3, 2025
3a2c060
Simplify code
CyrusNajmabadi Jan 3, 2025
571c103
Simplify code
CyrusNajmabadi Jan 3, 2025
b7e0970
Simplify code
CyrusNajmabadi Jan 3, 2025
1a6f76e
Simplify code
CyrusNajmabadi Jan 3, 2025
ef140ca
Switch to a multi dictionary
CyrusNajmabadi Jan 3, 2025
04b07b0
Delete unnecessary type
CyrusNajmabadi Jan 3, 2025
56e9b8e
rename and reorder
CyrusNajmabadi Jan 3, 2025
1c3acff
in progress
CyrusNajmabadi Jan 3, 2025
033dab6
move comptuation
CyrusNajmabadi Jan 3, 2025
d4c5063
be more consistent
CyrusNajmabadi Jan 3, 2025
8c4ea43
be more consistent
CyrusNajmabadi Jan 3, 2025
bba3545
Remove another unnecessary annotation container
CyrusNajmabadi Jan 4, 2025
cf2eaf2
delete
CyrusNajmabadi Jan 4, 2025
48d5703
delete
CyrusNajmabadi Jan 4, 2025
a5fe5d1
Siplify
CyrusNajmabadi Jan 4, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,6 @@ protected override bool TreatOutAsRef
protected override bool IsInPrimaryConstructorBaseType()
=> this.SelectionResult.GetContainingScopeOf<PrimaryConstructorBaseTypeSyntax>() != null;

protected override VariableInfo CreateFromSymbol(
ISymbol symbol, ITypeSymbol type, VariableStyle style, bool variableDeclared)
{
return CreateFromSymbolCommon(symbol, type, style);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


protected override ITypeSymbol? GetRangeVariableType(IRangeVariableSymbol symbol)
{
var info = this.SemanticModel.GetSpeculativeTypeInfo(SelectionResult.FinalSpan.Start, SyntaxFactory.ParseName(symbol.Name), SpeculativeBindingOption.BindAsExpression);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ internal sealed partial class CSharpExtractMethodService
{
internal sealed partial class CSharpMethodExtractor
{
private abstract partial class CSharpCodeGenerator : CodeGenerator<StatementSyntax, SyntaxNode, CSharpCodeGenerationOptions>
private abstract partial class CSharpCodeGenerator : CodeGenerator<SyntaxNode, CSharpCodeGenerationOptions>
{
private readonly SyntaxToken _methodName;

Expand Down Expand Up @@ -159,7 +159,7 @@ private static bool IsExpressionBodiedMember(SyntaxNode node)
=> node is MemberDeclarationSyntax member && member.GetExpressionBody() != null;

private static bool IsExpressionBodiedAccessor(SyntaxNode node)
=> node is AccessorDeclarationSyntax accessor && accessor.ExpressionBody != null;
=> node is AccessorDeclarationSyntax { ExpressionBody: not null };

private SimpleNameSyntax CreateMethodNameForInvocation()
{
Expand Down Expand Up @@ -662,8 +662,7 @@ protected override StatementSyntax CreateDeclarationStatement(

if (variableInfos is [var singleVariable])
{
var type = singleVariable.GetVariableType();
var typeNode = type.GenerateTypeSyntax();
var typeNode = singleVariable.SymbolType.GenerateTypeSyntax();

var originalIdentifierToken = singleVariable.GetOriginalIdentifierToken(cancellationToken);

Expand Down Expand Up @@ -704,15 +703,15 @@ protected override StatementSyntax CreateDeclarationStatement(
{
left = TupleExpression(
[.. variableInfos.Select(v => Argument(v.ReturnBehavior == ReturnBehavior.Initialization
? DeclarationExpression(v.GetVariableType().GenerateTypeSyntax(), SingleVariableDesignation(v.Name.ToIdentifierToken()))
? DeclarationExpression(v.SymbolType.GenerateTypeSyntax(), SingleVariableDesignation(v.Name.ToIdentifierToken()))
: v.Name.ToIdentifierName()))]);
}

return ExpressionStatement(AssignmentExpression(SyntaxKind.SimpleAssignmentExpression, left, initialValue));
}
}

protected override async Task<GeneratedCode> CreateGeneratedCodeAsync(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

protected override async Task<SemanticDocument> PerformFinalTriviaFixupAsync(
SemanticDocument newDocument, CancellationToken cancellationToken)
{
// in hybrid code cases such as extract method, formatter will have some difficulties on where it breaks lines in two.
Expand All @@ -731,7 +730,7 @@ protected override async Task<GeneratedCode> CreateGeneratedCodeAsync(
newDocument = await newDocument.WithSyntaxRootAsync(
root.ReplaceNode(methodDefinition, newMethodDefinition), cancellationToken).ConfigureAwait(false);

return await base.CreateGeneratedCodeAsync(newDocument, cancellationToken).ConfigureAwait(false);
return newDocument;
}

private static MethodDeclarationSyntax TweakNewLinesInMethod(MethodDeclarationSyntax method)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ internal sealed partial class CSharpMethodExtractor(
SelectionResult result, ExtractMethodGenerationOptions options, bool localFunction)
: MethodExtractor(result, options, localFunction)
{
protected override CodeGenerator CreateCodeGenerator(AnalyzerResult analyzerResult)
=> CSharpCodeGenerator.Create(this.OriginalSelectionResult, analyzerResult, this.Options, this.LocalFunction);
protected override CodeGenerator CreateCodeGenerator(SelectionResult selectionResult, AnalyzerResult analyzerResult)
=> CSharpCodeGenerator.Create(selectionResult, analyzerResult, this.Options, this.LocalFunction);

protected override AnalyzerResult Analyze(CancellationToken cancellationToken)
{
Expand Down Expand Up @@ -170,12 +170,6 @@ await semanticDocument.WithSyntaxRootAsync(result.Root, cancellationToken).Confi
result);
}

protected override Task<GeneratedCode> GenerateCodeAsync(InsertionPoint insertionPoint, SelectionResult selectionResult, AnalyzerResult analyzeResult, ExtractMethodGenerationOptions options, CancellationToken cancellationToken)
{
var codeGenerator = CSharpCodeGenerator.Create(selectionResult, analyzeResult, options, this.LocalFunction);
return codeGenerator.GenerateAsync(insertionPoint, cancellationToken);
}

protected override AbstractFormattingRule GetCustomFormattingRule(Document document)
=> FormattingRule.Instance;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public override SyntaxNode GetContainingScope()
return CSharpSyntaxFacts.Instance.GetRootStandaloneExpression(scope);
}

public override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfo(CancellationToken cancellationToken)
protected override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfoWorker(CancellationToken cancellationToken)
{
if (GetContainingScope() is not ExpressionSyntax node)
{
Expand Down Expand Up @@ -130,19 +130,31 @@ private static (ITypeSymbol? typeSymbol, bool returnsByRef) GetRegularExpression
return !info.Type.IsObjectType() ? info.GetTypeWithAnnotatedNullability() : info.GetConvertedTypeWithAnnotatedNullability();
}
}
}

private static bool IsCoClassImplicitConversion(TypeInfo info, Conversion conversion, INamedTypeSymbol? coclassSymbol)
{
if (!conversion.IsImplicit ||
info.ConvertedType == null ||
info.ConvertedType.TypeKind != TypeKind.Interface)
private static bool IsCoClassImplicitConversion(TypeInfo info, Conversion conversion, INamedTypeSymbol? coclassSymbol)
{
return false;
if (!conversion.IsImplicit ||
info.ConvertedType == null ||
info.ConvertedType.TypeKind != TypeKind.Interface)
{
return false;
}

// let's see whether this interface has coclass attribute
return info.ConvertedType.HasAttribute(coclassSymbol);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
var container = this.GetInnermostStatementContainer();

Contract.ThrowIfNull(container);
Contract.ThrowIfFalse(
container.IsStatementContainerNode() ||
container is BaseListSyntax or TypeDeclarationSyntax or ConstructorDeclarationSyntax or CompilationUnitSyntax);

return container;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,33 +32,27 @@ public override bool ContainingScopeHasAsyncKeyword()

return node switch
{
AccessorDeclarationSyntax _ => false,
MethodDeclarationSyntax method => method.Modifiers.Any(SyntaxKind.AsyncKeyword),
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),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lambdas are subclasses of AnonymousFunctionExpressionSyntax .

AnonymousFunctionExpressionSyntax anonymousFunction => anonymousFunction.AsyncKeyword != default,
_ => false,
};
}

public override SyntaxNode GetContainingScope()
{
Contract.ThrowIfNull(SemanticDocument);
Contract.ThrowIfTrue(IsExtractMethodOnExpression);

return GetFirstTokenInSelection().GetRequiredParent().AncestorsAndSelf().First(n =>
n is AccessorDeclarationSyntax or
LocalFunctionStatementSyntax or
BaseMethodDeclarationSyntax or
AccessorDeclarationSyntax or
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate in list.

AnonymousFunctionExpressionSyntax or
CompilationUnitSyntax);
}

public override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfo(CancellationToken cancellationToken)
protected override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfoWorker(CancellationToken cancellationToken)
{
Contract.ThrowIfTrue(IsExtractMethodOnExpression);

var node = GetContainingScope();
var semanticModel = SemanticDocument.SemanticModel;

Expand All @@ -72,7 +66,13 @@ public override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfo(C
_ => throw ExceptionUtilities.UnexpectedValue(node),
};

case MethodDeclarationSyntax methodDeclaration:
case LocalFunctionStatementSyntax localFunction:
{
var method = semanticModel.GetRequiredDeclaredSymbol(localFunction, cancellationToken);
return (method.ReturnType, method.ReturnsByRef);
}

case BaseMethodDeclarationSyntax methodDeclaration:
{
var method = semanticModel.GetRequiredDeclaredSymbol(methodDeclaration, cancellationToken);
return (method.ReturnType, method.ReturnsByRef);
Expand All @@ -89,6 +89,24 @@ public override (ITypeSymbol? returnType, bool returnsByRef) GetReturnTypeInfo(C
return default;
}
}

public override SyntaxNode GetOutermostCallSiteContainerToProcess(CancellationToken cancellationToken)
{
if (this.IsExtractMethodOnSingleStatement)
{
var firstStatement = this.GetFirstStatement();
return firstStatement.GetRequiredParent();
}

if (this.IsExtractMethodOnMultipleStatements)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (this.IsExtractMethodOnMultipleStatements)

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do in followup. tnx!

{
var firstStatement = this.GetFirstStatementUnderContainer();
var container = firstStatement.GetRequiredParent();
return container is GlobalStatementSyntax ? container.GetRequiredParent() : container;
}

throw ExceptionUtilities.Unreachable();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ public static async Task<CSharpSelectionResult> CreateAsync(
: new StatementResult(newDocument, selectionType, finalSpan);
}

protected override ISyntaxFacts SyntaxFacts
=> CSharpSyntaxFacts.Instance;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never used. removed.


protected override OperationStatus ValidateLanguageSpecificRules(CancellationToken cancellationToken)
{
// Nothing language specific for C#.
Expand Down Expand Up @@ -92,39 +89,6 @@ public static bool IsUnderAnonymousOrLocalMethod(SyntaxToken token, SyntaxToken
return false;
}

public override SyntaxNode GetOutermostCallSiteContainerToProcess(CancellationToken cancellationToken)
{
if (this.IsExtractMethodOnExpression)
{
var container = this.GetInnermostStatementContainer();

Contract.ThrowIfNull(container);
Contract.ThrowIfFalse(
container.IsStatementContainerNode() ||
container is BaseListSyntax or TypeDeclarationSyntax or ConstructorDeclarationSyntax or CompilationUnitSyntax);

return container;
}

if (this.IsExtractMethodOnSingleStatement)
{
var firstStatement = this.GetFirstStatement();
return firstStatement.Parent;
}

if (this.IsExtractMethodOnMultipleStatements)
{
var firstStatement = this.GetFirstStatementUnderContainer();
var container = firstStatement.Parent;
if (container is GlobalStatementSyntax)
return container.Parent;

return container;
}

throw ExceptionUtilities.Unreachable();
}

public override StatementSyntax GetFirstStatementUnderContainer()
{
Contract.ThrowIfTrue(IsExtractMethodOnExpression);
Expand Down Expand Up @@ -189,23 +153,11 @@ public SyntaxNode GetInnermostStatementContainer()
return last.Parent.Parent;
}

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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renaming to 'exitPoints' for consistency.


public override ImmutableArray<StatementSyntax> GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray<SyntaxNode> jumpsOutOfRegion)
{
var container = commonRoot.GetAncestorsOrThis<SyntaxNode>().Where(a => a.IsReturnableConstruct()).FirstOrDefault();
if (container == null)
return [];

// now filter return statements to only include the one under outmost container
return jumpsOutOfRegion
.OfType<ReturnStatementSyntax>()
.Select(returnStatement => (returnStatement, container: returnStatement.GetAncestors<SyntaxNode>().Where(a => a.IsReturnableConstruct()).FirstOrDefault()))
.Where(p => p.container == container)
.SelectAsArray(p => p.returnStatement)
.CastArray<StatementSyntax>();
}
public override ImmutableArray<StatementSyntax> GetOuterReturnStatements(SyntaxNode commonRoot, ImmutableArray<SyntaxNode> exitPoints)
=> exitPoints.OfType<ReturnStatementSyntax>().ToImmutableArray().CastArray<StatementSyntax>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public override bool IsFinalSpanSemanticallyValidSpan(
ImmutableArray<StatementSyntax> returnStatements, CancellationToken cancellationToken)
Expand All @@ -226,22 +178,7 @@ public override bool IsFinalSpanSemanticallyValidSpan(
if (body.CloseBraceToken != GetLastTokenInSelection().GetNextToken(includeZeroWidth: true))
return false;

// alright, for these constructs, it must be okay to be extracted
switch (container.Kind())
{
case SyntaxKind.AnonymousMethodExpression:
case SyntaxKind.SimpleLambdaExpression:
case SyntaxKind.ParenthesizedLambdaExpression:
return true;
}

// now, only method is okay to be extracted out
if (body.Parent is not MethodDeclarationSyntax method)
return false;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. we're in a non-expr-returning function-like construct, which is fine.
  2. 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.

}
}
}
45 changes: 0 additions & 45 deletions src/Features/Core/Portable/ExtractMethod/InsertionPoint.cs

This file was deleted.

Loading