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

DoumentState does not import _Imports because C:\pathto\_Imports.razor is not treated the same as c:\pathto\_Imports.razor #10007

Closed
rsandbach opened this issue Feb 29, 2024 · 7 comments · Fixed by dotnet/vscode-csharp#6965
Assignees
Labels
Milestone

Comments

@rsandbach
Copy link

I am getting language service errors using dotnet/vscode-csharp that components do not exist.

I think the issue is that DocumentStates.GetImportsCore() is not bringing in any project _Imports.razor files when conducting document analysis on the current file (App.razor). It does include the default global:: usings.

I can resolve the errors by adding a using statement to App.razor.

In my case, item.PhysicalPath has an uppercase "C" for the drive letter, but all of the dictionary keys (file paths) for _document in projectSnapshot have a lowercase "c". The result of project.GetDocument(item.PhysicalPath) is a null, instead of the actual object import file contents.

While debugging I can confirm that the import files are added by each feature here:

if (feature.GetImports(projectItem) is { } featureImports)
{
importItems.AddRange(featureImports);
}

Since the result of project.GetDocument() is null, they aren't actually added for analysis.

foreach (var item in importItems)
{
if (item.PhysicalPath is null)
{
// This is a default import.
var defaultImport = new ImportDocumentSnapshot(project, item);
imports.Add(defaultImport);
}
else if (project.GetDocument(item.PhysicalPath) is { } import)
{
imports.Add(import);
}
}

The ComponentImportProjectFeature seems to be returning the document paths with an uppercase drive

// We add hierarchical imports second so any default directive imports can be overridden.
imports.AddRange(GetHierarchicalImports(ProjectEngine.FileSystem, projectItem));
return imports;

We can see there is a mismatch between the file paths in documents and import files.

image

The uppercase letter seems to be coming from however HostProject.FilePath is exposed, but not sure if this is a razor or vscode-csharp issue. :)

var rootDirectoryPath = Path.GetDirectoryName(HostProject.FilePath).AssumeNotNull();

@davidwengier
Copy link
Member

I don't think its the uppercase/lowercase letter. project.GetDocument() calls into

if (!_documents.TryGetValue(filePath, out var result) &&
State.Documents.TryGetValue(filePath, out var state))

but _documents uses a special comparer that ignores casing and normalizes slashes to avoid issues.
_documents = new Dictionary<string, DocumentSnapshot>(FilePathNormalizer.Comparer);

Unless you're on a case sensitive operating system? But given the screenshots are from Windows I'm guessing you're not bothering to go through the fun of remote debugging (and if you are, do you want a job, because I don't think I know how to do that myself!)

The "item" in your last sceeenshot is ToolsWeb/_Imports.razor and the projects _documents dictionary contains a ToolsWeb/**Components**/_Imports.razor, so it seems the project really doesn't know about the file being asked for. Why it doesn't know is the real issue here.

@rsandbach
Copy link
Author

For better or worse, my project has an _Import.razor at both of those location. The set of files it is trying to add is correct. dotnet has no issue compiling.

I did start looking at FilePathNormlaize, but came to the conclusion I was out of my depth and needed to some direction on whether the bug is in comparing paths or within the way with vscode delivers the "ProjectPath". I didn't see a test for the comparer for this specific scenario of mismatched dive letter caseing.

I am debugging (on windows) by building razor on my machine and attaching it to the running instance of rzls that the debug instance of vcode launches when you run the "Launch Extension" debug task.

@davidwengier
Copy link
Member

For better or worse, my project has an _Import.razor at both of those location. The set of files it is trying to add is correct. dotnet has no issue compiling.

Yes, no argument there. Your screenshot shows them correctly in importItems, the issue is they're not in _project._documents. Apologies if I somehow implied that there was something wrong with your project, there isn't, it's our understanding of your project that is the issue.

rsandbach added a commit to rsandbach/razor that referenced this issue Mar 2, 2024
@rsandbach
Copy link
Author

@davidwengier Not at all, that wasn't how I took it. Turns out I was tired enough to not even realize that 1 of the 2 import files didn't actually exist in my project. The "temporary API method" GetHierarchicalImports(...) method returns an entry for each folder level between the current ComponentFile's folder and the project directory:

image

Result.razor is nested a few layers deep, only one of the _Imports.razor files actually exists:

image

Even with the change I made to FilePathNormalizer.cs, I am still running into issues with DefaultRazorTagHelperRewritePhase missing TagHelpers from components defined in the project (and/or referenced projects) that lead to false errors/warnings. Maybe I'm fighting multiple issues in trying to debug/test against main, versus the release branch (2.18.16). I noticed there were few changes to the TagHelperRewrite mid February.

image

I can't, for the time being at least, get my head wrapped around how the list of valid TagHelpers is built.

The other odd thing of note is how often the snapshot DocumentCount and _documents.Count is mismatched while RazorEngine is executing the phases. I would expect _documents to stay consistent once the project fully loaded, this could also be bad assumption.

image

Apologies for all the discourse.

@davidwengier
Copy link
Member

I can't, for the time being at least, get my head wrapped around how the list of valid TagHelpers is built.

There are two halves to how Razor works in VS Code: One half runs in Roslyn, and looks through the compilation for all "tag helpers" and serializes that information to the project.razor.vscode.bin file. The code for that is in this repo (eg, here is where we find tag helpers) but it runs in the Roslyn process, so breakpoints won't hit. The dotnet.server.waitForDebugger VS Code setting can be used to debug into the Roslyn process if you're keen. I put "tag helpers" in quotes because the file contains all things that the compiler calls tag helpers, which includes actual MVC tag helpers, and blazor components, directives etc.

The other half is the code in our repo that reads the project.razor.vscode.bin file and hydrates the project snapshots that you see when debugging. If a tag helper is missing in the compiler phase, its important to try to discover if its in the bin file. If not, the problem is on the other side of the fence. If it is in the bin file, then the question would be how did the language server not find out about it. eg. file watchers not firing, state being lost, etc. etc.

how often the snapshot DocumentCount and _documents.Count is mismatched

DocumentCount represents information about the documents that exist in the project (again, info that comes from project.razor.vscode.bin) and _documents is lazily added to only when a GetDocument call is made, which creates the actual DocumentSnapshot. That snapshot is an immutable state of the world including the state of the project at the time of its creation, and the thing that can actually run the compiler etc. The document state on the other hand is lighter (though probably not by much), and can be carried forward over time as things change, if the document it represents doesn't change.

@davidwengier
Copy link
Member

Apologies for all the discourse.

Not at all. If its easier to chat over Discord or email, feel free to reach out. I'm the same alias on the C# Discord (https://discord.gg/csharp) or my email is david dot wengier at microsoft dot com.

@davidwengier
Copy link
Member

I found a (the?) bug!

I must apologise @rsandbach, I was wrong, and case sensitivity absolutely could be the issue. I am investigating an internal bug and it seems that whilst we do specify the converter on the dictionary, and that converter is case insensitive, its GetHashCode method has a bug, and is causing the TryGetValue call to fail before the Equals call ever gets a chance to do its comparison. PR coming soon, very keen to hear if it fixes your issue.

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 a pull request may close this issue.

3 participants