Add iteration methods to Dictionary #99

lilizoey · 2023-01-31T13:35:26Z

add 2 methods to dictionary:

iter_shared for iterating over (Variant,Variant) key-values
keys_shared for iterating over Variant keys

in addition iterators have:

.typed() to convert an untyped iterator into a typed iterator

Bromeon

Thanks a lot for this pull request! Very nice to see the API grow 🙂

Now we have:

keys_array      -> Array
values_array    -> Array
iter_shared     -> Iter over (Variant, Variant)
iter_shared_as  -> Iter over (K, V)
keys_shared     -> Iter over Variant
keys_shared_as  -> Iter over K

But we don't have an iterator over values. If we added that, we would have 8 methods already. This adds quite a bit of API surface, and our "shared" naming convention has the potential to make things rather unreadable 😬

I wonder if it's possible to have a generic iterator adapter over these 🤔

dict.keys_shared()                  -> Iter over Variant
dict.keys_shared().typed::<K>()     -> Iter over K
dict.iter_shared().typed::<K, V>()  -> Iter over (K, V)
...

This could potentially be reusable for arrays as well.

We could even consider .array() or .collect::<Array>(); not sure how much indirection it would add. But generally it's really nice to have such composable building blocks 🏛️

Also, I'd start with a panicking adapter that directly returns the elements, rather than delivering Result<...> -- the Result is mostly useful if you want to extract the result out of iteration and combine with ?, and I'd guess that many people would just call try_from_variant() in that case.

godot-core/src/builtin/dictionary.rs

Bromeon · 2023-02-01T21:59:00Z

godot-core/src/builtin/dictionary.rs

+    /// Returns an iterator over the key-value pairs from the `Dictionary` by reference. Instead of
+    /// references to key-value pairs as you might expect, the iterator returns a (cheap, shallow)
+    /// copy of each key-value pair.


I would explicitly mention the (Variant, Variant) item type.

Also, "by reference" is unclear, see my comment in the other PR.

godot-core/src/builtin/dictionary.rs

Bromeon · 2023-02-01T22:04:57Z

godot-core/src/builtin/dictionary.rs

+        // The function returns a bool
+        assert!(success == 0 || success == 1);
+        let success = success != 0;
+
+        if success {
+            assert!(valid);
+            Some(variant)
+        } else {
+            None
+        }


I'd name the first variable is_valid rather than success, closer the parameter name in the header.

And then you can write this as:

match is_valid { 0 => None, 1 => Some(variant), _ => panic!("Invalid valid status {is_valid}"), }

that would make it misleading, as valid is a different boolean from success. valid represents whether the variant sent in is valid or not, whereas the return is whether the function succeeded at returning an iterator

I think it's what they use in the header though?

Success is fine for me too, important is the match 😉

godot-core/src/builtin/dictionary.rs

itest/rust/src/dictionary_test.rs

Bromeon · 2023-02-01T22:13:29Z

itest/rust/src/dictionary_test.rs

+// The below tests, test to ensure no panicking happens when mutating
+// a dictionary while iterating. Though the result of doing is not
+// guaranteed.


Though the result of doing is not guaranteed.

Probably you mean that the tests passing is no guarantee that mutating while iterating is safe?

I agree that we can't be 100% certain, but I still think we can make the tests more robust. At the moment they don't contain any assertion, so which operation do you expect to panic? If you can't find a deliberately wrong implementation that would make the test fail, it's not a good test.

Even if iteration order can be random (which I think was changed to be deterministic), can we not try to evaluate at least some properties of the result?

I can think of many wrong implementations that would make these tests fail, mainly with accidentally dereferencing invalid variants/null pointers. but im not sure there are many good properties i could test of these would be meaningful except for "it didn't panic or do any UB while i performed the operations". most relevant properties would cause these functions to panic anyway before i get a chance to test for them.

The only thing i can think of right now would be testing that the dictionaries are valid after the iteration? which i can certainly do, but it doesn't really test the core of what i want these tests to test. though i should test that either way, because i dont think i have tested that anywhere.

As of now i can't see anything in the dictionary docs guaranteeing anything about the state of a dictionary after you mutate it mid-iteration, so i'm not sure if we should rely on an undocumented implementation detail to verify the correctness of our implementation.

but im not sure there are many good properties i could test of these would be meaningful except for "it didn't panic or do any UB while i performed the operations"

But panic is defined Rust behavior, and Godot methods will never do it. So what can panic? Dereferencing null pointers doesn't.

And how do you test for UB? We can run Godot compiled with address sanitizers, but at the moment we simply won't see it, except by pure chance.

What we can test is that the data structures are in a valid (even if unexpected) state. You're right that Godot doesn't specify behavior, but there's probably something we can assume. I'd rather have a test that breaks next year when they change implementation, than no test at all 🙂

But panic is defined Rust behavior, and Godot methods will never do it. So what can panic? Dereferencing null pointers doesn't.

I have some assertions in code like checking various invariants that could panic if the c++ bindings don't work the way i expect them to, and if those change behavior in the future then they would panic.

And how do you test for UB? We can run Godot compiled with address sanitizers, but at the moment we simply won't see it, except by pure chance.

That was the point with what i said.

What we can test (...)

As i mentioned i will add some tests to ensure that the dictionary doesn't get messed up by the iteration with mutation. But if you don't mind relying on unspecified behavior of iteration then i can also add some tests to confirm that our iteration implementation has the same behavior as the current version of Godot, which should be true as they use the same method internally.

But if you don't mind relying on unspecified behavior of iteration then i can also add some tests to confirm that our iteration implementation has the same behavior as the current version of Godot, which should be true as they use the same method internally.

That sounds good to me! If we see that implementation changes too often for this to become a maintenance burden, we can reduce the testing.

And don't over-do it, having even some checks is already better than none at all 🙂 I would maybe also focus on a few checks that cover a lot of the behavior, rather than 5-10 very similar test cases. Furthermore, it's enough if the "base" versions (i.e. keys_shared() and iter_shared()) appear in these tests; all the QoL APIs on top use the same principles, and they already have their own tests.

itest/rust/src/dictionary_test.rs

lilizoey · 2023-02-02T01:40:06Z

[...]
But we don't have an iterator over values. If we added that, we would have 8 methods already. This adds quite a bit of API surface, and our "shared" naming convention has the potential to make things rather unreadable grimacing
[...]

Regarding just this point, i decided against adding that because there isn't (to my knowledge) a better way of iterating over only values other than iterating over values_array() or some equivalent method.

Bromeon · 2023-02-02T18:49:34Z

Any feedback on the iterator adaptors I mentioned here?
Maybe also from @ttencate, as you worked a lot on arrays?

ttencate · 2023-02-02T20:36:38Z

godot-core/src/builtin/dictionary.rs

+    }
+
+    fn next_key(&mut self) -> Option<Variant> {
+        let Some(next_var) = self.next_var.take() else { return None };


Wait what is this magic syntax 🤯

Yeah ok googled it. Why did nobody tell me this exists now!

i love let else, but it is actually pointless in this case.

let else is really awesome. Can't wait until clippy forces me to use it 😁

ttencate · 2023-02-02T20:55:38Z

Any feedback on the iterator adaptors I mentioned here? Maybe also from @ttencate, as you worked a lot on arrays?

The non-_as versions could maybe be omitted, since Variant also implements FromVariant? We just decided that the extra copy is not an issue for arrays, so it shouldn't be an issue here either. Unfortunately, generic arguments to functions can't have defaults, so <K: FromVariant = Variant> won't work. Fortunately, iterating over raw Variants is probably not very common so users will have to specify a type most of the time anyway.

Or go the other route: only have Variant iterators but add a helper to FromVariant:

trait FromVariant: Sized {
    ...
    fn expect_from_variant(v: Variant) -> Self {
        Self::try_from_variant(&v).unwrap()
    }
}

Which lets you do:

dict.keys_shared()
    .map(GodotString::expect_from_variant)
    .for_each(|s| println!("{s}"));

It's more ceremony for tuples though, so a reusable iterator adaptor that just calls FromVariant sounds nice. Whether it's useful for arrays depends on how typed arrays will turn out...

Bromeon · 2023-02-02T21:46:54Z

dict.keys_shared()
    .map(GodotString::expect_from_variant)
    .for_each(|s| println!("{s}"));

sounds good, my suggestion here was to use something like the cloned() standard adapter:

dict.keys_shared()
    .typed::<GodotString>()
    .for_each(|s| println!("{s}"));

or, depending on type inference:

dict.keys_shared()
    .typed()
    .for_each(|s: GodotString| println!("{s}"));

Naming to be defined. Can also be something along "from_variant" (although not exactly that, it would lead to naming ambiguities).

Bromeon

Very nice, this PR starts to look really good! 😃

About TryFrom implementations between Dictionary and HashMap: there was some discussion last week, generally going in the direction that once iterators and collect() are available, TryFrom is no longer needed.

What's the general opinion on this? I could imagine that Dictionary <-> HashMap as well as Array <-> Vec conversions happen relatively often. Do they deserve direct methods, or is iter_shared().typed::<..>().collect() good enough?

bors try

godot-core/src/builtin/dictionary.rs

itest/rust/src/dictionary_test.rs

Bromeon · 2023-02-03T22:10:06Z

godot-core/src/builtin/dictionary.rs

+    }
+
+    fn next_key(&mut self) -> Option<Variant> {
+        let Some(next_var) = self.next_var.take() else { return None };


let else is really awesome. Can't wait until clippy forces me to use it 😁

itest/rust/src/dictionary_test.rs

bors · 2023-02-03T22:54:13Z

try

Build succeeded:

full-ci

Bromeon

Mostly good, a few small comments left.
You can gladly squash the commits in the next change 🙂

bors try

godot-core/src/builtin/dictionary.rs

Bromeon · 2023-02-04T12:08:56Z

godot-core/src/builtin/dictionary.rs

+            GDExtensionConstVariantPtr,
+            GDExtensionVariantPtr,
+            *mut GDExtensionBool,
+        ) -> GDExtensionBool,


Please don't import sys::*, but use explicit sys prefixes.
I wrote down the code ready to copy-paste in my last comment 😉

Bromeon · 2023-02-04T12:11:45Z

godot-core/src/builtin/dictionary.rs

+    /// SAFETY:
+    /// `iter_fn` must point to a valid function that interprets the parameters according to their type specification.


For API documentation (///), the convention is to add a new section (h1 as # Safety).
The // SAFETY: syntax is for code comments (//).

Clippy should check the first.

Suggested change

/// SAFETY:

/// `iter_fn` must point to a valid function that interprets the parameters according to their type specification.

/// # Safety

/// `iter_fn` must point to a valid function that interprets the parameters according to their type specification.

bors · 2023-02-04T12:27:32Z

try

Build succeeded:

full-ci

Bromeon · 2023-02-04T14:09:09Z

godot-core/src/builtin/dictionary.rs

+    ffi_methods, interface_fn, GDExtensionBool, GDExtensionConstVariantPtr, GDExtensionVariantPtr,
+    GodotFfi,
+};
+use sys::{types::*, TagVariant};


This import is outdated, instead write:

use sys::types::*;

Bromeon · 2023-02-04T16:02:38Z

Checking again with newly merged #109. This CI run should fail due to the import.

bors try

bors · 2023-02-04T16:13:39Z

try

Build failed:

full-ci

Bromeon · 2023-02-05T17:27:42Z

bors try

bors · 2023-02-05T17:42:08Z

try

Build succeeded:

full-ci

# Conflicts: # itest/rust/src/dictionary_test.rs

Bromeon · 2023-02-11T13:57:55Z

Merged master into this branch. Thanks a lot!

bors r+

bors · 2023-02-11T14:11:48Z

Build succeeded:

full-ci

Bromeon force-pushed the master branch from 7d4582d to c1a36da Compare February 1, 2023 18:12

Bromeon reviewed Feb 1, 2023

View reviewed changes

ttencate reviewed Feb 2, 2023

View reviewed changes

lilizoey force-pushed the feature/dictionary-iter branch from 8b47fec to 6025605 Compare February 3, 2023 17:27

lilizoey requested a review from Bromeon February 3, 2023 17:32

Bromeon added feature Adds functionality to the library c: core Core components labels Feb 3, 2023

Bromeon reviewed Feb 3, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Feb 3, 2023

Try #99:

bce0918

lilizoey force-pushed the feature/dictionary-iter branch 2 times, most recently from 3e01478 to 3ede7dc Compare February 4, 2023 00:24

lilizoey marked this pull request as ready for review February 4, 2023 00:30

Bromeon reviewed Feb 4, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Feb 4, 2023

Try #99:

b9add6c

Bromeon reviewed Feb 4, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Feb 4, 2023

Try #99:

d4868bd

Bromeon mentioned this pull request Feb 5, 2023

Dictionary support #34

Closed

Add dictionary iterators

f15d606

lilizoey force-pushed the feature/dictionary-iter branch from 3ede7dc to f15d606 Compare February 5, 2023 16:51

bors bot added a commit that referenced this pull request Feb 5, 2023

Try #99:

56373eb

Bromeon mentioned this pull request Feb 11, 2023

Project status overview #24

Open

Merge branch 'master' into pr/99

46fe0cd

# Conflicts: # itest/rust/src/dictionary_test.rs

bors bot merged commit 18f3a7b into godot-rust:master Feb 11, 2023

lilizoey deleted the feature/dictionary-iter branch April 16, 2023 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add iteration methods to Dictionary #99

Add iteration methods to Dictionary #99

lilizoey commented Jan 31, 2023 •

edited

Bromeon left a comment •

edited

Bromeon Feb 1, 2023

Bromeon Feb 1, 2023

lilizoey Feb 2, 2023

Bromeon Feb 2, 2023

Bromeon Feb 1, 2023

lilizoey Feb 2, 2023

Bromeon Feb 2, 2023

lilizoey Feb 2, 2023

Bromeon Feb 2, 2023

lilizoey commented Feb 2, 2023

Bromeon commented Feb 2, 2023

ttencate Feb 2, 2023 •

edited

lilizoey Feb 3, 2023

Bromeon Feb 3, 2023

ttencate commented Feb 2, 2023

Bromeon commented Feb 2, 2023 •

edited

Bromeon left a comment

Bromeon Feb 3, 2023

bors bot commented Feb 3, 2023

Bromeon left a comment

Bromeon Feb 4, 2023

Bromeon Feb 4, 2023

bors bot commented Feb 4, 2023

Bromeon Feb 4, 2023

Bromeon commented Feb 4, 2023

bors bot commented Feb 4, 2023

Bromeon commented Feb 5, 2023

bors bot commented Feb 5, 2023

Bromeon commented Feb 11, 2023

bors bot commented Feb 11, 2023

		/// SAFETY:
		/// `iter_fn` must point to a valid function that interprets the parameters according to their type specification.

Add iteration methods to Dictionary #99

Add iteration methods to Dictionary #99

Conversation

lilizoey commented Jan 31, 2023 • edited

Bromeon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey commented Feb 2, 2023

Bromeon commented Feb 2, 2023

ttencate Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttencate commented Feb 2, 2023

Bromeon commented Feb 2, 2023 • edited

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors bot commented Feb 3, 2023

try

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors bot commented Feb 4, 2023

try

Choose a reason for hiding this comment

Bromeon commented Feb 4, 2023

bors bot commented Feb 4, 2023

try

Bromeon commented Feb 5, 2023

bors bot commented Feb 5, 2023

try

Bromeon commented Feb 11, 2023

bors bot commented Feb 11, 2023

lilizoey commented Jan 31, 2023 •

edited

Bromeon left a comment •

edited

ttencate Feb 2, 2023 •

edited

Bromeon commented Feb 2, 2023 •

edited