Change Method Signature: change definition

[kuhlenh]

It would be nice if while in the Change Method Signature you could change the modifiers, rename, and return type.

[davidwengier]

After doing a few simple bug fixes the other week, I thought I'd have a go at this as a bit of a larger project. I spent a few hours on it last week but have hit a roadblock and would be very grateful for any help.

I've got the UI working for allowing edit of a parameter name, and changing the parameter name, but renaming the references to the parameter is proving to be a challenge. Here is where I am so far:

I've managed to get to the point where re-ordering parameters works (ie, still works), or renaming parameters works, but doing both throws a NullReferenceException, and also hits a Debug.Assert in the renamer.

The story so far:

• Added a new property to IParameterSymbol for AlternateName, and updated the dialog to allow editing of parameter name and updating the preview
  • I hindsight there is an alternative approach I should be able to use for this, as I'm not a fan.
  • Code for this is here: d8d3a32f7d0bfd6f3b84175dde475bd4d77834c3
• Added renaming of parameters at the point where the method declaration is updated by changing the identifier on the parameter
  • This is the point at which the GIF above was captured
  • Code for this is here: 3041603#diff-f82d716a4b52569093434496de25eca3R422
• Tried to use the same approach for changing references to the parameters by rewriting the body of the method with a rewriter,
  • This didn't work because the Identifier instances are not the same, which I assumed (hoped?) they would be
  • Code for this is here: davidwengier@49332b4#diff-104c781c6b53c283c7d6cd4d96643ae3R29
• Found the Renamer class and tried using that instead, after the parameters are reordered
  • This works if the parameters aren't reordered, but if they are it hits a debug assert and null reference exception.
  • Moving the renaming to happen before reordering the parameters didn't work either
  • I assume this is because the syntax tree that the rename is trying to use is no longer valid after the parameters have been reordered, but I'm out of my depth in knowing what to do about it.
  • Code for how I'm calling the renamer is here: davidwengier@49332b4#diff-1e9a015d9c1b3f078275e7014e57f0eeR365

I did notice that the Renamer class doesn't seem to have a heap of uses in the code, and unfortunately I no longer have access to the one Analyzer I wrote once that renamed fields so I can't remember how I achieved that.

Any pointers as to where to go from here would be greatly appreciated. I could rewrite the body with my rewriter matching only on an identifiers name, and that would work, but then would break if there was another identifier with the same name (eg, field with the same name as parameter) and wouldn't account for renaming in local functions or named parameters.

Thank you for reading!

[agocke]

cc @dotnet/roslyn-ide

[dpoeschl]

Hey @davidwengier, nice progress! Are you still interested in this?

Writing down some initial thoughts if you're still interested:

roslyn/src/Features/Core/Portable/ChangeSignature/ChangeSignatureCodeAction.cs

Line 11 in 0c884ee

internal class ChangeSignatureCodeAction : CodeActionWithOptions

On line 26, you'll see the call to GetChangeSignatureOptions which returns the ChangeSignatureOptionsResult, which contains the SignatureChange, which I think is where you could encode the changed names. Changing IParameterSymbol can't be the final approach here; it'll have to be encoded specially in the optionsresult. Of course, do whatever you want with IParameterSymbol while prototyping. 😄

Then, when ComputeOperationsAsync gets called and passed that ChangeSignatureOptionsResult you created from the UI-based configuration, it starts by doing the ChangeSignatureWithContext which produces the new solution:

roslyn/src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

Line 198 in 0c884ee

private bool TryCreateUpdatedSolution(

Now it gets slightly tricky. Here's what Rename does when renaming a parameter:

ChangeSignature updates declarations & references to the invoked method and its overrides/etc. As you can see, Rename does not do that. There's also the case with an override that changes the parameter name, but we can deal with that later.

So, if you want the parameter rename to cascade everywhere, either the Renamer needs updated (I don't think we should do this as part of this), or we need to do the rename manually as we visit each location and make whatever other changes are necessary -- for declarations just changing the name, and for references updating named arguments if necessary. This visiting & updating of each location is done for C# in the Service here:

roslyn/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs

Line 230 in 0c884ee

public override SyntaxNode ChangeSignature(

Let me know what you think!

[davidwengier]

Thanks for the comments @dpoeschl. I admit I did abandon this because life got in the way, but I'm happy to keep plugging away. Thanks for the tip that Renamer is barking up the wrong tree too.

I did change CSharpChangeSignatureService.cs as part of trying, and indeed it does do a lot of the work just by changing PermuteDeclaration like this:

https://github.com/davidwengier/roslyn/blob/e306708d30c3889f04cc53bc36a9fef952e3e786/src/Features/CSharp/Portable/ChangeSignature/CSharpChangeSignatureService.cs#L422-L425

That works great for the method parameters etc. The problem I had was when trying to rewrite the body of the methods to change the usages of those parameters. I added a PermuteBody method and a ParameterRenamerRewriter to visit every identifier in the method body, and attempt to write out the new name. Unfortunately the identifiers do not match (presumably from two different syntax trees at that point) and I didn't know where to go from there.

My attempt at matching was just this:

https://github.com/davidwengier/roslyn/blob/e306708d30c3889f04cc53bc36a9fef952e3e786/src/Features/CSharp/Portable/ChangeSignature/ParameterRenamerRewriter.cs#L28-L37

If I change that to be naive and just match on identifier names (the ValueText property) alone, the code does work and everything changes as required, but obviously thats too naive and there a million ways it could be wrong. So what I really need help with is how to match identifiers from the ones that come in as part of the SignatureChange and the ones that are visited as the service is rewriting the tree.

[davidwengier]

Also I need to work out how to get those cool inline code snippets :)

[davidwengier]

Okay I found the IsEquivalentTo extension method and so I've got the renaming all working fine, and the reordering of the parameters works great.

I took the suggestion and reverted any changes to the IParameterSymbol and am using the signature result for tracking the renames which works fine, but unfortunately breaks the signature preview in the dialog itself. That just uses the ToDisplayParts method of the IParameterSymbol so without making changes to that I'm unsure how I can get the new name in there.

I initially tried simply wrapping the IParameterSymbol, implementing everything through the existing parameter except the Name, but that doesn't work help because the actual method that produces the display parts is static, and gets the parameter passed in, so it ends up with the wrapped parameter, not my wrapper.

Any suggestions for how to approach this without changing IParameterSymbol would be appreciated.

[sharwell]

Design review conclusion: this dialog was implemented in the current form to meet parity with prior releases as part of the minimum viable product for the initial Roslyn release. Expanding it in a well-defined manner seems like a good idea overall. The following items are especially valuable:

1. Changing the ref kind of a parameter (ref/in/none for C#, or ByRef/ByVal for VB)
2. Making a parameter optional, with the ability to optionally update call sites to omit the argument when it matches the default value
3. Adding or removing params/ParamArray from a parameter, with the ability to optionally update call sites
4. Renaming a parameter (lower priority since this already works well in the text editor)
5. Renaming the method (lower priority since this already works well in the text editor, and there is no current place in the dialog to make this change)

The following items would need to be separate issues due to additional design concerns:

1. Changing the type of a parameter
2. Changing the return type of the method

🔗 Note that adding parameters is already filed as a separate issue in #33534.

[allisonchou]

There has been an ask to consolidate Change Signature feature requests into one issue. Since this enhancement is being tracked by the overarching issue #33977, will close as a dupe.