Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yaml: Implement IndexMut #19

Closed
wants to merge 1 commit into from
Closed

yaml: Implement IndexMut #19

wants to merge 1 commit into from

Conversation

alistair23
Copy link

This implements the IndexMut trait for Yaml. This allows indexing the Yaml type while having a mutable reference.

Unlike the Index, this will panic on a failure. That is allowed as per the Rust documentation [1]. We don't have the option of returning a mutable reference to BAD_VALUE as that is unsafe. So instead we just panic.

1: https://doc.rust-lang.org/std/ops/trait.IndexMut.html#tymethod.index_mut

Resolves: chyh1990#123

This implements the IndexMut trait for Yaml. This allows indexing the
Yaml type while having a mutable reference.

Unlike the Index, this will panic on a failure. That is allowed as per
the Rust documentation [1]. We don't have the option of returning a
mutable reference to BAD_VALUE as that is unsafe. So instead we just
panic.

1: https://doc.rust-lang.org/std/ops/trait.IndexMut.html#tymethod.index_mut

Resolves: chyh1990#123
Signed-off-by: Alistair Francis <[email protected]>
Copy link
Owner

@Ethiraric Ethiraric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A couple changes and this can be merged.

Please make sure there is absolutely no way around an unsafe. The more we let the compiler do our work, the better ;)

/// Get a mutable reference to the inner object in the YAML enum if it is a `$t`.
///
/// # Return
/// If the variant of `self` is `Yaml::$yt`, return `Some(&$t)` with the `$t` contained. Otherwise,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// If the variant of `self` is `Yaml::$yt`, return `Some(&$t)` with the `$t` contained. Otherwise,
/// If the variant of `self` is `Yaml::$yt`, return `Some(&mut $t)` with the `$t` contained. Otherwise,

Comment on lines +690 to +711
fn index_mut(&mut self, idx: usize) -> &mut Yaml {
if let Some(v) = self.as_mut_vec() {
let yaml = v.get_mut(idx).unwrap();

// This transmute just casts the lifetime away. Since Rust only
// has SESE regions, this early return cannot be worked out and
// such that the borrow region of self includes the whole block.
// The explixit lifetimes in the function signature ensure that
// this is safe.
// From: https://github.com/image-rs/image-gif/blob/c814b40079a36af0b26adc36237f2c742d69c994/src/reader/decoder.rs#L227-L229
unsafe {
return mem::transmute::<&mut Yaml, &mut Yaml>(yaml);
}
}

if let Some(v) = self.as_mut_hash() {
let key = Yaml::Integer(i64::try_from(idx).unwrap());
v.get_mut(&key).unwrap()
} else {
panic!("Not a hash or verctor type");
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My other comment didn't get through.

You do not need the unsafe block if you only borrow self once.

While you're allowed to panic as per IndexMut's interface, you're only allowed to do so if the index it out of range.
In this particular case, we have an additional condition in which we may panic which is the variant contained in self and this should be documented.

Suggested change
fn index_mut(&mut self, idx: usize) -> &mut Yaml {
if let Some(v) = self.as_mut_vec() {
let yaml = v.get_mut(idx).unwrap();
// This transmute just casts the lifetime away. Since Rust only
// has SESE regions, this early return cannot be worked out and
// such that the borrow region of self includes the whole block.
// The explixit lifetimes in the function signature ensure that
// this is safe.
// From: https://github.com/image-rs/image-gif/blob/c814b40079a36af0b26adc36237f2c742d69c994/src/reader/decoder.rs#L227-L229
unsafe {
return mem::transmute::<&mut Yaml, &mut Yaml>(yaml);
}
}
if let Some(v) = self.as_mut_hash() {
let key = Yaml::Integer(i64::try_from(idx).unwrap());
v.get_mut(&key).unwrap()
} else {
panic!("Not a hash or verctor type");
}
}
/// Perform indexing if `self` is a sequence or a mapping.
///
/// # Panics
/// This function panics if the index given is out of range (as per [`IndexMut`]). If `self` i
/// a [`Yaml::Array`], this is when the index is bigger or equal to the length of the
/// underlying `Vec`. If `self` is a [`Yaml::Hash`], this is when the mapping sequence does no
/// contain [`Yaml::Integer`]`(idx)` as a key.
///
/// This function also panics if `self` is not a [`Yaml::Array`] nor a [`Yaml::Hash`].
fn index_mut(&mut self, idx: usize) -> &mut Yaml {
match self {
Yaml::Array(sequence) => sequence.index_mut(idx),
Yaml::Hash(mapping) => {
let key = Yaml::Integer(i64::try_from(idx).unwrap());
mapping.get_mut(&key).unwrap()
}
_ => panic!("Attempting to index but `self` is not a sequence nor a mapping"),
}
}

Copy link

@aweraw aweraw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this - migrating from serde_yaml this was the only thing I was missing in yaml-rust2.

@Ethiraric
Copy link
Owner

I applied the suggestion locally and amended the commit, and it seems the procedure makes Github not consider this merged.

Your patch is applied in 4ca0fc4, thank you!

@Ethiraric Ethiraric closed this Mar 30, 2024
@alistair23 alistair23 deleted the alistair/index-mut branch April 2, 2024 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mutable access.
3 participants