Store Property / Method list in ScriptInstanceData #647

[TitanNano]

Instead of accepting a reference to a slice of PropertyInfos or MethodInfos from the trait and then hoping that the length didn't change when freeing the list, we now store the lists inside the ScriptInstanceData.

When the engine requests to free the list pointer, we can use the list to recover the length and later drop the rust data as well.

Fixes #647 [edit Bromeon]
cc @lilizoey

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-650

[lilizoey]

The current implementation has some cases of UB that should be relatively easy to fix.

However on a more general point, by changing from returning slice references to an owned value, you've had to introduce the whole mutex + hashmap. is there a reason you chose to do this over just using Godot 4.3's new property list implementation which passes the length of the list to free_property_list? does this need to be available in a version of godot < 4.3?

[TitanNano]

is there a reason you chose to do this over just using Godot 4.3's new property list implementation which passes the length of the list to free_property_list? does this need to be available in a version of godot < 4.3?

@lilizoey I have a personal interest in keeping the compatibility with 4.2 as I am currently using it with 4.2 and 4.3 has not been released yet.

Regardless of that, the current implementation works, beside this issue, with 4.2, so I think it's quite heavy-handed to remove support for 4.2 entirely.

Additionally, the trait currently ensures that the &[PropertyInfo] outlives &self but it doesn't stop the slice from being mutated once the trait function returned. What happens when a Vec gets extended beyond its capacity and the buffer is reallocated? I assume it will have similar issues as what you pointed out with the Box. Therefore, I believe it's much saver for the unsafe code to hold ownership (and control) of the data it is handing raw pointers out to.

[Bromeon]

@lilizoey I have a personal interest in keeping the compatibility with 4.2 as I am currently using it with 4.2 and 4.3 has not been released yet.

We need to balance this need with the code complexity in gdext, and 4.3 is expected to be released in April. It would be quite a bit of extra effort for us to test and maintain two separate implementations across this version boundary, especially considering that not many people seem to use ScriptInstance at the moment.

Is using a 4.3-dev version not an option for you?

---

Regardless of that, the current implementation works, beside this issue, with 4.2, so I think it's quite heavy-handed to remove support for 4.2 entirely.

We would eventually still need to modernize it to make use of the improvements in 4.3, so the question is how we would want to do that. #[cfg] based conditional compilation? Or drop 4.2 support once 4.3 is released?

[TitanNano]

Is using a 4.3-dev version not an option for you?

I would prefer not to rely on a development version of the engine, but of course, if it is the best way forward, it is an acceptable compromise.

We would eventually still need to modernize it to make use of the improvements in 4.3, [...]

If the assumptions of my third paragraph are correct, we would have to keep most of this patch for 4.3 as well. Regrading

#[cfg] based conditional compilation? Or drop 4.2 support once 4.3 is released?

I would say whatever is more practical. What is the general idea for backwards compatibility with older engine versions?

[lilizoey]

Additionally, the trait currently ensures that the &[PropertyInfo] outlives &self but it doesn't stop the slice from being mutated once the trait function returned. What happens when a Vec gets extended beyond its capacity and the buffer is reallocated? I assume it will have similar issues as what you pointed out with the Box. Therefore, I believe it's much saver for the unsafe code to hold ownership (and control) of the data it is handing raw pointers out to.

Doing so would require the user to use interior mutability to modify the vec while also handing out a reference to the contents of that vec. i'm not entirely sure what mechanism could be used to do that? if you could then im pretty sure you could have UB even in safe rust, since you could then do this:

// imagine some type that stores a vec it can both mutate and hand out a slice-reference to. 
impl SomeType {
  fn mutating_getter(&self) -> &[i32] {
    self.mutate_vec();
    self.get_vec_ref()
  }
}

fn main() {
  let foo = SomeType::new();
  let slice_1 = foo.mutating_getter();
  let slice_2 = foo.mutating_getter(); // this might then invalidate slice_1?
  println!("{slice_1:?}"); // this might be UB then 
}

[lilizoey]

Ok so i think there is a way. since we dont track the fact that there's a shared reference to the instance when execution is handed over to godot, that means that gd-cell potentially doesn't know that it can't hand out mutable references. this would let the vec be mutated and the slice may get invalidated.

So to solve this we would either need to:

1. store the shared ref guard gd-cell hands out, this likely needs mutex + hashmap too
2. add some way to "fake" a shared reference with gd-cell, for instance a pair of methods increment_shared_ref and an unsafe decrement_shared_ref.

additionally there's this solution.

finally i suspect it may be possible to avoid owning any values, and just temporarily leaking the strings in propertyinfo before reclaiming them in the free method. this might require a bit more work though so i dont think it's something that we should do in this PR. but it could work better as a long term solution.

[TitanNano]

@lilizoey i was thinking of something along the lines of:

-> engine calls get_property_list(...)
-> engine calls call("method_that_can_mutate_the_property_list") in an other thread
-> engine accesses a pointer in the property list

---

I think solution 1. Is not really viable as we would have to make the trait receive and return the refguard which is something we can't do as GdCell types are to remain private. This was also my first approach but it felt too cumbersome for the implementor.

---

How should we move forward for now?

[lilizoey]

I think it's fine to keep this solution as is for now. (though fix the couple of UB issues i identified ofc)

Maybe in the future we can look into one of the other solutions as i think they need some more infrastructure work. Since i think i agree that solution 1 isn't going to be the best and all the other solutions require figuring out some other things in the backend, either in gd-cell or propertyinfo.

Besides we do need a way to get the list length in godot 4.2 so we can't really avoid something along these lines until 4.3 is stable.

[lilizoey]

I think this looks good! Apart from that you left in some allows by accident?

[lilizoey]

Looks good, thanks!

The CI error seems unrelated to this PR, so im going to just approve it for now. I'm not very familiar with how that part of the CI works so i think that's something you need to fix @Bromeon? I tried rerunning the job once but it still failed. Feel free to merge this when it works.

[Bromeon]

Thank you!

[Bromeon]

Let's see if the fix in #657 works...