Skip to content

Conversation

@mavasani
Copy link
Contributor

@mavasani mavasani commented Jun 27, 2023

Closes dotnet/roslyn#68553

- 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
package.json Outdated
Comment on lines 1280 to 1283
"activeFile",
"openFiles",
"fullSolution",
"none"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json Outdated
Comment on lines 1287 to 1292
"Current document (not yet supported)",
"Open documents",
"Entire solution",
"None"
],
"description": "Run background code analysis for:",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Descriptions match VS side descriptions in Tools Options dialog

@CyrusNajmabadi
Copy link
Member

I agree that we should not talk about solution crawling.

@mavasani mavasani force-pushed the BackgroundAnalysisOptions branch from d439e77 to d36fcd0 Compare June 28, 2023 07:44
@mavasani
Copy link
Contributor Author

I agree that we should not talk about solution crawling.

This has now been fixed.

@dibarbet
Copy link
Member

@Cosifne if this merges without the server side change to enable the option, will that cause errors?

@arunchndr
Copy link
Contributor

@Cosifne if this merges without the server side change to enable the option, will that cause errors?

Good point. Here is the server version that needs to be taken in - 4.7.0-3.23328.2.

@Cosifne
Copy link
Member

Cosifne commented Jun 28, 2023

@Cosifne if this merges without the server side change to enable the option, will that cause errors?

If client change goes in before server change.
It will act like the user can specify a value in the client, but there is no effect.

If server change goes in before client change.
Server would keep asking the client the value of the option, but client would always return string.Empty to server.

@mavasani
Copy link
Contributor Author

@Cosifne @dibarbet are we okay to merge this?

@dibarbet
Copy link
Member

@mavasani since it looks like the main roslyn side PRs are merged, would you mind updating the roslyn version as well in the PR, so that it all works end to end? Instructions here - https://github.com/dotnet/vscode-csharp/blob/main/CONTRIBUTING.md#updating-the-roslyn-server-version (let me know if they're good instructions)

No worries if not, Shen mentioned nothing should break if the roslyn version isn't updated, and I can update later this week

@mavasani
Copy link
Contributor Author

mavasani commented Jun 29, 2023

@mavasani since it looks like the main roslyn side PRs are merged, would you mind updating the roslyn version as well in the PR, so that it all works end to end? Instructions here - https://github.com/dotnet/vscode-csharp/blob/main/CONTRIBUTING.md#updating-the-roslyn-server-version (let me know if they're good instructions)

No worries if not, Shen mentioned nothing should break if the roslyn version isn't updated, and I can update later this week

Thanks Dave. I tried those steps but gulp installDependencies --interactive still fails to find the new package (I tried a signed build from 24 hours ago, still no luck). I did perform the authentication step, which succeeded but still unable to find the package:

C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\NuGet.targets(156,5): error : Failed to download package 'Microso
ft.CodeAnalysis.LanguageServer.4.7.0-3.23328.5' from 'https://pkgs.dev.azure.com/azure-public/3ccf6661-f8ce-4e8a-bb2e-e
ff943ddd3c7/_packaging/36a629e1-6c5b-4bcd-aa2e-6018802d6b99/nuget/v3/flat2/microsoft.codeanalysis.languageserver/4.7.0-
3.23328.5/microsoft.codeanalysis.languageserver.4.7.0-3.23328.5.nupkg'. [c:\vscode-csharp\server\ServerDownload.csproj]
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\NuGet.targets(156,5): error : Response status code does not indic
ate success: 401 (Unauthorized - No local versions of package 'microsoft.codeanalysis.languageserver'; please provide a
uthentication to access versions from upstream that have not yet been saved to your feed. (DevOps Activity ID: DA658997
-7077-4FEC-BA50-3469AEDE9730)). [c:\vscode-csharp\server\ServerDownload.csproj]
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\NuGet.targets(156,5): error : Central Directory corrupt. [c:\vsco
de-csharp\server\ServerDownload.csproj]
C:\Program Files\dotnet\sdk\8.0.100-preview.5.23303.2\NuGet.targets(156,5): error :   The parameter is incorrect. : 'c:
\vscode-csharp\out\.nuget\microsoft.codeanalysis.languageserver\4.7.0-3.23328.5\fzqkzsy1.yj1' [c:\vscode-csharp\server\
ServerDownload.csproj]
[08:33:12] 'installDependencies' errored after 1.3 min
[08:33:12] Error: Failed to download nuget package microsoft.codeanalysis.languageserver.4.7.0-3.23328.5
    at ChildProcess.<anonymous> (c:\vscode-csharp\tasks\offlinePackagingTasks.ts:163:23)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:552:15)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)
    at Process.callbackTrampoline (node:internal/async_hooks:130:17)

@mavasani mavasani merged commit 1c11b4e into dotnet:main Jun 29, 2023
@mavasani mavasani deleted the BackgroundAnalysisOptions branch June 29, 2023 04:58
"enumDescriptions": [
"Open documents",
"Entire solution",
"None"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would have loved the descriptions and the enum member names to match :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These match the enum field names and user facing descriptions in VS. I think it would be reasonable to change the enum field names in Roslyn, and once the new values flow into this repo, we can rename the field names here as well. However, these are just internal implementation details, so not sure if it critical to make these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SUGGESTION] Solution-wide analysis #104

5 participants