Address review feedback from Heejae: add DiagnosticDescriptor.WithOnException...

Address review feedback from Heejae: add DiagnosticDescriptor.WithOnException to create a new descriptor with hooked up exception handler for reporting exception diagnostics from LocalizableStrings, instead of mutating fields of LocalizableString implementations.

src/Compilers/CSharp/Test/Semantic/Diagnostics/DiagnosticAnalyzerTests.cs

@@ -3,10 +3,8 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.Immutable;
-using System.Globalization;
 using System.Linq;
 using System.Runtime.Serialization;
-using System.Threading;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
 using Microsoft.CodeAnalysis.Diagnostics;
@@ -936,6 +934,6 @@ public override void Initialize(AnalysisContext context)
                     ctxt.ReportDiagnostic(CodeAnalysis.Diagnostic.Create(Desciptor1, method.Locations[0], method.ToDisplayString()));
                 }, SymbolKind.Method);
             }
-        } 
+        }
     }
 }

src/Compilers/Core/AnalyzerDriver/AnalyzerDriver.projitems

@@ -15,8 +15,6 @@
     <Compile Include="$(MSBuildThisFileDirectory)DeclarationInfo.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)DiagnosticAnalysisContextHelpers.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)DiagnosticAnalyzerAction.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)DiagnosticDescriptorExtensions.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)DiagnosticStartAnalysisScope.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)IExceptionSafeLocalizableString.cs" />
   </ItemGroup>
 </Project>
\ No newline at end of file

src/Compilers/Core/AnalyzerDriver/AnalyzerManager.cs

@@ -183,17 +183,34 @@ public async Task<bool> GetAnalyzerHasDependentCompilationEndAsync(DiagnosticAna
                 analyzerExecutor.ExecuteAndCatchIfThrows(analyzer, () => { supportedDiagnostics = analyzer.SupportedDiagnostics; });
 
                 // Set the exception handler for exceptions from lazily evaluated localizable strings in the descriptors.
-                foreach (var descriptor in supportedDiagnostics)
-                {
-                    descriptor.SetOnLocalizableStringException(analyzer, analyzerExecutor.OnAnalyzerException);
-                }
-
-                return supportedDiagnostics;
+                return WithOnException(supportedDiagnostics, analyzer, analyzerExecutor.OnAnalyzerException);
             });
 
             return (ImmutableArray<DiagnosticDescriptor>)descriptors;
         }
 
+        private static ImmutableArray<DiagnosticDescriptor> WithOnException(
+            ImmutableArray<DiagnosticDescriptor> descriptors,
+            DiagnosticAnalyzer analyzer,
+            Action<Exception, DiagnosticAnalyzer, Diagnostic> onAnalyzerException)
+        {
+            Action<Exception> onException = ex =>
+            {
+                var diagnostic = AnalyzerExecutor.GetAnalyzerDiagnostic(analyzer, ex);
+                onAnalyzerException?.Invoke(ex, analyzer, diagnostic);
+            };
+
+            var builder = ImmutableArray.CreateBuilder<DiagnosticDescriptor>();
+
+            foreach (var descriptor in descriptors)
+            {
+                var newDescriptor = descriptor.WithOnException(onException);
+                builder.Add(newDescriptor);
+            }
+
+            return builder.ToImmutable();
+        }
+
         /// <summary>
         /// Returns true if all the diagnostics that can be produced by this analyzer are suppressed through options.
         /// </summary>

src/Compilers/Core/AnalyzerDriver/DiagnosticDescriptorExtensions.cs

-using System;
-using System.Collections.Generic;
-using System.Text;
-using Microsoft.CodeAnalysis;
-using Microsoft.CodeAnalysis.Diagnostics;
-
-namespace Microsoft.CodeAnalysis
-{
-    internal static class DiagnosticDescriptorExtensions
-    {
-        public static void SetOnLocalizableStringException(this DiagnosticDescriptor descriptor, DiagnosticAnalyzer owner, Action<Exception, DiagnosticAnalyzer, Diagnostic> onLocalizableStringException)
-        {
-            Action<Exception> onException = ex =>
-            {
-                if (onLocalizableStringException != null)
-                {
-                    var diagnostic = AnalyzerExecutor.GetAnalyzerDiagnostic(owner, ex);
-                    onLocalizableStringException(ex, owner, diagnostic);
-                }
-            };
-
-            ((IExceptionSafeLocalizableString)descriptor.Title).SetOnException(onException);
-            ((IExceptionSafeLocalizableString)descriptor.MessageFormat).SetOnException(onException);
-            ((IExceptionSafeLocalizableString)descriptor.Description).SetOnException(onException);
-        }
-    }
-}

src/Compilers/Core/AnalyzerDriver/IExceptionSafeLocalizableString.cs

-// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
-
-using System;
-
-namespace Microsoft.CodeAnalysis
-{
-    internal interface IExceptionSafeLocalizableString
-    {
-        void SetOnException(Action<Exception> onException);
-    }
-}

src/Compilers/Core/CodeAnalysisTest/Diagnostics/DiagnosticLocalizationTests.cs

@@ -261,9 +261,9 @@ private static void TestDescriptorIsExceptionSafeCore(DiagnosticDescriptor descr
             // Verify exceptions from LocalizableResourceString are raised if OnException is set.
             var exceptions = new List<Exception>();
             Action<Exception> onException = ex => exceptions.Add(ex);
-            ((IExceptionSafeLocalizableString)localizableTitle).SetOnException(onException);
-            ((IExceptionSafeLocalizableString)localizableMessage).SetOnException(onException);
-            ((IExceptionSafeLocalizableString)localizableDescription).SetOnException(onException);
+            localizableTitle = localizableTitle.WithOnException(onException);
+            localizableMessage = localizableMessage.WithOnException(onException);
+            localizableDescription = localizableDescription.WithOnException(onException);
 
             // Access and evaluate localizable fields.
             var unused1 = localizableTitle.ToString();

src/Compilers/Core/Portable/Diagnostic/DiagnosticDescriptor.cs

@@ -207,5 +207,26 @@ internal bool IsNotConfigurable()
         {
             return AnalyzerManager.HasNotConfigurableTag(this.CustomTags);
         }
+
+        /// <summary>
+        /// Hooks up a delegate to be invoked when the descriptor or any of it's fields throw an unhandled exception.
+        /// </summary>
+        /// <param name="onException"></param>
+        /// <returns></returns>
+        public DiagnosticDescriptor WithOnException(Action<Exception> onException)
+        {
+            var title = Title.WithOnException(onException);
+            var messageFormat = MessageFormat.WithOnException(onException);
+            var description = Description.WithOnException(onException);
+
+            if (ReferenceEquals(title, Title) &&
+                ReferenceEquals(messageFormat, MessageFormat) &&
+                ReferenceEquals(description, Description))
+            {
+                return this;
+            }
+
+            return new DiagnosticDescriptor(Id, title, messageFormat, Category, DefaultSeverity, IsEnabledByDefault, description, HelpLinkUri, (ImmutableArray<string>)CustomTags);
+        }
     }
 }

src/Compilers/Core/Portable/Diagnostic/LocalizableResourceString.ExceptionSafeLocalizableString.cs

@@ -9,12 +9,12 @@ public abstract partial class LocalizableString
         private sealed class ExceptionSafeLocalizableString : LocalizableString
         {
             private readonly LocalizableString _innerLocalizableString;
-            internal Action<Exception> OnException { get; set; }
+            private readonly Action<Exception> _onException;
 
-            public ExceptionSafeLocalizableString(LocalizableString innerLocalizableString)
+            public ExceptionSafeLocalizableString(LocalizableString innerLocalizableString, Action<Exception> onException = null)
             {
                 _innerLocalizableString = innerLocalizableString;
-                OnException = null;
+                _onException = onException;
             }
 
             public override bool Equals(LocalizableString other)
@@ -25,17 +25,27 @@ public override bool Equals(LocalizableString other)
                     other = otherExceptionSafe._innerLocalizableString;
                 }
 
-                return ExecuteAndCatchIfThrows(_innerLocalizableString.Equals, other, false, OnException);
+                return ExecuteAndCatchIfThrows(_innerLocalizableString.Equals, other, false, _onException);
             }
 
             public override int GetHashCode()
             {
-                return ExecuteAndCatchIfThrows(_innerLocalizableString.GetHashCode, 0, OnException);
+                return ExecuteAndCatchIfThrows(_innerLocalizableString.GetHashCode, 0, _onException);
             }
 
             public override string ToString(IFormatProvider formatProvider)
             {
-                return ExecuteAndCatchIfThrows(_innerLocalizableString.ToString, formatProvider, string.Empty, OnException);
+                return ExecuteAndCatchIfThrows(_innerLocalizableString.ToString, formatProvider, string.Empty, _onException);
+            }
+
+            internal override LocalizableString WithOnException(Action<Exception> onException)
+            {
+                if (onException == _onException)
+                {
+                    return this;
+                }
+
+                return new ExceptionSafeLocalizableString(_innerLocalizableString, onException);
             }
         }
     }

src/Compilers/Core/Portable/Diagnostic/LocalizableResourceString.FixedLocalizableString.cs

@@ -45,6 +45,12 @@ public override int GetHashCode()
             {
                 return _fixedString != null ? _fixedString.GetHashCode() : 0;
             }
+
+            internal override LocalizableString WithOnException(Action<Exception> onException)
+            {
+                // FixedLocalizableString can't throw.
+                return this;
+            }
         }
     }
 }

src/Compilers/Core/Portable/Diagnostic/LocalizableResourceString.cs

@@ -19,8 +19,7 @@ public sealed class LocalizableResourceString : LocalizableString, IObjectReadab
         private readonly ResourceManager _resourceManager;
         private readonly Type _resourceSource;
         private readonly string[] _formatArguments;
-
-        internal Action<Exception> OnException { get; set; }
+        private readonly Action<Exception> _onException;
 
         /// <summary>
         /// Creates a localizable resource string with no formatting arguments.
@@ -41,6 +40,7 @@ public LocalizableResourceString(string nameOfLocalizableResource, ResourceManag
         /// <param name="resourceSource">Type handling assembly's resource management. Typically, this is the static class generated for the resources file from which resources are accessed.</param>
         /// <param name="formatArguments">Optional arguments for formatting the localizable resource string.</param>
         public LocalizableResourceString(string nameOfLocalizableResource, ResourceManager resourceManager, Type resourceSource, params string[] formatArguments)
+            : this(nameOfLocalizableResource, resourceManager, resourceSource, onAnalyzerException: null, formatArguments: formatArguments)
         {
             if (nameOfLocalizableResource == null)
             {
@@ -61,12 +61,15 @@ public LocalizableResourceString(string nameOfLocalizableResource, ResourceManag
             {
                 throw new ArgumentNullException(nameof(formatArguments));
             }
+        }
 
+        private LocalizableResourceString(string nameOfLocalizableResource, ResourceManager resourceManager, Type resourceSource, Action<Exception> onAnalyzerException, params string[] formatArguments)
+        {
             _resourceManager = resourceManager;
             _nameOfLocalizableResource = nameOfLocalizableResource;
             _resourceSource = resourceSource;
+            _onException = onAnalyzerException;
             _formatArguments = formatArguments;
-            OnException = null;
         }
 
         private LocalizableResourceString(ObjectReader reader)
@@ -111,7 +114,7 @@ void IObjectWritable.WriteTo(ObjectWriter writer)
 
         public override string ToString(IFormatProvider formatProvider)
         {
-            return ExecuteAndCatchIfThrows(ToStringCore, formatProvider, string.Empty, OnException);
+            return ExecuteAndCatchIfThrows(ToStringCore, formatProvider, string.Empty, _onException);
         }
 
         private string ToStringCore(IFormatProvider formatProvider)
@@ -125,7 +128,7 @@ private string ToStringCore(IFormatProvider formatProvider)
 
         public override bool Equals(LocalizableString other)
         {
-            return ExecuteAndCatchIfThrows(EqualsCore, other, false, OnException);
+            return ExecuteAndCatchIfThrows(EqualsCore, other, false, _onException);
         }
 
         private bool EqualsCore(LocalizableString other)
@@ -140,7 +143,7 @@ private bool EqualsCore(LocalizableString other)
 
         public override int GetHashCode()
         {
-            return ExecuteAndCatchIfThrows(GetHashCodeCore, 0, OnException);
+            return ExecuteAndCatchIfThrows(GetHashCodeCore, 0, _onException);
         }
 
         private int GetHashCodeCore()
@@ -150,5 +153,15 @@ private int GetHashCodeCore()
                 Hash.Combine(_resourceSource.GetHashCode(),
                 Hash.CombineValues(_formatArguments))));
         }
+
+        internal override LocalizableString WithOnException(Action<Exception> onException)
+        {
+            if (onException == _onException)
+            {
+                return this;
+            }
+
+            return new LocalizableResourceString(_nameOfLocalizableResource, _resourceManager, _resourceSource, onException, _formatArguments);
+        }
     }
 }

src/Compilers/Core/Portable/Diagnostic/LocalizableString.cs

 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
 using System;
+using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis
 {
@@ -8,7 +9,7 @@ namespace Microsoft.CodeAnalysis
     /// A string that may possibly be formatted differently depending on culture.
     /// NOTE: Types implementing <see cref="LocalizableString"/> must be serializable.
     /// </summary>
-    public abstract partial class LocalizableString : IFormattable, IEquatable<LocalizableString>, IExceptionSafeLocalizableString
+    public abstract partial class LocalizableString : IFormattable, IEquatable<LocalizableString>
     {
         /// <summary>
         /// Formats the value of the current instance using the optionally specified format. 
@@ -55,24 +56,10 @@ internal LocalizableString MakeExceptionSafe()
             return new ExceptionSafeLocalizableString(this);
         }
 
-        void IExceptionSafeLocalizableString.SetOnException(Action<Exception> onException)
+        internal virtual LocalizableString WithOnException(Action<Exception> onException)
         {
-            if (this is FixedLocalizableString)
-            {
-                // FixedLocalizableString can't throw.
-                return;
-            }
-
-            var localizableResourceString = this as LocalizableResourceString;
-            if (localizableResourceString != null)
-            {
-                localizableResourceString.OnException = onException;
-                return;
-            }
-
-            // Must be a wrapped ExceptionSafeLocalizableString.
-            var exceptionSafeLocalizableString = (ExceptionSafeLocalizableString)this;
-            exceptionSafeLocalizableString.OnException = onException;
+            // Should only be invoked for our internal LocalizableString implementations.
+            throw ExceptionUtilities.Unreachable;
         }
 
         internal static T ExecuteAndCatchIfThrows<T>(Func<T> action, T defaulValueOnException, Action<Exception> onLocalizableStringException)

src/Compilers/Core/Portable/PublicAPI.txt

@@ -222,6 +222,7 @@ Microsoft.CodeAnalysis.DiagnosticDescriptor.Id.get
 Microsoft.CodeAnalysis.DiagnosticDescriptor.IsEnabledByDefault.get
 Microsoft.CodeAnalysis.DiagnosticDescriptor.MessageFormat.get
 Microsoft.CodeAnalysis.DiagnosticDescriptor.Title.get
+Microsoft.CodeAnalysis.DiagnosticDescriptor.WithOnException(System.Action<System.Exception> onException)
 Microsoft.CodeAnalysis.DiagnosticFormatter
 Microsoft.CodeAnalysis.DiagnosticSeverity
 Microsoft.CodeAnalysis.DiagnosticSeverity.Error = 3