Reenable [ThreadSafeImmutable] tests. (#2836)

These tests had been disabled and were not actually asserting on
failures.
Remove some #ifdefs !NET7_0_OR_GREATER

No real product change other than adding some more annotations. 

See #1519.

Add IRNode to list.

---------

Co-authored-by: Mike <[email protected]>

src/libraries/Microsoft.PowerFx.Connectors/Execution/HttpFunctionInvoker.cs

@@ -568,6 +568,7 @@ public async Task<FormulaValue> InvokeAsync(string url, BaseRuntimeConnectorCont
         }
     }
 
+    [ThreadSafeImmutable]
     internal interface IConvertToUTC
     {
         DateTime ToUTC(DateTimeValue d);

src/libraries/Microsoft.PowerFx.Core/Annotations/ThreadSafeImmutableAttribute.cs

@@ -7,6 +7,10 @@ namespace Microsoft.PowerFx
 {
     /// <summary>
     /// This type is thread safe because it's immutable. 
+    /// This means that:
+    /// - all of its properties/fields must also be immutable. 
+    /// - all derived classes are assumed to also be immutable, unless otherwise marked 
+    /// as <see cref="NotThreadSafeAttribute"/>.
     /// </summary>
     [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface)]
     internal class ThreadSafeImmutableAttribute : Attribute

src/libraries/Microsoft.PowerFx.Core/IR/IRContext.cs

@@ -6,7 +6,8 @@
 using Microsoft.PowerFx.Types;
 
 namespace Microsoft.PowerFx.Core.IR
-{
+{
+    [ThreadSafeImmutable]
     internal class IRContext
     {
         // This field may be null, for example when nodes are generated by the compiler

src/libraries/Microsoft.PowerFx.Core/IR/Nodes/IntermediateNode.cs

@@ -3,6 +3,8 @@
 
 namespace Microsoft.PowerFx.Core.IR.Nodes
 {
+    // IRNodes are immutable. But the actual IR can tree can be recreated and shared. 
+    [ThreadSafeImmutable]
     internal abstract class IntermediateNode
     {
         public IRContext IRContext { get; }

src/libraries/Microsoft.PowerFx.Interpreter/Marshal/ObjectMarshaller.cs

@@ -28,7 +28,7 @@ public class ObjectMarshaller : ITypeMarshaller
         /// </summary>
         public delegate (FormulaType fieldType, FieldValueMarshaller fieldValueMarshaller) FieldTypeAndValueMarshallerGetter();
 
-        private readonly Dictionary<string, FieldTypeAndValueMarshallerGetter> _fieldGetters; 
+        private readonly IReadOnlyDictionary<string, FieldTypeAndValueMarshallerGetter> _fieldGetters; 
 
         /// <inheritdoc/>
         FormulaType ITypeMarshaller.Type => Type;

src/tests/Microsoft.PowerFx.Connectors.Tests.Shared/ThreadingTests.cs

@@ -28,10 +28,6 @@ public void CheckConnector()
             AnalyzeThreadSafety.CheckStatics(asm, bugsFieldType, bugNames);
         }
 
-        // $$$ Supersedes ImmutabilityTests.
-        // This is more aggressive (includes private fields), but they don't all pass. So assert is disabled.
-        // Run this test under a debugger, and failure list is written to Debugger output window.
-        // Per https://github.com/microsoft/Power-Fx/issues/1561, enable assert here. 
         [Fact]
         public void CheckImmutableTypeInConnector()
         {
@@ -40,7 +36,14 @@ public void CheckImmutableTypeInConnector()
                 typeof(OpenApiParser).Assembly
             };
 
-            AnalyzeThreadSafety.CheckImmutableTypes(assemblies);
+            // https://github.com/microsoft/Power-Fx/issues/1561
+            // These types are marked as [ThreadSafeImmutable], but they fail the enforcement checks. 
+            var knownFailures = new HashSet<Type>
+            {
+                typeof(ConnectorSettings)
+            };
+
+            AnalyzeThreadSafety.CheckImmutableTypes(assemblies, knownFailures);
         }
     }
 }

src/tests/Microsoft.PowerFx.Core.Tests.Shared/AnalyzeThreadSafety.cs

@@ -9,7 +9,9 @@
 using System.Linq;
 using System.Reflection;
 using System.Runtime.CompilerServices;
+using System.Security.AccessControl;
 using System.Threading;
+using Microsoft.PowerFx.Core.Functions;
 using Xunit;
 
 namespace Microsoft.PowerFx.Core.Tests
@@ -19,11 +21,7 @@ namespace Microsoft.PowerFx.Core.Tests
     /// </summary>
     public class AnalyzeThreadSafety
     {
-        // We cannot use this code with .Net 5.0+ as it uses System.Runtime.CompilerServices.IsExternalInit hack defined in PFX.Core and which is only valid pre-Net 5.0
-        // The symbol would be defined twice
-#if !NET7_0_OR_GREATER
-        // Return true if safe, false on error. 
-        public static bool VerifyThreadSafeImmutable(Type t)
+        public static bool IsThreadSafeImmutable(Type t)
         {
             int errors = 0;
 
@@ -35,7 +33,7 @@ public static bool VerifyThreadSafeImmutable(Type t)
                 var name = prop.Name;
                 if (prop.CanWrite)
                 {
-                    var isInitKeyword = prop.SetMethod.ReturnParameter.GetRequiredCustomModifiers().Contains(typeof(System.Runtime.CompilerServices.IsExternalInit));
+                    var isInitKeyword = HasInitKeyword(prop);
                     if (!isInitKeyword)
                     {
                         // No mutable properties allowed. Init only ok. 
@@ -99,8 +97,7 @@ public static bool VerifyThreadSafeImmutable(Type t)
 
             return true;
         }
-#endif
-
+
         // Verify there are no "unsafe" static fields that could be threading issues.
         // Bugs - list of field types types that don't work. This should be driven to 0. 
         // BugNames - list of "Type.Field" that don't work. This should be driven to 0. 
@@ -192,22 +189,65 @@ public static void CheckStatics(Assembly asm, HashSet<Type> bugsFieldType, HashS
             Assert.Empty(errors);
         }
 
-        // $$$ Supersedes ImmutabilityTests.
-        // This is more aggressive (includes private fields), but they don't all pass. So assert is disabled.
-        public static void CheckImmutableTypes(Assembly[] assemblies, bool enableAssert = false)
+        // Does the property have 'init' keyword.
+        private static bool HasInitKeyword(PropertyInfo prop)
+        {
+            var attrs = prop.SetMethod.ReturnParameter.GetRequiredCustomModifiers();
+
+            // Can't use typeof() because it may not be available in .Net 5.0+
+            // So check type name instead. 
+            // typeof(System.Runtime.CompilerServices.IsExternalInit)
+            bool hasInit = attrs.Any(type => type.Name == "IsExternalInit");
+
+            return hasInit;
+        }
+
+        // Does this type, or any of its base types or interfaces have [ThreadSafeImmutable].
+        public static bool InheritsThreadSafeImmutable(Type type)
+        {
+            var interfaces = type.GetInterfaces();
+            foreach (var x in interfaces)
+            {
+                if (x.GetCustomAttributes().OfType<ThreadSafeImmutableAttribute>().Any())
+                {
+                    return true;
+                }
+            }
+
+            var attribute = type.GetCustomAttribute<ThreadSafeImmutableAttribute>(inherit: false);
+
+            if (attribute != null)
+            {
+                return true;
+            }
+
+            return false;
+        }
+
+        // Check all types in the assembly list that have or inherit [ThreadSafeImmutable].
+        // See output window for verbose details on failures.
+        // Excuse knownFailures (thhose should be tracked down and fixed separately). 
+        public static void CheckImmutableTypes(Assembly[] assemblies, HashSet<Type> knownFailures = null)
         {
+            var countPassed = new HashSet<string>();
+            var countFailed = new HashSet<string>();
+
             foreach (var assembly in assemblies)
             {
                 foreach (Type type in assembly.GetTypes())
                 {
+                    if (knownFailures != null && knownFailures.Contains(type))
+                    {
+                        continue;
+                    }
+
                     if (type.Name.StartsWith("<", StringComparison.OrdinalIgnoreCase))
                     {
                         continue; // exclude compiler generated closures. 
                     }
 
                     // includes base types 
-                    var attr = type.GetInterfaces().Select(x => x.GetCustomAttributes().OfType<ThreadSafeImmutableAttribute>());
-                    if (attr == null)
+                    if (!InheritsThreadSafeImmutable(type))
                     {
                         continue;
                     }
@@ -224,17 +264,23 @@ public static void CheckImmutableTypes(Assembly[] assemblies, bool enableAssert
 
                         continue;
                     }
-#if !NET7_0_OR_GREATER
-                    bool ok = AnalyzeThreadSafety.VerifyThreadSafeImmutable(type);
 
-                    // Enable this, per  https://github.com/microsoft/Power-Fx/issues/1519
-                    if (enableAssert)
+                    bool ok = AnalyzeThreadSafety.IsThreadSafeImmutable(type);
+                    if (ok)
                     {
-                        Assert.True(ok);
+                        countPassed.Add(type.FullName);
+                    }
+                    else
+                    {
+                        countFailed.Add(type.FullName);
                     }
-#endif
                 }
             }
+
+            Debugger.Log(0, string.Empty, $"{countPassed.Count} passed, {countFailed.Count} failed. {countPassed.Count + countFailed.Count} total.\r\n");
+
+            string failMsg = string.Join(", ", countFailed);
+            Assert.True(countFailed.Count == 0, $"Types failed. See output window: {failMsg}");
         }
 
         private static bool IsTypeConcurrent(Type type)
@@ -271,6 +317,8 @@ private static bool IsFieldVolatile(FieldInfo field)
             typeof(System.Text.RegularExpressions.Regex),
             typeof(System.Numerics.BigInteger),
             typeof(NumberFormatInfo),
+            typeof(CultureInfo),
+            typeof(TimeZoneInfo),
 
             // Generics        
             typeof(IReadOnlyDictionary<,>),
@@ -280,9 +328,11 @@ private static bool IsFieldVolatile(FieldInfo field)
             typeof(IEnumerable<>),
             typeof(KeyValuePair<,>),
             typeof(ISet<>),
+            typeof(IServiceProvider),
 
             // Concurrent types are thread safe.
-            typeof(ReaderWriterLockSlim)
+            typeof(ReaderWriterLockSlim),
+            typeof(System.Resources.ResourceManager)
         };
 
         // If the instance is readonly, is the type itself immutable ?

src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/ThreadingTests.cs

@@ -7,6 +7,7 @@
 using System.Reflection;
 using Microsoft.PowerFx.Core;
 using Microsoft.PowerFx.Core.Tests;
+using Microsoft.PowerFx.Functions;
 using Xunit;
 
 namespace Microsoft.PowerFx.Interpreter.Tests
@@ -31,22 +32,26 @@ public void CheckInterpreter()
         [Fact]
         public void CheckImmutableTypeInInterpreter()
         {
-#if !NET7_0_OR_GREATER
-            // Per https://github.com/microsoft/Power-Fx/issues/1519,
-            // Add ThreadSafeImmutable and get these to pass. 
-            AnalyzeThreadSafety.VerifyThreadSafeImmutable(typeof(Core.IR.Nodes.IntermediateNode));
-            AnalyzeThreadSafety.VerifyThreadSafeImmutable(typeof(ReadOnlySymbolValues));
-            AnalyzeThreadSafety.VerifyThreadSafeImmutable(typeof(ComposedReadOnlySymbolValues));
-            AnalyzeThreadSafety.VerifyThreadSafeImmutable(typeof(ParsedExpression));
-#endif
-
             var assemblies = new Assembly[] 
             {
                 typeof(RecalcEngine).Assembly,
                 typeof(Types.FormulaType).Assembly
             };
 
-            AnalyzeThreadSafety.CheckImmutableTypes(assemblies);
+            // https://github.com/microsoft/Power-Fx/issues/1519
+            // These types are marked as [ThreadSafeImmutable], but they fail the enforcement checks. 
+            var knownFailures = new HashSet<Type>
+            {
+                typeof(Core.Functions.TexlFunction),
+                typeof(Core.Types.DType),
+                typeof(Core.Utils.DPath),
+                typeof(PowerFx.Syntax.TexlNode),
+                typeof(ReadOnlySymbolTable),
+                typeof(FunctionInvokeInfo),
+                typeof(PowerFx.TableMarshallerProvider.TableMarshaller)
+            };
+
+            AnalyzeThreadSafety.CheckImmutableTypes(assemblies, knownFailures);
         }
     }
 }