Threading tests (#1520)

Threading tests to help enforce #1519 .
Includes a bunch of little fixes and reducing failure list.

Tests don't aggressively assert yes, too many failures. But we can at
least run and drive to 0.

---------

Co-authored-by: Mike Stall <jmstall@microsoft.com>

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

@@ -0,0 +1,23 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+using System;
+
+namespace Microsoft.PowerFx
+{
+    /// <summary>
+    /// This type is Not thread safe - even if the base class is marked as thread safe. 
+    /// This can be used to mark a known type as single-threaded.
+    /// Or cases when the derived type is not thread safe, but the base type is.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface)]
+    internal class NotThreadSafeAttribute : Attribute
+    {
+        /// <summary>
+        /// Initializes a new instance of the <see cref="NotThreadSafeAttribute"/> class.
+        /// </summary>
+        public NotThreadSafeAttribute()
+        {
+        }
+    }
+}

src/libraries/Microsoft.PowerFx.Core/Entities/External/IExternalEntityScope.cs

@@ -8,6 +8,7 @@ using Microsoft.PowerFx.Syntax;
 
 namespace Microsoft.PowerFx.Core.Entities
 {
+    [ThreadSafeImmutable]
     internal interface IExternalEntityScope
     {
         bool TryGetNamedEnum(DName identName, out DType enumType);

src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs

@@ -289,7 +289,7 @@ namespace Microsoft.PowerFx.Core.Functions
 
                 return _signatureConstraint;
             }
-            protected set => _signatureConstraint = value;
+            protected init => _signatureConstraint = value;
         }
 
         /// <summary>

src/libraries/Microsoft.PowerFx.Core/Public/Config/DeferredSymbolTable.cs

@@ -43,8 +43,9 @@ namespace Microsoft.PowerFx
 
         // Must be thread safe!!!        
         // We can have multiple threads reading; which means they may be populating the cache. 
-        private readonly object _lock = new object();        
+        private readonly object _lock = new object();
 
+        [ThreadSafeProtectedByLock(nameof(_lock))]
         private readonly SlotMap<NameLookupInfo?> _slots = new SlotMap<NameLookupInfo?>();
 
         // Override lookup.

src/libraries/Microsoft.PowerFx.Core/Public/Config/SymbolTable.cs

@@ -21,7 +21,8 @@ namespace Microsoft.PowerFx
     /// SymbolTables are mutable to support sessionful scenarios and can be chained together. 
     /// This is a publicly facing class around a <see cref="INameResolver"/>.
     /// </summary>
-    [DebuggerDisplay("{DebugName}")]
+    [DebuggerDisplay("{DebugName}")]
+    [NotThreadSafe]
     public class SymbolTable : ReadOnlySymbolTable, IGlobalSymbolNameResolver
     {
         private readonly SlotMap<NameLookupInfo?> _slots = new SlotMap<NameLookupInfo?>();

src/libraries/Microsoft.PowerFx.Core/Public/Config/SymbolTableOverRecordType.cs

@@ -23,6 +23,7 @@ namespace Microsoft.PowerFx
 
         // Mapping between slots and logical names on RecordType.
         // name --> Slot; used at design time to ensure same slot per field. 
+        [ThreadSafeProtectedByLock("_map")]
         private readonly Dictionary<string, NameSymbol> _map = new Dictionary<string, NameSymbol>();
 
         internal RecordType Type => _type;

src/libraries/Microsoft.PowerFx.Core/Public/DisplayNames/DisplayNameProvider.cs

@@ -10,7 +10,8 @@ using Microsoft.PowerFx.Core.UtilityDataStructures;
 using Microsoft.PowerFx.Core.Utils;
 
 namespace Microsoft.PowerFx.Core
-{
+{
+    [ThreadSafeImmutable]
     public abstract class DisplayNameProvider
     {
         public abstract bool TryGetLogicalName(DName displayName, out DName logicalName);

src/libraries/Microsoft.PowerFx.Core/Types/LazyTypeProvider.cs

@@ -17,6 +17,7 @@ namespace Microsoft.PowerFx.Core.Types
     /// Wrapper class, provides access to derived TryGetFieldType, as well as identity of lazy types via AggregateType.
     /// Also provides faciility to fully expand a level of the type when needed by operations such as Union/AddColumns/...
     /// </summary>
+    [ThreadSafeImmutable]
     internal sealed class LazyTypeProvider
     {
         private readonly ConcurrentDictionary<DName, DType> _expandedFields = new ();

src/libraries/Microsoft.PowerFx.Core/Types/TypeSymbolTable/DefinedTypeSymbolTable.cs

@@ -14,6 +14,7 @@ using Microsoft.PowerFx.Types;
 
 namespace Microsoft.PowerFx.Core
 {
+    [NotThreadSafe]
     internal class DefinedTypeSymbolTable : TypeSymbolTable, IGlobalSymbolNameResolver
     {
         private readonly BidirectionalDictionary<string, FormulaType> _definedTypes = new ();

src/libraries/Microsoft.PowerFx.Core/Types/TypedName.cs

@@ -9,6 +9,7 @@ namespace Microsoft.PowerFx.Core.Types
 {
     // Represents a (simple) name together with an DType.
     // TASK: 67008 - Make this public, or expose a public shim in Document.
+    [ThreadSafeImmutable]
     internal struct TypedName : IEquatable<TypedName>, ICheckable
     {
         public readonly DName Name;

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

@@ -2,10 +2,14 @@
 // Licensed under the MIT license.
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
+using System.Diagnostics;
+using System.Globalization;
 using System.Linq;
 using System.Reflection;
 using System.Runtime.CompilerServices;
+using Microsoft.PowerFx.Core.Texl.Builtins;
 using Xunit;
 
 namespace Microsoft.PowerFx.Core.Tests
@@ -14,7 +18,85 @@ namespace Microsoft.PowerFx.Core.Tests
     /// Analyze assemblies for thread safety issues. 
     /// </summary>
     public class AnalyzeThreadSafety
-    {
+    {
+        // Return true if safe, false on error. 
+        public static bool VerifyThreadSafeImmutable(Type t)
+        {
+            int errors = 0;
+
+            var flags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly;
+
+            // Check out all fields and properties. 
+            foreach (var prop in t.GetProperties(flags)) 
+            {
+                var name = prop.Name;
+                if (prop.CanWrite)
+                {
+                    var isInitKeyword = prop.SetMethod.ReturnParameter.GetRequiredCustomModifiers().Contains(typeof(System.Runtime.CompilerServices.IsExternalInit));
+                    if (!isInitKeyword)
+                    {
+                        // No mutable properties allowed. Init only ok. 
+                        Debugger.Log(0, string.Empty, $"{t.Name}.{name} has setter\r\n");
+                        errors++;
+                    }
+                }
+
+                Assert.True(prop.CanRead);
+
+                var propType = prop.PropertyType;
+
+                if (!IsTypeImmutable(propType))
+                {
+                    // valuetypes are copies, so no contention
+                    if (!prop.PropertyType.IsValueType) 
+                    {
+                        // Fail. 
+                        Debugger.Log(0, string.Empty, $"{t.Name}.{name} returns mutable value\r\n");
+                        errors++;
+                    }
+                }
+            }
+            
+            foreach (var field in t.GetFields(flags))
+            {
+                var name = field.Name;
+
+                if (name.StartsWith("<"))
+                {
+                    // Ignore compile generated fields.
+                    continue;
+                }
+
+                // ReadOnly
+                if (!field.IsInitOnly)
+                {
+                    Debugger.Log(0, string.Empty, $"{t.Name}.{name} is not readonly\r\n");
+                    errors++;
+                }
+
+                if (field.GetCustomAttributes<ThreadSafeProtectedByLockAttribute>().Any() ||
+                    IsTypeConcurrent(field.FieldType))
+                {
+                    continue;
+                }
+
+                if (!IsTypeImmutable(field.FieldType))
+                {
+                    // Fail. 
+                    Debugger.Log(0, string.Empty, $"{t.Name}.{name} returns mutable value\r\n");
+                    errors++;
+                }
+            }
+
+            if (errors > 0)
+            {
+                Debugger.Log(0, string.Empty, $"\r\n");
+                return false;
+            }
+
+            return true;
+        }
+
         // 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. 
@@ -105,6 +187,20 @@ namespace Microsoft.PowerFx.Core.Tests
             // Batch up errors so we can see all at once. 
             Assert.Empty(errors);
         }
+
+        private static bool IsTypeConcurrent(Type type)
+        {
+            if (type.IsGenericType)
+            {
+                var genericDef = type.GetGenericTypeDefinition();
+                if (genericDef == typeof(ConcurrentDictionary<,>))
+                {
+                    return true;
+                }
+            }
+
+            return false;
+        }
 
         private static bool IsFieldVolatile(FieldInfo field)
         {
@@ -119,12 +215,22 @@ namespace Microsoft.PowerFx.Core.Tests
         {
             // Primitives
             typeof(object),
-            typeof(string),
+            typeof(string),
+            typeof(System.Type),
             typeof(Random),
             typeof(DateTime),
             typeof(System.Text.RegularExpressions.Regex),
-            typeof(System.Numerics.BigInteger),
-        };
+            typeof(System.Numerics.BigInteger),
+            typeof(NumberFormatInfo),
+
+            // Generics        
+            typeof(IReadOnlyDictionary<,>),
+            typeof(IReadOnlyCollection<>),
+            typeof(IReadOnlyList<>),
+            typeof(Nullable<>),
+            typeof(IEnumerable<>),
+            typeof(KeyValuePair<,>)
+        };
 
         // If the instance is readonly, is the type itself immutable ?
         internal static bool IsTypeImmutable(Type t)
@@ -149,28 +255,20 @@ namespace Microsoft.PowerFx.Core.Tests
             // Collection classes should be a IReadOnly<T>. Verify their T is also safe.
             if (t.IsGenericType)
             {
-                var genericDef = t.GetGenericTypeDefinition();
-                if (genericDef == typeof(IReadOnlyDictionary<,>))
-                {
-                    // For a Dict<key,value>, need to make sure the values are also safe. 
-                    var valueArg = t.GetGenericArguments()[1];
-                    var isValueArgSafe = IsTypeImmutable(valueArg);
-                    return isValueArgSafe;
-                }
-
-                if (genericDef == typeof(IReadOnlyCollection<>))
-                {
-                    var keyArg = t.GetGenericArguments()[0];
-                    var isKeyArgSafe = IsTypeImmutable(keyArg);
-                    return isKeyArgSafe;
-                }
-
-                if (genericDef == typeof(IEnumerable<>)
-                    || genericDef == typeof(IReadOnlyList<>) || genericDef == typeof(Nullable<>))
-                {
-                    var valueArg = t.GetGenericArguments()[0];
-                    var isValueArgSafe = IsTypeImmutable(valueArg);
-                    return isValueArgSafe;
+                var genericDef = t.GetGenericTypeDefinition();
+                if (_knownImmutableTypes.Contains(genericDef))
+                { 
+                    var typeArgs = t.GetGenericArguments();
+                    foreach (var arg in typeArgs)
+                    {
+                        var isArgSafe = IsTypeImmutable(arg) || arg.IsValueType;
+                        if (!isArgSafe)
+                        {
+                            return false;
+                        }
+                    }
+
+                    return true;
                 }
             }
 

src/tests/Microsoft.PowerFx.Core.Tests/ImmutabilityTests.cs

@@ -16,6 +16,9 @@ namespace Microsoft.PowerFx.Core.Tests
     /// </summary>
     internal class ImmutabilityTests : PowerFxTest
     {
+        // Include non-public types
+        // Include non-public properties!
+        // Are private-setters ok?
         public static void CheckImmutability(Assembly asm)
         {
             var errors = new StringBuilder();

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

@@ -3,6 +3,9 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
+using Microsoft.PowerFx.Core;
 using Microsoft.PowerFx.Core.Tests;
 using Xunit;
 
@@ -20,5 +23,57 @@ namespace Microsoft.PowerFx.Interpreter.Tests
 
             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/1519, enable assert here. 
+        [Fact]
+        public void CheckImmutableType()
+        {
+            // 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));
+
+            var assemblies = new Assembly[] 
+            {
+                typeof(RecalcEngine).Assembly,
+                typeof(Types.FormulaType).Assembly
+            };
+
+            foreach (var assembly in assemblies)
+            {
+                foreach (Type type in assembly.GetTypes())
+                {
+                    // includes base types 
+                    var attr = type.GetCustomAttribute<ThreadSafeImmutableAttribute>();
+                    if (attr == null)
+                    {
+                        continue;
+                    }
+
+                    // Common pattern is a writeable derived type (like Dict vs. IReadOnlyDict). 
+                    var attrNotSafe = type.GetCustomAttribute<NotThreadSafeAttribute>(inherit: false);
+                    if (attrNotSafe != null)
+                    {
+                        attr = type.GetCustomAttribute<ThreadSafeImmutableAttribute>(inherit: false);
+                        if (attr != null)
+                        {
+                            Assert.True(false); // Class can't have both safe & unsafe together. 
+                        }
+
+                        continue;
+                    }
+
+                    bool ok = AnalyzeThreadSafety.VerifyThreadSafeImmutable(type);
+
+                    // Enable this, per  https://github.com/microsoft/Power-Fx/issues/1519
+                    // Assert.True(ok);                
+                }
+            }
+        }
     }
 }