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

Fix issue where closing sourcelink document threw if not opened #75046

Merged
merged 2 commits into from
Sep 10, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -976,4 +976,88 @@ await RunTestAsync(async path =>
Assert.True(result);
});
}

[Fact]
public async Task OpenThenClose()
{
var source = """
public class C
{
public int P { get; set; }
}
""";

await RunTestAsync(async path =>
{
var (project, symbol) = await CompileAndFindSymbolAsync(path, Location.Embedded, Location.Embedded, source, c => c.GetMember("C.P"));

using var workspace = (EditorTestWorkspace)project.Solution.Workspace;
var service = workspace.GetService<IMetadataAsSourceFileService>();
var file = await service.GetGeneratedFileAsync(project.Solution.Workspace, project, symbol, signaturesOnly: false, options: MetadataAsSourceOptions.Default, cancellationToken: CancellationToken.None);

var openResult = service.TryAddDocumentToWorkspace(file.FilePath, new StaticSourceTextContainer(SourceText.From(string.Empty)), out var documentId);
Assert.True(openResult);

var closeResult = service.TryRemoveDocumentFromWorkspace(file.FilePath);
Assert.True(closeResult);
});
}

[Fact, WorkItem("https://github.com/dotnet/vscode-csharp/issues/7514")]
public async Task CloseWithoutOpenDoesNotThrow()
{
var source = """
public class C
{
public int P { get; set; }
}
""";

await RunTestAsync(async path =>
{
var (project, symbol) = await CompileAndFindSymbolAsync(path, Location.Embedded, Location.Embedded, source, c => c.GetMember("C.P"));

using var workspace = (EditorTestWorkspace)project.Solution.Workspace;
var service = workspace.GetService<IMetadataAsSourceFileService>();
var file = await service.GetGeneratedFileAsync(project.Solution.Workspace, project, symbol, signaturesOnly: false, options: MetadataAsSourceOptions.Default, cancellationToken: CancellationToken.None);

var result = service.TryRemoveDocumentFromWorkspace(file.FilePath);
Assert.False(result);
});
}

[Fact, WorkItem("https://github.com/dotnet/vscode-csharp/issues/7514")]
public async Task OpenSameDocument()
{
var source = """
public class C
{
public int P1 { get; set; }

public int P2 { get; set; }
}
""";

await RunTestAsync(async path =>
{
var (project, symbol) = await CompileAndFindSymbolAsync(path, Location.Embedded, Location.Embedded, source, c => c.GetMember("C.P1"));

using var workspace = (EditorTestWorkspace)project.Solution.Workspace;
var service = workspace.GetService<IMetadataAsSourceFileService>();
var fileOne = await service.GetGeneratedFileAsync(project.Solution.Workspace, project, symbol, signaturesOnly: false, options: MetadataAsSourceOptions.Default, cancellationToken: CancellationToken.None);

var openResult = service.TryAddDocumentToWorkspace(fileOne.FilePath, new StaticSourceTextContainer(SourceText.From(string.Empty)), out var documentId);
Assert.True(openResult);

var compilation = await project.GetCompilationAsync(CancellationToken.None);
var symbolTwo = compilation.GetMember("C.P2");
var fileTwo = await service.GetGeneratedFileAsync(project.Solution.Workspace, project, symbolTwo, signaturesOnly: false, MetadataAsSourceOptions.Default, CancellationToken.None);
Assert.Equal(fileOne.FilePath, fileTwo.FilePath);
Assert.NotEqual(fileOne.IdentifierLocation, fileTwo.IdentifierLocation);

// Opening should still throw (should never be called as we should be able to find the previously
// opened document in the MAS workspace).
Assert.Throws<System.InvalidOperationException>(() => service.TryAddDocumentToWorkspace(fileTwo.FilePath, new StaticSourceTextContainer(SourceText.From(string.Empty)), out var documentIdTwo));
});
}
}
Original file line number Diff line number Diff line change
@@ -70,6 +70,11 @@ internal sealed class PdbSourceDocumentMetadataAsSourceFileProvider(
/// </summary>
private readonly ConcurrentDictionary<string, SourceDocumentInfo> _fileToDocumentInfoMap = new(StringComparer.OrdinalIgnoreCase);

/// <summary>
/// Only accessed and mutated in serial calls either from the UI thread or LSP queue.
/// </summary>
private readonly HashSet<DocumentId> _openedDocumentIds = new();

public async Task<MetadataAsSourceFile?> GetGeneratedFileAsync(
MetadataAsSourceWorkspace metadataWorkspace,
Workspace sourceWorkspace,
@@ -252,7 +257,14 @@ internal sealed class PdbSourceDocumentMetadataAsSourceFileProvider(
var documentInfos = CreateDocumentInfos(sourceFileInfos, encoding, projectId, sourceWorkspace, sourceProject);
if (documentInfos.Length > 0)
{
pendingSolution = pendingSolution.AddDocuments(documentInfos);
foreach (var documentInfo in documentInfos)
{
// The document might have already been added by a previous go to definition call.
if (!pendingSolution.ContainsDocument(documentInfo.Id))
{
pendingSolution = pendingSolution.AddDocument(documentInfo);
}
}
}

var navigateProject = pendingSolution.GetRequiredProject(projectId);
@@ -365,9 +377,12 @@ public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, strin
{
if (_fileToDocumentInfoMap.TryGetValue(filePath, out var info))
{
Contract.ThrowIfTrue(_openedDocumentIds.Contains(info.DocumentId));

workspace.OnDocumentAdded(info.DocumentInfo);
workspace.OnDocumentOpened(info.DocumentId, sourceTextContainer);
documentId = info.DocumentId;
_openedDocumentIds.Add(documentId);
return true;
}

@@ -382,9 +397,18 @@ public bool TryRemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace,
{
if (_fileToDocumentInfoMap.TryGetValue(filePath, out var info))
{
workspace.OnDocumentClosed(info.DocumentId, new WorkspaceFileTextLoader(workspace.Services.SolutionServices, filePath, info.Encoding));
workspace.OnDocumentRemoved(info.DocumentId);
return true;
// In LSP, while calls to TryAddDocumentToWorkspace and TryRemoveDocumentFromWorkspace are handled
// serially, it is possible that TryRemoveDocumentFromWorkspace called without TryAddDocumentToWorkspace first.
// This can happen if the document is immediately closed after opening - only feature requests that force us
// to materialize a solution will trigger TryAddDocumentToWorkspace, if none are made it is never called.
// However TryRemoveDocumentFromWorkspace is always called on close.
if (_openedDocumentIds.Contains(info.DocumentId))
{
workspace.OnDocumentClosed(info.DocumentId, new WorkspaceFileTextLoader(workspace.Services.SolutionServices, filePath, info.Encoding));
workspace.OnDocumentRemoved(info.DocumentId);
_openedDocumentIds.Remove(info.DocumentId);
return true;
}
}

return false;
@@ -426,6 +450,7 @@ public void CleanupGeneratedFiles(MetadataAsSourceWorkspace workspace)

// The MetadataAsSourceFileService will clean up the entire temp folder so no need to do anything here
_fileToDocumentInfoMap.Clear();
_openedDocumentIds.Clear();
_sourceLinkEnabledProjects.Clear();
_implementationAssemblyLookupService.Clear();
}