Resolve ambiguity in function pointer resolution.

[ImTheSquid]

Grain had an issue where its use of Normal function pointers led to resultion ambiguity that caused incorrect behavior. For example:

let t = 0; [1,2,3].for_each(|i| t += i); t

Before, Grain would bind i to this, resulting in an incorrect value for t of 6 instead of 3. Now this no longer happens due to my introduction of FnPtrType::Compiled, which properly tracks function pointer information for Grain calls.

[schungx]

Yes, I found that too. Because VM scripted functions are actually registered as a native callback, it threw off Rhai's logic. Therefore we have problems with functions like reduce and, of course, your example above.

Also it necessitates very convoluted things like makes_fn_pointers and takes_this... Routing stuff back to the AST walker.

I have been thinking of introducing a new VM-callback type. You beat me to it!

[schungx]

In this case, can we get rid of makes_fn_pointers and takes_this, thereby simplifying the VM a whole lot?

At least I believe makes_fn_pointers would have no need to exist, since its whole existence is due to needing to send the callback eval back to the AST walker. Also, I would consider it quite dangerous, as functions may create FnPtr's as their return values and the VM would have no way to predict whether function pointers will ever exist inside a program.

Also, there should now be no distinction between run and run_with_callback, as we can always provide a wrapper module by default... This would simplify the VM API...

[schungx]

I suggest we move this mechanism into call_raw itself, instead of inside _call_with_extra_args.

That is because call_raw is the main work-horse, while _call_with_extra_args is only intended for flexible-args type of callbacks. Most callbacks do not use those, but they'd still need to distinguish a VM-registered callback (because of where to put the this pointer).

Therefore most callbacks will actually land back into call_raw and you have the same problem that declared_shape is designed to fix. So move it to call_raw instead by removing the this_ptr if it is not necessary.

In fact, it can simply be like this:

// Inside `call_raw`...
// Embedded native Rust function?
match self.typ {
    FnPtrType::Native(ref func) => {
       :
    }
    #[cfg(all(feature = "grain", not(feature = "no_function")))]
    FnPtrType::Compiled { ref program, index } => {
        // Do we have to search the functions every time?
        // Can't we just store a `bind_this`  boolean here?
        if !program
            .functions()
            .get(index as usize)
            .map(|f| f.takes_this)
            .unwrap_or(false)
        {
            // The VM-compiled function has no need for `this`,
            // so remove `this_ptr`!
            this_ptr = None;
        }
    }
    _ => ()
}

// If `FnPtrType::Compiled` stores a boolean `bind_this` instead,
// it becomes this one-liner: FnPtrType::Compiled { bind_this: false } => this_ptr = None

_call_with_extra_args can revert back to previous, because all that's need to handle the situation would already be done.

There will also be no need for declared_shape as it can simply be inlined into the change as above. So the changes to fn_ptr.rs would actually be minimal.

[ImTheSquid]

The problem is that the way the arguments are laid out with args and extras, it can be ambiguous as to what type of resolution should occur, thus the logic for FnPtrType::Script. This PR essentially just applies the same logic to VM-managed functions. Your solution would result in incorrect behavior because it doesn't have the knowledge of the preferred function call arrangement from the native call.

[schungx]

Well, I think it actually can be done. And we can also do away with the entire needs_walker and falling back to AST-walking for closure functions using the this pointer.

In callback::wrappers, right now you skip all functions using this for this reason, falling back to including the AST for walker evaluation.

With FnPtrType::Compiled you don't have to. You can simply just register that function. And there will no longer be the need to needs_walker etc.

You just register two different types of wrappers: One without this usage, and one with this. The version with this usage (say invoke_with_this) simply pops the first argument as the this pointer, then go on with the rest.

[schungx]

it can be ambiguous as to what type of resolution should occur

Actually it isn't ambiguous. There is only one type of function VM-generated, which is a script. It can never be Rust-native. Therefore, it always would expect the this pointer in the zeroth position, just like Script.

The only difference whether Rhai would know to put that this_ptr in when the VM needs it, or it doesn't if the VM doesn't want it (so it won't end up with an extra argument).

That's why I'm proposing changing this to a simple bool.

[schungx]

@ImTheSquid Actually I have a version that does exactly this:

1. Removed needs_walker
2. Removed lib from Program and Parts
3. Only has one bind_this: bool in FnPtrType::Compiled
4. No falling back to AST-walking
5. Successfully handle callbacks in the VM that uses this
6. Successfully handle callbacks in the VM not using this

I'm trying to consolidate all of this such that it doesn't matter whether the VM function uses this...

Do you know how I can put a PR on top of this PR?

[ImTheSquid]

Removing arity from the pointer type results in a significant performance hit of like 50%. That needs to stay in. I'm also working on my own fixes and I am taking some of your suggestions, give me a bit please.

[schungx]

Gotcha. Just doing some experimentation on my side as well.

I don't see how the arity issue should affect performance. In fact it should simplify the situation.

That's because a compiled chunk has no arity conflicts. You can consider them to always be arity + 1, which are the arguments plus a this. It doesn't matter that the this pointer is not used. How I do it right now is, if it doesn't need it, Rhai binds it to a dummy temp with () in it.

So the arity issue is solved. The only thing remaining is to tell Rhai that this is actually a scripted functions instead of a native function. Thats what Compiled tells it.

[ImTheSquid]

I mean I ran a performance benchmark and it was significantly reduced, you can run the bench too and see what results you get. Sometimes this is unbound iirc so you can't just assume.

[schungx]

Hhhhmmm.... let me run that again. I ran it before and the difference was within error bars...

Right now in the experiment I cheated it: if this is unbound, then it'll be sent to a dummy with () as value.

Technically speaking, it should be returning with an ErrorUnboundThis...

[schungx]

Perhaps this should be declared as impl FnPtrType?

Then it is self.typ.declared_shape()....

[schungx]

In fact, I'd suggest removing this altogether when we move the logic inside call_raw, which simplifies the whole thing.

[ImTheSquid]

See my comment on the move

[schungx]

@ImTheSquid #1144 is there as a proof of concept.

[schungx]

@ImTheSquid I've been thinking a bit more on this topic. We might have been going at in from the wrong direction.

The Root of the Problem

The question is: what caused this call-shape problem?

It is the distinction between a scripted function and a native Rust function. In particular, a native Rust function cannot accept the this pointer... which I admit is likely a major design flaw but one that was based on history. It was originally implemented this way from the very first versions.

Due to the fact that a native Rust function cannot accept the receiver of the function call, Rhai was adapted to allow putting the receiver as the first &mut slot. Which turns into the convention that obj.foo(1,2,3) is the same as foo(obj, 1, 2, 3) if foo is a native Rust function.

In conclusion: the root of the problem is inconsistent treatment of function call shape between scripted and native functions.

Our Problem

Our problem arose because the VM registers a bunch of callbacks. They are native functions, but they redirect back to the VM running scripted functions. This mismatch is causing all sorts of problems, because Rhai simply doesn't know how to treat this.

This behavior muddles the hard distinction between scripted and native functions assumed all over Rhai code.

Maybe...

Maybe what we need is something different.

Why register a native function for the callback? Why not register a scripted function instead?

The only problem stopping us is that we cannot provide an AST for the scripted function.

So, what we create a new AST node type just for this? Call it Stmt::GrainChunk...

Stmt::GrainChunk

Stmt::GrainChunk is created with a Box<dyn> containing the wrapper.

The VM registers its callbacks as ScriptFunc, with the AST body being only one statement: Stmt::GrainChung.

If Rhai sees Stmt::GrainChunk, it creates an EvalContext, then calls a standard VM function with that wrapper, and the VM can do the rest.

This will be a surgical change, with minimal changes on both sides, and removes the muddling of the native/scripted mismatch.