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

Discourage/deprecate set<double> and map<double, V> #178

Open
sfackler opened this issue Nov 30, 2018 · 4 comments
Open

Discourage/deprecate set<double> and map<double, V> #178

sfackler opened this issue Nov 30, 2018 · 4 comments
Assignees

Comments

@sfackler
Copy link
Member

Floating point keys are currently allowed as keys in map and set types. This poses particular problems for the Rust Conjure implementation, where floating point values can't be used as keys in set and map types. We could in theory fix this but only through pretty drastic measures like having our own custom collection types and associated traits. Instead, I'm just planning on not supporting set<double> and map<double, V> in generated code.

Beyond that specific language integration issue, floating point keys are a bit iffy generally IMO since it's so easy to drift just slightly off of the exact numeric value that you're expecting. It seems like it might be best to discourage the use of these types minimally.

@ferozco
Copy link
Contributor

ferozco commented Dec 3, 2018

Adding a deprecation warning sounds reasonable to me. Long term it does seem desirable to disallow such usage. We've spoken about this exact issue before and from what I recall the only reason we didn't remove set<double> and map<double, V from the language was out of concern for breaking current usage.

@ferozco ferozco self-assigned this Dec 3, 2018
@bmoylan
Copy link
Contributor

bmoylan commented Feb 2, 2019

+1 I am currently working on jumping through hoops to support these types in conjure-go (which does not natively support json-encoding of floating point map keys, and will straight-up fail to serialize NaN (which is not comparable) without a very custom implementation)

@raiju
Copy link
Contributor

raiju commented Dec 8, 2021

@carterkozak This might be an interesting one to add to the --strict flag?

@sfackler
Copy link
Member Author

sfackler commented Dec 8, 2021

Follow up note here - conjure-rust does now support these types, but via a bit of a gross API and I think they're semantically a bit iffy in general due to precision issues: palantir/conjure-rust#155

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

5 participants