Ensure diagnostics are logged before proceeding with further project loading #70467
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
dibarbet
force-pushed
the
fix_error_logging
branch
from
53b5f4a
to
a44f928
Compare
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs
Show resolved
Hide resolved
| @@ -205,6 +206,15 @@ private async Task<(LSP.MessageType? failureType, BuildHostProcessKind? preferre | |||
| if (await buildHost.IsProjectFileSupportedAsync(projectPath, cancellationToken)) | |||
| { | |||
| var loadedFile = await buildHost.LoadProjectFileAsync(projectPath, cancellationToken); | |||
| var diagnosticLogItems = await loadedFile.GetDiagnosticLogItemsAsync(cancellationToken); | |||
| LogDiagnostics(projectPath, diagnosticLogItems); | |||
There was a problem hiding this comment.
One subtle bit: unfortunately how the existing code works is all the diagnostics created for loading get dumped into the same collection, both for evaluation and execution passes. So you probably only want to call LogDiagnostics() if you're in your if check and bailing; otherwise you can have the later LogDiagnostics() call. If both places call it though Warnings might get logged twice.
We should also make LogDiagnostics() just return the most severe type of LSP.MessageType so we can inline that.
dibarbet
force-pushed
the
fix_error_logging
branch
from
44432f2
to
35b29cf
Compare
dibarbet
force-pushed
the
fix_error_logging
branch
from
35b29cf
to
70b64e2
Compare
| _logger.Log(logItem.Kind is WorkspaceDiagnosticKind.Failure ? LogLevel.Error : LogLevel.Warning, $"{logItem.Kind} while loading {logItem.ProjectFilePath}: {logItem.Message}"); | ||
| } | ||
|
|
||
| return diagnosticLogItems.Any(logItem => logItem.Kind is WorkspaceDiagnosticKind.Failure) ? LSP.MessageType.Error : LSP.MessageType.Warning; |
There was a problem hiding this comment.
Nit: if you're already doing the loop you could just have a local of severity you assign warning, and while in the existing loop just change it to error on failure. Or leave it looping twice, not a big deal I don't think.
ghost
added this to the 
Related to dotnet/vscode-csharp#6356
Diagnostics were retrieved on the first step of project loading, but logged at the end after the rest of the steps. This meant if there was a diagnostic in the first step that didn't outright fail, but caused a later step to fail we wouldn't log it.