Allow dynamic use objects of different, identically-named, types. (#25191)

* Allow dynamic use objects of different, identically-named, types.

Fixes #25190

* Remove ResetBindException

No longer used.

* Cache types' arity for FindSymWithMatchingArity instead of on each loop

* Move byref handling out of LoadSymbolsFromType()

Only one caller can possibly pass byref types, so handle it there.

* Further simplify type matching.

Since we're looking to match types, and types with different arity
don't match, just look for the type itself.

* Remove dead code and tidy up.

Paths within the case of the type .IsNullableType() handle the
possibility of the type itself being Nullable<T>, but that type
returns false for the nullable type check (possibly not true in
the static compiler?). Remove the branches for that, and tidy up.

* Move handling of nullable types.

Since there can be corresponding AggregateSymbols we have to test for
it anyway, so may as well test before any more work is done.

* Use InsertRange instead of repeated inserts.

Fewer allocations and copies.

Author: JonHanna

src/Microsoft.CSharp/src/Microsoft.CSharp.csproj

@@ -41,7 +41,6 @@
     <Compile Include="Microsoft\CSharp\RuntimeBinder\ExpressionTreeCallRewriter.cs" />
     <Compile Include="Microsoft\CSharp\RuntimeBinder\ICSharpBinder.cs" />
     <Compile Include="Microsoft\CSharp\RuntimeBinder\ICSharpInvokeOrInvokeMemberBinder.cs" />
-    <Compile Include="Microsoft\CSharp\RuntimeBinder\ResetBindException.cs" />
     <Compile Include="Microsoft\CSharp\RuntimeBinder\RuntimeBinder.cs" />
     <Compile Include="Microsoft\CSharp\RuntimeBinder\RuntimeBinderException.cs" />
     <Compile Include="Microsoft\CSharp\RuntimeBinder\RuntimeBinderExtensions.cs" />

src/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ResetBindException.cs

@@ -103,35 +103,7 @@ public Expression Bind(
 
             lock (_bindLock)
             {
-                // this is a strategy for realizing correct binding when the symboltable
-                // finds a name collision across different types, e.g. one dynamic binding
-                // uses a type "N.T" and now a second binding uses a different type "N.T".
-
-                // In order to make this work, we have to reset the symbol table and begin
-                // the second binding over again when we detect the collision. So this is
-                // something like a longjmp to the beginning of binding. For a single binding,
-                // if we have to do this more than once, we give an RBE--this would be a
-                // scenario that needs to know about both N.T's simultaneously to work.
-
-                // See SymbolTable.LoadSymbolsFromType for more information.
-
-                try
-                {
-                    return BindCore(binder, parameters, args, out deferredBinding);
-                }
-                catch (ResetBindException)
-                {
-                    Reset();
-                    try
-                    {
-                        return BindCore(binder, parameters, args, out deferredBinding);
-                    }
-                    catch (ResetBindException)
-                    {
-                        Reset();
-                        throw Error.BindingNameCollision();
-                    }
-                }
+                return BindCore(binder, parameters, args, out deferredBinding);
             }
         }
 

src/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/RuntimeBinder.cs

@@ -638,17 +638,9 @@ private TypeParameterType AddTypeParameterToSymbolTable(
         #region LoadTypeChain
         /////////////////////////////////////////////////////////////////////////////////
 
-        private CType LoadSymbolsFromType(Type originalType)
+        private CType LoadSymbolsFromType(Type type)
         {
-            List<object> declarationChain = BuildDeclarationChain(originalType);
-
-            Type type = originalType;
-            CType ret = null;
-            bool bIsByRef = type.IsByRef;
-            if (bIsByRef)
-            {
-                type = type.GetElementType();
-            }
+            List<object> declarationChain = BuildDeclarationChain(type);
 
             NamespaceOrAggregateSymbol current = NamespaceSymbol.Root;
 
@@ -657,106 +649,55 @@ private CType LoadSymbolsFromType(Type originalType)
             for (int i = 0; i < declarationChain.Count; i++)
             {
                 object o = declarationChain[i];
-                NamespaceOrAggregateSymbol next;
-                if (o is Type)
+                if (o is Type t)
                 {
-                    Type t = o as Type;
-                    next = _symbolTable.LookupSym(GetName(t), current, symbmask_t.MASK_AggregateSymbol) as AggregateSymbol;
-
-                    // Make sure we match arity as well when we find an aggregate.
-                    if (next != null)
+                    if (t.IsNullableType())
                     {
-                        next = FindSymWithMatchingArity(next as AggregateSymbol, t);
+                        return _typeManager.GetNullable(GetCTypeFromType(t.GetGenericArguments()[0]));
                     }
 
-                    // In the event that two different types exist that have the same name, they
-                    // cannot both have entries in the symbol table with our current architecture.
-                    // This can happen in dynamic, since the runtime binder lives across all
-                    // call sites in an appdomain, and assemblies can have been loaded at runtime
-                    // that have different types with the same name.
-
-                    // In the real compiler, this would have been an error and name lookup would
-                    // be ambiguous, but here we never have to lookup names of types for real (only
-                    // names of members).
-
-                    // The tactical fix is this: if we encounter this situation, where we have
-                    // identically named types that are not the same, then we are going to clear
-                    // the entire symbol table and restart this binding. This solution is not
-                    // without its own problems, since it is possible to conceive of a single
-                    // dynamic binding that needs to simultaneously know about both of the
-                    // similarly named types, but we are not going to try to solve that
-                    // scenario here.
-
-                    if (next != null)
-                    {
-                        Type existingType = (next as AggregateSymbol).AssociatedSystemType;
-                        Type newType = t.IsGenericType ? t.GetGenericTypeDefinition() : t;
-
-                        // We use "IsEquivalentTo" so that unified local types for NoPIA do
-                        // not trigger a reset. There are other mechanisms to make those sorts
-                        // of types work in some scenarios.
-                        if (!existingType.IsEquivalentTo(newType))
-                        {
-                            throw new ResetBindException();
-                        }
-                    }
+                    AggregateSymbol next = FindSymForType(
+                        _symbolTable.LookupSym(GetName(t), current, symbmask_t.MASK_AggregateSymbol), t);
 
                     // If we haven't found this type yet, then add it to our symbol table.
-                    if (next == null || t.IsNullableType())
+                    if (next == null)
                     {
                         // Note that if we have anything other than an AggregateSymbol,
                         // we must be at the end of the line - that is, nothing else can
                         // have children.
-
                         CType ctype = ProcessSpecialTypeInChain(current, t);
                         if (ctype != null)
                         {
-                            // If we had an aggregate type, its possible we're not at the end.
-                            // This will happen for nullable<T> for instance.
-                            if (ctype is AggregateType cat)
-                            {
-                                next = cat.GetOwningAggregate();
-                            }
-                            else
-                            {
-                                ret = ctype;
-                                break;
-                            }
-                        }
-                        else
-                        {
-                            // This is a regular class.
-                            next = AddAggregateToSymbolTable(current, t);
+                            Debug.Assert(!(ctype is AggregateType));
+                            return ctype;
                         }
+
+                        // This is a regular class.
+                        next = AddAggregateToSymbolTable(current, t);
                     }
 
                     if (t == type)
                     {
-                        ret = GetConstructedType(type, next as AggregateSymbol);
-                        break;
+                        return GetConstructedType(type, next);
                     }
+
+                    current = next;
                 }
-                else if (o is MethodInfo)
+                else if (o is MethodInfo m)
                 {
                     // We cant be at the end.
                     Debug.Assert(i + 1 < declarationChain.Count);
-                    ret = ProcessMethodTypeParameter(o as MethodInfo, declarationChain[++i] as Type, current as AggregateSymbol);
-                    break;
+                    return ProcessMethodTypeParameter(m, declarationChain[++i] as Type, current as AggregateSymbol);
                 }
                 else
                 {
                     Debug.Assert(o is string);
-                    next = AddNamespaceToSymbolTable(current, o as string);
+                    current = AddNamespaceToSymbolTable(current, o as string);
                 }
-                current = next;
             }
 
-            Debug.Assert(ret != null);
-            if (bIsByRef)
-            {
-                ret = _typeManager.GetParameterModifier(ret, false);
-            }
-            return ret;
+            Debug.Fail("Should be unreachable");
+            return null;
         }
 
         /////////////////////////////////////////////////////////////////////////////////
@@ -804,18 +745,17 @@ private CType GetConstructedType(Type type, AggregateSymbol agg)
 
         private CType ProcessSpecialTypeInChain(NamespaceOrAggregateSymbol parent, Type t)
         {
-            CType ctype;
             if (t.IsGenericParameter)
             {
                 AggregateSymbol agg = parent as AggregateSymbol;
                 Debug.Assert(agg != null);
-                ctype = LoadClassTypeParameter(agg, t);
-                return ctype;
+                return LoadClassTypeParameter(agg, t);
             }
-            else if (t.IsArray)
+
+            if (t.IsArray)
             {
                 // Now we return an array of nesting level corresponding to the rank.
-                ctype = _typeManager.GetArray(
+                return _typeManager.GetArray(
                     GetCTypeFromType(t.GetElementType()), 
                     t.GetArrayRank(),
 #if netcoreapp
@@ -824,37 +764,14 @@ private CType ProcessSpecialTypeInChain(NamespaceOrAggregateSymbol parent, Type
                     t.GetElementType().MakeArrayType() == t
 #endif
                     );
-                return ctype;
             }
-            else if (t.IsPointer)
+
+            if (t.IsPointer)
             {
                 // Now we return the pointer type that we want.
-                ctype = _typeManager.GetPointer(GetCTypeFromType(t.GetElementType()));
-                return ctype;
-            }
-            else if (t.IsNullableType())
-            {
-                // Get a nullable type of the underlying type.
-                if (t.GetGenericArguments()[0].DeclaringType == t)
-                {
-                    // If the generic argument for nullable is our child, then we're
-                    // declaring the initial Nullable<T>.
-                    AggregateSymbol agg = _symbolTable.LookupSym(
-                        GetName(t), parent, symbmask_t.MASK_AggregateSymbol) as AggregateSymbol;
-                    if (agg != null)
-                    {
-                        agg = FindSymWithMatchingArity(agg, t);
-                        if (agg != null)
-                        {
-                            Debug.Assert(agg.getThisType().AssociatedSystemType == t);
-                            return agg.getThisType();
-                        }
-                    }
-                    return AddAggregateToSymbolTable(parent, t).getThisType();
-                }
-                ctype = _typeManager.GetNullable(GetCTypeFromType(t.GetGenericArguments()[0]));
-                return ctype;
+                return _typeManager.GetPointer(GetCTypeFromType(t.GetElementType()));
             }
+
             return null;
         }
 
@@ -906,40 +823,41 @@ private static List<object> BuildDeclarationChain(Type callingType)
                     Debug.Assert(bAdded);
                 }
             }
+
             callChain.Reverse();
 
             // Now take out the namespaces and add them to the end of the chain.
-
             if (callingType.Namespace != null)
             {
-                string[] namespaces = callingType.Namespace.Split('.');
-                int index = 0;
-                foreach (string s in namespaces)
-                {
-                    callChain.Insert(index++, s);
-                }
+                callChain.InsertRange(0, callingType.Namespace.Split('.'));
             }
             return callChain;
         }
 
-        /////////////////////////////////////////////////////////////////////////////////
+        // We have an aggregate symbol of the correct parent and full name, but it may have the wrong arity, or, due to
+        // dynamic loading or creation, two different types can exist that have the same name.
+
+        // In the static compiler, this would have been an error and name lookup would be ambiguous, but here we never have
+        // to lookup names of types for real (only names of members).
 
-        private AggregateSymbol FindSymWithMatchingArity(AggregateSymbol aggregateSymbol, Type type)
+        // For either case, move onto the next symbol in the chain, and check again for appropriate type.
+        private AggregateSymbol FindSymForType(Symbol sym, Type t)
         {
-            for (AggregateSymbol agg = aggregateSymbol;
-                agg != null;
-                agg = BSYMMGR.LookupNextSym(agg, agg.Parent, symbmask_t.MASK_AggregateSymbol) as AggregateSymbol)
+            while (sym != null)
             {
-                if (agg.GetTypeVarsAll().Count == type.GetGenericArguments().Length)
+                // We use "IsEquivalentTo" so that unified local types match.
+                if (sym is AggregateSymbol agg)
+                if (agg.AssociatedSystemType.IsEquivalentTo(t.IsGenericType ? t.GetGenericTypeDefinition() : t))
                 {
                     return agg;
                 }
+
+                sym = sym.nextSameName;
             }
+
             return null;
         }
 
-        /////////////////////////////////////////////////////////////////////////////////
-
         private NamespaceSymbol AddNamespaceToSymbolTable(NamespaceOrAggregateSymbol parent, string sz)
         {
             Name name = GetName(sz);
@@ -974,10 +892,10 @@ internal CType[] GetCTypeArrayFromTypes(Type[] types)
 
         /////////////////////////////////////////////////////////////////////////////////
 
-        internal CType GetCTypeFromType(Type t)
-        {
-            return LoadSymbolsFromType(t);
-        }
+        internal CType GetCTypeFromType(Type type) => type.IsByRef
+            ? _typeManager.GetParameterModifier(LoadSymbolsFromType(type.GetElementType()), false)
+            : LoadSymbolsFromType(type);
+
         #endregion
 
         #region Aggregates

src/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/SymbolTable.cs

@@ -6,9 +6,6 @@
 using System.Linq;
 using System.Linq.Expressions;
 using System.Linq.Expressions.Tests;
-using System.Reflection;
-using System.Reflection.Emit;
-using System.Runtime.CompilerServices;
 using Microsoft.CSharp.RuntimeBinder;
 using Xunit;
 
@@ -748,27 +745,6 @@ public void BinaryCallSiteBinder_DynamicExpression(bool useInterpreter)
             Assert.Equal("42", func().ToString());
         }
 
-#if FEATURE_COMPILE // We're not testing compilation, but we do need Reflection.Emit for the test
-        [Fact]
-        public void OperationOnTwoObjectsDifferentTypesOfSameName()
-        {
-            object objX = Activator.CreateInstance(
-                AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("TestAssembly"), AssemblyBuilderAccess.Run)
-                    .DefineDynamicModule("TestModule").DefineType("TestType", TypeAttributes.Public).CreateType());
-            object objY = Activator.CreateInstance(
-                AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("TestAssembly"), AssemblyBuilderAccess.Run)
-                    .DefineDynamicModule("TestModule").DefineType("TestType", TypeAttributes.Public).CreateType());
-
-            CallSiteBinder binder =
-                Microsoft.CSharp.RuntimeBinder.Binder.BinaryOperation(
-                    CSharpBinderFlags.None, ExpressionType.Equal,
-                    GetType(), new[] { CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, null), CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, null) });
-            var cs = CallSite<Func<CallSite, object, object, object>>.Create(binder);
-            var t = cs.Target;
-            Assert.Throws<RuntimeBinderException>(() => t(cs, objX, objY));
-        }
-#endif
-
         private class BinaryCallSiteBinder : BinaryOperationBinder
         {
             public BinaryCallSiteBinder() : base(ExpressionType.Add) {}