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

Use compile-time solution for solution crawler and EnC #52537

Merged
merged 7 commits into from
Apr 19, 2021

Conversation

tmat
Copy link
Member

@tmat tmat commented Apr 10, 2021

Implements #51265

As a side effect Razor design-time documents are no longer used for reporting diagnostics.

Only impacts .NET 6 projects -- when source generators are used for Razor projects and when Razor LSP editor is enabled (Razor.LSP.Editor feature flag).

Validation build: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/317367

@tmat tmat requested a review from a team as a code owner April 10, 2021 00:35
@tmat
Copy link
Member Author

tmat commented Apr 10, 2021

@NTaylorMullen
Copy link
Contributor

As a side effect Razor design-time documents are no longer used for reporting diagnostics.

Is this always or is this only when running? We should chat about the implications, just because it sounds scary 😄

@tmat
Copy link
Member Author

tmat commented Apr 12, 2021

Always on. That's what I wanted to chat about on Fri.

@tmat
Copy link
Member Author

tmat commented Apr 15, 2021

@mavasani PTAL

@NTaylorMullen
Copy link
Contributor

CTI validated the change here. Consider me signed off!

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Ugh. I applaud you for keeping the hack relatively self-contained.

namespace Microsoft.VisualStudio.LanguageServices
{
/// <summary>
/// Provides a compile-time view of the current workspace solution.
Copy link
Member

Choose a reason for hiding this comment

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

I admit I didn't care for the name "compile-time" because it was ambiguous in precisely this way -- we didn't want to call it "RuntimeSolutionProvider"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow. What is it ambiguous with?

Comment on lines +103 to +105
_lazyCompileTimeSolution = currentDesignTimeSolution
.RemoveAnalyzerConfigDocuments(configIdsToRemove.ToImmutable())
.RemoveDocuments(documentIdsToRemove.ToImmutable());
Copy link
Member

Choose a reason for hiding this comment

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

Note that trying to do a "single" remove operation here rather than just putting it inside the loop over each project state might actually be slower: RemoveDocuments here only works by doing a GroupBy(d => d.ProjectId) and then processing each group individually.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll revisit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Observations:

  1. The equivalent Remove methods on Project call the Solution ones - so removing multiple documents from each project is worse than from the Solution.
  2. Removing each document individually seems to create a lot throw away objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll follow up on this in a separate PR if we conclude there is a better way.

@tmat tmat merged commit d7236c6 into dotnet:main Apr 19, 2021
@ghost ghost added this to the Next milestone Apr 19, 2021
@tmat tmat deleted the CompileTimeSolution branch April 19, 2021 19:51
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants