Improve API consistency and flexibility#47
Conversation
The index lifetime is not the same as the value lifetime. Tying them together means that it is not possible to look up a value using an index with a shorter lifetime. Because this changes the signature of the public Index trait, it is a breaking change, though probably nobody would notice.
This implementation matches `serde_json`, plus the addition of `Cow` which is specific to this crate. This also adds a bound to the return lifetime to make sure that the borrow checker does not require callers to add their own bounds, since it is known that `'ctx` must be at least as long as `'a`.
This is an API-breaking change, but it aligns with how `serde_json` works and how JSON works. `null` is distinct from an undefined property and needs to be treated that way for JSON values to be used correctly in all cases. This updates the Rust edition to 2024 to allow use of if-let chain syntax.
This is an API-breaking change. Callers that called `insert` with a manual `.into()` conversion will need to remove those.
This allows a `Value::Object` to be created directly using the standard Rust conversion types with appropriate values.
| } | ||
|
|
||
| /// If the Value is an Array, returns the associated Array. Returns None otherwise. | ||
| pub fn as_array_mut(&mut self) -> Option<&mut Vec<Value<'ctx>>> { |
There was a problem hiding this comment.
&mut is not allowed
https://github.com/PSeitz/serde_json_borrow?tab=readme-ov-file#mutability
There was a problem hiding this comment.
This is not OwnedValue, and it is not doing any unsafe anything that would violate the rules of the borrow checker. I don’t think that linked write-up is accurate. My understanding of safety issues with unsafe mutability in Rust are to do with aliasing violations (e.g. mutating an object can cause it to move which would invalidate all the other references). This has nothing to do with lifetimes. OwnedValue as implemented is actually already unsound and should probably just be deleted:
let use_after_free = {
let source = OwnedValue::from_string(r#"["bad"]"#.into()).unwrap();
source.as_array().unwrap()[0].clone()
};
// Corruption
use_after_free;The changes in this PR do not cause unsoundness.
(Edit: I changed the example to run against the API of the current main branch to demonstrate that the unsoundness is not due to changes from this PR, and tried to expand the explanation to be less abrasive.)
There was a problem hiding this comment.
Given the confusion, perhaps it is also worth me pointing out that (1) a consumer cannot get a mutable reference through a Deref (DerefMut has to be implemented separately for that) so such functions would not be callable through some hypothetically sound OwnedValue, and (2) a consumer can already get a mutable reference to literally anything inside of a Value by destructuring it themselves. let Value::Object(obj) = &mut value is the same thing.
There was a problem hiding this comment.
You are right, this change is fine.
The linked write-up doesn't talk much about lifetimes, so I'm not sure which part is inaccurate. If you can make it more accurate a PR would be welcome.
Aliasing can be an issue with unsafe, but your use-after-free example is a typical lifetime issue. Aliasing, which means multiple variables point to the same memory location, is a bit more specific and is used to tell the compiler which optimizations it can do without changing correctness. A theoretical line-by-line execution without any optimizations (which does not exist in the rust compiler) would still be fine with broken aliasing as I understand it.
I recently stumbled over a performance issue regarding aliasing, which I think is pretty interesting: rust-lang/rust#149670
That being said, OwnedValue is a bit of a headache. It's incredible useful, but hard to limit its usage to a valid subset.
|
Thanks for the PR! |
2 participants
Hi,
Because I do not have energy at the moment to split individual feature commits into single PRs (and these are not very big), this is just a short list of changes that I needed to make to serde_json_borrow to use it for what I wanted to use it for. I think all of these changes make sense to upstream, so here they are! (The first two commits duplicate the two PRs I opened earlier.)
Most of these changes should require only minor changes by consumers, if any, except for the one which changes the
Value::getAPI to conform with theserde_jsonAPI (and the JSON itself, since an undefined property andnullare not the same thing). This change means it returns anOption<&Value>now.Thanks for the library, it saved me a bit of time doing the same work.