Change RazorProjectInfo to use HostProject and HostDocument #11030

DustinCampbell · 2024-10-16T00:35:06Z

This pushes the HostProject and HostDocument types down to MS.ANC.Razor.ProjectEngineHost and uses them in RazorProjectInfo. RazorProjectInfo tracks all of the same information as a HostProject, so it makes sense to bring them together. In addition, RazorProjectInfo exposed an ImmutableArray<DocumentSnapshotHandle> but DocumentSnapshotHandle is identical to HostDocument. So, that's been unified as well. In addition, I've gone ahead and simplified some of the code in RazorProjectService to take advantage of HostProject and HostDocument.

These changes require an increment to the serialization format versions. So, this will need to make its way to VS Code as well. (Although, since we don't save bin files to disk anymore, is that strictly necessary for VS Code?)

HostDocument is a record and now it correctly implements equality

It turns out that DocumentSnapshotHandle and HostDocument are essentially identical, so we can unify on HostDocument.

We should check the version before verifying the array header.

ryzngard · 2024-10-16T01:43:27Z

(Although, since we don't save bin files to disk anymore, is that strictly necessary for VS Code?)

Correct. This is not a breaking change for VS but will block VS Code until a dual insertion. I'll ping you offline for timelines

ryzngard

I'd like to get #11015 in before this goes in to facilitate making the snap for prerelease this week in VS Code.

ryzngard · 2024-10-16T02:13:12Z

...re.Razor.ProjectEngineHost/Serialization/MessagePack/Formatters/RazorProjectInfoFormatter.cs


-        var version = reader.ReadInt32();
+        reader.ReadArrayHeaderAndVerify(4);


Why not have this above the verify, and just have the reader move in VerifyVersionNumber since it's already passed by ref?

I guess because that felt a bit more awkward to me when I wrote this. The problem is that we need to verify the array header length after the version check. Otherwise, we may never get to the version check before we blow up because the array size changed if a new version happens to add or remove RazorProjectInfo field. I felt that it was better to have a separate method to verify the version by peeking ahead and then leaving this method to do the real data read.

since it's already passed by ref?

MessagePackReader really should be passed by ref in some way. It's a ref struct and a ref has already been taken for it. I'll update VerifyVersionNumber to ref readonly.

ryzngard · 2024-10-16T02:14:02Z

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/RazorProjectInfoFactory.cs

+        {
+            return null;
+        }
+
        var projectPath = Path.GetDirectoryName(project.FilePath);
        if (projectPath is null)


is it possible for this to be null if the project.FilePath is checked above?

I don't believe so, but it's annotated to be string? on .NET without a [NotNullIfNotNull] attribute.

Actually, it turns out that it is possible for this to return null if project.FilePath is not null. Project.GetDirectoryName(...) returns null if the file path pass to it is null OR if it is a root without a directory name, such as / or C:.

ryzngard · 2024-10-16T02:15:50Z

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/RazorProjectInfoFactory.cs

-        defaultNamespace = rootNamespace ?? "ASP"; // TODO: Source generator does this. Do we want it?
-
-        return razorConfiguration;
+        return (configuration, rootNamespace ?? "ASP"); // TODO: Source generator does this. Do we want it?


I think we can just remove the TODO from this. It's what we've been doing for a while

Or honestly just leave it null

Or honestly just leave it null

This is how subtle bugs creep in! 😛

Yeah, I'm not comfortable making an intentional behavior change in this method when I was just making a small refactoring.

Fair enough. Can we remove the null check on line 83 at least?

and change the return tuple to (RazorConfiguration configuration, string rootNamespace)

For sure! I had marked string rootNamespace as nullable before I even looked at the implementation, since I know that HostProject.RootNamespace is nullable. Clearly it can't be null though.

davidwengier · 2024-10-16T02:56:45Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectSystem/RazorProjectService.cs

    {
-        var normalizedPath = FilePathNormalizer.Normalize(filePath);


This worries me. If previous RazorProjectService would ensure normalized paths, but HostProject does not, I think we'll end up [re-]introducing subtle bugs. We've definitely seen scenarios where CPS and Roslyn don't agree on what a path looks like, with both systems even changing for a specific project during load.

The simplest fix here would be to have HostProject normalize the path in its constructor. That seems to fit well with the rest of the PR, centralizing our APIs on a single type that has all the necessary logic.

The other option would be to have HostProject use the normalizing comparer, and doing a pass through everything that uses HostProject.FilePath or IProjectSnapshot.FilePath for comparisons and making them use the normalizing comparer too.

As an example, I think this could now break:
https://github.com/dotnet/razor/blob/main/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectSnapshotManager.cs#L160

That gets called from our CPS data flow receiver, and if it has a project in it that comes from the fallback project manager via Roslyn, it might not see it as the same project.

I am aware that "always call the file path normalizer" is a little bit of cargo cult programming, but I'm not sure I trust our tests to provide coverage and confidence that we can move away from it. Thankfully the Good Ship Cohosting should save us :D

I love that you spotted this! It was the only place that I was aware of removing a call to FilePathNormalizer.Normalize(...). I fully agree that the whole file-normalization business is super error prone, and I think HostProject is great place to unify some of that. I'll take a closer look at that for this pull request.

Thankfully the Good Ship Cohosting should save us :D

goodshipcohosting.com appears to be available for purchase.

FWIW, as I've started looking at this, I'm finding that it's unclear whether HostProject.FIlePath should be normalized or not. Currently, it looks like the VS ProjectSnapshotManager does not contain normalized project paths but the language server ProjectSnapshotManager does. Since CPS only accesses the VS ProjectSnapshotManager, I can't see how it would be affected by the language server's ProjectSnapshotManager suddenly not having normalized file paths. This looks to me like a mess and I'm not sure that I know how it's supposed to work.

The VS side not having normalized paths is not too surprising I guess, since it doesn't have a RazorProjectService, so perhaps my worry about that specific location wasn't valid (or is already broken 😁). I still worry about changing the language server side to not be normalized, but perhaps its worth rolling the die for a preview 1?

On the other hand, I don't think normalizing everywhere is likely to introduce issues, the only downside really is reinforcing the cargo cult.

perhaps its worth rolling the die for a preview 1

That doesn't feel good to me. I want to get to the bottom of this. 😄

davidwengier · 2024-10-16T03:11:50Z

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Utilities/RazorProjectInfoFactory.cs

-        defaultNamespace = rootNamespace ?? "ASP"; // TODO: Source generator does this. Do we want it?
-
-        return razorConfiguration;
+        return (configuration, rootNamespace ?? "ASP"); // TODO: Source generator does this. Do we want it?


Or honestly just leave it null

This is how subtle bugs creep in! 😛

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/TestRazorProjectService.cs

jjonescz · 2024-10-17T09:26:21Z

src/Shared/Microsoft.AspNetCore.Razor.Serialization.Json/ObjectReaders.cs

btw, why these files live in Shared? they are not used by the razor compiler, are they?

Before we switched to message pack, these were compiled into both VS and the Razor language server. Today, I think there's a desire for the compiler to use them rather than a different TagHelperDescriptor JSON converter in tests. Seems reasonable to share that code someday.

DustinCampbell added 14 commits October 15, 2024 13:28

Remove HostDocumentComparer

3218628

HostDocument is a record and now it correctly implements equality

Move HostDocument to MS.ANC.Razor.ProjectEngineHost

82a6d14

Add JSON and MessagePack serialization for HostDocument

f123af0

Replace DocumentSnapshotHandle with HostDocument

67168aa

It turns out that DocumentSnapshotHandle and HostDocument are essentially identical, so we can unify on HostDocument.

Remove DocumentSnapshotHandle

0b85574

Clean up RazorProjectService.TestAccessor

871dac6

Move HostProject to MS.ANC.Razor.ProjectEngineHost

b1bfaad

Add JSON and MessagePack serialization for HostProject

b6b95ac

Use HostProject within RazorProjectInfo

196ff15

Remame RazorProjectInfo.ProjectKey to Key

78b6175

Use HostProject in RazorProjectService

7bf4ca7

Increment SerializationFormat version numbers

c3a7f90

Tweak version check in MessagePack

e89fbfd

We should check the version before verifying the array header.

Update serialized JSON test files

76d210c

DustinCampbell requested review from a team as code owners October 16, 2024 00:35

ryzngard approved these changes Oct 16, 2024

View reviewed changes

davidwengier reviewed Oct 16, 2024

View reviewed changes

DustinCampbell added 3 commits October 16, 2024 12:23

Update parameter to ref readonly

3357d6c

Remove unused local

473b6d4

Make rootNamespace non-nullable

9e66c0c

jjonescz reviewed Oct 17, 2024

View reviewed changes

DustinCampbell added 4 commits October 22, 2024 14:21

Merge branch 'main' into hostdocument-ftw

dbfd530

Merge branch 'main' into hostdocument-ftw

ecbdde1

Merge branch 'main' into hostdocument-ftw

8de93cb

Merge branch 'main' into hostdocument-ftw

0de2fa8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change RazorProjectInfo to use HostProject and HostDocument #11030

Change RazorProjectInfo to use HostProject and HostDocument #11030

DustinCampbell commented Oct 16, 2024

ryzngard commented Oct 16, 2024

ryzngard left a comment

ryzngard Oct 16, 2024

DustinCampbell Oct 16, 2024

ryzngard Oct 16, 2024

DustinCampbell Oct 16, 2024

DustinCampbell Oct 23, 2024

ryzngard Oct 16, 2024

ryzngard Oct 16, 2024

davidwengier Oct 16, 2024

DustinCampbell Oct 16, 2024

ryzngard Oct 16, 2024

ryzngard Oct 16, 2024

DustinCampbell Oct 16, 2024 •

edited

Loading

davidwengier Oct 16, 2024

DustinCampbell Oct 16, 2024

DustinCampbell Oct 23, 2024

davidwengier Oct 23, 2024

DustinCampbell Oct 23, 2024

davidwengier Oct 16, 2024

jjonescz Oct 17, 2024

DustinCampbell Oct 22, 2024


		var version = reader.ReadInt32();
		reader.ReadArrayHeaderAndVerify(4);

		{
		var normalizedPath = FilePathNormalizer.Normalize(filePath);

Change RazorProjectInfo to use HostProject and HostDocument #11030

Are you sure you want to change the base?

Change RazorProjectInfo to use HostProject and HostDocument #11030

Conversation

DustinCampbell commented Oct 16, 2024

ryzngard commented Oct 16, 2024

ryzngard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell Oct 16, 2024 •

edited

Loading