Skip to content

Normative: Validate unit-valued options after all options are read #3130

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sffc
Copy link
Collaborator

@sffc sffc commented Jul 11, 2025

See #3116

This preserves the original call to GetOption, but it always calls it with the full list of units, and I pull the subset-checking logic into a separate AO.

I have two commits: an editorial change to split the AO in two, and a small normative change to move the validation later down at the call sites.

@nekevss @Manishearth

@sffc sffc requested a review from ptomato July 11, 2025 00:50
Copy link

codecov bot commented Jul 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.85%. Comparing base (a10d38d) to head (80852fe).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3130   +/-   ##
=======================================
  Coverage   96.85%   96.85%           
=======================================
  Files          21       21           
  Lines        9983     9983           
  Branches     1829     1829           
=======================================
  Hits         9669     9669           
  Misses        268      268           
  Partials       46       46           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sffc sffc changed the title Normative: Validate Temporal-valued unit options after all options are read Normative: Validate unit-valued options after all options are read Jul 11, 2025
@sffc sffc force-pushed the validate-unit branch 2 times, most recently from 70fa5fa to b3f6a0f Compare July 11, 2025 01:09
@sffc
Copy link
Collaborator Author

sffc commented Jul 11, 2025

The code that changes behavior with this PR is something like:

obj = {
    get largestUnit() {
        console.log("get largestUnit");
        return "years";
    },
    get smallestUnit() {
        console.log("get smallestUnit");
        return "seconds";
    },
}

Temporal.PlainTime.from("00:00").round(obj)

Current behavior: console logs "get largestUnit" and then RangeError is thrown.

New behavior: console logs both "get largestUnit" and "get smallestUnit", and then RangeError is thrown.

Note: the RangeError is because PlainTime does not allow "years" as a largestUnit. There is no difference in behavior if get largestUnit() returns an invalid string such as "INVALID".

(as noted previously, there doesn't appear to be Test262 coverage for this)

Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

The intended changes as I understand them do seem both reasonable and narrow, but I think the algorithms steps fail to properly implement them.

Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

This is roughly what I was imagining, thanks for doing all this work!

As Richard has noted, handling unset/default values may be tricky. The way I was expecting it to work was that the get allows you to specify REQUIRED, and Validate doesn't need to care, which roughly appears to be what you're doing here.

@sffc sffc requested a review from gibson042 July 14, 2025 22:01
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I'm surprised that this didn't show up in more places. But I guess smallestUnit is often the last option read anyway, because S is relatively late in the alphabet. So I'm pleasantly surprised at the small extent of the normative change. (I am assuming this PR is the full extent of the changes!)

All in all I'd still prefer not to do this. (It isn't a bug in the proposal, it sets a new precedent that TC39 hasn't really shown appetite for so far, and I just don't see the alternative workaround in temporal_rs as that big of a deal.) But it's small and targeted enough that if I'm the only opposing voice, I won't stand in the way.

Logistical note: to make the best impression when presenting to TC39, I'd recommend squashing the fixup commits into their appropriate editorial or normative commits, then splitting out the editorial commit into its own PR which I can review quickly and merge. That way, there's less for delegates to review and it's much clearer what is changing.

@sffc
Copy link
Collaborator Author

sffc commented Jul 17, 2025

This PR fixes the issues involving unit-valued options, which is the current scope of the issue and all I'm asking for at this time.

Yes, often smallestUnit is the final read, so no change was needed in those cases.

Yes, splitting the editorial change into its own PR is something I'm happy to do.

@dminor
Copy link

dminor commented Jul 18, 2025

@anba Do you have any concerns about this change?

@sffc
Copy link
Collaborator Author

sffc commented Jul 18, 2025

I opened #3135 for the editorial changes.

@ptomato
Copy link
Collaborator

ptomato commented Jul 19, 2025

The editorial changes are merged, and I rebased this. So now it's just the minimal set of normative changes.

@anba
Copy link
Contributor

anba commented Jul 21, 2025

@anba Do you have any concerns about this change?

I don't think so. It'll require some small refactoring because the current SM implementation sometimes uses AUTO as a placeholder to represent absent values. This approach is no longer possible when "auto" must always initially be parsed as a valid Temporal unit option. But apart from that I don't see any implementation issues.

(The SM implementation already parses the unit options accepting all units and only later check which units are actually allowed. )

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.

6 participants