[SourceLink] Create output window for logging, and tell the user what is going on #57996
Conversation
davidwengier
force-pushed
the
SourceLinkLogging
branch
from
f72c9d8
to
5c8d4b8
Compare
davidwengier
force-pushed
the
SourceLinkLogging
branch
from
5c8d4b8
to
5a557e2
Compare
...isualStudio/Core/Def/Implementation/PdbSourceDocument/PdbSourceDocumentOutputWindowLogger.cs
Outdated
Show resolved
Hide resolved
davidwengier
force-pushed
the
SourceLinkLogging
branch
from
5a557e2
to
33072b9
Compare
|
Updated. Sorry I had to rebase, and therefore force push, because of changes to the other PR |
Just realised I'll probably be rebasing again when that PR merges 🤦♂️ |
|
|
||
| public void Clear() | ||
| { | ||
| _threadingContext.JoinableTaskFactory.Run(async () => |
There was a problem hiding this comment.
Should these methods be async so we're not potentially blocking up a background thread?
There was a problem hiding this comment.
I also worry about deadlock risk here if we end up with a UI thread blocking on the call to GetGeneratedFileAsync, and then this trying to hop back. Given this is only logging, should this just be queuing work to the UI thread and we keep the actual updating of the pane asynchronous? It'd remove the deadlock risk, and also would probably help performance so you're not doing so many UI transitions during the main code path.
| return _outputPane; | ||
| } | ||
|
|
||
| private IVsOutputWindowPane? CreateOutputPane(IVsOutputWindow outputWindow) |
| // Try to get the workspace pane if it has already been registered | ||
| var workspacePaneGuid = s_outputPaneGuid; | ||
|
|
||
| // If the pane has already been created, CreatePane returns it |
There was a problem hiding this comment.
Comment doesn't make sense -- GetPane is returning it...?
| // Despite being named "ThreadSafe" unfortunately OutputStringThreadSafe does not play well with JTF | ||
| // which the debugger needs for SourceLink |
There was a problem hiding this comment.
The docs for the "ThreadSafe" method imply it's for pumping reasons, and there's some advice about casting to a different interface instead. Should we be doing that?
| if (pane == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| pane.OutputStringThreadSafe(value + Environment.NewLine); |
There was a problem hiding this comment.
pane?.OutputStringThreadSafe...?
| @@ -54,22 +54,29 @@ internal sealed class PdbSourceDocumentMetadataAsSourceFileProvider : IMetadataA | |||
| if (signaturesOnly) | |||
| return null; | |||
|
|
|||
| var assemblyName = symbol.ContainingAssembly.Identity.Name; | |||
|
|
|||
| _logger?.Clear(); | |||
There was a problem hiding this comment.
It's implicit we're never doing two calls to GetGeneratedFIleAsync at once?
| // If this is a reference assembly then we won't have the right information available, so bail out | ||
| // TODO: find the implementation assembly for the reference assembly, and keep going: https://github.com/dotnet/roslyn/issues/55834 | ||
| var isReferenceAssembly = symbol.ContainingAssembly.GetAttributes().Any(attribute => attribute.AttributeClass?.Name == nameof(ReferenceAssemblyAttribute) | ||
| && attribute.AttributeClass.ToNameDisplayString() == typeof(ReferenceAssemblyAttribute).FullName); | ||
| if (isReferenceAssembly) | ||
| { | ||
| _logger?.Log(""); |
|
|
||
| var compilation = await project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false); | ||
|
|
||
| if (compilation.GetMetadataReference(symbol.ContainingAssembly) is not PortableExecutableReference { FilePath: not null and var dllPath }) | ||
| { | ||
| _logger?.Log(FeaturesResources.Source_is_a_reference_assembly); |
There was a problem hiding this comment.
Did you mean to put this earlier? The message here should probably be different.
|
Closing this. Going to combine with the document tab changes, and add some telemetry, in a follow up since this missed Preview 2 anyway. |
Fixes #57352
Part of #55834
Includes #57978 so just review
33072b9(#57996)The idea is to help library authors work out why their embedded or source link sources might not be working, we can provide this output window. It will also be a good source for us to ask for information from, for feedback issues.
Screenshots:
