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

[WIP] Get generated docs from the generator #8678

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

chsienki
Copy link
Contributor

Heres a (very) WIP PR for the work I've been doing to get the docs from the generator. It builds on top of a couple of other PRs, but should give you a feel for the basic shape.

Conceptually it's not too complicated, there are two new services, one InProc, one LSP that can talk to the matching Roslyn endpoint and pull out the docs.

@chsienki
Copy link
Contributor Author

@chsienki
Copy link
Contributor Author

chsienki commented May 10, 2023

Theres still a lot of 'vestigial' code in here from different approaches that I took, so I'll try and keep cleaning it up a bit as I go. I'll probably re-base and open a new PR for actual review, but feel free to leave any comments you have as you see them.

RequestedOutput = name,
};

var response = await _notifier.SendRequestAsync<HostOutputRequest, HostOutputResponse>(RazorLanguageServerCustomMessageTargets.RazorHostOutputsEndpointName, request, CancellationToken.None);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, there needs to be TypeScript changes for any use of ClientNotifierService (if you expect it to work in VS Code), so either we need to do that so we don't break OmniSharp, or we have to make all of this behaviour optional and leave omnisharp using the old code. Obviously doesn't need an answer now, but hopefully one of those two things is on the todo list

namespace Microsoft.CodeAnalysis.Razor.Workspaces;

[ExportWorkspaceServiceFactory(typeof(IRazorGeneratedDocumentProvider), ServiceLayer.Default)]
internal class InProcRazorGeneratedDocumentProviderFactory : IWorkspaceServiceFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this fella and what does he do? It says InProc and is a roslyn service, but then is using LSP to call into Roslyn, which seems at odds with that. Can't it just pull generated files out of the Project?

UPDATE: Wait, maybe this isn't using LSP. Is RazorHostOutputHandler a Roslyn thing that we're accessing over IVT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly that. There's just a static method that we can call via IVT that pulls the docs out of the generator workspace. I'll separate out the types to make it clearer.

{
public override DirectiveTokenDescriptor? ReadJson(JsonReader reader, Type objectType, DirectiveTokenDescriptor? existingValue, bool hasExistingValue, JsonSerializer serializer)
{
var obj = JObject.Load(reader);
Copy link
Member

Choose a reason for hiding this comment

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

I know this is a prototype, but these serializers definitely need to be looked at for performance before putting them in the product. I'm currently rewriting the existing JSON.NET converters in the tooling layer because they do horrible inefficient things like using JObject within a converter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right now the serialization is not great. My plan was to get it to point where we can profile it, then start improving, although it sounds like you're already re-writing some of them anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it'd be good to get us to a place where the serialization logic can just go into Shared.Utilities. TagHelperDescriptor serialization currently represents a large amount of memory allocation in Razor scenarios. So, there's already a fair amount of analysis that's been done around what's good/bad with JSON.NET. #8675 rewrites the existing JSON.NET converters in the tooling layer and avoids changing behavior, since we still have to read the project.razor.json files that are already in the wild. Once this goes in, I'll introduce versioning into serialization so that we can use a much tighter format.

See #8113 for some of the details.

private readonly int rewritePhaseIndex = -1;

public SourceGeneratorProjectEngine(DefaultRazorProjectEngine projectEngine)
: base(projectEngine.Configuration, projectEngine.Engine, projectEngine.FileSystem, projectEngine.ProjectFeatures)
Copy link
Member

Choose a reason for hiding this comment

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

Tabs vs. Spaces?

@@ -218,6 +218,7 @@ private static void WriteAllowedChildTags(JsonWriter writer, AllowedChildTagDesc
writer.WriteEndObject();
}

// TODO: looks like we have bound attributes not part of tag helpers. We need to extract this to be an actual type converter and call into it here instead.
private static void WriteBoundAttribute(JsonWriter writer, BoundAttributeDescriptor boundAttribute, JsonSerializer serializer)
Copy link
Member

Choose a reason for hiding this comment

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

This is all being rewritten. 😄

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.

4 participants