-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Normative: Fix out-of-range NaNs in date set methods #2136
Conversation
Looks good. Couple of consistency things:
|
Is this editorial? |
I think so: the current state is nonsensical by a strict reading and the only plausible interpretation by a loose reading (that |
hmm - DateFromTime takes a time value which is allowed to be (similarly for the other examples in this PR) |
I would strongly prefer to filter out |
These operations already are defined to accept a time value, which allows NaN - so if we want to make that change, we also have to redefine all of them to only accept integers. |
I am happy to additionally add the word "finite" before the words "time value" in the headers of the various intermediate operations, sure. |
7833394
to
365cebe
Compare
I've updated this to be more thorough. Unfortunately, in some of these cases there were multiple plausible interpretations and engines do not agree, so I've marked this as normative. Specifically, when invoking the (new Date(NaN)).setHours(
{ valueOf(){ print('coerced hour'); } },
{ valueOf(){ print('coerced min'); } },
{ valueOf(){ print('coerced sec'); } },
{ valueOf(){ print('coerced ms'); } },
); produces
The semantics I've specified in this PR match Spidermonkey's behavior, since it seemed the most natural to me and because I believe it matches the intent of the the spec as written. |
spec.html
Outdated
1. If _min_ is not present, let _m_ be MinFromTime(_t_). | ||
1. If _sec_ is not present, let _s_ be SecFromTime(_t_). | ||
1. If _ms_ is not present, let _milli_ be msFromTime(_t_). | ||
1. Let _date_ be MakeDate(Day(_t_), MakeTime(_h_, _m_, _s_, _milli_)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we consistently named these newDate
.
3d0c24c
to
7a79833
Compare
Rebased. I've also opened a PR with tests: tc39/test262#3362. Once those land, so can this. Edit: landed. |
https://bugs.webkit.org/show_bug.cgi?id=235271 Reviewed by Alexey Shvayka. JSTests: * test262/expectations.yaml: Source/JavaScriptCore: Even if the input Date is NaN or the result looks like NaN, we need to coerce passed arguments to Number[1] since it has observable side effect. [1]: tc39/ecma262#2136 * runtime/DatePrototype.cpp: (JSC::applyToNumbersToTrashedArguments): (JSC::fillStructuresUsingTimeArgs): (JSC::fillStructuresUsingDateArgs): (JSC::setNewValueFromTimeArgs): (JSC::setNewValueFromDateArgs): Canonical link: https://commits.webkit.org/246086@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288066 268f45cc-cd09-0410-ab3c-d52691b4dbfc
https://bugs.webkit.org/show_bug.cgi?id=235271 Reviewed by Alexey Shvayka. JSTests: * test262/expectations.yaml: Source/JavaScriptCore: Even if the input Date is NaN or the result looks like NaN, we need to coerce passed arguments to Number[1] since it has observable side effect. [1]: tc39/ecma262#2136 * runtime/DatePrototype.cpp: (JSC::applyToNumbersToTrashedArguments): (JSC::fillStructuresUsingTimeArgs): (JSC::fillStructuresUsingDateArgs): (JSC::setNewValueFromTimeArgs): (JSC::setNewValueFromDateArgs): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@288066 268f45cc-cd09-0410-ab3c-d52691b4dbfc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
…onkey-reviewers,jandem We never implemented this spec change, which introduced some subtle changes how `NaN` values have to be treated. This part updates all Date setters to match the current spec text. Normative change from: tc39/ecma262#2136 Tests are in: tc39/test262#4258 Differential Revision: https://phabricator.services.mozilla.com/D225248
…onkey-reviewers,jandem We never implemented this spec change, which introduced some subtle changes how `NaN` values have to be treated. This part updates all Date setters to match the current spec text. Normative change from: tc39/ecma262#2136 Tests are in: tc39/test262#4258 Differential Revision: https://phabricator.services.mozilla.com/D225248
…onkey-reviewers,jandem We never implemented this spec change, which introduced some subtle changes how `NaN` values have to be treated. This part updates all Date setters to match the current spec text. Normative change from: tc39/ecma262#2136 Tests are in: tc39/test262#4258 Differential Revision: https://phabricator.services.mozilla.com/D225248 UltraBlame original commit: 8887a6541d7abe020db25a831b41c32498324702
…onkey-reviewers,jandem We never implemented this spec change, which introduced some subtle changes how `NaN` values have to be treated. This part updates all Date setters to match the current spec text. Normative change from: tc39/ecma262#2136 Tests are in: tc39/test262#4258 Differential Revision: https://phabricator.services.mozilla.com/D225248 UltraBlame original commit: 8887a6541d7abe020db25a831b41c32498324702
…onkey-reviewers,jandem We never implemented this spec change, which introduced some subtle changes how `NaN` values have to be treated. This part updates all Date setters to match the current spec text. Normative change from: tc39/ecma262#2136 Tests are in: tc39/test262#4258 Differential Revision: https://phabricator.services.mozilla.com/D225248 UltraBlame original commit: 8887a6541d7abe020db25a831b41c32498324702
Marked as editoral due to current text being nonsensical and new text matching implementations.