Refactor CommonCompiler.RunCore #23059
Conversation
RunCore previously had a mix of diagnostic reporting methods including lists of Diagnostics, lists of DiagnosticInfos, ConcurrentQueues of Diagnostics, and just printing things directly to the console. This commit tries to simplify things by moving to using Diagnostics and DiagnosticBags wherever possible in the compile+emit phase of RunCore.
|
ping @dotnet/roslyn-compiler for review |
| @@ -429,6 +429,10 @@ public bool ReportErrors(IEnumerable<Diagnostic> diagnostics, TextWriter console | |||
| return hasErrors; | |||
| } | |||
|
|
|||
| private bool ReportErrors(DiagnosticBag diagnostics, TextWriter consoleOutput, ErrorLogger errorLoggerOpt) | |||
| => ReportErrors(diagnostics.ToReadOnly(), consoleOutput, errorLoggerOpt); | |||
| @@ -385,7 +385,7 @@ internal static DiagnosticInfo ToFileReadDiagnostics(CommonMessageProvider messa | |||
| return diagnosticInfo; | |||
| } | |||
|
|
|||
| public bool ReportErrors(IEnumerable<Diagnostic> diagnostics, TextWriter consoleOutput, ErrorLogger errorLoggerOpt) | |||
| internal bool ReportErrors(IEnumerable<Diagnostic> diagnostics, TextWriter consoleOutput, ErrorLogger errorLoggerOpt) | |||
There was a problem hiding this comment.
Could IEnumerable be switched to ImmutableArray, or are there callers that still need IEnumerable?
There was a problem hiding this comment.
They still use IEnumerable unfortunately.
| analyzerCts = null; | ||
| reportAnalyzer = false; | ||
| analyzerDriver = null; | ||
| AnalyzerManager analyzerManager = null; |
There was a problem hiding this comment.
I think analyzerManager can be declared at line 660, where it is used.
| WithOutputNameOverride(outputName). | ||
| WithPdbFilePath(PathUtilities.NormalizePathPrefix(finalPdbFilePath, Arguments.PathMap)); | ||
|
|
||
| // This feature flag is being maintained until our next major release to avoid unnecessary |
There was a problem hiding this comment.
next major releas [](start = 67, length = 17)
This is not introduced by this PR, but which release is the "next major release". Was it 15.0? Is it 16.0?
Could you file a follow-up issue? #Closed
There was a problem hiding this comment.
I believe that is this issue: #19592 #Resolved
| { | ||
| return GetWin32ResourcesInternal(MessageProvider, arguments, compilation, out errors); | ||
| return GetWin32ResourcesInternal(MessageProvider, arguments, compilation, diagnostics); | ||
| } | ||
|
|
||
| // internal for testing | ||
| internal static Stream GetWin32ResourcesInternal(CommonMessageProvider messageProvider, CommandLineArguments arguments, Compilation compilation, out IEnumerable<DiagnosticInfo> errors) |
There was a problem hiding this comment.
Nit: I think we could change IEnumerable to ImmutableArray in GetWin32ResourcesInternal and GetWin32Resources signatures too
This is the monolith method for batch compilation in C# and VB.
It is large and confusing. I've tried to make it better by moving to a single
error reporting system for every piece of the pipeline (Diagnostics in DiagnosticBags).
It may be helpful to look at each commit separately.