search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.04k stars 4.03k forks source link

CodeFix/CodeRefactoring Support for AdditionalFiles and AnalyzerConfigFiles #62877

Closed mgoertz-msft closed 2 weeks ago

mgoertz-msft commented 2 years ago

Summary

Add CodeFix/CodeRefactoring Support for AdditionalFiles to support XAML lightbulbs for MAUI.

Required to fix Hot Reload rude edit reporting issue https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1579223

Features

This work requires 3 different API sets:

Background and Motivation

.NET MAUI adds XAML files to C#/VB projects as AdditionalFiles. This breaks XAML lightbulb functionality, etc. because we create special XAML shadow projects to support that. While analyzers are now supported for AdditionalFiles (see issue #44131) we cannot provide any code fixes or refactorings that way.

At least for MAUI we would then be able to avoid creating XAML shadow projects, which cause additional issues with the MAUI source generators.

Proposed Feature

Code fixes use CodeActions, which are based on Document. We would need to either allow the creation of XAML Documents in AdditionalFiles (something I would still like to see in order to support SyntaxTrees for XAML) or support CodeActions for AdditionalDocument/TextDocument.

Alternative Designs

see above

mgoertz-msft commented 2 years ago

FYI @tmat

mavasani commented 2 years ago

@mgoertz-msft Quick question for you. I remember when we discussed this in the past, you mentioned there are couple of blocking issues for you to move the XAML DocumentDiagnosticAnalyzer to a regular diagnostic analyzer based on RegisterAdditionalFileAction:

_As far as using AdditionalFiles for the longer term is concerned, there are two blocking issues:

  1. We can’t use targets to add XAML files to AdditionalFiles
  2. Code fixes don’t support AdditionalFiles either_

This issues tracks the latter. Were you able to resolve the former? Just want to confirm whether implementing this feature request will completely unblock you or there are additional blockers that would need to be resolved.

mgoertz-msft commented 2 years ago

@mavasani Correct, the first issue is not really an issue anymore since MAUI does that be default. We would still have to do that for other platforms though (WPF, UWP, WinUI, and Xamarin Forms).

We wouldn't need the DocumentDiagnosticAnalyzer anymore. However, we will still need those analyzers to run in-process. Moving our language service entirely out of process would be a much bigger undertaking that would require more blockers to be resolved (for example #44099 but I'm sure there will be more).

It would also be good if these analyzers could be registered by ContentType so that Roslyn would only run the applicable analyzers for each file type.

mavasani commented 2 years ago

Thank you for the context @mgoertz-msft. In that case, I'll prioritize this issue for adding code fixes/refactorings support in XAML additional files.

However, we will still need those analyzers to run in-process.

This is going to be unfortunate. I don't believe we want to add any public API/support to DiagnosticAnalyzers to allow specifying such host restrictions. So, that would mean your analyzer would still need to be special cased in our IDE diagnostic service.

It would also be good if these analyzers could be registered by ContentType so that Roslyn would only run the applicable analyzers for each file type.

This would need to be a compiler feature request. Please file a separate issue for this.

mavasani commented 2 years ago

This work requires 3 different API sets: CodeRefactoring API, CodeFix API and FixAll API.

Proposal in this comment is for CodeRefactoring API. Proposed API enables writing code refactorings for all text document kinds.

[OLD] CodeRefactoring API Proposal

1. TextDocumentCodeRefactoringContext

namespace Microsoft.CodeAnalysis.CodeRefactorings
{
    public readonly struct TextDocumentCodeRefactoringContext
    {
        public TextDocument Document { get; }
        public TextSpan Span { get; }
        public CancellationToken CancellationToken { get; }

        public void RegisterRefactoring(CodeAction action);
    }
}

2. TextDocumentCodeRefactoringProvider

namespace Microsoft.CodeAnalysis.CodeRefactorings
{
    /// <summary>
    /// Inherit this type to provide code refactorings for <see cref="TextDocument"/>s.
    /// Remember to use <see cref="ExportCodeRefactoringProviderAttribute"/> so the host environment can offer your refactorings in a UI.
    /// </summary>
    public abstract class TextDocumentCodeRefactoringProvider
         : CodeRefactoringProvider
    {
        public abstract ImmutableArray<TextDocumentKind> SupportedDocumentKinds { get; }

        public abstract Task ComputeRefactoringsAsync(TextDocumentCodeRefactoringContext context);

        public sealed override Task ComputeRefactoringsAsync(CodeRefactoringContext context)
            => Task.CompletedTask;
    }
}

3. CodeAction.Create overload

namespace Microsoft.CodeAnalysis.CodeActions
{
    public abstract class CodeAction
    {
        /// <summary>
        /// Creates a <see cref="CodeAction"/> for a change to a single <see cref="TextDocument"/>.
        /// Use this factory when the change is expensive to compute and should be deferred until requested.
        /// </summary>
        /// <param name="title">Title of the <see cref="CodeAction"/>.</param>
        /// <param name="createChangedTextDocument">Function to create the <see cref="TextDocument"/>.</param>
        /// <param name="equivalenceKey">Optional value used to determine the equivalence of the <see cref="CodeAction"/> with other <see cref="CodeAction"/>s. See <see cref="CodeAction.EquivalenceKey"/>.</param>
        public static CodeAction Create(string title, Func<CancellationToken, Task<TextDocument>> createChangedTextDocument, string? equivalenceKey = null);
    }
}

Alternative API shapes considered

  1. Do not create a new sub-type TextDocumentCodeRefactoringProvider, but add new virtual public APIs Task ComputeRefactoringsAsync(TextDocumentCodeRefactoringContext context) and ImmutableArray<TextDocumentKind> SupportedDocumentKinds { get; } to CodeRefactoringProvider type itself: This leads to unnecessarily polluting the API surface for CodeRefactoringProvider and also might lead to users not overriding one of the two new APIs being added here.
  2. TextDocumentCodeRefactoringProvider does not sub-type CodeRefactoringProvider, but instead they both implement a common internal interface: We need to duplicate all the discovery mechanism and/or use the interface and add type checking at places where we need to know the concrete type.
  3. Add specific typed refactoring providers for each TextDocumentKind, AdditionalDocumentCodeRefactoringProvider, AnalyzerConfigDocumentCodeRefactoringProvider, etc.: We are specifically trying to avoid adding parallel APIs for each non-source document kind. This leads to having to keep adding parallel APIs everywhere whenever we add a new TextDocumentKind in future.

Precedent for Proposed API shape

Current proposed design also has a precedence in how we designed the API for DiagnosticSuppressors: https://github.com/dotnet/roslyn/blob/087baa2770faf2ac1f0ce179d4c23d92b2a75d26/src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticSuppressor.cs#L9-L12 It seals the N/A APIs on DiagnosticAnalyzer, and introduces new suppressor related APIs: https://github.com/dotnet/roslyn/blob/087baa2770faf2ac1f0ce179d4c23d92b2a75d26/src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticSuppressor.cs#L14-L22

Other related changes

We would add a new supported content type attribute, [ContentType("text")] to our quick action source: https://github.com/dotnet/roslyn/blob/087baa2770faf2ac1f0ce179d4c23d92b2a75d26/src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSourceProvider.cs#L28-L37

Example Refactoring

    [ExportCodeRefactoringProvider(LanguageNames.CSharp)]
    [Shared]
    internal sealed class MyRefactoring : TextDocumentCodeRefactoringProvider
    {
        [ImportingConstructor]
        [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
        public MyRefactoring()
        {
        }

        public override ImmutableArray<TextDocumentKind> SupportedDocumentKinds
            => ImmutableArray.Create(TextDocumentKind.AdditionalDocument, TextDocumentKind.AnalyzerConfigDocument);

        public override Task ComputeRefactoringsAsync(TextDocumentCodeRefactoringContext context)
        {
            context.RegisterRefactoring(CodeAction.Create("My Refactoring",
                createChangedSolution: async ct =>
                {
                    var document = context.Document;
                    var text = await document.GetTextAsync(ct).ConfigureAwait(false);
                    var newText = SourceText.From(text.ToString() + Environment.NewLine + "# Refactored" + Environment.NewLine);
                    if (document.Kind == TextDocumentKind.AdditionalDocument)
                        return document.Project.Solution.WithAdditionalDocumentText(document.Id, newText);
                    return document.Project.Solution.WithAnalyzerConfigDocumentText(document.Id, newText);
                }));

            return Task.CompletedTask;
        }
    }

Prototype/Demo

1. AdditionalDocument

CodeActionsAdditionalDocument

2. AnalyzerConfigDocument

CodeActionsAnalyzerConfigDocument

jasonmalinowski commented 2 years ago

During the design meeting we generally liked the feature and agreed on the motivation for it. Even though something like XAML/Razor can go to LSP to make their own fixes, we can think of scenarios where this is still needed, like our PublicAPI checker.

We however came up with a different API shape: on CodeRefactoringContext, just add the TextDocument property like we would like to add, and make the existing Document property just throw (rather than return null) if it's improperly accessed. I'm not sure if we came to a precise conclusion on how the opt-in would look, and whether that involved overriding a method or opting in via the Export attribute, but either way since it's an explicit opt-in, code fixes that didn't opt in won't have any surprises accessing the Document property in that case.

jasonmalinowski commented 2 years ago

@mavasani: should I move this to "needs proposal" and we'd re-review the updated version, or just want to do that via PR at that point?

mavasani commented 2 years ago

@mavasani: should I move this to "needs proposal" and we'd re-review the updated version, or just want to do that via PR at that point?

I think directly doing the PR is fine.

mgoertz-msft commented 2 years ago

It would also be good if these analyzers could be registered by ContentType so that Roslyn would only run the applicable analyzers for each file type.

@mavasani Looking at how analyzers for additional files are registered I'm not so sure anymore that this actually makes sense. However, for code fix/refactoring providers I think it still does, so I created issue #63547 for it.

mavasani commented 2 years ago

Update on the [ContentType("text")] attribute

Change

We would add a new supported content type attribute, [ContentType("text")] to our quick action source for Roslyn quick actions to show up in additional documents and analyzer config documents: https://github.com/dotnet/roslyn/blob/087baa2770faf2ac1f0ce179d4c23d92b2a75d26/src/EditorFeatures/Core.Wpf/Suggestions/SuggestedActionsSourceProvider.cs#L28-L37

Performance Issue

By default, the above change would lead to Roslyn binaries getting loaded for text files in non-Roslyn projects, which would be a performance regression.

Solution

After talking with the editor team, we have decided to accompany [ContentType("text")] attribute with [DeferCreation(OptionName = IsRoslynPackageLoadedOption.OptionName)], for a new boolean editor option. This editor option will be set to true when RoslynPackage is loaded, and reset to false when the package is disposed.

jasonmalinowski commented 2 years ago

Just want to make one small comment here:

This editor option will be set to true when RoslynPackage is loaded, and reset to false when the package is disposed.

We don't technically want to condition this on RoslynPackage loading, for a few reasons:

  1. RoslynPackage isn't strictly required to be loaded before we could need this. It gets loaded in some legacy situations but it's possible for a CPS project to get loaded with all files closed and it won't load. It gets loaded for the opening of any .cs/.vb file, but since we're talking about additional files here that's not a guarantee either!
  2. I can imagine we might load RoslynPackage for F#/TypeScript still and the goal here was to avoid loading in other scenarios if they don't need this.

We have code here that updates some UI contexts when a project is loaded; we could update the option there. We could also do something stronger like "there's an additional file in the workspace".

mavasani commented 2 years ago

it's possible for a CPS project to get loaded with all files closed and it won't load. It gets loaded for the opening of any .cs/.vb file

I tried this under the debugger, and the RoslynPackage seems to get loaded on opening a CPS project with all .cs/.vb files closed. I didn't have to open any file in the project for the package to get loaded.

I can imagine we might load RoslynPackage for F#/TypeScript still and the goal here was to avoid loading in other scenarios if they don't need this.

Note that we are not adding a separate suggested actions source provider for non-source documents, but instead just enhancing the existing one to support both source and non-source documents. I think we do want to continue loading the provider when any project with a Roslyn based language is opened, F#/TS included, for quick actions in source documents.

In other words, RoslynPackage load met the necessary and sufficient conditions for setting this option for my testing scenarios. I'd like to have a concrete test scenario where this is violated to ensure we are making the right change.

jasonmalinowski commented 2 years ago

In other words, RoslynPackage load met the necessary and sufficient conditions for setting this option for my testing scenarios

So I guess if something changed that might be the case today, but there's zero guarantee that's the case tomorrow. So it's better to figure out a more appropriate place that clearly links to your scenario (a project being created, an additional file being added through the project system etc.)

mavasani commented 2 years ago

In other words, RoslynPackage load met the necessary and sufficient conditions for setting this option for my testing scenarios

So I guess if something changed that might be the case today, but there's zero guarantee that's the case tomorrow. So it's better to figure out a more appropriate place that clearly links to your scenario (a project being created, an additional file being added through the project system etc.)

I am wondering if it is better to separate out the suggested action providers then. I'll retain the current one as-is for source documents, and add a new one just for "text" content type, and add the DeferCreationAttribute only to the latter. This should allow us to harden the requirement when the option is set to only do so when we see an additional/analyzer config document being added to the workspace. Let me try this out and update the PR.

mavasani commented 2 years ago

@jasonmalinowski I have pushed https://github.com/dotnet/roslyn/pull/64364/commits/c891363e60b61c9a865dd6cc9d4930fd4a643d2d to separate out the suggested actions source providers and also set the option when we first open any additional or analyzer config document in the workspace. This should hopefully match your preference.

mavasani commented 2 years ago

Code Refactoring and Code Fix Public API Proposal

@jasonmalinowski @CyrusNajmabadi @jmarolf - this is the same API proposal that was approved at the last IDE design meeting. This comment tracks reviewing/approving it at the Roslyn API review meeting.

Code Refactoring API

namespace Microsoft.CodeAnalysis.CodeRefactorings
{
    public readonly struct CodeRefactoringContext
    {
        /// <summary>
        /// TextDocument corresponding to the <see cref="CodeRefactoringContext.Span"/> to refactor.
        /// This property should be used instead of <see cref="CodeRefactoringContext.Document"/> property by
        /// code refactorings that support non-source documents by providing a non-default value for
        /// <see cref="ExportCodeRefactoringProviderAttribute.DocumentKinds"/>
        /// </summary>
        public TextDocument TextDocument { get; }

        /// <summary>
        /// Creates a code refactoring context to be passed into <see cref="CodeRefactoringProvider.ComputeRefactoringsAsync(CodeRefactoringContext)"/> method.
        /// </summary>
        public CodeRefactoringContext(TextDocument document, TextSpan span, Action<CodeAction> registerRefactoring, CancellationToken cancellationToken);
    }

    public sealed class ExportCodeRefactoringProviderAttribute : ExportAttribute
    {
        /// <summary>
        /// The document kinds for which this provider can provide refactorings. See <see cref="TextDocumentKind"/>.
        /// By default, the provider supports refactorings only for source documents, <see cref="TextDocumentKind.Document"/>.
        /// </summary>
        public TextDocumentKind[] DocumentKinds { get; set; }

        /// <summary>
        /// The document extensions for which this provider can provide refactorings.
        /// By default, this value is null and the document extension is not considered to determine applicability of refactorings.
        /// </summary>
        public string[]? DocumentExtensions { get; set; }
    }
}

Code Fix API

namespace Microsoft.CodeAnalysis.CodeFixes
{
    public readonly struct CodeFixContext
    {
        /// <summary>
        /// TextDocument corresponding to the <see cref="Span"/> to fix.
        /// This property should be used instead of <see cref="Document"/> property by
        /// code fixes that support non-source documents by providing a non-default value for
        /// <see cref="ExportCodeFixProviderAttribute.DocumentKinds"/>
        /// </summary>
        public TextDocument TextDocument { get; }

        /// <summary>
        /// Creates a code fix context to be passed into <see cref="CodeFixProvider.RegisterCodeFixesAsync(CodeFixContext)"/> method.
        /// </summary>
        /// <param name="document">Text document to fix.</param>
        /// <param name="diagnostic">
        /// Diagnostic to fix.
        /// The <see cref="Diagnostic.Id"/> of this diagnostic must be in the set of the <see cref="CodeFixProvider.FixableDiagnosticIds"/> of the associated <see cref="CodeFixProvider"/>.
        /// </param>
        /// <param name="registerCodeFix">Delegate to register a <see cref="CodeAction"/> fixing a subset of diagnostics.</param>
        /// <param name="cancellationToken">Cancellation token.</param>
        /// <exception cref="ArgumentNullException">Throws this exception if any of the arguments is null.</exception>
        public CodeFixContext(TextDocument document, Diagnostic diagnostic, Action<CodeAction, ImmutableArray<Diagnostic>> registerCodeFix, CancellationToken cancellationToken);

        /// <summary>
        /// Creates a code fix context to be passed into <see cref="CodeFixProvider.RegisterCodeFixesAsync(CodeFixContext)"/> method.
        /// </summary>
        /// <param name="document">Text document to fix.</param>
        /// <param name="span">Text span within the <paramref name="document"/> to fix.</param>
        /// <param name="diagnostics">
        /// Diagnostics to fix.
        /// All the diagnostics must have the same <paramref name="span"/>.
        /// Additionally, the <see cref="Diagnostic.Id"/> of each diagnostic must be in the set of the <see cref="CodeFixProvider.FixableDiagnosticIds"/> of the associated <see cref="CodeFixProvider"/>.
        /// </param>
        /// <param name="registerCodeFix">Delegate to register a <see cref="CodeAction"/> fixing a subset of diagnostics.</param>
        /// <param name="cancellationToken">Cancellation token.</param>
        /// <exception cref="ArgumentNullException">Throws this exception if any of the arguments is null.</exception>
        /// <exception cref="ArgumentException">
        /// Throws this exception if the given <paramref name="diagnostics"/> is empty,
        /// has a null element or has an element whose span is not equal to <paramref name="span"/>.
        /// </exception>
        public CodeFixContext(TextDocument document, TextSpan span, ImmutableArray<Diagnostic> diagnostics, Action<CodeAction, ImmutableArray<Diagnostic>> registerCodeFix, CancellationToken cancellationToken);
    }

    public sealed class ExportCodeFixProviderAttribute : ExportAttribute
    {
        /// <summary>
        /// The document kinds for which this provider can provide code fixes. See <see cref="TextDocumentKind"/>.
        /// By default, the provider supports code fixes only for source documents, <see cref="TextDocumentKind.Document"/>.
        /// </summary>
        public TextDocumentKind[] DocumentKinds { get; set; }

        /// <summary>
        /// The document extensions for which this provider can provide code fixes.
        /// By default, this value is null and the document extension is not considered to determine applicability of code fixes.
        /// </summary>
        public string[]? DocumentExtensions { get; set; }
    }
}

Examples

Example code refactoring

[ExportCodeRefactoringProvider(
    LanguageNames.CSharp,
    DocumentKinds = new[] { TextDocumentKind.AdditionalDocument, TextDocumentKind.AnalyzerConfigDocument },
    DocumentExtensions = new[] { ".xaml", ".txt", ".editorconfig", ".globalconfig" })]
[Shared]
internal sealed class MyRefactoring : CodeRefactoringProvider
{
    [ImportingConstructor]
    [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
    public MyRefactoring()
    {
    }

    public override Task ComputeRefactoringsAsync(CodeRefactoringContext context)
    {
        context.RegisterRefactoring(CodeAction.Create("My Refactoring",
            createChangedSolution: async ct =>
            {
                var document = context.TextDocument;
                var text = await document.GetTextAsync(ct).ConfigureAwait(false);
                var newText = SourceText.From(text.ToString() + Environment.NewLine + "# Refactored" + Environment.NewLine);
                if (document.Kind == TextDocumentKind.AdditionalDocument)
                    return document.Project.Solution.WithAdditionalDocumentText(document.Id, newText);
                return document.Project.Solution.WithAnalyzerConfigDocumentText(document.Id, newText);
            }));

        return Task.CompletedTask;
    }
}

Example code fix provider

    [ExportCodeFixProvider(
        LanguageNames.CSharp,
        DocumentKinds = new[] { TextDocumentKind.AdditionalDocument },
        DocumentExtensions = new[] { ".txt", ".xaml" })]
    [Shared]
    internal sealed class AdditionalFileCodeFixer : CodeFixProvider
    {
        [ImportingConstructor]
        [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
        public AdditionalFileCodeFixer()
        {
        }

        public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create("My0001");

        public override Task RegisterCodeFixesAsync(CodeFixContext context)
        {
            context.RegisterCodeFix(CodeAction.Create("My Code Fix",
                createChangedSolution: async ct =>
                {
                    var document = context.TextDocument;
                    var text = await document.GetTextAsync(ct).ConfigureAwait(false);
                    var newText = SourceText.From(text.ToString() + Environment.NewLine + "# Fixed" + Environment.NewLine);
                    return document.Project.Solution.WithAdditionalDocumentText(document.Id, newText);
                }),
                context.Diagnostics[0]);

            return Task.CompletedTask;
        }
    }
333fred commented 2 years ago

API Review

CyrusNajmabadi commented 2 weeks ago

Closing out as there's been no progress on this in years. If this is needed, we need to drive this as a cross team ask with a clear scheduled deliverable date.