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

Enable EnC diagnostics in LSP #54500

Merged

Conversation

dibarbet
Copy link
Member

Adds support for EnC diagnostics via LSP

enc_diagnostics_lsp

Note that there seems to still be an issue on the Razor side with squiggle color (I verified we are returning the appropriate EnC tag for Razor diagnostics) cc @NTaylorMullen
razor_enc_diagnostics_lsp

@dibarbet dibarbet added Area-IDE LSP issues related to the roslyn language server protocol implementation labels Jun 30, 2021
@dibarbet dibarbet requested a review from tmat June 30, 2021 20:12
@dibarbet dibarbet requested review from a team as code owners June 30, 2021 20:12
@NTaylorMullen
Copy link
Contributor

Note that there seems to still be an issue on the Razor side with squiggle color (I verified we are returning the appropriate EnC tag for Razor diagnostics) cc @NTaylorMullen

Thought that might be the case. We're probably serializing things differently

return _lazyCompileTimeSolution;

// Only re-use when the lazy solution is from the same branch as the requested solution.
if (_lazyCompileTimeSolution.BranchId == designTimeSolution.BranchId)
Copy link
Member

Choose a reason for hiding this comment

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

_lazyCompileTimeSolution

Should we cache 2 snapshots (branches)?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah that seems reasonable, currently it ends up flip-flopping depending on how the requests come in. I'll have one cached snapshot for the primary branch and another item cached for a different branch (note that the branchId for the LSP solution changes on every text change since we fork from the workspace each time)

Copy link
Member Author

Choose a reason for hiding this comment

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

@tmat updated to include a cache for the primary branch as well as another snapshot cache for forked requests

@dibarbet dibarbet requested a review from tmat July 1, 2021 00:05
@dibarbet dibarbet changed the base branch from main-vs-deps to release/dev17.0-vs-deps July 1, 2021 18:37
@dibarbet dibarbet force-pushed the enc_lsp_diagnostics branch 2 times, most recently from 5f56712 to 96c8859 Compare July 2, 2021 19:28
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@JoeRobich
Copy link
Member

Merging to unblock insertion.

@JoeRobich JoeRobich merged commit d16c8bd into dotnet:release/dev17.0-vs-deps Jul 6, 2021
@dibarbet dibarbet deleted the enc_lsp_diagnostics branch July 6, 2021 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved to merge Area-IDE LSP issues related to the roslyn language server protocol implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants