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

[SourceLink] Go To Def tab improvements #57995

Closed
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,18 @@ protected static async Task RunTestAsync(Func<string, Task> testRunner)
buildReferenceAssembly,
windowsPdb: false);

await GenerateFileAndVerifyAsync(project, symbol, source, expectedSpan, expectNullResult);
await GenerateFileAndVerifyAsync(project, symbol, sourceLocation, source, expectedSpan, expectNullResult);
}

protected static async Task GenerateFileAndVerifyAsync(
Project project,
ISymbol symbol,
Location sourceLocation,
string expected,
Text.TextSpan expectedSpan,
bool expectNullResult)
{
var (actual, actualSpan) = await GetGeneratedSourceTextAsync(project, symbol, expectNullResult);
var (actual, actualSpan) = await GetGeneratedSourceTextAsync(project, symbol, sourceLocation, expectNullResult);

if (actual is null)
return;
Expand All @@ -121,6 +122,7 @@ protected static async Task RunTestAsync(Func<string, Task> testRunner)
protected static async Task<(SourceText?, TextSpan)> GetGeneratedSourceTextAsync(
Project project,
ISymbol symbol,
Location sourceLocation,
bool expectNullResult)
{
using var workspace = (TestWorkspace)project.Solution.Workspace;
Expand All @@ -140,6 +142,15 @@ protected static async Task RunTestAsync(Func<string, Task> testRunner)
Assert.NotSame(NullResultMetadataAsSourceFileProvider.NullResult, file);
}

if (sourceLocation == Location.OnDisk)
{
Assert.True(file.DocumentTitle.Contains($"[{FeaturesResources.external}]"));
}
else
{
Assert.True(file.DocumentTitle.Contains($"[{FeaturesResources.embedded}]"));
}

AssertEx.NotNull(file, $"No source document was found in the pdb for the symbol.");

var masWorkspace = service.TryGetWorkspace();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ public class C
// Now delete the PDB
File.Delete(GetPdbPath(path));

await GenerateFileAndVerifyAsync(project, symbol, source, expectedSpan, expectNullResult: true);
await GenerateFileAndVerifyAsync(project, symbol, Location.OnDisk, source, expectedSpan, expectNullResult: true);
});
}

Expand All @@ -423,7 +423,7 @@ public class C
// Now delete the DLL
File.Delete(GetDllPath(path));

await GenerateFileAndVerifyAsync(project, symbol, source, expectedSpan, expectNullResult: true);
await GenerateFileAndVerifyAsync(project, symbol, Location.OnDisk, source, expectedSpan, expectNullResult: true);
});
}

Expand All @@ -444,7 +444,7 @@ public class C
// Now delete the source
File.Delete(GetSourceFilePath(path));

await GenerateFileAndVerifyAsync(project, symbol, source, expectedSpan, expectNullResult: true);
await GenerateFileAndVerifyAsync(project, symbol, Location.OnDisk, source, expectedSpan, expectNullResult: true);
});
}

Expand All @@ -463,7 +463,7 @@ public class C
var (project, symbol) = await CompileAndFindSymbolAsync(path, Location.OnDisk, Location.OnDisk, metadataSource, c => c.GetMember("C.E"), windowsPdb: true);

//TODO: This should not be a null result: https://github.com/dotnet/roslyn/issues/55834
await GenerateFileAndVerifyAsync(project, symbol, source, expectedSpan, expectNullResult: true);
await GenerateFileAndVerifyAsync(project, symbol, Location.OnDisk, source, expectedSpan, expectNullResult: true);
});
}

Expand All @@ -485,7 +485,7 @@ public class C
// Now make the PDB a zero byte file
File.WriteAllBytes(GetPdbPath(path), new byte[0]);

await GenerateFileAndVerifyAsync(project, symbol, source, expectedSpan, expectNullResult: true);
await GenerateFileAndVerifyAsync(project, symbol, Location.OnDisk, source, expectedSpan, expectNullResult: true);
});
}

Expand All @@ -509,7 +509,7 @@ public class C
var corruptPdb = new byte[] { 66, 83, 74, 66, 68, 87 };
File.WriteAllBytes(GetPdbPath(path), corruptPdb);

await GenerateFileAndVerifyAsync(project, symbol, source, expectedSpan, expectNullResult: true);
await GenerateFileAndVerifyAsync(project, symbol, Location.OnDisk, source, expectedSpan, expectNullResult: true);
});
}

Expand Down Expand Up @@ -545,7 +545,7 @@ public class C
File.Delete(pdbFilePath);
File.Move(archivePdbFilePath, pdbFilePath);

await GenerateFileAndVerifyAsync(project, symbol, source1, expectedSpan, expectNullResult: true);
await GenerateFileAndVerifyAsync(project, symbol, Location.OnDisk, source1, expectedSpan, expectNullResult: true);
});
}

Expand Down Expand Up @@ -573,7 +573,7 @@ public class C

File.WriteAllText(GetSourceFilePath(path), source2, Encoding.UTF8);

await GenerateFileAndVerifyAsync(project, symbol, metadataSource, expectedSpan, expectNullResult: true);
await GenerateFileAndVerifyAsync(project, symbol, Location.OnDisk, metadataSource, expectedSpan, expectNullResult: true);
});
}

Expand Down Expand Up @@ -609,7 +609,7 @@ public class C

var (project, symbol) = await CompileAndFindSymbolAsync(path, pdbLocation, Location.Embedded, encodedSourceText, c => c.GetMember("C.E"));

var (actualText, _) = await GetGeneratedSourceTextAsync(project, symbol, expectNullResult: false);
var (actualText, _) = await GetGeneratedSourceTextAsync(project, symbol, Location.Embedded, expectNullResult: false);

AssertEx.NotNull(actualText);
AssertEx.NotNull(actualText.Encoding);
Expand Down Expand Up @@ -638,7 +638,7 @@ public class C

var (project, symbol) = await CompileAndFindSymbolAsync(path, pdbLocation, Location.Embedded, encodedSourceText, c => c.GetMember("C.E"));

var (actualText, _) = await GetGeneratedSourceTextAsync(project, symbol, expectNullResult: false);
var (actualText, _) = await GetGeneratedSourceTextAsync(project, symbol, Location.Embedded, expectNullResult: false);

AssertEx.NotNull(actualText);
AssertEx.NotNull(actualText.Encoding);
Expand Down Expand Up @@ -667,7 +667,7 @@ public class C

var (project, symbol) = await CompileAndFindSymbolAsync(path, pdbLocation, Location.Embedded, encodedSourceText, c => c.GetMember("C.E"), fallbackEncoding: encoding);

var (actualText, _) = await GetGeneratedSourceTextAsync(project, symbol, expectNullResult: false);
var (actualText, _) = await GetGeneratedSourceTextAsync(project, symbol, Location.Embedded, expectNullResult: false);

AssertEx.NotNull(actualText);
AssertEx.NotNull(actualText.Encoding);
Expand Down
8 changes: 8 additions & 0 deletions src/Features/Core/Portable/FeaturesResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -2948,4 +2948,12 @@ Zero-width positive lookbehind assertions are typically used at the beginning of
<data name="Silent" xml:space="preserve">
<value>Silent</value>
</data>
<data name="embedded" xml:space="preserve">
<value>embedded</value>
<comment>Embedded is a technical term for "Embedded source", where souce files are embedded into the PDB</comment>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On behalf of the translation teams, thank you for adding comments like this. 😄

</data>
<data name="external" xml:space="preserve">
<value>external</value>
<comment>External means "external source", meaning source files that are not part of the current solution</comment>
Comment on lines +2956 to +2957
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to tag this specially, if it's opening a file on your machine? Like does this get strange if you then want to edit that file like a local file, or add that missing project to your solution?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or are we taking the on-disk file and copying it to a temp folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you were right, this is a file on your machine.. but we open it readonly, and if you were to edit it, then we wouldn't find it because the checksum wouldn't match. If we didn't tag it specifically, what would we say? I don't think it should look like a normal file, because its not in the solution, so lots of things don't work like normal (and as mentioned, its readonly). I don't think the tab should say "from metadata", because its the real source. So "external" is the best I could come up with. "local" I guess could work?

</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ internal interface IPdbSourceDocumentLoaderService
Task<SourceFileInfo?> LoadSourceDocumentAsync(string tempFilePath, SourceDocument sourceDocument, Encoding encoding, IPdbSourceDocumentLogger? logger, CancellationToken cancellationToken);
}

internal sealed record SourceFileInfo(string FilePath, TextLoader Loader);
internal sealed record SourceFileInfo(string FilePath, string SourceDescription, TextLoader Loader);
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public PdbSourceDocumentLoaderService([Import(AllowDefault = true)] ISourceLinkS
// We might have already navigated to this file before, so it might exist, but
// we still need to re-validate the checksum and make sure its not the wrong file
if (File.Exists(filePath) &&
LoadSourceFile(filePath, sourceDocument, encoding) is { } existing)
LoadSourceFile(filePath, sourceDocument, encoding, FeaturesResources.embedded) is { } existing)
{
return existing;
}
Expand Down Expand Up @@ -96,7 +96,7 @@ public PdbSourceDocumentLoaderService([Import(AllowDefault = true)] ISourceLinkS
}
}

return LoadSourceFile(filePath, sourceDocument, encoding);
return LoadSourceFile(filePath, sourceDocument, encoding, FeaturesResources.embedded);
}

return null;
Expand All @@ -112,7 +112,8 @@ public PdbSourceDocumentLoaderService([Import(AllowDefault = true)] ISourceLinkS

if (sourceFile is not null)
{
return LoadSourceFile(sourceFile.SourceFilePath, sourceDocument, encoding);
// Since "SourceLink" is the name of the technology it is deliberately not localized
return LoadSourceFile(sourceFile.SourceFilePath, sourceDocument, encoding, "SourceLink");
}

return null;
Expand All @@ -122,13 +123,13 @@ public PdbSourceDocumentLoaderService([Import(AllowDefault = true)] ISourceLinkS
{
if (File.Exists(sourceDocument.FilePath))
{
return LoadSourceFile(sourceDocument.FilePath, sourceDocument, encoding);
return LoadSourceFile(sourceDocument.FilePath, sourceDocument, encoding, FeaturesResources.external);
}

return null;
}

private static SourceFileInfo? LoadSourceFile(string filePath, SourceDocument sourceDocument, Encoding encoding)
private static SourceFileInfo? LoadSourceFile(string filePath, SourceDocument sourceDocument, Encoding encoding, string sourceDescription)
{
return IOUtilities.PerformIO(() =>
{
Expand All @@ -141,7 +142,7 @@ public PdbSourceDocumentLoaderService([Import(AllowDefault = true)] ISourceLinkS
{
var textAndVersion = TextAndVersion.Create(sourceText, VersionStamp.Default, filePath);
var textLoader = TextLoader.From(textAndVersion);
return new SourceFileInfo(filePath, textLoader);
return new SourceFileInfo(filePath, sourceDescription, textLoader);
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,13 +164,12 @@ internal sealed class PdbSourceDocumentMetadataAsSourceFileProvider : IMetadataA
var navigateLocation = await MetadataAsSourceHelpers.GetLocationInGeneratedSourceAsync(symbolId, document, cancellationToken).ConfigureAwait(false);
var navigateDocument = navigateProject.GetDocument(navigateLocation.SourceTree);

// TODO: "from metadata" is technically correct, but could be confusing. From PDB? From Source? https://github.com/dotnet/roslyn/issues/55834
var documentName = string.Format(
"{0} [{1}]",
navigateDocument!.Name,
FeaturesResources.from_metadata);
sourceFileInfos[0]!.SourceDescription);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean for sourceFileInfos to be an array here? If the first is significant/special, can we extract that out to a local (that's also been null-suppressed once) rather than duplicating it everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately for now, the first is not special, but whilst GoToDef supports multiple locations, MetadataAsSoruce doesn't, so there is some refactoring of things that needs to happen I haven't started to look at yet. A local is probably a good idea


return new MetadataAsSourceFile(documentPath, navigateLocation, documentName, navigateDocument.FilePath);
return new MetadataAsSourceFile(documentPath, navigateLocation, documentName, sourceDocuments[0].FilePath);
}

private static ProjectInfo? CreateProjectInfo(Workspace workspace, Project project, ImmutableDictionary<string, string> pdbCompilationOptions, string assemblyName)
Expand Down
10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/Features/Core/Portable/xlf/FeaturesResources.pt-BR.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading