Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve function interop #1074

Merged
merged 2 commits into from
Feb 4, 2022
Merged

Improve function interop #1074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8017db6
script functions should resolve to same reference
KurtGokhan Feb 3, 2022
d07d779
fix construct crashing without context
KurtGokhan Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 67 additions & 0 deletions Jint.Tests/Runtime/FunctionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,5 +158,72 @@ void addListener(Func<JsValue, JsValue[], JsValue> callback)
ev(null, new JsValue[] { 20 });
Assert.Equal(30, engine.Evaluate("a"));
}


[Fact]
public void BoundFunctionsCanBePassedToHost()
{
var engine = new Engine();
Func<JsValue, JsValue[], JsValue> ev = null;

void addListener(Func<JsValue, JsValue[], JsValue> callback)
{
ev = callback;
}

engine.SetValue("addListener", new Action<Func<JsValue, JsValue[], JsValue>>(addListener));

engine.Execute(@"
var a = 5;

(function() {
addListener(function (acc, val) {
a = (val || 0) + acc;
}.bind(null, 10));
})();
");

Assert.Equal(5, engine.Evaluate("a"));

ev(null, new JsValue[0]);
Assert.Equal(10, engine.Evaluate("a"));

ev(null, new JsValue[] { 20 });
Assert.Equal(30, engine.Evaluate("a"));
}

[Fact]
public void ConstructorsCanBePassedToHost()
{
var engine = new Engine();
Func<JsValue, JsValue[], JsValue> ev = null;

void addListener(Func<JsValue, JsValue[], JsValue> callback)
{
ev = callback;
}

engine.SetValue("addListener", new Action<Func<JsValue, JsValue[], JsValue>>(addListener));

engine.Execute(@"addListener(Boolean)");

Assert.Equal(true, ev(JsValue.Undefined, new JsValue[] { "test" }));
Assert.Equal(true, ev(JsValue.Undefined, new JsValue[] { 5 }));
Assert.Equal(false, ev(JsValue.Undefined, new JsValue[] { false }));
Assert.Equal(false, ev(JsValue.Undefined, new JsValue[] { 0}));
Assert.Equal(false, ev(JsValue.Undefined, new JsValue[] { JsValue.Undefined }));
}


[Fact]
public void FunctionsShouldResolveToSameReference()
{
var engine = new Engine();
engine.SetValue("equal", new Action<object, object>(Assert.Equal));
engine.Execute(@"
function testFn() {}
equal(testFn, testFn);
");
}
}
}
2 changes: 1 addition & 1 deletion Jint/Native/Function/FunctionInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ internal void OrdinaryCallBindThis(ExecutionContext calleeContext, JsValue thisA
}
else
{
if (thisArgument.IsNullOrUndefined())
if (thisArgument is null || thisArgument.IsNullOrUndefined())
{
var globalEnv = calleeRealm.GlobalEnv;
thisValue = globalEnv.GlobalThisValue;
Expand Down
3 changes: 2 additions & 1 deletion Jint/Native/Function/ScriptFunctionInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
{
try
{
var result = OrdinaryCallEvaluateBody(_engine._activeEvaluationContext, arguments, calleeContext);
var context = _engine._activeEvaluationContext ?? new EvaluationContext(_engine);
var result = OrdinaryCallEvaluateBody(context, arguments, calleeContext);
Comment on lines -141 to +142
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// The DebugHandler needs the current execution context before the return for stepping through the return point
if (_engine._isDebugMode && result.Type != CompletionType.Throw)
Expand Down
5 changes: 2 additions & 3 deletions Jint/Native/Object/ObjectInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,10 +959,9 @@ private object ToObject(ObjectTraverseStack stack)
break;

case ObjectClass.Function:
if (this is FunctionInstance function)
if (this is ICallable function)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah the BoundFunction... OK that was the pain point we discussed earlier. Well it's a start to make ICallable internal at least..

{
converted = new Func<JsValue, JsValue[], JsValue>(
(thisVal, args) => function.Engine.Invoke(function, (object) thisVal, args));
converted = (Func<JsValue, JsValue[], JsValue>) function.Call;
}

break;
Expand Down