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.

Sign up for GitHub

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

Jump to bottom

Features/22051 Introduce Parameter #51357

Merged
merged 79 commits into from
Apr 19, 2021
Merged

Features/22051 Introduce Parameter #51357

merged 79 commits into from
Apr 19, 2021

Conversation

akhera99
Copy link
Member

@akhera99 akhera99 commented Feb 20, 2021 •
edited
Loading

#22051

Hello!

This PR contains the current working solution for introducing a parameter and doing the refactoring at the method locations. It only adds code action options based on if the expression trying to be introduced as a parameter is in a parameterized method and contains parameters.

Currently does not support trampolining/overloads for optional parameters because the CodeGenerationSymbolFactory does not handle optional parameters. Will file a bug for that, but that is why support for that is limited. Otherwise, supports all cases besides params.

@akhera99 akhera99 self-assigned this Feb 20, 2021
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
...eatures/CSharp/Portable/IntroduceVariable/CSharpIntroduceParameterCodeRefactoringProvider.cs Outdated
}

protected override SyntaxNode UpdateArgumentListSyntax(SyntaxNode argumentList, SeparatedSyntaxList<SyntaxNode> arguments)
=> (argumentList as ArgumentListSyntax)!.WithArguments(arguments);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
=> (argumentList as ArgumentListSyntax)!.WithArguments(arguments);
=> ((ArgumentListSyntax)argumentList).WithArguments(arguments);

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs Outdated
return;
}

if (methodSymbol.Language.Equals("Visual Basic") && methodSymbol.Name.Equals("Finalize") && methodSymbol.IsOverride)
Copy link
Member

Choose a reason for hiding this comment

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

lol. no.

  1. why are you filtering this out?
  2. dont' do a language speicif check in the base class. this is what abstract methods are for :)

Copy link
Member

Choose a reason for hiding this comment

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

WellKnownMemberNames.DestructorName

Copy link
Member Author

Choose a reason for hiding this comment

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

lol

  1. because I want to keep it consistent with c# and destructors don't have parameters
  2. okay
    Is there a better way to check whether or not the method symbol is a destructor in the VB case without just comparing the method name?

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs
}
}

return (true, containsClassSpecificStatement);
Copy link
Member

Choose a reason for hiding this comment

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

you should doc all this code. so that the 'why' make ssense. i.e. 'why do we not display the code action if IsParams is true? someone reading this wouldnt' know the reason you did this.

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 docs

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs Outdated
using var actionsBuilderAllOccurrences = TemporaryArray<CodeAction>.Empty;
var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
var singleLineExpression = syntaxFacts.ConvertToSingleLine(expression);
var nodeString = singleLineExpression.ToString();
Copy link
Member

Choose a reason for hiding this comment

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

this seems weird. you are doing this in the toplevel code actions as well. this feels like a lot of redundancy. can you show me a picture of what the lightbulb items look like now?

Copy link
Member Author

Choose a reason for hiding this comment

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

image
but imagine "callsites" is "call sites"

Copy link
Member

Choose a reason for hiding this comment

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

"into new overload of x+y"?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to into new overload

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs
else
{
var newRoot = await ModifyDocumentInvocationsAndIntroduceParameterAsync(compilation,
originalDocument, document, mappingDictionary, containingMethod,
Copy link
Member

Choose a reason for hiding this comment

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

the reversing of originalDocument/document here versus ModifyDocumentInvocationsTrampolineOverloadAndIntroduceParameterAsync will give me a brain embolism :)

Copy link
Member Author

Choose a reason for hiding this comment

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

switched lol

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs
{
var reference = refLocation.Location.FindNode(cancellationToken).GetRequiredParent();
var originalReference = reference;
if (reference is not (TObjectCreationExpressionSyntax or TInvocationExpressionSyntax))
Copy link
Member

Choose a reason for hiding this comment

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

interesting. do you have any tests where teh callee does something like this.Foo(x, y z). I would have expecte the reference to point at Foo and GetRequiredParent to return this.Foo, not the invocation. Can you verify (or add) test here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a test for that, it's called TestExpressionWithSingleMethodCallAndAccessorsTrampoline. It does as you say, and then the parent of this.Foo is the invocation

Copy link
Member

Choose a reason for hiding this comment

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

What about a.b.Foo?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, TestExpressionWithSingleMethodCallMultipleAccessorsTrampoline

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs Outdated
return insertionIndex;
}

private static bool ShouldIndexBeIncremented(Compilation compilation, IParameterSymbol parameter)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe: ShouldParameterBeSkipped.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm bad with naming

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs
return GenerateNewArgumentListSyntaxForTrampoline(compilation, syntaxFacts, semanticFacts, invocationSemanticModel, generator,
parameterToArgumentMap, currentArgumentListSyntax,
argumentListSyntax, invocation, validParameters, parameterName, newMethodIdentifier, insertionIndex, cancellationToken);
});
Copy link
Member

Choose a reason for hiding this comment

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

somethign hre feels a bit weird. there's a dictionary above with nothing in it. which you teh pass to this helper. Does the helper populate the dictionary? and use it across multiple calls? this is worth doc'ing in code what is going on.

Copy link
Member Author

Choose a reason for hiding this comment

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

the helper populates the dictionary for every invocation, yeah. I will add docs

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs
{
var expressionsWithConditionalAccessors = conditionalRoot.ReplaceNode(invocation, newMethodInvocation);
allArguments = AddArgumentToArgumentList(currentInvocationArguments, generator, expressionsWithConditionalAccessors, parameterName, insertionIndex, named);
}
Copy link
Member

Choose a reason for hiding this comment

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

doc what's going on here.

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs Outdated
// In the VB case sometimes the the member access expression's expression is nothing
// so I generate a member binding expression instead.
methodName = generator.MemberAccessExpression(expression: null, newMethodIdentifier);
}
Copy link
Member

Choose a reason for hiding this comment

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

these codepaths are the same. you don't need the if check.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs
}
else if (syntaxFacts.IsMemberBindingExpression(fullExpression))
{
methodName = generator.MemberBindingExpression(generator.IdentifierName(newMethodIdentifier));
Copy link
Member

Choose a reason for hiding this comment

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

do you have tests where the original method is generic? is the presumption that we don't have to pass type arguments along because they'll all be inferred?

Copy link
Member

Choose a reason for hiding this comment

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

if so, that seems olike an overly aggressive assumption. however, determining if we need type args or not is challenging, so we can leave out os cope for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not have tests for that, I can add it

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 16, 2021
View reviewed changes
src/Features/Core/Portable/IntroduceVariable/AbstractIntroduceParameterService.cs Outdated
}
}

private enum SelectedCodeAction
Copy link
Member

Choose a reason for hiding this comment

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

IntroduceParameterCodeActionKind

CyrusNajmabadi
CyrusNajmabadi approved these changes Apr 16, 2021
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

LGTM with the issues i referenced fixed. let's get this in, and we can discuss next steps on how to clean some of the patterns up.

@akhera99 akhera99 merged commit 7cdef0b into dotnet:release/dev17.0 Apr 19, 2021
@vatsalyaagrawal
Copy link
Contributor

🎉🎉

@akhera99 akhera99 deleted the features/22051_introduce_parameter_restart branch June 18, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmarolf jmarolf jmarolf left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@davidwengier davidwengier davidwengier left review comments

@ryzngard ryzngard Awaiting requested review from ryzngard

@mikadumont mikadumont Awaiting requested review from mikadumont

Assignees

@akhera99 akhera99

Labels
Area-IDE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@akhera99 @CyrusNajmabadi @vatsalyaagrawal @ryzngard @davidwengier @jmarolf