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

Every Js.Date is equal to every other Js.Date #3455

Closed
mlms13 opened this issue Mar 28, 2019 · 7 comments
Closed

Every Js.Date is equal to every other Js.Date #3455

mlms13 opened this issue Mar 28, 2019 · 7 comments

Comments

@mlms13
Copy link
Contributor

mlms13 commented Mar 28, 2019

Using == in Reason (or = in OCaml), all Js.Date.t values are equal:

let a = Js.Date.fromFloat 1553816680458.0
let b = Js.Date.fromFloat 0.0
let _ = Js.log (a = b) (* true *)

I tried looking through the caml_equal function, but I got a bit lost. :)

@bobzhang
Copy link
Member

hi, = is unsafe unless you know what you do. We have a warning flag (102) to warn unsafe usage of =, we accept patches to make the default behavior less surprising

@yawaramin
Copy link
Contributor

More details on the unsafety here https://blog.janestreet.com/the-perils-of-polymorphic-compare/

@cristianoc
Copy link
Collaborator

For reference: this is the point where dates would be handled:
https://github.com/BuckleScript/bucklescript/blob/fafc9fd350a29275bb523c17564aed27879efc96/jscomp/runtime/caml_obj.ml#L328

After O.isArray one would check O.isDate which presumably would use instanceof Date.
Date equality does not work with == in JS so one would need to use <= and >=.

@bobzhang
Copy link
Member

bobzhang commented Apr 3, 2019

It makes sense to make the default less surprising, we are going to have this fixed in next release

@yawaramin
Copy link
Contributor

One option to kind of cover this and other surprises with polymorphic compares is to by default enable 102 as a warning. Actually, now that I think about it, I want to propose some stricter warnings and errors by default anyway. I'll open a separate issue.

@bobzhang
Copy link
Member

bobzhang commented Apr 4, 2019

@yawaramin it used to be default, but people don't like it ..

@yawaramin
Copy link
Contributor

@bobzhang that makes sense actually–I had it turned on for a bit and realized it's not great because the warning doesn't go away–it keeps appearing even if the file isn't touched afterwards 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants