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

Fix problematic things on IProjectSnapshot #9871

Merged
merged 8 commits into from
Jan 30, 2024

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Jan 29, 2024

Part of #9519

As per our meeting the other day, there are a few things on IProjectSnapshot that are problematic. This PR resolves all of the issues in one of two ways:

  • For Configuration, GetProjectEngine() and ProjectWorkspaceState the cohost implementations now simply throw
    • There was some usage of these inside cohosting, because it re-used methods on DocumentState to do jobs that the source gen will eventually take over. To make this clearer, I modified the helper methods so they took the underlying data, rather than pulling it from IProjectSnapshot directly
  • Changes the TagHelpers property into a GetTagHelpersAsync(CancellationToken) method
    • Because of the virality of async, I recommend reviewing this PR commit-at-a-time, just so this last commit is seen separately, for your sanity.
    • There were only two places where moving to async would have been very very annoying, so I added a GetTagHelpersSynchronously() extension method that validates that its not called from cohosting. It's only called from legacy editor code.

Note that this isn't the proposed solution in the aforementioned meeting, that change can still come later. I was hoping to take that idea and use DI to enforce something better than just a runtime check, but it turned out that wasn't possible because some MEF services need these properties in non-cohosting. This PR at least introduces the runtime check, so we can continue moving forward.

Integration tests are running and should show up in in the checks list.

@davidwengier davidwengier requested a review from a team as a code owner January 29, 2024 05:34
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Looks good!

There are number of new Task returning methods in this PR. For each of these, consider whether ValueTask would be better to avoid extra allocations.

@@ -30,7 +32,7 @@ internal interface IProjectSnapshot
string DisplayName { get; }
VersionStamp Version { get; }
LanguageVersion CSharpLanguageVersion { get; }
ImmutableArray<TagHelperDescriptor> TagHelpers { get; }
Task<ImmutableArray<TagHelperDescriptor>> GetTagHelpersAsync(CancellationToken cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

Consider ValueTask to avoid an extra allocation.

@@ -33,4 +37,12 @@ public static RazorProjectInfo ToRazorProjectInfo(this IProjectSnapshot project,
projectWorkspaceState: project.ProjectWorkspaceState,
documents: documents.DrainToImmutable());
}

public static ImmutableArray<TagHelperDescriptor> GetTagHelpersSynchronously(this IProjectSnapshot projectSnapshot)
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind creating an issue to clean this up? I'm about to work on VisualStudioDocumentTracker as part of the MEF clean up, and I'd be happy to get rid of this at the same time.

@davidwengier davidwengier merged commit ac68bf7 into main Jan 30, 2024
12 checks passed
@davidwengier davidwengier deleted the StopCallingBadThingsInCohost branch January 30, 2024 04:17
@ghost ghost added this to the Next milestone Jan 30, 2024
@jjonescz jjonescz modified the milestones: Next, 17.10 P2 Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants