Improve PDB source document project handling#59643
Conversation
This mapping is mostly useless, because there are only two providers, and we know which ones are in use from the Lazy<T>s anyway. In the case of Source Link its actively harmful, as there can be multiple files downloaded via SourceLink, so we want to give the provider a chance to find each one it downloaded, not just the first one that the user opened in Visual Studio.
| var provider = exportProvider.GetExportedValues<IMetadataAsSourceFileProvider>().OfType<PdbSourceDocumentMetadataAsSourceFileProvider>().Single(); | ||
| var mappedProject = provider.MapDocument(document); | ||
| Assert.NotNull(mappedProject); | ||
| Assert.Equal(project.Id, mappedProject!.Id); |
There was a problem hiding this comment.
No new tests, but this adds an extra verification to all tests in PdbSourceDocumentTests.cs
|
|
||
| internal sealed record SourceDocument(string FilePath, SourceHashAlgorithm HashAlgorithm, ImmutableArray<byte> Checksum, byte[]? EmbeddedTextBytes, string? SourceLinkUrl); | ||
|
|
||
| internal record struct SourceDocumentInfo(DocumentId DocumentId, Encoding Encoding, ProjectId SourceProjectId, Workspace SourceWorkspace); |
There was a problem hiding this comment.
Naming these things is hard, but 4 element tuples felt a bit big. Will fix up the names in a follow up (because I have to add another one of these to track tab title and tooltip 🤦♂️)
There was a problem hiding this comment.
So we have both a SourceDocument and a SourceDocumentInfo? How to best think about the difference here?
There was a problem hiding this comment.
They're info about the same document, but at different stages of the system. One is with info from PDB/SourceLink/etc. before we've loaded the file contents and created the Document, the other is after we've created the Document to store stuff about it and where it came from.
There was a problem hiding this comment.
That seems like that might be worth documenting or something?
| /// </summary> | ||
| private readonly SemaphoreSlim _gate = new(initialCount: 1); | ||
|
|
||
| private readonly Dictionary<string, IMetadataAsSourceFileProvider> _tempFileToProviderMap = new(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Slightly longer explanation for removing this is in the first commit, but TL;DR is:
- Unnecessary because only two providers
- Only allows one file per symbol which doesn't hold for PDB source
src/EditorFeatures/CSharpTest/PdbSourceDocument/AbstractPdbSourceDocumentTests.cs
Outdated
Show resolved
Hide resolved
| if (!provider.IsValueCreated) | ||
| continue; |
There was a problem hiding this comment.
How expensive do we expect the creation of these providers to be? I would have assumed it was a relatively cheap MEF part. If somebody were to miss a IsValueCreated check in a loop in one of the sites, are we going to break RPS or something?
There was a problem hiding this comment.
Perhaps not in this spot, but in TryAddDocument..., which is called from an RTD event for every document that is opened, I think its best to avoid the part creation.
|
|
||
| internal sealed record SourceDocument(string FilePath, SourceHashAlgorithm HashAlgorithm, ImmutableArray<byte> Checksum, byte[]? EmbeddedTextBytes, string? SourceLinkUrl); | ||
|
|
||
| internal record struct SourceDocumentInfo(DocumentId DocumentId, Encoding Encoding, ProjectId SourceProjectId, Workspace SourceWorkspace); |
There was a problem hiding this comment.
So we have both a SourceDocument and a SourceDocumentInfo? How to best think about the difference here?
src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceFileService.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
left a comment
There was a problem hiding this comment.
Functionaly seems fine; a comment explaining better the SourceDocument vs. SourceDocumentInfo might be useful. Or maybe not.
|
Thanks for the review. Since the snap is Monday and I'm OOF for the week, I'm going to merge and follow up with rename/doc of the records later. |
…ures/required-members * upstream/main: (187 commits) Add GlobalOptions.SetBackgroundAnalysisScope and PythiaGlobalOptions External Access API (#59794) Update source-build dependency to source-build-externals (#59549) Do not retry on Clipboard API for StackTraceExplorer (#59658) Remove unnecessary accesses on XML end tag (#59771) Threading lint Improve PDB source document project handling (#59643) Disable Auto-Open behavior for Stack Trace Explorer (#59785) REmove comment Push async up Simplify Remove stale remarks Update src/Tools/ExternalAccess/FSharp/Navigation/FSharpDocumentNavigationService.cs Remove unnecessary code Simplify threading Update tests Make the IDocumentNavigationSerivice entirely async. Disable additional text comparer in generator driver (#59776) [LSP] Cache parsed xml snippets for razor (#59605) Make static ...
Fixes #59138
Part of #55834
The main thing this does is ensure that additional documents found via Source Link are shown in the right project context. The affect is seen when opening a Source Link file other than the first one the user navigated to.
Before:
File is in Misc Files, classification is broken (eg constructer parameters)
After:
Show in the right project, with a nice name, and full classification (and in dark mode apparently 😛)