fix: Adding FastBuffer* extension methods for multiple instantiations of a generic type causes runtime errors [MTT-3063] #2142
Conversation
… of a generic type causes runtime errors
| if (((ByReferenceType)parameters[1].ParameterType).ElementType.FullName == paramType.FullName && | ||
| ((ByReferenceType)parameters[1].ParameterType).ElementType.IsArray == paramType.IsArray) |
There was a problem hiding this comment.
| if (((ByReferenceType)parameters[1].ParameterType).ElementType.FullName == paramType.FullName && | |
| ((ByReferenceType)parameters[1].ParameterType).ElementType.IsArray == paramType.IsArray) | |
| if (((ByReferenceType)parameters[1].ParameterType).ElementType.FullName == paramType.FullName) |
Presumably you don't need the array check anymore since fullname should include []
There was a problem hiding this comment.
I THINK you're right on that, but I'm not confident enough in that thought to remove it. My thought was that leaving it wouldn't hurt anything, but I can't completely guarantee removing it won't... I think we have tests that should cover it if that breaks but again, not 100% sure that they exist or are thorough enough. I personally just kind of feel risk-averse in this part of the codebase.
| if (parameters[1].ParameterType.FullName == paramType.FullName && | ||
| parameters[1].ParameterType.IsArray == paramType.IsArray) |
There was a problem hiding this comment.
| if (parameters[1].ParameterType.FullName == paramType.FullName && | |
| parameters[1].ParameterType.IsArray == paramType.IsArray) | |
| if (parameters[1].ParameterType.FullName == paramType.FullName) |
| ((ByReferenceType)parameters[1].ParameterType).ElementType.FullName == paramType.FullName && | ||
| ((ByReferenceType)parameters[1].ParameterType).ElementType.IsArray == paramType.IsArray) |
There was a problem hiding this comment.
| ((ByReferenceType)parameters[1].ParameterType).ElementType.FullName == paramType.FullName && | |
| ((ByReferenceType)parameters[1].ParameterType).ElementType.IsArray == paramType.IsArray) | |
| ((ByReferenceType)parameters[1].ParameterType).ElementType.FullName == paramType.FullName) |
| @@ -225,62 +225,98 @@ public IEnumerator ExtensionMethodRpcTest() | |||
| var obj = new MyObject(256); | |||
| var obj2 = new MySharedObjectReferencedById(256); | |||
| var obj3 = new MyObjectPassedWithThisRef(256); | |||
| var intList = new List<int> { 5, 10, 15, 5, 1 }; | |||
| var strList = new List<string> { "foo", "bar", "baz", "qux" }; | |||
There was a problem hiding this comment.
nit: Maybe a test for nested generics (List<List<string>>) or multiple type generics (KeyValuePair<K, V>)? Shouldn't matter with the new code change so not terribly important - but not sure how comprehensive you're looking for the tests to be.
There was a problem hiding this comment.
In an ideal world I would like to make the tests much more comprehensive here. I'd also like to support fully generic extension methods. In the current world - keeping in mind that I've been out for a month and don't know how priorities might have shifted during that time - I didn't think the engineering effort to make them super comprehensive would be worth it.
JesseOlmer
deleted the
fix/fastbuffer_extension_methods_for_generic_types
branch
…of a generic type causes runtime errors [MTT-3063] (Unity-Technologies#2142) Fixed RPC codegen failing to choose the correct extension methods for FastBufferReader and FastBufferWriter when the parameters were a generic type (i.e., List<int>) and extensions for multiple instantiations of that type have been defined (i.e., List<int> and List<string>)
The code that was finding extension methods was not differentiating between different instantiations of generic types - for example, an extension method for
List<int>would match when searching forList<string>, and invalid bytecode would be created as a result.Unfortunately,
GenericInstanceTypedoesn't seem to be something that can be compared with==so I had to make it compare the names instead, which kind of feels bad but I couldn't find another way to do it.fixes #1582
Changelog
List<int>) and extensions for multiple instantiations of that type have been defined (i.e.,List<int>andList<string>)Testing and Documentation