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.

Sign up for GitHub

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

Jump to bottom

Ensure diagnostics are logged before proceeding with further project loading #70467

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Ensure diagnostics are logged before proceeding with further project loading #70467

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a44f928
Ensure diagnostics are logged before proceeding with further project …
dibarbet Oct 19, 2023
70b64e2
Ensure execution diagnostics are logged as well
dibarbet Oct 21, 2023
74f50f2
Merge remote-tracking branch 'upstream/main' into fix_error_logging
dibarbet Oct 23, 2023
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
48 changes: 32 additions & 16 deletions ...Server/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.ProjectTelemetry;
using Microsoft.CodeAnalysis.MSBuild;
using Microsoft.CodeAnalysis.MSBuild.Logging;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.ProjectSystem;
using Microsoft.CodeAnalysis.Shared.TestHooks;
Expand Down Expand Up @@ -192,7 +193,7 @@ private async ValueTask LoadOrReloadProjectsAsync(ImmutableSegmentedList<Project
return binaryLogPath;
}

private async Task<(LSP.MessageType? failureType, BuildHostProcessKind? preferredKind)> LoadOrReloadProjectAsync(ProjectToLoad projectToLoad, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken)
private async Task<(LSP.MessageType? FailureType, BuildHostProcessKind? PreferredKind)> LoadOrReloadProjectAsync(ProjectToLoad projectToLoad, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken)
{
BuildHostProcessKind? preferredBuildHostKind = null;

Expand All @@ -205,6 +206,14 @@ private async ValueTask LoadOrReloadProjectsAsync(ImmutableSegmentedList<Project
if (await buildHost.IsProjectFileSupportedAsync(projectPath, cancellationToken))
{
var loadedFile = await buildHost.LoadProjectFileAsync(projectPath, cancellationToken);
var diagnosticLogItems = await loadedFile.GetDiagnosticLogItemsAsync(cancellationToken);
if (diagnosticLogItems.Any(item => item.Kind is WorkspaceDiagnosticKind.Failure))
dibarbet marked this conversation as resolved.
Show resolved Hide resolved
{
_ = LogDiagnostics(projectPath, diagnosticLogItems);
// We have total failures in evaluation, no point in continuing.
return (LSP.MessageType.Error, preferredBuildHostKind);
}

var loadedProjectInfos = await loadedFile.GetProjectFileInfosAsync(cancellationToken);

// The out-of-proc build host supports more languages than we may actually have Workspace binaries for, so ensure we can actually process that
Expand Down Expand Up @@ -247,23 +256,14 @@ private async ValueTask LoadOrReloadProjectsAsync(ImmutableSegmentedList<Project
}

await _projectLoadTelemetryReporter.ReportProjectLoadTelemetryAsync(projectFileInfos, projectToLoad, cancellationToken);

var diagnosticLogItems = await loadedFile.GetDiagnosticLogItemsAsync(cancellationToken);
if (diagnosticLogItems.Any())
{
foreach (var logItem in diagnosticLogItems)
{
var projectName = Path.GetFileName(projectPath);
_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, preferredBuildHostKind);
}
else
diagnosticLogItems = await loadedFile.GetDiagnosticLogItemsAsync(cancellationToken);
var errorLevel = LogDiagnostics(projectPath, diagnosticLogItems);
if (errorLevel == null)
{
_logger.LogInformation($"Successfully completed load of {projectPath}");
return (null, null);
_logger.LogInformation($"Successfully completed load of {projectToLoad}");
}

return (errorLevel, preferredBuildHostKind);
}

return (null, null);
Expand All @@ -273,5 +273,21 @@ private async ValueTask LoadOrReloadProjectsAsync(ImmutableSegmentedList<Project
_logger.LogError(e, $"Exception thrown while loading {projectToLoad.Path}");
return (LSP.MessageType.Error, preferredBuildHostKind);
}

LSP.MessageType? LogDiagnostics(string projectPath, ImmutableArray<DiagnosticLogItem> diagnosticLogItems)
{
if (diagnosticLogItems.IsEmpty)
{
return null;
}

foreach (var logItem in diagnosticLogItems)
{
var projectName = Path.GetFileName(projectPath);
_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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

}
}
}
Loading