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

WorkspacePullDiagnosticHandler should handle background analysis scope options for both compiler and analyzer diagnostics #68797

Closed
mavasani opened this issue Jun 27, 2023 · 1 comment · Fixed by #68818
Assignees
Labels
Area-IDE Bug LSP issues related to the roslyn language server protocol implementation Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@mavasani
Copy link
Contributor

mavasani commented Jun 27, 2023

Currently, enabling FSA for either compiler or analyzer diagnostics reports full solution diagnostics for both compiler and analyzers:

async Task AddDocumentsAndProject(Project project, CancellationToken cancellationToken)
{
var fullSolutionAnalysisEnabled = globalOptions.IsFullSolutionAnalysisEnabled(project.Language);
if (!fullSolutionAnalysisEnabled)
return;
var documents = ImmutableArray<TextDocument>.Empty.AddRange(project.Documents).AddRange(project.AdditionalDocuments);

/// <summary>
/// Returns true if the entire solution will be analyzed in the background
/// to compute up-to-date diagnostics for the error list.
/// Note that the background analysis scope for compiler diagnostics and
/// analyzers can be different. If you want to fetch individual values for
/// whether or not full solution analysis is enabled for compiler diagnostics
/// and analyzers, use the other overload
/// <see cref="IsFullSolutionAnalysisEnabled(IGlobalOptionService, string, out bool, out bool)"/>.
/// </summary>
public static bool IsFullSolutionAnalysisEnabled(
this IGlobalOptionService globalOptions,
string language)
=> globalOptions.IsFullSolutionAnalysisEnabled(language, out _, out _);

We need to trigger FSA for selected analyzers (compiler and/or other analyzers) based on both options. For optimal performance, this should be done as a pre-filtering of analyzers to execute rather than post-filtering of diagnostics reported from both compiler and analyzers.

@mavasani mavasani added Bug Area-IDE LSP issues related to the roslyn language server protocol implementation Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. labels Jun 27, 2023
@mavasani mavasani self-assigned this Jun 27, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 27, 2023
@mavasani
Copy link
Contributor Author

FYI @dibarbet - I'll work on a fix.

mavasani added a commit to mavasani/roslyn that referenced this issue Jun 27, 2023
…P/VSCode

Work towards dotnet#68553.
This needs a corresponding change in dotnet/vscode-csharp repo. I verified that this change + the corresponding vscode-csharp change enables FSA support in C# DevKit extension in VSCode.

There also needs to be a follow-up bug fix for dotnet#68797 to treat compiler and analyzer FSA options appropriately.
mavasani added a commit to mavasani/roslyn that referenced this issue Jun 27, 2023
…P/VSCode

Work towards dotnet#68553.
This needs a corresponding change in dotnet/vscode-csharp repo. I verified that this change + the corresponding vscode-csharp change enables FSA support in C# DevKit extension in VSCode.

Needs couple more Roslyn side fixes as a follow-up: dotnet#68797 and dotnet#68798
mavasani added a commit to mavasani/vscode-csharp that referenced this issue Jun 27, 2023
- Adds 2 new background analysis scope options: one for compiler diagnostics and one for analyzer diagnostics
- Both the options allow the same set of values as in VS: Open documents, Entire solution, Current document and None (disabled). Currently, `Current document` option is not yet supported in LSP as we do not have a way to know which is the current active document in LSP server.
- Builds on top of dotnet/roslyn#68799
- Needs couple more Roslyn side changes as a follow-up: dotnet/roslyn#68797 and dotnet/roslyn#68798
mavasani added a commit to mavasani/roslyn that referenced this issue Jun 28, 2023
…oth compiler and analyzer diagnostics

Fixes [AB#1815692](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1815692)
Fixes dotnet#68797

Currently, enabling FSA for either compiler or analyzer diagnostics computes and reports full solution diagnostics for both compiler and analyzers. We need to trigger FSA for selected analyzers (compiler and/or other analyzers) based on both options. For optimal performance, this is done as a pre-filtering of analyzers to execute rather than post-filtering of diagnostics reported from both compiler and analyzers.
arunchndr added a commit to dotnet/vscode-csharp that referenced this issue Jul 10, 2023
* Increase release CI timeout

* Add CI for release branch

* Add eslint and fix warnings

* Implement report issue command for prerelease extension

* Update O# Roslyn and prepare for release

* Switch to non-deprecated action for asset upload

* Upload all VSIX in one step

* Update .github/workflows/release-ci.yml

Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

* Update release-ci.yml

* review feedback

* use vscode-uri

* Add write permissions to job

* Fix options schema

* Explicitly throw if runtimeId is missing

* Use real looking runtimeId in fake

* Use issue reporter command

* Add server path as an option to allow Razor to re-use the runtime resolver

* Fix brace

* More lint

* Update README.md to notify user about .NET Runtime Tool

* Enable full solution background analysis

- Adds 2 new background analysis scope options: one for compiler diagnostics and one for analyzer diagnostics
- Both the options allow the same set of values as in VS: Open documents, Entire solution, Current document and None (disabled). Currently, `Current document` option is not yet supported in LSP as we do not have a way to know which is the current active document in LSP server.
- Builds on top of dotnet/roslyn#68799
- Needs couple more Roslyn side changes as a follow-up: dotnet/roslyn#68797 and dotnet/roslyn#68798

* Remove the un-supported `Current Document` option

* File renames

* Remove TSLint and configure eslint

* Enable remaining rules from tslint and remove

* Run prettier automatic fixer

* address a couple remaining eslint errors

* Remove incorrectly cased folders

* Remove duplicate license header

* Fix options schema after package.json changes

* Fix chai test assertion library usages

* Avoid using 'solution crawler' terminology in the VSCode option name

* Address feedback and fix test

* Revert unintentional changes from merge

* Some more reverts

* Address feedback and also fix linting issues

* Revert "Merged PR 475885: Add support for F5 for csproj and sln" (#5875)

This reverts commit 173be18.

* Update README.md to make runtime extension note numbered

* Bump Razor to 7.0.0-preview.23328.2

* Allow updatePackageDependencies to work with Razor (well, with prerelease packages)

* Add task for updating Razor deps

* Move to newer version of Roslyn LSP server

* Remove unsupported feature

* Update regexes

* small edits to readme

* deleted extra space in readme

* Try a less recent Razor version

* Update roslyn

---------

Co-authored-by: David Barbet <dabarbet@microsoft.com>
Co-authored-by: David Barbet <dibarbet@gmail.com>
Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: leslierichardson95 <LERICH@microsoft.com>
Co-authored-by: Manish Vasani <mavasani@microsoft.com>
Co-authored-by: Andrew Wang <waan@microsoft.com>
Co-authored-by: David Wengier <git@wengier.com>
Co-authored-by: Andrew Hall (METAL) <andrha@microsoft.com>
Co-authored-by: Mika Dumont <midumont@microsoft.com>
Co-authored-by: Mika Dumont <46729679+mikadumont@users.noreply.github.com>
Co-authored-by: David Wengier <david.wengier@microsoft.com>
arunchndr added a commit to dotnet/vscode-csharp that referenced this issue Jul 18, 2023
* Increase release CI timeout

* Add CI for release branch

* Add eslint and fix warnings

* Implement report issue command for prerelease extension

* Update O# Roslyn and prepare for release

* Switch to non-deprecated action for asset upload

* Upload all VSIX in one step

* Update .github/workflows/release-ci.yml

Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

* Update release-ci.yml

* review feedback

* use vscode-uri

* Add write permissions to job

* Fix options schema

* Explicitly throw if runtimeId is missing

* Use real looking runtimeId in fake

* Use issue reporter command

* Add server path as an option to allow Razor to re-use the runtime resolver

* Fix brace

* More lint

* Update README.md to notify user about .NET Runtime Tool

* Enable full solution background analysis

- Adds 2 new background analysis scope options: one for compiler diagnostics and one for analyzer diagnostics
- Both the options allow the same set of values as in VS: Open documents, Entire solution, Current document and None (disabled). Currently, `Current document` option is not yet supported in LSP as we do not have a way to know which is the current active document in LSP server.
- Builds on top of dotnet/roslyn#68799
- Needs couple more Roslyn side changes as a follow-up: dotnet/roslyn#68797 and dotnet/roslyn#68798

* Remove the un-supported `Current Document` option

* File renames

* Remove TSLint and configure eslint

* Enable remaining rules from tslint and remove

* Run prettier automatic fixer

* address a couple remaining eslint errors

* Remove incorrectly cased folders

* Remove duplicate license header

* Fix options schema after package.json changes

* Fix chai test assertion library usages

* Avoid using 'solution crawler' terminology in the VSCode option name

* Address feedback and fix test

* Revert unintentional changes from merge

* Some more reverts

* Address feedback and also fix linting issues

* Revert "Merged PR 475885: Add support for F5 for csproj and sln" (#5875)

This reverts commit 173be18.

* Update README.md to make runtime extension note numbered

* Bump Razor to 7.0.0-preview.23328.2

* Allow updatePackageDependencies to work with Razor (well, with prerelease packages)

* Add task for updating Razor deps

* Move to newer version of Roslyn LSP server

* Remove unsupported feature

* Update regexes

* small edits to readme

* deleted extra space in readme

* Try a less recent Razor version

* Update roslyn

* Remove pwa- prefix from BlazorDebug (#5908)

This PR removes pwa- from msedge and chrome as this was an older debug
type that is now depricated.

See microsoft/vscode-js-debug#1305

* Add determineBrowserType to CSharpExtensionExports (#5911)

This PR exports the
BlazorDebugConfigurationProvider.determineBrowserType method that it
could be used in vscode-dotnettools to help determine the browser the
user wants to use.

* Bump semver from 5.6.0 to 5.7.2

Bumps [semver](https://github.com/npm/node-semver) from 5.6.0 to 5.7.2.
- [Release notes](https://github.com/npm/node-semver/releases)
- [Changelog](https://github.com/npm/node-semver/blob/v5.7.2/CHANGELOG.md)
- [Commits](npm/node-semver@v5.6.0...v5.7.2)

---
updated-dependencies:
- dependency-name: semver
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update debugger packages to 2.0.2 (#5914)

Co-authored-by: Isadora Rodopoulos <isgarcia@microsoft.com>

* Fix static variables used in determineBrowserType (#5919)

This PR fixes the 'this' to be 'BlazorDebugConfigurationProvider' for
the new static variables from #5911

* Add call to setupTelemetryEnvironmentAsync (#5922)

* Add call to setupTelemetryEnvironmentAsync

* Make optional

* Update Roslyn server (#5925)

* Update Roslyn server

* Update package.json

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: David Barbet <dabarbet@microsoft.com>
Co-authored-by: David Barbet <dibarbet@gmail.com>
Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: leslierichardson95 <LERICH@microsoft.com>
Co-authored-by: Manish Vasani <mavasani@microsoft.com>
Co-authored-by: Andrew Wang <waan@microsoft.com>
Co-authored-by: David Wengier <git@wengier.com>
Co-authored-by: Andrew Hall (METAL) <andrha@microsoft.com>
Co-authored-by: Mika Dumont <midumont@microsoft.com>
Co-authored-by: Mika Dumont <46729679+mikadumont@users.noreply.github.com>
Co-authored-by: David Wengier <david.wengier@microsoft.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Isadora <isaecia@gmail.com>
Co-authored-by: Isadora Rodopoulos <isgarcia@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Bug LSP issues related to the roslyn language server protocol implementation Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
1 participant