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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add code action to extract local functions #39745

Open
wants to merge 18 commits into
base: master
from

Conversation

@allisonchou
Copy link
Contributor

allisonchou commented Nov 9, 2019

This PR allows users to extract local functions from methods in C#. It works similarly to the existing extract method code action and much of the code builds off the existing implementation.

Fixes #32135.

Example

  1. Original code
    extractlocal0
  2. Highlighted code
    extractlocal0 5
  3. Ctrl+. or right-click selection to open up code action menu
    extractlocal1
  4. Applied code action
    extractlocal2

The code action respects the user's IDE0062 preference, csharp_prefer_static_local_function. However, even if a user prefers static local functions, a static modifier will only be generated if it is possible in the current context.

If extracting code from an existing local function, a new local function will be generated within the existing local function.

Special thanks to @mavasani and @ryzngard for their help with this PR. 馃槃

@allisonchou allisonchou added the Area-IDE label Nov 9, 2019
@allisonchou allisonchou added this to the 16.5.P1 milestone Nov 9, 2019
@allisonchou allisonchou requested review from ryzngard, dotnet/roslyn-ide and dpoeschl Nov 9, 2019
@kendrahavens

This comment has been minimized.

Copy link
Contributor

kendrahavens commented Nov 11, 2019

Looks good!

  • Since this refactoring won't appear very often as it is only when text is selected I think appending it below the 'Extract method' refactoring is good.
  • It was brought up that we could use a fly-out menu that says 'Extract' if we are concerned about the (Ctrl +.) menu having too many options. I think we can ignore this in initial implementation because:
    • It is less likely the menu will have many options since a range is selected
    • Adding an 'extract' fly out menu only reduces the list by 1 option
    • The user is still required to do 1 extra click with the flyout
@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented Nov 12, 2019

@allisonchou would you recommend that reviewers go through one commit at a time? or just look at the final code?

Copy link
Member

dpoeschl left a comment

Some initial comments today, but by no means complete feedback. Will update soon.

@@ -190,7 +280,8 @@ internal static class MethodGenerator
tokens.Add(SyntaxFactory.Token(SyntaxKind.SealedKeyword));
}

if (method.IsStatic)
var preferStaticFunction = workspace.Options.GetOption(CSharpCodeStyleOptions.PreferStaticLocalFunction).Value;
if (method.IsStatic && preferStaticFunction)

This comment has been minimized.

Copy link
@dpoeschl

dpoeschl Nov 12, 2019

Member

If the code being extracted calls an existing non-static local function, then the generated local function also cannot be static (even if there are no this captures etc.), but I don't think that's being accounted for when the symbol is being generated in the local function case.

@@ -175,6 +175,31 @@ protected static T Cast<T>(object value)
}
}

protected static void CheckDeclarationNode<TDeclarationNode1, TDeclarationNode2, TDeclarationNode3, TDeclarationNode4, TDeclarationNode5>(SyntaxNode destination)

This comment has been minimized.

Copy link
@dpoeschl

dpoeschl Nov 12, 2019

Member

There's a bug in the previous CheckDeclarationNode (with 4 type arguments) where it's using the wrong resource string.

@@ -598,6 +598,9 @@
<data name="Unknown" xml:space="preserve">
<value>Unknown</value>
</data>
<data name="Extract_Local_Method" xml:space="preserve">
<value>Extract Local Method</value>

This comment has been minimized.

Copy link
@dpoeschl

dpoeschl Nov 12, 2019

Member

All the docs I can find call these "Local Functions." Should we try to be consistent with the docs? Or do you have other examples of this verbiage?

This comment has been minimized.

Copy link
@ryzngard

ryzngard Nov 12, 2019

Contributor

We should use function here. Method usually is associated with an object

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

"Extract local function".

if (localFunctionNames == null)
{
return NameGenerator.GenerateUniqueName(baseName, string.Empty,
n => _semanticModel.LookupSymbols(contextNode.SpanStart, /*container*/null, n).Length == 0);

This comment has been minimized.

Copy link
@dpoeschl

dpoeschl Nov 12, 2019

Member

named argument instead of comment?

@@ -32,6 +32,13 @@ protected override async Task<InsertionPoint> GetInsertionPointAsync(SemanticDoc
var root = await document.Document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var basePosition = root.FindToken(position);

// Check if we are extracting a local function and are within a local function

This comment has been minimized.

Copy link
@dpoeschl

dpoeschl Nov 12, 2019

Member

Unrelated but I don't see where else to put it... When we Extract Method on some code that calls an existing local function, we always provide a warning to the user (because it could clearly break when moving the code to a new method entirely which cannot access the local function). It seems like we can do better and omit the warning in many cases when extracting to a local function.

var leadingTrivia = result.MethodDeclarationNode.GetLeadingTrivia();
if (!leadingTrivia.Any(t => t.IsKind(SyntaxKind.EndOfLineTrivia)))
{
resultMethodDeclarationNode = result.MethodDeclarationNode.WithPrependedLeadingTrivia(SpecializedCollections.SingletonEnumerable(SyntaxFactory.CarriageReturnLineFeed));

This comment has been minimized.

Copy link
@dpoeschl

dpoeschl Nov 12, 2019

Member

Not sure if we're supposed to figure out the right endline characters here.

Copy link
Contributor

ryzngard left a comment

Mostly looks good, some feedback and need to add tests for the change to checking static preference

}
}

protected ImmutableHashSet<string> GetLocalFunctionNamesIfScopeIsMethod(SyntaxNode scope)

This comment has been minimized.

Copy link
@ryzngard

ryzngard Nov 12, 2019

Contributor

Do we need to provide these names if we're not generating a local function?

var leadingTrivia = result.MethodDeclarationNode.GetLeadingTrivia();
if (!leadingTrivia.Any(t => t.IsKind(SyntaxKind.EndOfLineTrivia)))
{
resultMethodDeclarationNode = result.MethodDeclarationNode.WithPrependedLeadingTrivia(SpecializedCollections.SingletonEnumerable(SyntaxFactory.CarriageReturnLineFeed));

This comment has been minimized.

Copy link
@ryzngard

ryzngard Nov 12, 2019

Contributor

Did you try always prepending an elastic carriage return and letting the formatter determine if it belongs?

@@ -74,26 +74,6 @@ public override async Task<SelectionResult> GetValidSelectionAsync(CancellationT
}
}

// Warn if local functions are in selection since data flow analysis
// cannot correctly analyze them
// https://github.com/dotnet/roslyn/issues/14214

This comment has been minimized.

Copy link
@ryzngard

ryzngard Nov 12, 2019

Contributor

nice!

@@ -111,7 +111,12 @@ protected override TDeclarationNode AddField<TDeclarationNode>(TDeclarationNode

protected override TDeclarationNode AddMethod<TDeclarationNode>(TDeclarationNode destination, IMethodSymbol method, CodeGenerationOptions options, IList<bool> availableIndices)
{
CheckDeclarationNode<TypeDeclarationSyntax, CompilationUnitSyntax, NamespaceDeclarationSyntax>(destination);
if (destination is PropertyDeclarationSyntax || destination is IndexerDeclarationSyntax)

This comment has been minimized.

Copy link
@ryzngard

ryzngard Nov 12, 2019

Contributor

Why are these checked before the call to CheckDeclarationNode below?

@@ -190,7 +280,8 @@ internal static class MethodGenerator
tokens.Add(SyntaxFactory.Token(SyntaxKind.SealedKeyword));
}

if (method.IsStatic)
var preferStaticFunction = workspace.Options.GetOption(CSharpCodeStyleOptions.PreferStaticLocalFunction).Value;

This comment has been minimized.

Copy link
@ryzngard

ryzngard Nov 12, 2019

Contributor

Likely need to add tests to the old extract method for this change where the PreferStaticLocalFunction is set to false

using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.ExtractLocalMethod

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

as painful as it will be, these should all be updated to be 'ExtractLocalFunction'. 'local function' is what the langauge (and our docs) call these uys.

// We insert an empty line between the generated local method and the previous statements if there is not one already.
var (resultDocument, resultMethodDeclarationNode, resultInvocationNameToken) = await InsertNewLineBeforeLocalMethodIfNecessaryAsync(result, cancellationToken).ConfigureAwait(false);

var description = FeaturesResources.Extract_Local_Method;

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

inline this.

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

should be Extract_local_function

if (result.Succeeded || result.SucceededWithSuggestion)
{
// We insert an empty line between the generated local method and the previous statements if there is not one already.
var (resultDocument, resultMethodDeclarationNode, resultInvocationNameToken) = await InsertNewLineBeforeLocalMethodIfNecessaryAsync(result, cancellationToken).ConfigureAwait(false);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

not sure why this is done here. feels like it should be done in the callback that is passed to MyCodeAction.

var codeAction = new MyCodeAction(description, c => AddRenameAnnotationAsync(resultDocument, resultInvocationNameToken, c));
var methodBlock = resultMethodDeclarationNode;

return (codeAction, methodBlock.ToString());

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

why are we ToString'in a node and passing it back to the caller?

@@ -609,12 +611,23 @@ protected override async Task<GeneratedCode> CreateGeneratedCodeAsync(OperationS
// here, we explicitly insert newline at the end of "{" of auto generated method decl so that anchor knows how to find out
// indentation of inserted statements (from users code) with user code style preserved
var root = newDocument.Root;
var methodDefinition = root.GetAnnotatedNodes<MethodDeclarationSyntax>(this.MethodDefinitionAnnotation).First();

var newMethodDefinition = TweakNewLinesInMethod(methodDefinition);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

feels like you can keep the old code, but just update TweakNewLinesInMethod

methodDefinition.ExpressionBody.ArrowToken,
methodDefinition.ExpressionBody.ArrowToken.WithPrependedLeadingTrivia(
SpecializedCollections.SingletonEnumerable(SyntaxFactory.ElasticCarriageReturnLineFeed)));
}

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

really uncertain why this method is necessary...

var returnOperations = methodOperation.DescendantsAndSelf().OfType<IReturnOperation>();

foreach (var returnOperation in returnOperations)
if (syntaxNode is MethodDeclarationSyntax methodDeclaration)

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

it's really unfortunate to have all this special casing for MethodDeclaration and LocalFunctionDeclaration. A better way to do this would be to make this class abstract and generic (parameterized on something like TMethodDeclaration). Then have the core algorithms work uniformly on this, and extract MethodDecl/LocalFunc specific behavior to the subclasses.

}
else
{
throw new NotSupportedException("SyntaxNode expected to be a MethodDeclarationSyntax or LocalFunctionStatementSyntax.");

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

this would not be necessary anymre.

{
return (node, location, annotation) => AnnotationResolver(node, location, annotation, callsite, methodDefinition.Body, methodDefinition.ExpressionBody, methodDefinition.SemicolonToken);
}
else if (method is LocalFunctionStatementSyntax localMethodDefinition)

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

more unfortunate duplciation.

I'm pausing on the review for now. IMO it needs to be restructured to keep all the logic uniform, and have only the subclasses have to do anything special about MethodDecls versus LocalFuncs.

Feel free to come to gitten if you'd like to discuss this at all. :)

@@ -10,6 +10,6 @@ namespace Microsoft.CodeAnalysis.ExtractMethod
{
internal interface IExtractMethodService : ILanguageService
{
Task<ExtractMethodResult> ExtractMethodAsync(Document document, TextSpan textSpan, OptionSet options = null, CancellationToken cancellationToken = default);
Task<ExtractMethodResult> ExtractMethodAsync(Document document, TextSpan textSpan, bool extractLocalFunction = false, OptionSet options = null, CancellationToken cancellationToken = default);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

would likely be cleaner as exposing two separate functions. i.e. ExtractMethodAsync and ExtracLocalFunctionAsync. I don't care hugely though.

This boolean sholud be non-optional though

return NameGenerator.GenerateUniqueName(baseName, string.Empty,
n => _semanticModel.LookupSymbols(contextNode.SpanStart, /*container*/null, n).Length == 0);
n => _semanticModel.LookupSymbols(contextNode.SpanStart, /*container*/null, n).Length == 0 && !localFunctionNames.Contains(n));

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

i don't get why we need special logic here. somthing feels wonky as NameGenerator.GenerateUniqueName should have smarts to do this properly.

@@ -598,6 +598,9 @@
<data name="Unknown" xml:space="preserve">
<value>Unknown</value>
</data>
<data name="Extract_Local_Method" xml:space="preserve">
<value>Extract Local Method</value>
</data>
<data name="Extract_Method" xml:space="preserve">
<value>Extract Method</value>

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

this should likely be "Extract method" as well. Not sure why 'Method' is capitalized. Before changing this though, talk to the team about which is preferred: "Title Cased", or "Sentence cased". And made both consistent with each other :)

return destination;
}

CheckDeclarationNode<TypeDeclarationSyntax, CompilationUnitSyntax, NamespaceDeclarationSyntax, MethodDeclarationSyntax, LocalFunctionStatementSyntax>(destination);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

this doesn't seem right. you shouldn't be able to AddMethod to anything but a TypeDeclarationSyntax, CompilationUnitSyntax, NamespaceDeclarationSyntax. I also don't think we particularly need an AddLocalFunction helper. It's just a statement, so it should be easy to add to the method we're in.

if (destination is LocalFunctionStatementSyntax localMethodDeclaration)
{
return Cast<TDeclarationNode>(MethodGenerator.AddMethodTo(localMethodDeclaration, method, Workspace, options));
}

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Nov 12, 2019

Contributor

i wouldn't go through these codepaths for local functions if you can avoid it.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented Nov 12, 2019

Done with initial pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.