Catching errors in code fix using Renamer

[sharwell]

We have a Code Fix which uses Renamer to rename a symbol according to naming conventions for a project. While the diagnostic we use for breaking the naming convention should always be reported, we would like to either suppress the code fix or revert its behavior if the rename operation results in two symbols with conflicting names.

:question: What is the recommended practice for this scenario?

For reference, the issue was initially raised in DotNetAnalyzers/StyleCopAnalyzers#436, where we hope to establish a standard practice for code fixes that rename symbols.

[mattwar]

You should avoid doing anything computationally expensive when determining whether to offer a code fix action. Finding rename conflicts can be very expensive. Save expensive work to run under the code action.

Also, you can use ConflictAnnotation and WarningAnnotation to identify the places where the user should be notified and/or agree to before the code fix is applied. The Renamer should be adding conflict annotations where conflicts occur.

Do you not get conflict warnings in the code fix preview if the rename has identified conflicts?

[sharwell]

Right now I only tested this functionality within unit tests. I need to run the same functionality in-IDE and I'll let you know the results.

I agree with the performance implications. Several code fixes we have need to be updated to lazily compute the results. That is mostly a separate issue, but relates here because it means we should always propose the rename, even if it results in a conflict.

[srivatsn]

Normally when there's a need to rename something in a codefix, it's best to just attach a RenameAnnotation to the token that needs to be renamed. Doing that will make it so that when the user applies the fix, it will start an inline rename session and then all conflict resolution happens as is usual.

For this particular fix, you want to simply add a I to the front of the interface name - even there it might be reasonable to make that change and start the inline rename session and have the user commit the rename explicitly - that way they can choose to accept or decline based on conflicts etc.

[sharwell]

@mattwar The preview UI did not behave as expected, because renaming the symbol caused the problem to appear on the identifier for the second interface definition, which is not visible in the preview for the code fix.

Before

Preview

After

[sharwell]

@srivatsn I applied RenameAnnotation to the original definition I was renaming, and then used Renamer to rename the symbol. This did cause the rename UI to appear, but did not behave in a useful manner. As you can see in the following image, both instances of IFoo are now highlighted.

• Typing a different name to avoid the conflict causes both interfaces to take the new name, so conflicts are unavoidable.
• Pressing Esc does not cancel the rename operation.

[srivatsn]

You are right. The RenameAnnotation applies the fix and then starts the rename session (you just need the annotation btw and don't need to call Renamer). But what you want is to initiate a rename session and then apply the fix of adding the 'I' and let the rename session continue to point out the conflicts. We don't have anything for that unfortunately. It would be nice if the RenameAnnotation took a string called initialText and populated the rename session with that text - that would solve your problem.

Meanwhile, with regards to your other reply, I think the preview is supposed to show the conflicts when there are conflict annotations. I wonder if the Renamer API strips them out before handing out the final solution. Either way it sounds like a problem worth thinking more about.

[srivatsn]

Regardless, the rename API should probably return the conflicts as well instead of just the new solution.

[dpoeschl]

Regardless, the rename API should probably return the conflicts as well instead of just the new solution.

We have a couple internal work items tracking this, including 917587 & 916524.

I think the preview is supposed to show the conflicts when there are conflict annotations. I wonder if the Renamer API strips them out before handing out the final solution.

We currently remove all rename annotations before returning the updated Solution.

It would be nice if the RenameAnnotation took a string called initialText and populated the rename session with that text - that would solve your problem.

:+1:

[dpoeschl]

@jmarolf This is the bug ("It would be nice if the RenameAnnotation took a string called initialText and populated the rename session with that text - that would solve your problem.") I mentioned to you a couple days ago.

[CyrusNajmabadi]

Closing due to lack of movement.