Expose IRefactoringHelpersService for third party CodeRefactoringProviders

[mavasani]

IRefactoringHelpersService was added a while back to aid our code refactorings to identify the syntax nodes which are applicable for a given refactoring span. This API is used by all our refactorings to make the refactorings consistent and discoverable.

Recently, we added support to allow third party analyzer NuGet packages to export CodeRefactoringProviders. Our first attempt to add such a refactoring in roslyn-analyzers repo found out that we really need the IRefactoringHelpersService for a decent implementation of refactorings. The refactoring that we attempted to add was specific to Roslyn.sln, hence cannot be added to Roslyn repo.

We have few options to move forward here:

1. Cleanup and make IRefactoringHelpersService public.
2. Use IVT to access IRefactoringHelpersService, with the risk of breaking changes. The risk is pretty low if the only package using it is Roslyn.Diagnostics.Analyzers, which is our internal package that can be easily fixed up and updated.
3. Add a new project in Roslyn.sln, similar to CodeStyle projects, for writing Roslyn specific refactorings. The package will publish only on internal roslyn feeds and consumed in Roslyn.sln.
4. Clone the IRefactoringHelpersService and the associated helpers into roslyn-analyzers repo. Note that this service uses ISyntaxFacts and ISemanticFacts, along with the extensions used by these services, so we would need to clone a transitive closure of helper code. This has been prototyped in Port IRefactoringHelpers from dotnet/roslyn roslyn-analyzers#3691.

Personally, I am fine with any of the first 3 approaches. I think we can take approach 2 for short term to unblock writing refactorings in roslyn-analyzers repo, while we work on 1.

[mavasani]

@sharwell @CyrusNajmabadi

[sharwell]

I'm not confident that option 1 (new public API) will meet the needs of refactoring providers in the short term. While I liked the behavior in most cases, I would expect refactorings to need to tweak the implementation to get specific behavior in cases we didn't foresee.

Option 2 (IVT) has proven permanent historically. Since the analyzer assembly in question is not run through RPS, I would consider an external access assembly to be a viable alternative.

I have no objections to either options 3 or 4.

[CyrusNajmabadi]

My concern here is that the nature of this service is legitimately to be underspecified. i.e. we expect that we can and will continue to change it's behavior to serve our personal purposes. We can currently do that without concerns, as we own all teh consumers and can validate that the new behavior they exhibit is considered a net positive.

That's not the case with external consumers. We may trivially break an expected use case for a consumer by shrinking a refactoring span. We may trivially degrade an experience by having a span grow too much.

[mavasani]

I would consider an external access assembly to be a viable alternative

This sounds most promising, with least amount of work and reasonable amount of compat guarantee, without requiring to clone code or support a public API that we are not sure can guarantee non-breaking semantics.

[CyrusNajmabadi]

THat works for me. We shoudl doc that it intentionally is expected to change over time, so consumers should keep that in mind and test/validate so they can respond if/when that happens.

[sharwell]

@CyrusNajmabadi We would be the only consumer. No other library would have access to it except through reflection.

[CyrusNajmabadi]

Ah, that changes my view on it entirely :)

[jmarolf]

Design Meeting Notes

• @sharwell will try the copy source approach if that becomes too much work we will use reflection

[sharwell]

🔗 This is being used as a shared project in roslyn-analyzers starting with dotnet/roslyn-analyzers#3692
🔗 We requested API feedback from Roslynator via dotnet/roslynator#687