Add with to PlainYearMonth

[sebastianjacmatt]

Restructured pull request of #216

Worked with @HenrikTennebekk on #143

Had to change temporal_capi/plain_year_month.rs hope this is fine?

[nekevss]

This looks fantastic!

I have one issue, but it doesn't really have anything to do with the code, so much as it does some weird oddities with PlainYearMonth. I tried to give as detailed of an explanation as I could. Let me know what you'd prefer.

[nekevss]

praise: great tests!

[nekevss]

issue: so this one is a little tricky, but the ISO day property shouldn't be touched.

For reference, see the basic PlainYearMonth.prototype.with test

[nekevss]

issue (part 2 on test review comment): so this macro impl specifically on PartialDate is a bit problematic for specifically PlainYearMonth.

The method steps at play here are 6-7:

6. Let partialYearMonth be ? [PrepareCalendarFields](https://tc39.es/proposal-temporal/#sec-temporal-preparecalendarfields)(calendar, temporalYearMonthLike, « year, month, month-code », « », partial).
7. Set fields to [CalendarMergeFields](https://tc39.es/proposal-temporal/#sec-temporal-calendarmergefields)(calendar, fields, partialYearMonth).

PrepareCalendarFields is a bit of a complex abstract operations that's main purpose is to pull fields from a JavaScript object. In order to handle this, the partial objects were created.

You correctly noted that the partial cannot be empty when submit into with, but the actual assertion technically occurs here in step 6, not 3 (that abstract operation does ALOT).

This method implemented by this macro is temporal_rs's answer for step 7. The only issue is that day should never be set downstream by the caller due to step 6.

The simple solution would be to manually implement a with_fallback_year_month that does not touch the partial.day (like the other year_month method above this).

The other solution (that after typing this all out I'm more convinced is the correct approach) is to move that invariant of not touching the day field into the type system and create a PartialYearMonth struct that can then be used on all PlainYearMonth paths that are using PartialDate.

The second solution is probably a bit of scope creep, so if you'd like to go with the first solution, that's fine by me. We can review and merge this, and I'll open up another issue for implementing PartialYearMonth.

If you want to do the second solution, I think PartialYearMonth should look something like:

struct PartialYearMonth {
    pub year: Option<i32>,
    pub month: Option<u8>,
    pub month_code: Option<MonthCode>,
    pub era: Option<TinyAsciiStr<19>>,
    pub era_year: Option<i32>,
    pub calendar: Calendar,
}

[sffc]

Another solution could be to make the function return an Err if the PartialDate day field is populated, and then just use the regular macro?

But I like the type system approach of PartialYearMonth. (If you do, it looks like it should go in year_month.rs)

[nekevss]

Another solution could be to make the function return an Err if the PartialDate day field is populated, and then just use the regular macro?

Yeah, the incoming partial would still have to throw on partail.day.is_some(), but the regular macro still sets the day according to the fallback, which would still be invalid as well I believe. The day field is "resolved" by the calendar, at least according to the specification, in step 2.

[sebastianjacmatt]

I’ve implemented a solution that avoids modifying the day field while still using the macro. Instead of implementing a separate with_fallback_year_month or type system, I’ve tweaked the macro to require explicit day specification, so PlainYearMonth can use it without setting day.
It’s set to None via ..Default::default(), but I’m concerned this doesn’t strictly align with spec step 6 of 9.3.13. Is this okay, or should I continue with the type system instead?

Check out the latest commit. I’ve also changed the test cases which now ignores any attempt to change the day of a PlainYearMonth.

[nekevss]

I saw the change you had made to the macro 😃 It's a pretty creative one!

Strictly aligning to the spec on step 6 is incredibly hard due to its reliance on JavaScript objects, and essentially requires more of an interpretation rather than step by step adherence, which is how the partial objects came to be.

The best way to test is to see if it passes the test262 test suite. Have you run the test262 test suite with Boa by chance to test conformance?

I still have to do a more proper review, but I think I'm fine with merging this current approach for now. But as mentioned above, I think long term moving to a PartialYearMonth struct is the best option overall. So a follow up PR to complete that is definitely best.

[sebastianjacmatt]

The macro trick was all @sffc—credit goes to him😅. Right now, testing Temporal with Boa doesn’t seem to work for me🤔. I suspect it may relate to the MonthCode change in temporal, I’m not entirely sure. If the fix is trivial, I’ll try and run Test262.

Here are some of the errors i am experiencing

error[E0277]: the `?` operator can only be used on `Result`s, not `Option`s, in a method that returns `Result`
   --> core/engine/src/builtins/temporal/plain_date/mod.rs:334:19
    |
321 |     fn get_era(this: &JsValue, _: &[JsValue], _: &mut Context) -> JsResult<JsValue> {
    |     ------------------------------------------------------------------------------- this function returns a `Result`
...
334 |             .era()?
    |                   ^ use `.ok_or(...)?` to provide an error compatible with `Result<value::JsValue, error::JsError>`
    |
    = help: the trait `FromResidual<std::option::Option<Infallible>>` is not implemented for `Result<value::JsValue, error::JsError>`
    = help: the trait `FromResidual<Result<Infallible, E>>` is implemented for `Result<T, F>`

error[E0599]: `TinyAsciiStr<16>` is not an iterator
   --> core/engine/src/builtins/temporal/plain_date/mod.rs:335:14
    |
332 |           Ok(date
    |  ____________-
333 | |             .inner
334 | |             .era()?
335 | |             .map(|s| JsString::from(s.as_str()))
    | |             -^^^ `TinyAsciiStr<16>` is not an iterator
    | |_____________|
    |
    |
   ::: /Users/sebastianmatthews/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tinystr-0.8.1/src/ascii.rs:15:1
    |
15  |   pub struct TinyAsciiStr<const N: usize> {
    |   --------------------------------------- doesn't satisfy `TinyAsciiStr<16>: std::iter::Iterator`
    |
    = note: the following trait bounds were not satisfied:
            `TinyAsciiStr<16>: std::iter::Iterator`
            which is required by `&mut TinyAsciiStr<16>: std::iter::Iterator`
            `str: std::iter::Iterator`
            which is required by `&mut str: std::iter::Iterator`

There is a total of 34 errors

[nekevss]

Has this been rebased or merged with main recently? I thought more or less Boa was up to date ... but maybe its a bit more out of sync then I had thought. I can have an update complete by tonight.

[sebastianjacmatt]

I've rebased onto the main branch and successfully tested with the test262 suite.
Note: I modified the get_option call in boa, removing the .unwrap_or_default();

[nekevss]

Following up on this PR for the CI failure.

We've started checking in the cpp bindings for temporal_capi. Since this PR makes a change to temporal_capi. You'll need to update the bindings. To do so, simply run cargo run -p diplomat-gen. This will update the bindings and then you just add them to history. Let me know if you have any issues.

[nekevss]

I was double checking the PR and noticed this method. This should be removed as PlainYearMonth has no day accessor on the builtin.

[sebastianjacmatt]

I'm having an issue with the FFI bindings from boa-dev/temporal to boa. When I run cargo test in boa I get the error:

core/engine/src/builtins/temporal/plain_year_month/mod.rs:372:53
let result = year_month.inner.with(partial, overflow)?;
expected `Option<ArithmeticOverflow>`, found `ArithmeticOverflow`

I ran cargo run -p diplomat-gen. PlainYearMonth.hpp shows a change from ArithmeticOverflow to ArithmeticOverflow_option, so it appears the bindings were updated, yet the error persists🤔

Here is the full error for reference:

[nekevss]

Ah, that looks like it's the call from boa_engine that is causing the compilation error.

You'd need to update the code in Boa. The get_option call will need it's unwrap_or_default removed

EDIT: just to note, that failure shouldn't affect this PR. Currently, it looks like you only need to run Rustfmt 😄

[lockels]

if you want to fix the lint errors you should run:

cargo clippy --all-features --all-targets

and fix the errors in the code til there are none left:)

[nekevss]

Yeah, I'm not actually sure of a good way to fix this. Although, I am curious if there is a way.

In the meantime, just annotate the macro with the recommend allow clippy lint: #[allow(clippy::needless_update)].

[nekevss]

LGTM! Thanks for working on this!