Merge pull request dotnet#70365 from CyrusNajmabadi/deprecateAPI

Deprecate API and add NFW telemetry for it

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxNode.cs

@@ -9,6 +9,7 @@
 using System.Linq;
 using System.Threading;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.ErrorReporting;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Roslyn.Utilities;
 
@@ -182,6 +183,7 @@ public override string Language
         /// <summary>
         /// Deserialize a syntax node from the byte stream.
         /// </summary>
+        [Obsolete(SerializationDeprecationException.Text, error: false)]
         public static SyntaxNode DeserializeFrom(Stream stream, CancellationToken cancellationToken = default)
         {
             if (stream == null)
@@ -194,6 +196,8 @@ public static SyntaxNode DeserializeFrom(Stream stream, CancellationToken cancel
                 throw new InvalidOperationException(CodeAnalysisResources.TheStreamCannotBeReadFrom);
             }
 
+            // Report NFW to see if this is being used in the wild.
+            FatalError.ReportNonFatalError(new SerializationDeprecationException());
             using var reader = ObjectReader.TryGetReader(stream, leaveOpen: true, cancellationToken);
 
             if (reader == null)

src/Compilers/CSharp/Test/Syntax/Syntax/SerializationTests.cs

@@ -13,6 +13,8 @@
 using Roslyn.Test.Utilities;
 using Xunit;
 
+#pragma warning disable CS0618 // Type or member is obsolete
+
 namespace Microsoft.CodeAnalysis.CSharp.UnitTests
 {
     public class SerializationTests
@@ -305,3 +307,5 @@ public class C
         }
     }
 }
+
+#pragma warning restore CS0618 // Type or member is obsolete

src/Compilers/Core/Portable/InternalUtilities/FatalError.cs

@@ -27,7 +27,9 @@ public OperationCanceledIgnoringCallerTokenException(Exception innerException)
     internal static class FatalError
     {
         public delegate void ErrorReporterHandler(Exception exception, ErrorSeverity severity, bool forceDump);
+
         private static ErrorReporterHandler? s_handler;
+        private static ErrorReporterHandler? s_nonFatalHandler;
 
 #pragma warning disable IDE0052 // Remove unread private members - We want to hold onto last exception to make investigation easier
         private static Exception? s_reportedException;
@@ -37,21 +39,15 @@ internal static class FatalError
         /// <summary>
         /// Set by the host to handle an error report; this may crash the process or report telemetry.
         /// </summary>
-        [DisallowNull]
-        public static ErrorReporterHandler? Handler
+        /// <param name="nonFatalHandler">A handler that will not crash the process when called.  Used when calling <see
+        /// cref="ReportNonFatalError(Exception, ErrorSeverity, bool)"/></param>
+        public static void SetHandlers(ErrorReporterHandler handler, ErrorReporterHandler? nonFatalHandler)
         {
-            get
-            {
-                return s_handler;
-            }
-
-            set
+            if (s_handler != handler)
             {
-                if (s_handler != value)
-                {
-                    Debug.Assert(s_handler == null, "Handler already set");
-                    s_handler = value;
-                }
+                Debug.Assert(s_handler == null, "Handler already set");
+                s_handler = handler;
+                s_nonFatalHandler = nonFatalHandler;
             }
         }
 
@@ -72,25 +68,31 @@ public static void OverwriteHandler(ErrorReporterHandler? value)
         /// This file is in linked into multiple layers, but we want to ensure that all layers have the same copy.
         /// This lets us copy the handler in this instance into the same in another instance.
         /// </remarks>
-        public static void CopyHandlerTo(Assembly assembly)
+        public static void CopyHandlersTo(Assembly assembly)
         {
-            var targetType = assembly.GetType(typeof(FatalError).FullName!, throwOnError: true)!;
-            var targetHandlerProperty = targetType.GetProperty(nameof(FatalError.Handler), BindingFlags.Static | BindingFlags.Public)!;
-            if (Handler is not null)
-            {
-                // We need to convert the delegate type to the type in the linked copy since they won't have identity.
-                var convertedDelegate = Delegate.CreateDelegate(targetHandlerProperty.PropertyType, Handler.Target, method: Handler.Method);
-                targetHandlerProperty.SetValue(obj: null, value: convertedDelegate);
-            }
-            else
+            copyHandlerTo(assembly, s_handler, nameof(s_handler));
+            copyHandlerTo(assembly, s_nonFatalHandler, nameof(s_nonFatalHandler));
+
+            static void copyHandlerTo(Assembly assembly, ErrorReporterHandler? handler, string handlerName)
             {
-                targetHandlerProperty.SetValue(obj: null, value: null);
+                var targetType = assembly.GetType(typeof(FatalError).FullName!, throwOnError: true)!;
+                var targetHandlerProperty = targetType.GetField(handlerName, BindingFlags.Static | BindingFlags.NonPublic)!;
+                if (handler is not null)
+                {
+                    // We need to convert the delegate type to the type in the linked copy since they won't have identity.
+                    var convertedDelegate = Delegate.CreateDelegate(targetHandlerProperty.FieldType, handler.Target, handler.Method);
+                    targetHandlerProperty.SetValue(obj: null, value: convertedDelegate);
+                }
+                else
+                {
+                    targetHandlerProperty.SetValue(obj: null, value: null);
+                }
             }
         }
 
         /// <summary>
         /// Use in an exception filter to report an error without catching the exception.
-        /// The error is reported by calling <see cref="Handler"/>.
+        /// The error is reported by calling <see cref="s_handler"/>.
         /// </summary>
         /// <returns><see langword="false"/> to avoid catching the exception.</returns>
         [DebuggerHidden]
@@ -101,7 +103,7 @@ public static bool ReportAndPropagate(Exception exception, ErrorSeverity severit
         }
 
         /// <summary>
-        /// Use in an exception filter to report an error (by calling <see cref="Handler"/>), unless the
+        /// Use in an exception filter to report an error (by calling <see cref="s_handler"/>), unless the
         /// operation has been cancelled. The exception is never caught.
         /// </summary>
         /// <returns><see langword="false"/> to avoid catching the exception.</returns>
@@ -117,7 +119,7 @@ public static bool ReportAndPropagateUnlessCanceled(Exception exception, ErrorSe
         }
 
         /// <summary>
-        /// <para>Use in an exception filter to report an error (by calling <see cref="Handler"/>), unless the
+        /// <para>Use in an exception filter to report an error (by calling <see cref="s_handler"/>), unless the
         /// operation has been cancelled at the request of <paramref name="contextCancellationToken"/>. The exception is
         /// never caught.</para>
         ///
@@ -152,7 +154,7 @@ public static bool ReportAndPropagateUnlessCanceled(Exception exception, Cancell
 
         /// <summary>
         /// Report an error.
-        /// Calls <see cref="Handler"/> and doesn't pass the exception through (the method returns true).
+        /// Calls <see cref="s_handler"/> and doesn't pass the exception through (the method returns true).
         /// This is generally expected to be used within an exception filter as that allows us to
         /// capture data at the point the exception is thrown rather than when it is handled.
         /// However, it can also be used outside of an exception filter. If the exception has not
@@ -175,7 +177,7 @@ public static bool ReportWithDumpAndCatch(Exception exception, ErrorSeverity sev
         }
 
         /// <summary>
-        /// Use in an exception filter to report an error (by calling <see cref="Handler"/>) and catch
+        /// Use in an exception filter to report an error (by calling <see cref="s_handler"/>) and catch
         /// the exception, unless the operation was cancelled.
         /// </summary>
         /// <returns><see langword="true"/> to catch the exception if the error was reported; otherwise,
@@ -192,7 +194,7 @@ public static bool ReportAndCatchUnlessCanceled(Exception exception, ErrorSeveri
         }
 
         /// <summary>
-        /// <para>Use in an exception filter to report an error (by calling <see cref="Handler"/>) and
+        /// <para>Use in an exception filter to report an error (by calling <see cref="s_handler"/>) and
         /// catch the exception, unless the operation was cancelled at the request of
         /// <paramref name="contextCancellationToken"/>.</para>
         ///
@@ -230,12 +232,28 @@ public static bool ReportAndCatchUnlessCanceled(Exception exception, Cancellatio
         // into many assemblies and finding the right type can be much harder.
         [MethodImpl(MethodImplOptions.NoInlining)]
         private static void Report(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized, bool forceDump = false)
+        {
+            ReportException(exception, severity, forceDump, s_handler);
+        }
+
+        /// <summary>
+        /// Used to report a non-fatal-watson (when possible) to report an exception.  The exception is not caught. Does
+        /// nothing if no non-fatal error handler is registered.  See the second argument to <see
+        /// cref="SetHandlers(ErrorReporterHandler, ErrorReporterHandler?)"/>.
+        /// </summary>
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        public static void ReportNonFatalError(Exception exception, ErrorSeverity severity = ErrorSeverity.Uncategorized, bool forceDump = false)
+        {
+            ReportException(exception, severity, forceDump, s_nonFatalHandler);
+        }
+
+        private static void ReportException(Exception exception, ErrorSeverity severity, bool forceDump, ErrorReporterHandler? handler)
         {
             // hold onto last exception to make investigation easier
             s_reportedException = exception;
             s_reportedExceptionMessage = exception.ToString();
 
-            if (s_handler == null)
+            if (handler == null)
             {
                 return;
             }
@@ -256,7 +274,7 @@ private static void Report(Exception exception, ErrorSeverity severity = ErrorSe
                 exception.Data[s_reportedMarker] = s_reportedMarker;
             }
 
-            s_handler(exception, severity, forceDump);
+            handler(exception, severity, forceDump);
         }
     }
 

src/Compilers/Core/Portable/Syntax/SyntaxNode.cs

@@ -10,6 +10,7 @@
 using System.Linq;
 using System.Text;
 using System.Threading;
+using Microsoft.CodeAnalysis.ErrorReporting;
 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Utilities;
 
@@ -1391,6 +1392,7 @@ public bool IsEquivalentTo(SyntaxNode node, bool topLevel = false)
         /// Serializes the node to the given <paramref name="stream"/>.
         /// Leaves the <paramref name="stream"/> open for further writes.
         /// </summary>
+        [Obsolete(SerializationDeprecationException.Text, error: false)]
         public virtual void SerializeTo(Stream stream, CancellationToken cancellationToken = default)
         {
             if (stream == null)
@@ -1403,10 +1405,26 @@ public virtual void SerializeTo(Stream stream, CancellationToken cancellationTok
                 throw new InvalidOperationException(CodeAnalysisResources.TheStreamCannotBeWrittenTo);
             }
 
+            // Report NFW to see if this is being used in the wild.
+            FatalError.ReportNonFatalError(new SerializationDeprecationException());
             using var writer = new ObjectWriter(stream, leaveOpen: true, cancellationToken);
             writer.WriteValue(Green);
         }
 
+        /// <summary>
+        /// Specialized exception subtype to make it easier to search telemetry streams for this specific case.
+        /// </summary>
+        private protected sealed class SerializationDeprecationException : Exception
+        {
+            public const string Text = "Syntax serialization support is deprecated and will be removed in a future version of this API";
+
+            public SerializationDeprecationException()
+                : base(Text)
+            {
+
+            }
+        }
+
         #region Core Methods
 
         /// <summary>

src/Compilers/Server/VBCSCompiler/BuildServerController.cs

@@ -127,7 +127,7 @@ internal int RunServer(
                 }
 
                 compilerServerHost.Logger.Log("Keep alive timeout is: {0} milliseconds.", keepAlive?.TotalMilliseconds ?? 0);
-                FatalError.Handler = FailFast.Handler;
+                FatalError.SetHandlers(FailFast.Handler, nonFatalHandler: null);
 
                 var dispatcher = new ServerDispatcher(compilerServerHost, clientConnectionHost, listener);
                 dispatcher.ListenAndDispatchConnections(keepAlive, cancellationToken);

src/Compilers/Shared/Csc.cs

@@ -21,7 +21,7 @@ internal Csc(string responseFile, BuildPaths buildPaths, string[] args, IAnalyze
 
         internal static int Run(string[] args, BuildPaths buildPaths, TextWriter textWriter, IAnalyzerAssemblyLoader analyzerLoader)
         {
-            FatalError.Handler = FailFast.Handler;
+            FatalError.SetHandlers(FailFast.Handler, nonFatalHandler: null);
 
             var responseFile = Path.Combine(buildPaths.ClientDirectory, CSharpCompiler.ResponseFileName);
             var compiler = new Csc(responseFile, buildPaths, args, analyzerLoader);

src/Compilers/Shared/Vbc.cs

@@ -21,7 +21,7 @@ internal Vbc(string responseFile, BuildPaths buildPaths, string[] args, IAnalyze
 
         internal static int Run(string[] args, BuildPaths buildPaths, TextWriter textWriter, IAnalyzerAssemblyLoader analyzerLoader)
         {
-            FatalError.Handler = FailFast.Handler;
+            FatalError.SetHandlers(FailFast.Handler, nonFatalHandler: null);
 
             var responseFile = Path.Combine(buildPaths.ClientDirectory, VisualBasicCompiler.ResponseFileName);
             var compiler = new Vbc(responseFile, buildPaths, args, analyzerLoader);

src/Compilers/Test/Utilities/CSharp/CSharpTestSource.cs

@@ -45,13 +45,15 @@ public static SyntaxTree Parse(
 
         private static void CheckSerializable(SyntaxTree tree)
         {
+#pragma warning disable CS0618 // Type or member is obsolete
             using var stream = new MemoryStream();
             var root = tree.GetRoot();
             root.SerializeTo(stream);
             stream.Position = 0;
 
             // verify absence of exception:
             _ = CSharpSyntaxNode.DeserializeFrom(stream);
+#pragma warning restore CS0618 // Type or member is obsolete
         }
 
         public SyntaxTree[] GetSyntaxTrees(CSharpParseOptions parseOptions, string sourceFileName = "")

src/Compilers/Test/Utilities/VisualBasic/BasicTestSource.vb

@@ -28,13 +28,15 @@ Public Structure BasicTestSource
     End Function
 
     Private Shared Sub CheckSerializable(tree As SyntaxTree)
+#Disable Warning BC40000 ' Type or member is obsolete
         Using stream = New MemoryStream()
             Dim root = tree.GetRoot()
             root.SerializeTo(stream)
             stream.Position = 0
 
             ' verify absence of exception
             VisualBasicSyntaxNode.DeserializeFrom(stream)
+#Enable Warning BC40000 ' Type or member is obsolete
         End Using
     End Sub
 

src/Compilers/VisualBasic/Portable/Syntax/VisualBasicSyntaxNode.vb

@@ -7,6 +7,7 @@ Imports System.Collections.ObjectModel
 Imports System.ComponentModel
 Imports System.Reflection
 Imports System.Threading
+Imports Microsoft.CodeAnalysis.ErrorReporting
 Imports Microsoft.CodeAnalysis.PooledObjects
 Imports Microsoft.CodeAnalysis.Text
 Imports Microsoft.CodeAnalysis.VisualBasic.Symbols
@@ -125,6 +126,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
         ''' <summary>
         ''' Deserialize a syntax node from a byte stream.
         ''' </summary>
+        <Obsolete(SerializationDeprecationException.Text, False)>
         Public Shared Function DeserializeFrom(stream As IO.Stream, Optional cancellationToken As CancellationToken = Nothing) As SyntaxNode
             If stream Is Nothing Then
                 Throw New ArgumentNullException(NameOf(stream))
@@ -134,6 +136,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
                 Throw New InvalidOperationException(CodeAnalysisResources.TheStreamCannotBeReadFrom)
             End If
 
+            ' Report NFW to see if this Is being used in the wild.
+            FatalError.ReportNonFatalError(New SerializationDeprecationException())
+
             Using reader = ObjectReader.TryGetReader(stream, leaveOpen:=True, cancellationToken:=cancellationToken)
                 If reader Is Nothing Then
                     Throw New ArgumentException(CodeAnalysisResources.Stream_contains_invalid_data, NameOf(stream))

src/Compilers/VisualBasic/Test/Syntax/Syntax/SerializationTests.vb

@@ -11,8 +11,9 @@ Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
 Imports Roslyn.Test.Utilities
 Imports Xunit
 
-Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests
+#Disable Warning BC40000 ' Type or member is obsolete
 
+Namespace Microsoft.CodeAnalysis.VisualBasic.UnitTests
     Public Class SerializationTests
 
         Private Sub RoundTrip(text As String, Optional expectRecursive As Boolean = True)
@@ -461,3 +462,5 @@ End Module
         End Sub
     End Class
 End Namespace
+
+#Enable Warning BC40000 ' Type or member is obsolete

src/ExpressionEvaluator/Core/Source/ResultProvider/ResultProvider.cs

@@ -48,7 +48,7 @@ public abstract class ResultProvider : IDkmClrResultProvider
 
         static ResultProvider()
         {
-            FatalError.Handler = FailFast.Handler;
+            FatalError.SetHandlers(FailFast.Handler, nonFatalHandler: null);
         }
 
         internal ResultProvider(IDkmClrFormatter2 formatter2, IDkmClrFullNameProvider fullNameProvider)

src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Logging/RoslynLogger.cs

@@ -31,8 +31,9 @@ public static void Initialize(ITelemetryReporter? reporter, string? telemetryLev
         {
             Contract.ThrowIfTrue(_instance is not null);
 
-            FatalError.Handler = ReportFault;
-            FatalError.CopyHandlerTo(typeof(Compilation).Assembly);
+            FatalError.ErrorReporterHandler handler = ReportFault;
+            FatalError.SetHandlers(handler, nonFatalHandler: handler);
+            FatalError.CopyHandlersTo(typeof(Compilation).Assembly);
 
             if (reporter is not null && telemetryLevel is not null)
             {

src/Interactive/HostProcess/InteractiveHostEntryPoint.cs

@@ -17,7 +17,7 @@ internal static class InteractiveHostEntryPoint
     {
         private static async Task<int> Main(string[] args)
         {
-            FatalError.Handler = FailFast.Handler;
+            FatalError.SetHandlers(FailFast.Handler, nonFatalHandler: null);
 
             // Disables Windows Error Reporting for the process, so that the process fails fast.
             SetErrorMode(GetErrorMode() | ErrorMode.SEM_FAILCRITICALERRORS | ErrorMode.SEM_NOOPENFILEERRORBOX | ErrorMode.SEM_NOGPFAULTERRORBOX);

src/Tools/IdeCoreBenchmarks/SerializationBenchmarks.cs

@@ -13,6 +13,8 @@
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Text;
 
+#pragma warning disable CS0618 // Type or member is obsolete
+
 namespace IdeCoreBenchmarks
 {
     [MemoryDiagnoser]
@@ -96,3 +98,5 @@ public void DeserializeSyntaxNode()
         }
     }
 }
+
+#pragma warning restore CS0618 // Type or member is obsolete