Change RazorProjectInfo to use HostProject and HostDocument #11030
Conversation
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.
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 |
|
|
||
| var version = reader.ReadInt32(); | ||
| reader.ReadArrayHeaderAndVerify(4); |
There was a problem hiding this comment.
Why not have this above the verify, and just have the reader move in VerifyVersionNumber since it's already passed by ref?
There was a problem hiding this comment.
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.
| { | ||
| return null; | ||
| } | ||
|
|
||
| var projectPath = Path.GetDirectoryName(project.FilePath); | ||
| if (projectPath is null) |
There was a problem hiding this comment.
is it possible for this to be null if the project.FilePath is checked above?
There was a problem hiding this comment.
I don't believe so, but it's annotated to be string? on .NET without a [NotNullIfNotNull] attribute.
| 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? |
There was a problem hiding this comment.
I think we can just remove the TODO from this. It's what we've been doing for a while
There was a problem hiding this comment.
Or honestly just leave it null
There was a problem hiding this comment.
Or honestly just leave it null
This is how subtle bugs creep in! 😛
There was a problem hiding this comment.
Yeah, I'm not comfortable making an intentional behavior change in this method when I was just making a small refactoring.
There was a problem hiding this comment.
Fair enough. Can we remove the null check on line 83 at least?
There was a problem hiding this comment.
and change the return tuple to (RazorConfiguration configuration, string rootNamespace)
There was a problem hiding this comment.
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.
| { | ||
| var normalizedPath = FilePathNormalizer.Normalize(filePath); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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? |
There was a problem hiding this comment.
Or honestly just leave it null
This is how subtle bugs creep in! 😛
src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/TestRazorProjectService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
btw, why these files live in Shared? they are not used by the razor compiler, are they?
This pushes the
HostProjectandHostDocumenttypes down to MS.ANC.Razor.ProjectEngineHost and uses them inRazorProjectInfo.RazorProjectInfotracks all of the same information as aHostProject, so it makes sense to bring them together. In addition,RazorProjectInfoexposed anImmutableArray<DocumentSnapshotHandle>butDocumentSnapshotHandleis identical toHostDocument. So, that's been unified as well. In addition, I've gone ahead and simplified some of the code inRazorProjectServiceto take advantage ofHostProjectandHostDocument.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?)