[Merged by Bors] - Safe wrapper for JsDate

[lameferret]

This PR adds a safe wrapper around JavaScript JsDate from builtins::date, and is being tracked at #2098.

Implements following methods

• new Date()
• Date.prototype.getDate()
• Date.prototype.getDay()
• Date.prototype.getFullYear()
• Date.prototype.getHours()
• Date.prototype.getMilliseconds()
• Date.prototype.getMinutes()
• Date.prototype.getMonth()
• Date.prototype.getSeconds()
• Date.prototype.getTime()
• Date.prototype.getTimezoneOffset()
• Date.prototype.getUTCDate()
• Date.prototype.getUTCDay()
• Date.prototype.getUTCFullYear()
• Date.prototype.getUTCHours()
• Date.prototype.getUTCMilliseconds()
• Date.prototype.getUTCMinutes()
• Date.prototype.getUTCMonth()
• Date.prototype.getUTCSeconds()
• Date.prototype.getYear()
• Date.now()
• Date.parse() Issue 4
• Date.prototype.setDate()
• Date.prototype.setFullYear()
• Date.prototype.setHours() Issue 3
• Date.prototype.setMilliseconds()
• Date.prototype.setMinutes() Issue 3
• Date.prototype.setMonth()
• Date.prototype.setSeconds()
• Date.prototype.setTime()
• Date.prototype.setUTCDate()
• Date.prototype.setUTCFullYear()
• Date.prototype.setUTCHours()
• Date.prototype.setUTCMilliseconds()
• Date.prototype.setUTCMinutes()
• Date.prototype.setUTCMonth()
• Date.prototype.setUTCSeconds()
• Date.prototype.setYear()
• Date.prototype.toDateString() Issue 5
• Date.prototype.toGMTString() Issue 5
• Date.prototype.toISOString() Issue 5
• Date.prototype.toJSON() Issue 5
• Date.prototype.toLocaleDateString() Issue 5 and 6
• Date.prototype.toLocaleString() Issue 5 and 6
• Date.prototype.toLocaleTimeString() Issue 5 and 6
• Date.prototype.toString() Issue 5
• Date.prototype.toTimeString() Issue 5
• Date.prototype.toUTCString() Issue 5
• Date.UTC()
• Date.prototype.valueOf()

Issues

1. get_*() and some other methods - They take &self as input internally, and internal struct shouldn't be used in a wrapper API. Therefore, these would require input to be this: &JsValue, args: &[JsValue], context: &mut Context like others and use this_time_value()? Fixed using this_time_value()
2. to_string()- how can I use Date::to_string() rather than alloc::string::ToString. My bad it compiles, just rust-analyzer was showing it as an issue.
3. set_hours() and set_minutes() - they subtract local timezones when setting the value, e.g.
   • On further look:

      // both function call `builtins::date::mod.rs#L1038
     this.set_data(ObjectData::date(t));
     // `ObjectData::date` creates a new `Date` object `object::mods.rs#L423
     //                 | this date is chrono::Date<Tz(TimezoneOffset)> and Tz default is being used here which is GMT+0
     pub fn date(date: Date) -> Self {
       Self {
           kind: ObjectKind::Date(date),
           internal_methods: &ORDINARY_INTERNAL_METHODS,
       }
   }

   • BTW, in object::mod.rs's enum ObjectKind there is Date(chrono::Date) and it requires
     the generic argument, how is it being bypassed here?
   • Also in set_minutes() step 6, LocalTime should be used.

// reference date = 2000-01-01T06:26:53.984
date.set_hours(&[23.into(), 23.into(), 23.into(), 23.into()], context)?;
// would add tiemzone(+5:30) to it
// Is                 2000-01-01T17:53:23.023
// Should be    2000-01-01T23:23:23.023

4. parse() - it uses chrono::parse_from_rfc3339 internally, while es6 spec recommends ISO8601. And it can also parse other formats like from MDN 04 Dec 1995 00:12:00 GMT which fails. So what should be done about it.
5. to_*() - This is more general, as the internal date object uses chrono::NaiveDateTime which doesn't have timezone. It doesn't account for +4:00 in example below.

// Creates new `Date` object from given rfc3339 string.
let date = JsDate::new_from_parse(&JsValue::new("2018-01-26T18:30:09.453+04:00"), context);
println!("to_string: {:?}", date2.to_string(context)?); 
// IS:          Sat Jan 27 2018 00:00:09 GMT+0530
// Should:  Fri Jan 26 2018 20:00:09 GMT+0530

6. to_locale_*() - requires ToDateTimeOptions and localization would require chrono's unstable-locales feature, which is available for DateTime and not for NaiveDateTime.
   • I should have looked properly, to_date_time_options is already implemented in builtins::intl. Anyway, I would still need some tips on how to use it. What would function signature be like in wrapper API, how would options be passed to the said API.
   • So to_date_time_options() takes options: &JsValue as an argument and build an object from it and fetch properties through Object.get(). If I want options to be { weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' } what would JsValue look like to make it all work.

   date.to_locale_date_string(&[JsValue::new("en_EN"), OPTIONS], context)?;
   // OPTIONS need to be a JsValue which when converted into an object 
   // have these properties { weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' };

Possible improvements

1. Right now, object::jsdate::set_full_year() and alike (input is a slice) are like below, into() doesn't feel ergonomic.

   #[inline]
   pub fn set_full_year(&self, values: &[JsValue], context: &mut Context) -> JsResult<JsValue> {
       Date::set_full_year(&self.inner.clone().into(), values, context)
   }
   // Usage 
   date.set_full_year(&[2000.into(), 0.into(), 1.into()], context)?;
   // How can something like this be made to work
   #[inline]
   pub fn set_full_year<T>(&self, values: &[T], context: &mut Context) -> JsResult<JsValue>
   where
       T: Into<JsValue>,
   {
                                                         | expected reference `&[value::JsValue]` 
                                                         | found reference `&[T]`        
       Date::set_full_year(&self.inner.clone().into(), values, context)
   }

2. Any other suggestion?

[codecov]

Codecov Report

Merging #2181 (0a0c7d5) into main (2ffae5b) will increase coverage by 0.11%.
The diff coverage is 49.11%.

@@            Coverage Diff             @@
##             main    #2181      +/-   ##
==========================================
+ Coverage   38.69%   38.80%   +0.11%     
==========================================
  Files         314      315       +1     
  Lines       23887    24085     +198     
==========================================
+ Hits         9242     9347     +105     
- Misses      14645    14738      +93     

Impacted Files	Coverage Δ
boa_engine/src/object/builtins/jsdate.rs	0.00% <0.00%> (ø)
boa_engine/src/builtins/date/mod.rs	86.27% <87.40%> (+3.14%)	⬆️
boa_engine/src/value/serde_json.rs	84.44% <0.00%> (-3.93%)	⬇️
boa_ast/src/property.rs	18.96% <0.00%> (-1.73%)	⬇️
boa_engine/src/vm/mod.rs	42.74% <0.00%> (-0.35%)	⬇️
boa_engine/src/context/mod.rs	32.25% <0.00%> (-0.21%)	⬇️
boa_engine/src/builtins/error/uri.rs	100.00% <0.00%> (ø)
boa_engine/src/builtins/error/eval.rs	100.00% <0.00%> (ø)
boa_engine/src/builtins/error/range.rs	100.00% <0.00%> (ø)
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[raskad]

I took a first look at the changes in the builts themselves and left some suggestions.

The issues that you mention all sound out of scope of this PR to me. I think we should convert all of them to issues so they can get fixed. For the purpose of this PR we just want to expose the builtins, so we can use the existing funtionalities even if they are not 100% spec compliant.

[Razican]

This looks great to me :) I just recommend a bit of documentation in the JsDate structure itself, but it can be merged. Thank you!!

[Razican]

I would add some documentation to the structure itself, maybe with a small example. We should probably also add some documentation at the module level, even if it's just a one-liner.

[lameferret]

Thanks for the review, I have added a doc-example for JsDate but didn't rebase it as the code structure has changed from what I remember.

[Razican]

Hmm it shouldn't have changed a lot. The main difference might be that creating errors now uses JsNativeError::...() instead of context.construct...(). But checking at the merge conflicts, it seems it's just some import ordering and a different derive for Date.

[lameferret]

Hey I tried the changes, it had some fixable cascading effects, but I'm stuck on "lazzy error types" without it /boa_examples/jsdate,rs it spits "? couldn't convert the error to JsString", or should I implement lazzy error too?

[Razican]

@jedel1043 can you help here?

[jedel1043]

It's just the signature of main. It should return JsResult<()> instead of Result<(), JsValue>.

[lameferret]

@jedel1043 thank you so much, I was really confused trying to figure out the issue.

[raskad]

Thanks for the contribution! Sorry for the long wait on the review.

[bors]

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[raskad]

bors seems to have some problems with the github check status so I rebased the branch to hopefully fix that.

[raskad]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• Build and Test (macos-latest)
• Build and Test (windows-latest)
• Coverage
• Misc