Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Panic while adding Date with Duration #2512
Comments
|
I'm interested in taking on this issue if no one else is working on it. |
|
Hey @samuelvanderwaal, went ahead and assigned this to you. There's no pressure here either if you end up changing your mind, just let us know. |
|
I've had a look at the code and the panic is happening here // nu-data/value.rs
(Primitive::Date(x), Primitive::Duration(_)) => {
let result = match operator {
Operator::Plus => {
// FIXME: Not sure if I could do something better with the Span.
let y = Primitive::into_chrono_duration(rhs.clone(), Span::unknown())
.expect("Could not convert nushell Duration into chrono Duration.");
Ok(x.checked_add_signed(y).expect("Data overflow."))
}
_ => Err((left.type_name(), right.type_name())),
}?;
Ok(UntaggedValue::Primitive(Primitive::Date(result)))
}when // nu-protocol/value/primitive.rs
pub fn into_chrono_duration(self, span: Span) -> Result<chrono::Duration, ShellError> {
match self {
Primitive::Duration(duration) => {
let (secs, nanos) = duration.div_rem(&BigInt::from(NANOS_PER_SEC));
let secs = match secs.to_i64() {
Some(secs) => secs,
None => {
return Err(ShellError::labeled_error(
"Internal duration conversion overflow.",
"duration overflow",
span,
))
}
};
. . .A very simple fix is to match on the (Primitive::Date(x), Primitive::Duration(_)) => {
let result = match operator {
Operator::Plus => {
// FIXME: Not sure if I could do something better with the Span.
match Primitive::into_chrono_duration(rhs.clone(), Span::unknown()) {
Ok(y) => Ok(x.checked_add_signed(y).expect("Data overflow.")),
Err(_) => Err(("Date", "Duration overflow")),
}
}
_ => Err((left.type_name(), right.type_name())),
}?;
Ok(UntaggedValue::Primitive(Primitive::Date(result)))
}I've had a look through |
|
Also, if you prefer to iterate through a PR I can create one instead of dialoging in the issue thread here. |
|
@JosephTLyons changing all the places that call |
|
@samuelvanderwaal, I'm really sorry about my absence here. I've been interviewing for multiple jobs and haven't had a lot of free time for Nushell. That being said, I really don't know the best way of moving forward on this issue currently. Maybe @jonathandturner or @thegedge would know of a good way to move forward. |
|
No worries! I talked to Jonathan on Discord and will open a PR when I get a chance. |
Describe the bug
If we try to add a Duration to a Date, like so :
ls | get modified | = $it + 1000000000000000000yrThen a panic occur if the Duration is too big (like here), or if the result overflows.
From what I can gather, these originate in nu-data/src/value.rs, when adding
Primitive::DatetoPrimitive::Duration.To Reproduce
or
Expected behavior
Either perform the operation with overflow, or display an error message. I guess a third option would be changing Date so it is unbounded.
Configuration :