From d23fe1e66cf8fa0f6fe36bd96156384cf9b9095c Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Fri, 14 Jan 2022 13:23:57 -0800 Subject: [PATCH 1/2] Remove places where the compiler is swallowing exceptions There are a few places where compiler APIs are swallowing internal exceptions and returning null from APIs as a way to march on; this removes those places, and then removes our ReportAndCatch* APIs from the compiler layer entirely. We currently have no (cross platform) way of implementing those in a way that would allow for non-fatal error reporting. Closes https://github.com/dotnet/roslyn/issues/58375 --- .../Portable/Compilation/SemanticModel.cs | 14 +-- .../DiagnosticAnalyzer/AnalyzerDriver.cs | 6 +- .../DiagnosticAnalyzer/AnalyzerExecutor.cs | 4 +- .../Portable/InternalUtilities/FatalError.cs | 100 ++---------------- .../Portable/Operations/ControlFlowGraph.cs | 18 +--- .../Implementation/Watson/FaultReporter.cs | 1 - 6 files changed, 16 insertions(+), 127 deletions(-) diff --git a/src/Compilers/Core/Portable/Compilation/SemanticModel.cs b/src/Compilers/Core/Portable/Compilation/SemanticModel.cs index 6acdf61928c9d..61204b54280c5 100644 --- a/src/Compilers/Core/Portable/Compilation/SemanticModel.cs +++ b/src/Compilers/Core/Portable/Compilation/SemanticModel.cs @@ -75,19 +75,7 @@ public SyntaxTree SyntaxTree /// public IOperation? GetOperation(SyntaxNode node, CancellationToken cancellationToken = default(CancellationToken)) { - try - { - return GetOperationCore(node, cancellationToken); - } -#pragma warning disable CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete; tracked by https://github.com/dotnet/roslyn/issues/58375 - catch (Exception e) when (FatalError.ReportIfNonFatalAndCatchUnlessCanceled(e, cancellationToken)) -#pragma warning restore CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete - { - // Log a Non-fatal-watson and then ignore the crash in the attempt of getting operation - Debug.Assert(false, "\n" + e.ToString()); - } - - return null; + return GetOperationCore(node, cancellationToken); } protected abstract IOperation? GetOperationCore(SyntaxNode node, CancellationToken cancellationToken); diff --git a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs index 5746e6e7378d2..de8f06b696b73 100644 --- a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs +++ b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs @@ -2697,12 +2697,8 @@ ImmutableArray getOperationsToAnalyzeWithStackGuard(ImmutableArray /// Same as setting the Handler property except that it avoids the assert. This is useful in /// test code which needs to verify the handler is called in specific cases and will continually @@ -85,9 +83,6 @@ public static void CopyHandlerTo(Assembly assembly) { targetHandlerProperty.SetValue(obj: null, value: null); } - - var targetIsNonFatalProperty = targetType.GetProperty(nameof(HandlerIsNonFatal), BindingFlags.Static | BindingFlags.Public)!; - targetIsNonFatalProperty.SetValue(obj: null, value: HandlerIsNonFatal); } #endif @@ -149,6 +144,11 @@ public static bool ReportAndPropagateUnlessCanceled(Exception exception, Cancell return ReportAndPropagate(exception, severity); } + // Since the command line compiler has no way to catch exceptions, report them, and march on, we + // simply don't offer such a mechanism here to avoid accidental swallowing of exceptions. + +#if !COMPILERCORE + /// /// Report an error. /// Calls and doesn't pass the exception through (the method returns true). @@ -160,24 +160,14 @@ public static bool ReportAndPropagateUnlessCanceled(Exception exception, Cancell /// /// True to catch the exception. [DebuggerHidden] -#if COMPILERCORE - private -#else - public -#endif - static bool ReportAndCatch(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized) + public static bool ReportAndCatch(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized) { Report(exception, severity); return true; } [DebuggerHidden] -#if COMPILERCORE - private -#else - public -#endif - static bool ReportWithDumpAndCatch(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized) + public static bool ReportWithDumpAndCatch(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized) { Report(exception, severity, forceDump: true); return true; @@ -190,42 +180,13 @@ static bool ReportWithDumpAndCatch(Exception exception, ErrorSeverity severity = /// to catch the exception if the error was reported; otherwise, /// to propagate the exception if the operation was cancelled. [DebuggerHidden] -#if COMPILERCORE - private -#else - public -#endif - static bool ReportAndCatchUnlessCanceled(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized) - { - if (exception is OperationCanceledException) - { - return false; - } - - return ReportAndCatch(exception, severity); - } - - /// - /// Use in an exception filter to report an error (by calling ) and catch - /// the exception, unless the operation was cancelled. - /// - /// to catch the exception if the error was reported; otherwise, - /// to propagate the exception if the operation was cancelled. - // This is only a temporary shim; the removal is tracked by https://github.com/dotnet/roslyn/issues/58375. - [DebuggerHidden] - [Obsolete("This is only to support places the compiler is catching exceptions on the command line; do not use in new code.")] - public static bool ReportIfNonFatalAndCatchUnlessCanceled(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized) + public static bool ReportAndCatchUnlessCanceled(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized) { if (exception is OperationCanceledException) { return false; } - if (!HandlerIsNonFatal) - { - return true; - } - return ReportAndCatch(exception, severity); } @@ -249,12 +210,7 @@ public static bool ReportIfNonFatalAndCatchUnlessCanceled(Exception exception, E /// to catch the exception if the error was reported; otherwise, /// to propagate the exception if the operation was cancelled. [DebuggerHidden] -#if COMPILERCORE - private -#else - public -#endif - static bool ReportAndCatchUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity = ErrorSeverity.Uncategorized) + public static bool ReportAndCatchUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity = ErrorSeverity.Uncategorized) { if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken)) { @@ -264,43 +220,7 @@ static bool ReportAndCatchUnlessCanceled(Exception exception, CancellationToken return ReportAndCatch(exception, severity); } - /// - /// Use in an exception filter to report an error (by calling ) and - /// catch the exception, unless the operation was cancelled at the request of - /// . - /// - /// Cancellable operations are only expected to throw if the - /// applicable indicates cancellation is requested by setting - /// . Unexpected cancellation, i.e. an - /// which occurs without - /// requesting cancellation, is treated as an error by this method. - /// - /// This method does not require to match - /// , provided cancellation is expected per the previous - /// paragraph. - /// - /// A which will have - /// set if cancellation is expected. - /// to catch the exception if the error was reported; otherwise, - /// to propagate the exception if the operation was cancelled. - // This is only a temporary shim; the removal is tracked by https://github.com/dotnet/roslyn/issues/58375. - [DebuggerHidden] - [Obsolete("This is only to support places the compiler is catching and swallowing exceptions on the command line; do not use in new code.")] - public static bool ReportIfNonFatalAndCatchUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity = ErrorSeverity.Uncategorized) - { - if (ExceptionUtilities.IsCurrentOperationBeingCancelled(exception, contextCancellationToken)) - { - return false; - } - - if (!HandlerIsNonFatal) - { - // We'll catch the exception, but we won't report anything. - return true; - } - - return ReportAndCatch(exception, severity); - } +#endif private static readonly object s_reportedMarker = new(); diff --git a/src/Compilers/Core/Portable/Operations/ControlFlowGraph.cs b/src/Compilers/Core/Portable/Operations/ControlFlowGraph.cs index 0401307413e92..87468b40d3183 100644 --- a/src/Compilers/Core/Portable/Operations/ControlFlowGraph.cs +++ b/src/Compilers/Core/Portable/Operations/ControlFlowGraph.cs @@ -178,21 +178,9 @@ internal static ControlFlowGraph CreateCore(IOperation operation, string argumen throw new ArgumentException(CodeAnalysisResources.OperationHasNullSemanticModel, argumentNameForException); } - try - { - ControlFlowGraph controlFlowGraph = ControlFlowGraphBuilder.Create(operation); - Debug.Assert(controlFlowGraph.OriginalOperation == operation); - return controlFlowGraph; - } -#pragma warning disable CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete; tracked by https://github.com/dotnet/roslyn/issues/58375 - catch (Exception e) when (FatalError.ReportIfNonFatalAndCatchUnlessCanceled(e, cancellationToken)) -#pragma warning restore CS0618 // ReportIfNonFatalAndCatchUnlessCanceled is obsolete - { - // Log a Non-fatal-watson and then ignore the crash in the attempt of getting flow graph. - Debug.Assert(false, "\n" + e.ToString()); - } - - return null; + ControlFlowGraph controlFlowGraph = ControlFlowGraphBuilder.Create(operation); + Debug.Assert(controlFlowGraph.OriginalOperation == operation); + return controlFlowGraph; } /// diff --git a/src/VisualStudio/Core/Def/Implementation/Watson/FaultReporter.cs b/src/VisualStudio/Core/Def/Implementation/Watson/FaultReporter.cs index 3c5d8ea53dc3d..6f11867c7a558 100644 --- a/src/VisualStudio/Core/Def/Implementation/Watson/FaultReporter.cs +++ b/src/VisualStudio/Core/Def/Implementation/Watson/FaultReporter.cs @@ -27,7 +27,6 @@ internal static class FaultReporter public static void InitializeFatalErrorHandlers() { FatalError.Handler = static (exception, severity, forceDump) => ReportFault(exception, ConvertSeverity(severity), forceDump); - FatalError.HandlerIsNonFatal = true; FatalError.CopyHandlerTo(typeof(Compilation).Assembly); } From 496d572817d488c8be575b28ea3971197465471f Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Tue, 1 Feb 2022 19:05:04 -0800 Subject: [PATCH 2/2] Don't silently eat a certain class of analyzer-thrown exceptions If an analyzer threw an exception, we generate a message that tells you what diagnostic IDs the analzyer produces, so you know what to disable so you can work around the analyzer. If the analyzer then also were to throw exceptions while we ask it for it's SupportedDiagnostics, we were eating that exception. This includes that as well. --- .../CSharp/Test/CommandLine/CommandLineTests.cs | 8 +++++++- .../SuppressMessageAttributeCompilerTests.cs | 7 +++++-- .../DiagnosticAnalyzer/AnalyzerExecutor.cs | 14 +++++++++----- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs b/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs index 8e38f5de7ff54..206f936c3a5ff 100644 --- a/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs +++ b/src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs @@ -11722,7 +11722,13 @@ public void LoadinganalyzerNetStandard13() at TestAnalyzer.get_SupportedDiagnostics() at Microsoft.CodeAnalysis.Diagnostics.AnalyzerManager.AnalyzerExecutionContext.<>c__DisplayClass20_0.b__0(Object _) at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock[TArg](DiagnosticAnalyzer analyzer, Action`1 analyze, TArg argument, Nullable`1 info) ------", outputWithoutPaths); +----- +Analyzer 'TestAnalyzer' threw the following exception: +'System.NotImplementedException: 28 + at TestAnalyzer.get_SupportedDiagnostics() + at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.CreateDisablingMessage(DiagnosticAnalyzer analyzer, String analyzerName) +----- +'.", outputWithoutPaths); Assert.Equal(0, result.ExitCode); } diff --git a/src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs b/src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs index 3ef22bc03355a..9edf099ef6ca4 100644 --- a/src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs +++ b/src/Compilers/Core/CodeAnalysisTest/Diagnostics/SuppressMessageAttributeCompilerTests.cs @@ -154,13 +154,16 @@ public class C2 public async Task AnalyzerExceptionFromSupportedDiagnosticsCall() { var exception = new Exception(); + const string AnalyzerName = "Microsoft.CodeAnalysis.UnitTests.Diagnostics.SuppressMessageAttributeTests+ThrowExceptionFromSupportedDiagnostics"; var diagnostic = Diagnostic("AD0001", null) .WithArguments( - "Microsoft.CodeAnalysis.UnitTests.Diagnostics.SuppressMessageAttributeTests+ThrowExceptionFromSupportedDiagnostics", + AnalyzerName, "System.Exception", exception.Message, - (IFormattable)$@"{new LazyToString(() => exception.ToString().Substring(0, exception.ToString().IndexOf("---")))}-----") + (IFormattable)$@"{new LazyToString(() => + exception.ToString().Substring(0, exception.ToString().IndexOf("---")) + "-----" + Environment.NewLine + Environment.NewLine + + string.Format(CodeAnalysisResources.CompilerAnalyzerThrowsDescription, AnalyzerName, exception.ToString() + Environment.NewLine + "-----" + Environment.NewLine))}") .WithLocation(1, 1); await VerifyCSharpAsync("public class C { }", diff --git a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs index e290776b3dfda..22d5ca5db7c1f 100644 --- a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs +++ b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs @@ -1629,7 +1629,7 @@ internal static Diagnostic CreateAnalyzerExceptionDiagnostic(DiagnosticAnalyzer var analyzerName = analyzer.ToString(); var title = CodeAnalysisResources.CompilerAnalyzerFailure; var messageFormat = CodeAnalysisResources.CompilerAnalyzerThrows; - var contextInformation = string.Join(Environment.NewLine, CreateDiagnosticDescription(info, e), CreateDisablingMessage(analyzer)).Trim(); + var contextInformation = string.Join(Environment.NewLine, CreateDiagnosticDescription(info, e), CreateDisablingMessage(analyzer, analyzerName)).Trim(); var messageArguments = new[] { analyzerName, e.GetType().ToString(), e.Message, contextInformation }; var description = string.Format(CodeAnalysisResources.CompilerAnalyzerThrowsDescription, analyzerName, CreateDiagnosticDescription(info, e)); var descriptor = GetAnalyzerExceptionDiagnosticDescriptor(AnalyzerExceptionDiagnosticId, title, description, messageFormat); @@ -1647,19 +1647,23 @@ private static string CreateDiagnosticDescription(AnalysisContextInfo? info, Exc string.Format(CodeAnalysisResources.ExceptionContext, info?.GetContext()), e.CreateDiagnosticDescription()); } - private static string CreateDisablingMessage(DiagnosticAnalyzer analyzer) + private static string CreateDisablingMessage(DiagnosticAnalyzer analyzer, string analyzerName) { var diagnosticIds = ImmutableSortedSet.Empty.WithComparer(StringComparer.OrdinalIgnoreCase); try { foreach (var diagnostic in analyzer.SupportedDiagnostics) { - diagnosticIds = diagnosticIds.Add(diagnostic.Id); + // If a null diagnostic is returned, we would have already reported that to the user earlier; we can just skip this. + if (diagnostic != null) + { + diagnosticIds = diagnosticIds.Add(diagnostic.Id); + } } } - catch (Exception) + catch (Exception ex) { - // Intentionally empty + return string.Format(CodeAnalysisResources.CompilerAnalyzerThrowsDescription, analyzerName, ex.CreateDiagnosticDescription()); } if (diagnosticIds.IsEmpty)