Improve function interop #1074
Improve function interop #1074
Conversation
|
I think I may have introduced the old problem again. Will test this more before it is ready for merging. |
| @@ -959,10 +959,9 @@ private object ToObject(ObjectTraverseStack stack) | |||
| break; | |||
|
|
|||
| case ObjectClass.Function: | |||
| if (this is FunctionInstance function) | |||
| if (this is ICallable function) | |||
There was a problem hiding this comment.
There's a PR trying to hide ICallable and possibly trying to remove it at some point altogether if possible. In spec there are function objects (in Jint FunctionInstance) that may be callable (IsCallable)
There was a problem hiding this comment.
Are you suggesting I should check IsCallable? As I see it does this is ICallable for everything except Proxy.
Also this function is for converting script objects to their CLR counterpart. I don't think anything other than a function should be converted to a CLR function. So, proxies will not be hitting this line anyway.
As for removing ICallable, I can't imagine how it is going to work. If we agree on test cases, let's keep this for now and change when ICallable removed?
There was a problem hiding this comment.
Yeah this was just a note. Proxy is a good example that function object might not be callable, even though it has to implement ICallable if the target would happen to be callable. My thinking is that we should be able to get away by having FunctionInstance (write your own implementation), ScriptFunctionInstance (anything from script) and ClrFunctionInstance (basically native function). All these have Call method from FunctionInstance. IConstructor and ICallable are a bit artificial constructs, is all.
The more test coverage the better. Always comes back biting for advanced usage that people like you have when doing interop. All coverage is much appreciated!
There was a problem hiding this comment.
Ah, ok. It was FunctionInstance before but I noticed bound functions did not derive from FunctionInstance and ICallable seemed like the only thing that covers all of these cases.
There was a problem hiding this comment.
Ah the BoundFunction... OK that was the pain point we discussed earlier. Well it's a start to make ICallable internal at least..
| var result = OrdinaryCallEvaluateBody(_engine._activeEvaluationContext, arguments, calleeContext); | ||
| var context = _engine._activeEvaluationContext ?? new EvaluationContext(_engine); | ||
| var result = OrdinaryCallEvaluateBody(context, arguments, calleeContext); |
There was a problem hiding this comment.
I know this change is required (because _activeEvaluationContext is used this way everywhere else). My app was crashing because of this, but I could not write a test which reproduces the issue in isolation.
|
Ready for review |
The last fixes I did were not ideal. Same function would resolve to different reference, and bound functions would not work. Also
Callutility is a more proper way to solve this now I think.