Change C# reflection to avoid using expression trees #3794
Conversation
|
The original code does not work on platforms where reflection itself is prohibited ? |
|
Indeed - but the hope is that this will work on more platforms. In Unity (at least on some platforms), I believe reflection works, but expression trees don't. |
|
Humbug - doesn't quite work on Unity yet. At least one version of Mono used by Unity doesn't view enums and ints as convertible in the way that .NET does. Looking further... |
|
I've now managed to get this code formatting JSON correctly on a sample Unity3d project, so I think this PR is ready for review and merging. |
|
@jskeet there's been some progress towards supporting Unity in the grpc/grpc recently and some contributions have pointed out that I'd be good to have this PR merged. Just to doublecheck - are you currently aware of any limitations why this PR shouldn't be merged? |
|
I'm not aware of any problems, and I'd welcome further review. If anything, there should be a performance improvement as it doesn't require expression tree compilation... but this only affects reflection use cases (including JSON formatting and parsing). Ideally I'd like to see this tested in multiple environments that I don't have access to, but I have no reason to believe that it wouldn't at least work everywhere that the current code works. If you could review it carefully and add any comments, that would be good. |
| } | ||
| } | ||
|
|
||
| public enum SampleEnum |
There was a problem hiding this comment.
Note that this is already within an internal class, so it's not really public.
This should work on Unity, Mono and .NET 3.5 as far as I'm aware. It won't work on platforms where reflection itself is prohibited, but that's a non-starter basically.
For oneofs, to get the case, we need to call the property that returns the enum value. We really want it as an int, and modern runtimes allow us to create a delegate which returns an int from the method. (I suspect that the MS runtime has always allowed that.) Old versions of Mono (e.g. used by Unity3d) don't allow that, so we have to convert the enum value to an int via boxing. It's ugly, but it should work.
|
Rebased and force-pushed to get it back up to date and rerun the tests. |
| { | ||
| // .NET Core (at least netstandard1.0) doesn't have Delegate.CreateDelegate, and .NET 3.5 doesn't have | ||
| // MethodInfo.CreateDelegate. Proxy from one to the other on .NET 3.5... | ||
| internal static class MethodInfoExtensions |
There was a problem hiding this comment.
nit: only put #if NET35 around the static class, not around the whole file? (I think otherwise the conditional compilation is easy to miss when reading the code).
There was a problem hiding this comment.
I think it makes more logical sense for it to be for the whole file - otherwise on other targets the flie says "Here's a namespace declaration with no members being declared in it."
This matches what we've got in the other compatibility files, although in TypeExtensions it's after the using directives for no obvious reason.
| } | ||
|
|
||
| // Public to make the reflection simpler. | ||
| public static SampleEnum SampleEnumMethod() => SampleEnum.X; | ||
| } | ||
| } |
There was a problem hiding this comment.
Done, although I generally don't care about being consistent on that front.
| return Expression.Lambda<Func<IMessage, object>>(upcast, parameter).Compile(); | ||
| } | ||
| internal static Func<IMessage, object> CreateFuncIMessageObject(MethodInfo method) => | ||
| GetReflectionHelper(method.DeclaringType, method.ReturnType).CreateFuncIMessageObject(method); |
There was a problem hiding this comment.
It took me a while to understand the relationship between method.DeclaringType and IMessage here (and below).
If we are making a silent assumption that the methodInfo being passed needs to be of a method that is declared in a class that inherits from IMessage, we should make that more clear in the comment (looks like the method.DeclaringType is captured here as the concrete type of the protobuf message we are dealing with).
There was a problem hiding this comment.
Also the comment "cast the argument to the appropriate method target type" is not accurate IMHO. It cast the argument to method's declaring type.
There was a problem hiding this comment.
Rewritten, and then copy/pasted the new text to other comment.s
|
@anandolee @xfxyjwf any objections to me merging? |
|
Thanks for doing this! |
|
EDIT: It's working now. Forgot to disable the strip for Google.Protobuf in linker.xml.
|
|
Thanks for letting me know - I'll have a look as soon as I can. |
|
@jskeet No need to have a look now. Just check my edited comment. It's working now. I forgot to add the Google.Protobuf to linker to disable the strip. After I add it, it's working. |
|
@SetoKaiba: That's great to know, although it would be nice to be able to avoid that being required. |
|
@SetoKaiba are you running on a platform that requires AOT compilation or no? |
|
@ObsdianMinor of course requiring. ios il2cpp |
|
Just a small question, but do you know what kind of common function besides For now I would like to have a dirty fix to avoid those functions for the time being. For example if I want to just get a string representation, I can just dot the object to get the value and then https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.descriptor |
|
@5argon: Anything that wants to use reflection, basically. That's JSON parsing, creating a string representation, and anything your code might want to do with reflection itself. I'm not sure whether you're asking for us to create a "dirty fix" but I definitely don't want to spend any time finding a workaround when instead time needs to be spent understanding this properly. And regardless of whether you need string handling or not, other developers certainly will. All that being said, we still don't officially support Unity yet, so I'm not making any guarantees about what will happen. |
|
Hello @jskeet , sorry I made a mistype. (Not a native English speaker here) It's "I will just use a" instead of "I would like to have". Reading it again I am not sure why I typed that. Sorry that it comes out as offensive. That is to say for the time being I will just use a dirty fix for my own project, which is just avoid the funtions that uses the reflection until Unity correctly prevent the constructor to be stripped off. In that post I wanted to know is there anything else that I might come across so when I code I could avoid using them instead of encountering them at runtime. (I even thought ToString would enumerate all the variables in the generated code at first, I had no clue until the error comes up that it uses JSON) But per your reply if it is just JSON parsing and string representation then it is already very usable in Unity. |
|
@5argon: No problem - I'm sorry for getting so defensive :) I'm still surprised that this isn't fixable with linker configuration though. I definitely want to fix it properly; it's just a matter of finding time :( |
|
Earlier Unity team returned my report that they have been able to reproduce this "bug" that link.xml fails to prevent code stripping. Here's the link to track the bug report progress. https://fogbugz.unity3d.com/default.asp?1020952_kjitf02otf9p95rh |
|
@5argon: That's really good to know. It'll be interesting to see what they find out. |
|
They have replied :
Seems like IL2CPP needs knowledge about your custom protobuf class in the library code (Because of And also it is irrelevant to code stripping. ps. |
|
I'm just curious why my code does has relevant code generation while yours doesn't. EDIT: That's why I insist It's a bug jskeet should have a look at it instead of Unity in the later replies. |
|
@SetoKaiba "That's why I insist It's a bug" - well it's a problem with the Unity AOT compiler basically violating the expectations of normal .NET code. This kind of thing is precisely why I've been reluctant to support Unity from the start. While it would be nice to have, trying to operate in an environment where anything can go wrong in hard-to-predict ways makes it very hard to get anything done. Given that there can be any number of different enums involved, I'm not sure there's anything we can do here - the workaround helps in some situations but not others. I suspect that users will have to accept a restriction of "no reflection in Unity on IL2CPP platforms" until the workaround from Unity ships. |
|
@5argon: Thanks for including all of that detail. I'll get in touch with Unity to see if there are any options, but it really depends on exactly what combinations are problematic. If every enum counts as a different value type, there's no hope. If they treat all enums as basically being the same, we may have a chance, but I don't know when I'm going to have time to work on this. |
|
@jskeet First of all, I'm not blaming. I mean that the code for me doesn't strip the code generation for my protobuf classes. That's what I'm curious. Maybe it's something particular with his code. |
|
@SetoKaiba: I suspect that we'd need to know a lot of details about the exact configuration and versions of all kinds of things in order to work out why it works in one case but not another. That would certainly be a useful part of the next step in diagnosing the problem. But I'm not really in a position to investigate it further, not being a Unity developer or having protobuf as my main project. (The aim was always that I would work on the original C# implementation then move off the project. I can still do bits and pieces here and there, but not the sort of large-scale work that I believe this will require.) |
|
@jskeet He did say
So personally I think it is the best that we don't do anything at all. |
|
@5argon: Waiting for the situation to resolve itself is certainly the simplest option, although it won't be as helpful for developers using the existing versions of Unity. I've reached out by email so I can find out more and do a little exploratory work, but we'll see what happens. |
|
Good news: thanks to some great help from the Unity IL2CPP team, I've managed to create a repro for this with a simple console app, which will make it much easier to investigate. It's not yet clear what that will mean in terms of an end result, but I'm in a much better position to diagnose and test it. |
|
Great news. I found that I encountered 5argon's problem on other protobuf messages as well. |
|
@5argon @SetoKaiba if either or both of you are in a position to try out a new pull request, see #4559 Note that you'll still need a small amount of extra code in your application, e.g. // All generated enums should be listed here
FileDescriptor.ForceReflectionInitialization<Foo>();
FileDescriptor.ForceReflectionInitialization<Bar>();Note that you need to list all the enums you're using from any single source proto where you're using reflection with any of the types from that proto. That includes nested enums and the enums generated for one-of values. In the future we could potentially generate that along with the rest of the proto code, or I could write a small C# tool that would load an assembly and generate a single source file with all the enums for assemblies specified on the command line. But let's make sure it works first :) I believe you should then be okay without any other linker options. |
|
@jskeet Thank you for your great job. I'll test it later today. |
|
@jskeet Sorry for a late reply. Being busy before. It's working great with the latest merged master branch. |
|
@SetoKaiba: Excellent - glad it works. We can look into generating all the relevant code in the future, but unblocking everyone is at least a start :) |
|
while still exist such problem with protobuf 3.6.1 |
|
I have modify the protobuf code under .net framework 3.5,build xcode project in IL2CPP mode |
|
I've debuged here |
|
@unitymatrix: I'm afraid I don't fully understand your comments. But please note that this is only part of a workaround - in order to make this work at the moment, you have to call With those calls in place, it should be fine. If it isn't, please provide more detail, ideally in a complete example I could use to reproduce the problem. |
Change C# reflection to avoid using expression trees
|
Boy! this was one hell of a read. |
This should work on Unity, Mono and .NET 3.5 as far as I'm aware.
It won't work on platforms where reflection itself is prohibited,
but that's a non-starter basically.
Creating a PR to start with, but I'd like to test it in other environments before merging.
Once this is in, we can look at potentially officially supporting .NET 3.5 and Unity (#644).