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.

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

XAML: Provide better support for AdditionalFiles analyzers #44131

Closed
mgoertz-msft opened this issue May 11, 2020 · 15 comments
Closed

XAML: Provide better support for AdditionalFiles analyzers #44131

mgoertz-msft opened this issue May 11, 2020 · 15 comments
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Milestone

Comments

@mgoertz-msft
Copy link
Contributor

Problem
Currently the only way to create analyzers for additional files is via RegisterCompilationStartAction for example and accessing them via AnalyzerOptions.AdditionalFiles. Unfortunately this means that there is no way for analyzer optimizations, such as running only for a currently open additional document for example. This places the burden of any potential optimizations on each analyzer implementation and could result in performance problems.

Justification
With the internal DocumentDiagnosticAnalyzers being frowned upon and eventually going away an official replacement solution for additional files is needed to support analyzers for non-core Roslyn languages such as XAML.

Ask
Provide support for something like an AdditionalDocumentDiagnosticAnalyzer for example that can be optimized for additional files scenarios.

@jinujoseph
Copy link
Contributor

cc @mavasani

@jinujoseph jinujoseph added this to the Backlog milestone May 12, 2020
@sharwell

This comment has been minimized.

@mavasani
Copy link
Contributor

@sharwell I believe the request here is to support first class analysis callbacks for additional files, similar to source files. This will allow analyzers that analyze additional files to report diagnostics on opened additional files with the default analysis scope (Open documents). Additionally, it would be easier for third parties to write their own suggested actions (code fixes) for diagnostics in additional files.

@mgoertz-msft Can you confirm if this correctly represents your feature request?

@sharwell
Copy link
Member

@mavasani That makes sense, thanks!

@mgoertz-msft
Copy link
Contributor Author

@mavasani Yes, that's exactly the purpose of this request so we can represent XAML documents in additional files and run analyzers on XAML documents.

@sharwell
Copy link
Member

@mavasani Can you create an API proposal for review derived from the current syntax tree analysis APIs?

@mavasani
Copy link
Contributor

@sharwell Sure, I will create and post an API proposal. We would need to have a design review with the compiler team before working on the implementation.

@mavasani mavasani self-assigned this May 12, 2020
@mavasani mavasani added the Concept-API This issue involves adding, removing, clarification, or modification of an API. label May 12, 2020
@mavasani
Copy link
Contributor

Proposal

Design

Design API surface for additional file actions analogous to SyntaxTree actions:

  1. Add a new AdditionalFileAnalysisContext, analogous to SyntaxTreeAnalysisContext:
    public struct AdditionalFileAnalysisContext
    {
        public AdditionalText File { get; }
        public AnalyzerOptions Options { get; }
        public CancellationToken CancellationToken { get; }
        public Compilation Compilation { get; }

        public void ReportDiagnostic(Diagnostic diagnostic);
    }
  1. Add a new RegisterAdditionalFileAction API on all relevant analysis contexts where RegisterSyntaxTreeAction is provided:
        public void RegisterAdditionalFileAction(Action<AdditionalFileAnalysisContext> action);

Implementation

Identical analyzer action semantics to syntax tree action callbacks. From https://github.com/dotnet/roslyn/blob/master/docs/analyzers/Analyzer%20Actions%20Semantics.md:

A syntax tree action is invoked once per source document. A syntax tree action can be expected to be invoked as early as possible after a document is parsed, but this is not guaranteed.

Open question

Should we expose a similar API for analyzer config files? This will allow us to write analyzers for mistakes in editorconfig files, which show up on opening these files. For example, we can finally address #19055 if RegisterAnalyzerConfigFileAction were to be added.

@mavasani mavasani added the Need Design Review The end user experience design needs to be reviewed and approved. label May 14, 2020
@mavasani
Copy link
Contributor

@sharwell @vatsalyaagrawal We can bring the above design proposal for discussion at the next IDE design meeting if we can have a member from compiler team present at the discussion.

@mavasani
Copy link
Contributor

@mgoertz-msft can you confirm the above API would meet your request?

@mgoertz-msft
Copy link
Contributor Author

mgoertz-msft commented May 14, 2020

@mavasani Looks like that should work. I wonder if instead of public AdditionalText File { get; } it should be public AdditionalDocument File { get; }. What do you think? Would that be possible?

@sharwell
Copy link
Member

AdditionalDocument is a workspace API. It does not exist in compilation/analyzer scenarios.

mavasani added a commit to mavasani/roslyn that referenced this issue Jun 11, 2020
Addresses dotnet#44131
The added analyzer APIs are similar to the existing APIs for syntax tree callback. This commit adds the compiler APIs + tests

Future enhancement: Add analyzer action for reporting diagnostics in analyzer config files
@mavasani mavasani modified the milestones: Backlog, 16.8 Jun 11, 2020
@mavasani
Copy link
Contributor

The API proposal was approved at the design meeting.

@mavasani mavasani removed the Need Design Review The end user experience design needs to be reviewed and approved. label Jun 11, 2020
@mavasani mavasani added the 4 - In Review A fix for the issue is submitted for review. label Jun 11, 2020
mavasani added a commit to mavasani/roslyn that referenced this issue Jun 22, 2020
Addresses dotnet#44131
The added analyzer APIs are similar to the existing APIs for syntax tree callback. This commit adds the compiler APIs + tests

Future enhancement: Add analyzer action for reporting diagnostics in analyzer config files
@mavasani
Copy link
Contributor

@mgoertz-msft FYI that your original feature request has now been implemented for 16.8 P1 with #45076. I will keep this issue open to track exposing a similar API for analyzer config files.

@jinujoseph jinujoseph modified the milestones: 16.8, 16.9 Sep 11, 2020
@jinujoseph jinujoseph modified the milestones: 16.9, 16.10 Mar 28, 2021
@jinujoseph jinujoseph modified the milestones: 16.10, 17.0 Jul 16, 2021
@jinujoseph jinujoseph modified the milestones: 17.0, 17.1 Dec 10, 2021
@mavasani mavasani modified the milestones: 17.1, Backlog Jan 10, 2022
mavasani added a commit to mavasani/roslyn that referenced this issue Sep 22, 2022
Closes dotnet#44131

dotnet#45076 added support for reporting analyzer diagnostics in additional files. This PR addresses pending work from dotnet#44131 (comment) by adding similar support for editorconfig/globalconfig files in the project.

There are two primary public API additions
mavasani added a commit to mavasani/roslyn that referenced this issue Sep 22, 2022
Closes dotnet#44131

Primary public API additions:

1. Expose `AnalyzerConfigFiles` as text files from `AnayzerOptions`. Currently, we only expose `AdditionalFiles` from `AnalyzerOptions`.
2. Add `RegisterAnalyzerConfigFileAction` to `AnalysisContext` and `CompilationStartAnalysisContext` to report diagnostics in editorconfig files. `AnalyzerConfigFileAnalysisContext` is the new context type for this callback.
3. Expose `AnalyzerConfigFileDiagnostics` from `AnalysisResult` for analyzer hosts to fetch analyzer diagnostics reported in editorconfig files
@mavasani
Copy link
Contributor

I will keep this issue open to track exposing a similar API for analyzer config files.

Now tracked with #64213

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Projects
Archived in project
Development

No branches or pull requests

4 participants