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.

Sign up for GitHub

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

Jump to bottom

Time Args support for month string values #2413

Merged
merged 7 commits into from
Feb 19, 2023
Merged

Time Args support for month string values #2413

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ddd9316
Time Args support for month string values
b-n Feb 18, 2023
52cba1c
Replace escaping with something more sensible
b-n Feb 19, 2023
863f7ac
Add variant test cases for month string support
b-n Feb 19, 2023
7398bb4
Enable use of Kernel#Integer in other areas in rust sources
b-n Feb 19, 2023
b5088bb
Refactor month logic to be correct to MRI
b-n Feb 19, 2023
34ab1f1
Fix rightwards drift and if/else returning bare types
b-n Feb 19, 2023
4196689
Stricter assertation
b-n Feb 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 112 additions & 8 deletions artichoke-backend/src/extn/core/time/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::convert::to_int;
use crate::convert::{to_int, to_str};
use crate::extn::prelude::*;

#[derive(Debug, Copy, Clone)]
Expand Down Expand Up @@ -57,18 +57,52 @@ impl TryConvertMut<&mut [Value], Args> for Artichoke {
for (i, &arg) in args.iter().enumerate() {
match i {
0 => {
let arg = to_int(self, arg)?;
let arg: i64 = arg.try_convert_into(self)?;
let arg: i64 = to_int(self, arg).and_then(|arg| arg.try_convert_into(self))?;

result.year = i32::try_from(arg).map_err(|_| ArgumentError::with_message("year out of range"))?;
}
1 => {
// TODO: This should support 3 letter month names
// as per the docs. https://ruby-doc.org/3.1.2/Time.html#method-c-new
let arg = to_int(self, arg)?;
let arg: i64 = arg.try_convert_into(self)?;
// ```irb
// 3.1.2 => Time.utc(2022, 2).month
// => 2
// 3.1.2 => class I; def to_int; 2; end; end
// => :to_int
// 3.1.2 => Time.utc(2022, I.new).month
// => 2
// 3.1.2 > Time.utc(2022, "feb").month
// => 2
// 3.1.2 > class A; def to_str; "feb"; end; end
// => :to_str
// 3.1.2 > Time.utc(2022, A.new).month
// => 2
// ```
let month: i64 = if let Ok(month) = to_str(self, arg).and_then(|arg| {
b-n marked this conversation as resolved.
Show resolved Hide resolved
let month_str: &str = arg.try_convert_into_mut(self)?;
Copy link
Member

Choose a reason for hiding this comment

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

since we're checking against an ASCII allowlist, we can use &[u8] and b"jan" byte literals here to avoid a UTF-8 decode.

Copy link
Member Author

Choose a reason for hiding this comment

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

make_ascii_lowercase() - which nicely does convert in place - needed a &mut [u8] conversion for Artichoke (which doesn't exist currently). I assume that's for a reason rather than adding my own - so the only way this was possible was to make it convert to a Vec, and do the comparison of byte slices with as_slice().

Is there any particular reason there isn't a conversion to &mut [u8] as of yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note:

impl<'a> TryConvertMut<Value, &'a mut [u8]> for Artichoke {
    type Error = Error;

    fn try_convert_mut(&mut self, mut value: Value) -> Result<&'a mut [u8], Self::Error> {
        self.protect(value);
        let mut s = unsafe { String::unbox_from_value(&mut value, self)? };
        // SAFETY: This transmute modifies the lifetime of the byte slice pulled
        // out of the boxed `String`. This requires that no garbage collections
        // that reclaim `value` occur while this slice is alive. This is
        // enforced for at least this entry from an mruby trampoline by the call
        // to `protect` above.
        //
        // FIXME: does this unbound lifetime and transmute below allow
        // extracting `&'static [u8]`?
        let slice = unsafe { mem::transmute(s.as_inner_mut().as_mut_slice()) };
        Ok(slice)
    }
}

appears to be "valid rust". I am very okay in admitting that I'm not entirely sure if this makes 100% rust/memory sense (still a topic I need to get a properly deep understanding of)

match month_str {
"jan" => Ok(1),
"feb" => Ok(2),
"mar" => Ok(3),
"apr" => Ok(4),
"may" => Ok(5),
"jun" => Ok(6),
"jul" => Ok(7),
"aug" => Ok(8),
"sep" => Ok(9),
"oct" => Ok(10),
"nov" => Ok(11),
"dec" => Ok(12),
b-n marked this conversation as resolved.
Show resolved Hide resolved
// This error will never be seen due to the
// fallback of to_int conversion below
_ => Err(ArgumentError::with_message("unsupported mon value").into()),
}
}) {
Ok(month)
} else {
let arg = to_int(self, arg)?;
arg.try_convert_into(self)
}?;

result.month = match u8::try_from(arg) {
result.month = match u8::try_from(month) {
Ok(month @ 1..=12) => Ok(month),
_ => Err(ArgumentError::with_message("mon out of range")),
b-n marked this conversation as resolved.
Show resolved Hide resolved
}?;
Expand Down Expand Up @@ -300,6 +334,76 @@ mod tests {
assert_eq!(0, result.nanoseconds);
}

#[test]
fn month_supports_string_values() {
let mut interp = interpreter();

let table = [
b-n marked this conversation as resolved.
Show resolved Hide resolved
(b"[2022, \"jan\"]", 1),
(b"[2022, \"feb\"]", 2),
(b"[2022, \"mar\"]", 3),
(b"[2022, \"apr\"]", 4),
(b"[2022, \"may\"]", 5),
(b"[2022, \"jun\"]", 6),
(b"[2022, \"jul\"]", 7),
(b"[2022, \"aug\"]", 8),
(b"[2022, \"sep\"]", 9),
(b"[2022, \"oct\"]", 10),
(b"[2022, \"nov\"]", 11),
(b"[2022, \"dec\"]", 12),
];
b-n marked this conversation as resolved.
Show resolved Hide resolved

for (input, expected_month) in table {
let args = interp.eval(input).unwrap();
let mut ary_args: Vec<Value> = interp.try_convert_mut(args).unwrap();
let result: Args = interp.try_convert_mut(ary_args.as_mut_slice()).unwrap();

assert_eq!(expected_month, result.month);
b-n marked this conversation as resolved.
Show resolved Hide resolved
}
}

#[test]
fn month_supports_string_like_values() {
let mut interp = interpreter();

let args = interp
.eval(b"class A; def to_str; \"feb\"; end; end; [2022, A.new]")
b-n marked this conversation as resolved.
Show resolved Hide resolved
.unwrap();
let mut ary_args: Vec<Value> = interp.try_convert_mut(args).unwrap();
let result: Args = interp.try_convert_mut(ary_args.as_mut_slice()).unwrap();

assert_eq!(2, result.month);
}

#[test]
fn month_supports_int_like_values() {
let mut interp = interpreter();

let args = interp.eval(b"class A; def to_int; 2; end; end; [2022, A.new]").unwrap();
let mut ary_args: Vec<Value> = interp.try_convert_mut(args).unwrap();
let result: Args = interp.try_convert_mut(ary_args.as_mut_slice()).unwrap();

assert_eq!(2, result.month);
}

#[test]
fn invalid_month_string_responds_with_int_conversion_error() {
b-n marked this conversation as resolved.
Show resolved Hide resolved
let mut interp = interpreter();

let args = interp
.eval(b"class A; def to_str; \"aaa\"; end; end; [2022, A.new]")
b-n marked this conversation as resolved.
Show resolved Hide resolved
.unwrap();
let mut ary_args: Vec<Value> = interp.try_convert_mut(args).unwrap();
let result: Result<Args, Error> = interp.try_convert_mut(ary_args.as_mut_slice());
let error = result.unwrap_err();

assert_eq!(
error.message().as_bstr(),
b"no implicit conversion of A into Integer".as_bstr()
);
assert_eq!(error.name(), "TypeError");
}

#[test]
fn subsec_is_valid_micros_not_nanos() {
let mut interp = interpreter();
Expand Down