Fixes delegates created from expressions. #585
Conversation
…e as their first argument, causing mapping of parameters to fail.
| @@ -37,6 +38,12 @@ public DelegateWrapper(Engine engine, Delegate d) | |||
| public override JsValue Call(JsValue thisObject, JsValue[] jsArguments) | |||
| { | |||
| var parameterInfos = _d.Method.GetParameters(); | |||
|
|
|||
| if (parameterInfos.Length > 0 && parameterInfos[0].ParameterType.FullName == "System.Runtime.CompilerServices.Closure") | |||
There was a problem hiding this comment.
That was my first inclination, but System.Runtime.CompilerServices.Closure doesn't exist in dotnet framework - I tried adding and using pre-processor directives to only check if dotnet standard, but it didn't work with testing.
There was a problem hiding this comment.
Have your tried wrapping the code with
#if NETFRAMEWORK
... the code ...
#endif
Bot test case and this addition
There was a problem hiding this comment.
I had only done it on the addition before - I just applied it to both and now it works, but it is basically ignoring the test case, which is probably not desirable. Does it get tested with both flags?
There was a problem hiding this comment.
Could you please add this line to appveyor.yml which is in the root folder:
- dotnet test .\Jint.Tests\Jint.Tests.csproj -c Release -f net452
That should include the test in CI builds.
|
|
||
| if (parameterInfos.Length > 0 && parameterInfos[0].ParameterType.FullName == "System.Runtime.CompilerServices.Closure") | ||
| { | ||
| parameterInfos = parameterInfos.Skip(1).ToArray(); |
There was a problem hiding this comment.
Skip is a bit nasty as it uses LINQ and doesn't perform that well, especially on full framework, but this is fortunately quite isolated case.
There was a problem hiding this comment.
I updated the code to more performantly use Array.Copy, it looks like it is about 0.1 ticks faster on average, but is a fair amount more complicated. Alternatively I could use ArraySegment<> (or Span<> in C# 7), but that would change the signature downstream and make the non-NETFRAMEWORK case less performant.
There was a problem hiding this comment.
Yes it was bit of nitpicking, in Jint codebase there should be as little using System.Linq; as possible, which I see you kept 😉 LINQ is much wiser on .NET Core and can be used more safely, but it still adds some penalty - especially on allocation side. So ideally following the Roslyn ideology of not using it at all if possible.
Thanks for considering this too.
There was a problem hiding this comment.
Fixed lol.
I'm used to code bases where an extra object allocation is the least of it's sins 😄
…rage and only runs the test and check in NETFRAMEWORK
Fixes bug where delegates generated from expressions contain a closure as their first argument, causing mapping of parameters to fail.