v0.2.0 #108
Draft
v0.2.0 #108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The idea with `Reflect::type_descriptor` was to get the type descriptor
for the reflected value however it had one big footgun: `<Value as
Reflect>::type_descriptor` returns an opaque type, _not_ the type the
`Value` was created from.
For example:
```rust
fn show_editor_ui(reflect: &mut dyn Reflect) {
let type_descriptor = reflect.type_descriptor();
// show ui...
}
struct Foo {}
let foo = Foo {};
// convert the `foo` into a `Value`, perhaps to serialize
// it and send across FFI
let mut foo_value = foo.to_value();
// because `Value` implements `Reflect` it works as a `&dyn mut Reflect`
// but our `show_editor_ui` wont really work because we've lost the type
// information when we called `foo.to_value()`
show_editor_ui(&mut foo_value);
```
The solution is to capture the type descriptor explicitly and store that
together with `foo_value`:
```rust
fn show_editor_ui(reflect: &mut dyn Reflect, type_descriptor: TypeDescriptor) {
// show ui...
}
// store and serialize both the value and the original type descriptor
let mut foo_value = foo.to_value();
let foo_type_descriptor = <Foo as DescribeType>::type_descriptor();
show_editor_ui(&mut foo_value, foo_type_descriptor);
```
I think because of this it makes sense to remove
`Reflect::type_descriptor` as it should make it easier to users to do
the right thing.
I also considered making a `TypedValue` that is like `Value` but
preserves the type descriptor but I ran into a bunch of technical issues
with that.
* [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below Feels like an oversight that we don't return the popped key.
Same reason removing `Reflect::type_descriptor` in #90. I think keeping `type_name` is probably fine. Probably mostly used for pritning and (hopefully) not keys in maps or something. I've documented the footgun though. A breaking change so this PR goes into the `v0.2.0` branch.
The main ideas:
- Remove dependence on `BTreeMap` everywhere and replace it with new
`LinearMap` from `kollect` crate
- Support derive reflect for `std::HashMap` 🎉 (and the new
`tame_containers::{LinearMap, OrderedMap, UnorderedMap}`)
- ~~Add test for static `TypeDescriptor` hashing so that we can make
sure we don't break things in the future unintentionally~~ Postponed
after further thought
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes Pass over methods on `Array`, `List`, and `Map` that makes things more explicit and adds a few new useful methods: - Add `Array::swap` - Add `List::try_insert` - `List::push` renamed to `List::try_push` and now returns a result indicating that `FromReflect::from_reflect` failed - `Map::insert` renamed to `Map::try_insert` and also returns result - `Map::remove` renamed to `Map::try_remove` and also returns result Also deduplicated the `Map` impls. ### Related Issues - Fixes #132
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes Currently, `TypeDescriptor::default_value` doesn't respect the `Default` implementation and instead uses the default for each individual field. These changes address that problem by making structs, tuple structs, and enums properly return their `Default` for `default_value` if it is implemented for that type. This is achieved by making `#[derive(Reflect)]` assume that `Default` is implemented, which a user can opt out of through `#[reflect(opt_out(Default))]`. The opt out strategy was chosen over an opt in strategy as `Clone` and `Debug` follow opt out as well. This is a breaking change to the APIs! The default `Value` for a struct, tuple struct, or enum is cached inside their node. This is needed due to serialization support. If I understand correctly, each node is unique in its `TypeGraph` but not globally unique. This means that the default value for a struct will be duplicated across `TypeGraph`s. I think that improving the duplication between nodes is a good separate optimization step. Note that this PR focuses on resolving the linked issue. There should probably be further discussions on the design behind default values and what type of APIs to expose to interact nicely with it. ### Related Issues - #118 --------- Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes I was under the impression #119 was a breaking change to how we serialized struct values, but upon further inspection that doesn't seem to be the case! Just for good measure this adds a test that ensures 0.2 remain backwards compatible with 0.1
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes While working on enabling reflection for more of our internal types I ran into a bunch of `UnorderedPrimitiveSet<EntityId>`. That type doesn't implement `Reflect` because we don't support sets. I don't remember why we haven't had sets previously but I think it makes sense to have.
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes Noticed that 0.2 didn't build on wasm: ``` ❯ cargo build --target wasm32-unknown-unknown --no-default-features Compiling getrandom v0.2.12 Compiling indexmap v1.9.3 error: the wasm*-unknown-unknown targets are not supported by default, you may need to enable the "js" feature. For more information see: https://docs.rs/getrandom/#webassembly-support --> /Users/david.pedersen/.cargo/registry/src/index.crates.io-6f17d22bba15001f/getrandom-0.2.12/src/lib.rs:291:9 | 291 | / compile_error!("the wasm*-unknown-unknown targets are not supported by \ 292 | | default, you may need to enable the \"js\" feature. \ 293 | | For more information see: \ 294 | | https://docs.rs/getrandom/#webassembly-support"); | |________________________________________________________________________^ error[E0433]: failed to resolve: use of undeclared crate or module `imp` --> /Users/david.pedersen/.cargo/registry/src/index.crates.io-6f17d22bba15001f/getrandom-0.2.12/src/lib.rs:347:9 | 347 | imp::getrandom_inner(dest)?; | ^^^ use of undeclared crate or module `imp` For more information about this error, try `rustc --explain E0433`. error: could not compile `getrandom` (lib) due to 2 previous errors warning: build failed, waiting for other jobs to finish... ```
3 tasks
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes This fixes a few issues with arrays that I discovered: - `<[T; N]>::from_reflect` for `Value::List` would previously fail. It now works if the lengths match. - `Vec::as_array`, `Vec::as_array_mut`, `Vec::into_array` now correctly return `Some(_)`. - `Value::as_array`, `Value::as_array_mut`, `Value::into_array` now correctly return `Some(_)` for `Value::List`. Basically lists are a subtype of arrays so if something is a list then its also an array, i.e. `as_array` should work on a `&dyn List`.
This bumps `kollect` to 0.4.0, which I've just released, the only change being changing the default serialization format of maps to be as sequences of `(k, v)` pairs. Thus this is a breaking serialization change. We should therefore not bump this version into wim until the PR that intentionally breaks a bunch of data which this is being used for.
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes Does seem to actually be doable. ### TODO - [x] Fix `fields_mut` for `Quat` - [x] Fix `fields_mut` for `Mat2` ### Related Issues #134
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes Needed for something internally. Just copied the `Vec` impls and modified them as necessary.
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below
### Checklist * [x] I have read the [Contributor Guide](../../CONTRIBUTING.md) * [x] I have read and agree to the [Code of Conduct](../../CODE_OF_CONDUCT.md) * [x] I have added a description of my changes and why I'd like them included in the section below ### Description of Changes If you had an enum variant with a field called `name` then methods like `field` and `field_mut` wouldn't work due to variable name collisions in the generated code. This fixes that by prefixing generated variable names with `__mirror_mirror`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We can merge PRs with breaking changes into this branch, rather than keeping them open for a long time. Makes rebasing a bit easier.
TODO