Skip to content
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

Fix binary comparison operators #517

Closed
sffc opened this issue Apr 21, 2020 · 29 comments · Fixed by #632
Closed

Fix binary comparison operators #517

sffc opened this issue Apr 21, 2020 · 29 comments · Fixed by #632
Labels
has-consensus needs plenary input Needs to be presented to the committee and feedback incorporated

Comments

@sffc
Copy link
Collaborator

sffc commented Apr 21, 2020

People are going to use > and < on Temporal objects, whether we like it or not. It mostly works:

const d1 = Temporal.Date.from("2020-01-04");
const d2 = Temporal.Date.from("2020-01-08");

d1 < d2;  // true

However, it breaks in certain cases such as negative dates:

const d3 = Temporal.Date.from("-002020-01-04");
const d4 = Temporal.Date.from("-002222-01-04");

d3 < d4;  // true, but should be false, since 2020 BCE is after 2222 BCE

I think the reason is that < and > implicitly use toString(), and it just so happens to be that string comparison on the ISO string works for positive dates.

Related: #293

@sffc sffc added the bug label Apr 21, 2020
@sffc
Copy link
Collaborator Author

sffc commented Apr 21, 2020

The problem is fixed if we make .valueOf() return something correct, like a BigInt with the number of days since 0000-01-01. The problem of course is that we would be introducing a new concept into the language.

We could concatenate the parts together such that 2020-04-21 turns into 20200421n on valueOf(), except we'd need to do some kind of inversion for the negative years, because February is after January even in negative years according to the ISO calendar. (Right?)

@ptomato
Copy link
Collaborator

ptomato commented Apr 28, 2020

Hmm, this is a problem. I hadn't realized that < and > would actually give the right answer some of the time; often enough to be dangerous.

I think ideally we should somehow point people to the Temporal.X.compare() static methods when they use comparison operators. I don't think we should try to make the comparison operators "work" unless they can work 100% of the time like they would if operator overloading existed.

It's also the case that you cannot compare Temporal.Durations at all, so using < and > on them to compare by ISO string is always a bug.

We'd have to do this in such a way that it wouldn't preclude overloading the comparison operators on Temporal types if/when operator overloading is added to the language.

@sffc
Copy link
Collaborator Author

sffc commented Apr 28, 2020

If we bite the bullet and introduce valueOf(), we should be able to make the operators work all of the time. I suggested BigInt as one representation. It would be nice if we could use an Array, like [2020, 4, 28], but unfortunately, [11] < [2]; I guess JavaScript still uses toString on the Array. A Tuple would be a good option, but we aren't blocking on the Tuple proposal.

The alternative is to make < and > never work by making .toString() return the same string each time, like [object Temporal.Date]. @littledan suggested this in #293.

But, I have to say that I do have a certain affection for making < and > "just work". If it's technically feasible, I think we shouldn't discount that possibility.

@kaizhu256
Copy link
Contributor

as product-developer, comparing isostrings is legitimate and efficient when 99% of ux-applications only concern with date-ranges 1000-xx-xx to 9999-xx-xx.

its easier to do the above (and handle/invalidate uncommon date-ranges separately on per-ux-basis), than to use some builtin/overloaded temporal comparison operator.

@sffc
Copy link
Collaborator Author

sffc commented Apr 28, 2020

Another off-the-wall idea: restrict Temporal to only represent dates with positive ISO years, and use the extended ISO 8601 syntax "YYYYYY-MM-DD", such that string comparison always works.

@ljharb
Copy link
Member

ljharb commented Apr 28, 2020

If adding .valueOf (whether with strings or numbers or bigints) can "just work" 100% of the time, that's what Temporal should do.

@sffc
Copy link
Collaborator Author

sffc commented Apr 28, 2020

Reading the spec for how < and > actually work:

  1. The operations are defined in: https://www.ecma-international.org/ecma-262/10.0/index.html#sec-relational-operators
  2. The result is passed into the Abstract Relational Comparison: https://www.ecma-international.org/ecma-262/10.0/index.html#sec-abstract-relational-comparison
  3. That abstract operation calls ToPrimitive: https://www.ecma-international.org/ecma-262/10.0/index.html#sec-toprimitive
  4. If there is a @@toPrimitive method, use it. Otherwise, use either valueOf() or toString().

The primitive types are Undefined, Null, Boolean, String, Symbol, Number, and BigInt.

So, in conclusion, we're stuck with either String or a numeric type if we want to make < and > work.

@ptomato
Copy link
Collaborator

ptomato commented Apr 28, 2020

How about making @@toPrimitive return an implementation-defined value that must produce the correct comparison order? Or does that just invite problems if users try to store this implementation-defined value? Maybe if we specified it to be a BigInt then that would deter users from serializing it in JSON, at least.

Does this approach preclude operator overloading, if that ever happens?

If this does work then should we remove the compare() methods?

@ljharb
Copy link
Member

ljharb commented Apr 28, 2020

What's the benefit of not defining it ourselves? That seems like a recipe for bugs.

@ptomato
Copy link
Collaborator

ptomato commented Apr 29, 2020

My hand-wavy reasoning was that if it was implementation-defined, then users couldn't write code that depended on it being something specific, and it would leave us free to implement it via operator overloading in the future... emphasis on hand-wavy.

@ljharb
Copy link
Member

ljharb commented Apr 29, 2020

They'd still write code based on "whatever each of the browsers chose", and that would in fact force us to support all of those implementations eventually.

@gilmoreorless
Copy link
Contributor

They'd still write code based on "whatever each of the browsers chose", and that would in fact force us to support all of those implementations eventually.

Or, more likely, they'd write code based only on "whatever Chromium does" and claim that any other behaviour was a bug. (I'm thinking of Twitter's recent problem with caching DMs in Firefox for example.)

@sffc
Copy link
Collaborator Author

sffc commented Apr 29, 2020

The valueOf representations could be (all BigInt):

  1. Temporal.Date: number of days since an epoch (either 0000-01-01 or 1970-01-01).
  2. Temporal.DateTime: number of nanoseconds since that epoch.
  3. Temporal.Time: number of nanoseconds since midnight.
  4. Temporal.YearMonth: same representation as Temporal.Date, consistent with Data model for MonthDay #391.
  5. Temporal.MonthDay: this one is trickier when it comes to extended calendar systems. The easiest thing would be to say that we use the Temporal.Date representation and leave it up to the calendars to pick reference years that cause the ordering to be correct.
  6. Temporal.Absolute: epoch nanoseconds.

In addition, I would say that you can also pass these values back into the constructors. If the constructor receives exactly one numeric argument, then that argument should be interpreted as the corresponding valueOf representation. (The calendar would still be allowed as the second argument.) This is easy in the Temporal.Date and Temporal.DateTime constructors, since we already require at least three numeric arguments for the normal case. For the Temporal.Time constructor, we could start requiring at least hour, minute, and second, such that we can cleanly identify when the call to the constructor is passing the valueOf representation.

@ljharb
Copy link
Member

ljharb commented Apr 29, 2020

MonthDay could end up being a string, not a bigint?

@kaizhu256

This comment has been minimized.

@ljharb

This comment has been minimized.

@littledan
Copy link
Member

I believe we've previously discussed comparison with @pipobscure and @gibson042 . I pushed for valueOf to be removed, rather than trying to make this work, and forgot that toString would also make this work sometimes. See some discussion in #74 #25 #35 #92

I don't think we can make comparisons enabled through valueOf work well, e.g., enforcing the Temporal type hierarchy. It would also move us towards "weak typing" for Temporal in a way that mirrors Date and I think is a source of some confusion. For that reason, I'd prefer to not adopt @sffc 's suggestion.

Can we make the valueOf() method exist and throw an exception on all Temporal types?

@gibson042
Copy link
Collaborator

I think the best way to do that would be a Symbol.toPrimitive method that throws or returns undefined, on hint "number" if not universally. It's deeper than valueOf and used by the spec only for ToPrimitive (which itself is used by ToNumeric, ToNumber, (To)BigInt, and the Abstract Relational Comparison that is our focus here).

@littledan
Copy link
Member

@gibson042 I don't know if I agree with seeing ToPrimitive as "deeper"; I think it's more like, some extra logic to explain how Dates work in comparison. So I'd think about whether we want to go down that path with an exhaustive case analysis of all the observable effects it would have.

The effect of your suggestion would to be to make Temporal objects act like NaN in comparisons, which might be nice, since it'd make all comparisons return false. Another thing we could do is make Symbol.toPrimitive throw an exception with the Numeric hint. (I haven't thought through how other paths would be affected.)

@gibson042
Copy link
Collaborator

I guess the relevant difference between valueOf and Symbol.toPrimitive is that only the latter can differentiate between "default" and "number" hints. And you're right, it doesn't seem to be relevant, so perhaps it is better to use valueOf (with an assumption that it is in a non-"string" context).

@gibson042
Copy link
Collaborator

But remember that valueOf was removed per #74.

@kaizhu256

This comment has been minimized.

@littledan
Copy link
Member

Right, it was removed because we didn't want this particular comparison path to be taken. @sffc points out in this issue, that change didn't have the intended effect, since now the comparison is between strings. So, I propose that we include a valueOf method which throws an exception, to prevent comparison.

@sffc
Copy link
Collaborator Author

sffc commented May 1, 2020

I can see the concerns about .valueOf() causing programming errors by losing the strong typing of the Temporal types. However, those concerns are directly contradictory to how < and > work in JavaScript.

Making .valueOf() throw an exception or return undefined is hostile to users, especially since we have a technically feasible way to make binary comparison operators have the expected behavior. Breaking binary comparison operators because we want to promote strong typing is a decision not to be taken lightly.

I wasn't around for #74, but I'm really not convinced by the premise. If the concern is about people comparing objects of different types, that's a problem that ESLint and TypeScript should solve.

@sffc
Copy link
Collaborator Author

sffc commented May 1, 2020

Let me try to put the debate in different terms.

Consider how this decision affects ergonomics and Stack Overflow questions. There are two types of competing questions that end users might have:

  1. If we have valueOf():
    • Q: I tried comparing two different types of Temporal objects, and the results confuse me.
    • A: You shouldn't do that. Convert the objects to the same type before comparing.
  2. If we don't have valueOf():
    • Q: I tried using < between two Temporal objects, and I got an error. Why doesn't that work?
    • A: Adding those operators would open you up to mistakes when comparing objects of different identities, and TC39 thought that risk outweighed the benefit of making binary comparison operators work.

In my mind, the second question is far worse than the first.

Although JavaScript Date has flaws, it works the way JavaScript developers expect it to work: you can call .valueOf(), and binary comparison operators work. The bad part about JavaScript Date is its confusing data model, how it deals (or doesn't deal) with time zones, and the lack of useful operations like calendar-sensitive date arithmetic. Temporal fixes those problems. Using Temporal to also push against the grain of weak typing in the world's most widely used programming language is something I would have a difficult time supporting.

@littledan
Copy link
Member

Right, I guess this is just a difference in design philosophy. Correctness vs ergonomics is a spectrum, and it might not be best for users to be excessively picky/pedantic here. The history is that we decided to be pedantic, and include an explicit compare method, but we didn't specify/implement this properly. I'm all for revisiting past decisions not just for new data but also if we want to correct course on the design philosopy.

My take: by separating datetimes into several different types, Temporal is pushing against JavaScript's weak typing history. I see the strongly typed aspect of Temporal to really be a core design principle here. So I'd prefer 2: I think errors are far preferable to getting the wrong answer. I could see permitting coercion for certain things, but only when we feel like we can get a correct answer.

But that's just my take; I can see us making some other tradeoff. I just want us to make this tradeoff understanding the history and the failure modes.

@ptomato ptomato added meeting-agenda and removed bug labels May 7, 2020
@ptomato ptomato added the needs plenary input Needs to be presented to the committee and feedback incorporated label May 7, 2020
@littledan littledan added this to the Stage 3 milestone May 14, 2020
@ptomato ptomato modified the milestones: Stage 3, Stable API May 28, 2020
@ptomato
Copy link
Collaborator

ptomato commented May 28, 2020

Meeting, May 28: We've gathered feedback, and after considering it we intend to ship the initial polyfill with a valueOf() that throws. The complaint about not allowing comparison operators was unwieldy syntax, so we will see if that weighs highly in the feedback we get from the polyfill.

ptomato added a commit that referenced this issue May 29, 2020
This adds a valueOf() method to Absolute, Date, DateTime, Duration,
MonthDay, Time, and YearMonth, which throws a TypeError. This is in
order to prevent spurious comparisons using <, <=, >, and >= from
working.

Closes: #517
ptomato added a commit that referenced this issue May 29, 2020
This adds a valueOf() method to Absolute, Date, DateTime, Duration,
MonthDay, Time, and YearMonth, which throws a TypeError. This is in
order to prevent spurious comparisons using <, <=, >, and >= from
working.

Closes: #517
ryzokuken pushed a commit that referenced this issue May 29, 2020
This adds a valueOf() method to Absolute, Date, DateTime, Duration,
MonthDay, Time, and YearMonth, which throws a TypeError. This is in
order to prevent spurious comparisons using <, <=, >, and >= from
working.

Closes: #517
@bergus
Copy link
Contributor

bergus commented Oct 23, 2022

I'm new to this proposal, and I have to say that valueOf() throwing an exception is definitely unexpected. Having </<=/>=/> just work on two Date objects was one of the nice aspects of the old API, and I'd hate to loose that. Also calling .valueOf() was known to be the method that could be called on numeric wrapper objects to get a canonical representation. It would allow two objects to be compared no matter their type - it would work for numbers and dates and many custom classes - a.valueOf() === b.valueOf() is a known idiom that should work fine as long as a and b are of the same type.

I can see why it doesn't make sense to have valueOf() for the Plain… types, they're not simple wrappers around a single numeric value. But I would have expected it to work on the exact time types, Instant for sure and also on ZonedDateTime.

(The excellent docs pages asked for feedback, here it is)

@ptomato
Copy link
Collaborator

ptomato commented Nov 2, 2022

Thanks, I've been mulling over this and I think it's a good discussion to have in the context of a future addition to Temporal: js-temporal/proposal-temporal-v2#6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-consensus needs plenary input Needs to be presented to the committee and feedback incorporated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants