Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upAllow ADTs and records in Sets and as Dict keys #929
Conversation
brianhempel
added some commits
Dec 14, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
process-bot
Dec 20, 2017
Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!
Here is what to expect next, and if anyone wants to comment, keep these things in mind.
process-bot
commented
Dec 20, 2017
|
Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it! Here is what to expect next, and if anyone wants to comment, keep these things in mind. |
brianhempel
referenced this pull request
Dec 20, 2017
Closed
User-defined data types (and records) not comparable #774
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
evancz
Dec 20, 2017
Member
Based on reading the description and some links, it seems like this gives short-term benefits for your project, but it is not a coherent overall design that is well-integrated with the language.
My general policy is that "making changes" is not default good. It may be that everyone agrees that the current thing is bad. It may also be that some people think that a certain thing would be better, but also not what we'd want to do ultimately. I think it is best to err on the side of conservative changes in the language and core libraries, especially when work arounds exist.
So I don't think it makes sense to go for this now. I do think your idea of "internally comparable" vs comparable with normal functions is interesting. It is 100% observable via Map.foldr and Map.foldl though, so I don't know if that distinction is real in practice. (I.e. I can just define my own lessThan function using a Dict underneath.)
Do you mind writing up the particular thing you need to use as keys? No proposals, just a description of your situation. I think having real world data like this will be really valuable in finding a path that we'll be happy with in 20 years.
|
Based on reading the description and some links, it seems like this gives short-term benefits for your project, but it is not a coherent overall design that is well-integrated with the language. My general policy is that "making changes" is not default good. It may be that everyone agrees that the current thing is bad. It may also be that some people think that a certain thing would be better, but also not what we'd want to do ultimately. I think it is best to err on the side of conservative changes in the language and core libraries, especially when work arounds exist. So I don't think it makes sense to go for this now. I do think your idea of "internally comparable" vs comparable with normal functions is interesting. It is 100% observable via Do you mind writing up the particular thing you need to use as keys? No proposals, just a description of your situation. I think having real world data like this will be really valuable in finding a path that we'll be happy with in 20 years. |
evancz
closed this
Dec 20, 2017
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
evancz
Dec 21, 2017
Member
As a side note, making relatively serious changes to the language and core libraries is always preceded by conversation. I know other communities do this "PR or GTFO" insanity, as if it is in any way reasonable to have people write tons of code before having any conversations about expectations of timelines or viability.
I think "PR or GTFO" mindset is pretty crazy, and it definitely leads to negative interactions where folks feel "I did all this work, and your not merging it?!" Folks see themselves as giving and the maintainer as being unappreciative. My analysis is that every "contribution" is better framed as a "collaboration" and you have to talk and know each others plans to collaborate effectively.
Anyway, I think the best move is to write up the particular usage scenario so we can do some focused design work on it.
|
As a side note, making relatively serious changes to the language and core libraries is always preceded by conversation. I know other communities do this "PR or GTFO" insanity, as if it is in any way reasonable to have people write tons of code before having any conversations about expectations of timelines or viability. I think "PR or GTFO" mindset is pretty crazy, and it definitely leads to negative interactions where folks feel "I did all this work, and your not merging it?!" Folks see themselves as giving and the maintainer as being unappreciative. My analysis is that every "contribution" is better framed as a "collaboration" and you have to talk and know each others plans to collaborate effectively. Anyway, I think the best move is to write up the particular usage scenario so we can do some focused design work on it. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
brianhempel
Dec 21, 2017
In SnS we allow the user to select various pieces of a program's output, much like selection in a traditional graphics editor. We store these selected "features" in a set, as it doesn't make sense and could cause bugs if a feature was somehow double-selected. The hierarchy of selectable features is a fairly complex ADT which, previous to this PR, we were serializing and deserializing into strings. If we had anticipated how unwieldy our serialization/deserialization code would get, we would have simply used a plain list to store the selected features and used special functions on that list to treat it like a set. In other words, the best workaround for us would be to not use the built-in Set for our sets.
Skimming all the requests for adding comparability to everything, most people don't really want comparability, they want to be able to put anything into a set (c.f. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, and 11-last message in thread) which is why this PR eschews a broad public definition of comparability and opts for an internal one, much like the current implementation of ==. If you're trying to avoid typeclasses or functor-modules (you may not be—I don't know) I don't see how else sets can be appropriately generalized.
Edit: Removed an unhelpful emotional phrase.
brianhempel
commented
Dec 21, 2017
•
|
In SnS we allow the user to select various pieces of a program's output, much like selection in a traditional graphics editor. We store these selected "features" in a set, as it doesn't make sense and could cause bugs if a feature was somehow double-selected. The hierarchy of selectable features is a fairly complex ADT which, previous to this PR, we were serializing and deserializing into strings. If we had anticipated how unwieldy our serialization/deserialization code would get, we would have simply used a plain list to store the selected features and used special functions on that list to treat it like a set. In other words, the best workaround for us would be to not use the built-in Set for our sets. Skimming all the requests for adding comparability to everything, most people don't really want comparability, they want to be able to put anything into a set (c.f. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, and 11-last message in thread) which is why this PR eschews a broad public definition of comparability and opts for an internal one, much like the current implementation of Edit: Removed an unhelpful emotional phrase. |
brianhempel commentedDec 20, 2017
•
edited
Edited 1 time
-
brianhempel
edited Dec 20, 2017 (most recent)
It's a pain to serialize and deserialize ADTs and records to and from comparable types when you want to use them in a set. This patch allows ADTs and records to be stored in sets and used as dictionary keys. However, they are still not
comparable. The comparison operators> < >= <=continue to only operate on the previouscomparabletypes.This PR implements part (2) of this proposal which observes that you might allow any type in sets without making all types
comparable. Here's how it works:SetandDictimplementation need the key type to have an order, but the order itself is not important because order is not part of the mathematical contract for what aSetorDictis. This patch augments the Elm internalcmpfunction to also assign an order to values of any non-function type. (ADT values are ordered first alphabetically by constructor name, then by each argument; for records, fields are compared in alphabetical order.)comparable. The typechecker continues to ensure that the> < >= <=etc. functions are only applied to the normalcomparabletypes.This patch allows us to reduce the Sketch-n-Sketch codebase by ~225 hairy and opaque lines of serialization and deserialization.
As is, this PR is based off of Core 5.1.1 so we could use it in Sketch-n-Sketch right away. If the approach is acceptable I'm happy to rework it against the current
devbranch.