Human-centric let-rec, constructor and ability orderings #6039
Conversation
ChrisPenner
changed the title
|
Q before merging, @aryairani @mitchellwrosen , Maybe I should not do this for structural types just to be sure? |
|
@ChrisPenner did you see #5893 it gives the safe criteria you can use for sorting |
I'm going to do a quick transcript for this, but one answer is that Now what about for unique types? Do we not have the same problem? |
|
@ChrisPenner 0 = False; 1 = True Oh good, nothing's changed. Oh just kidding, it actually did change. 0 is True and 1 is False. It doesn't break any code, but it may break your mind. Same issue here, possibly less surprising. |
|
#3615 describes the (an?) issue with structural types and constructors of the same type. |
|
@aryairani Is Yeah this seems to be an issue with identical constructors in general, but of course this change would exacerbate it a lot. I think this means we should just leave constructors out of it this iteration and proceed with the effect lists and let-recs for now. EDIT: Looking into what Paul suggested now, sounds like a good plan if it's easy enough to implement. |
|
Okay I've implemented the constructor ordering approach suggested by @pchiusano in #5893 which appears to do the trick, it's a clever idea :) |
| & List.groupOn fst | ||
| -- head is okay since groupOn only returns populated lists. | ||
| <&> \grp -> (fst . head $ grp, snd <$> grp) | ||
| <&> \grp -> (fst . head $ grp, NEL.fromList (snd <$> grp)) |
There was a problem hiding this comment.
NEL.fromList is partial, but this list is guaranteed to be non-empty.
There was a problem hiding this comment.
Tweaked a few helpers to return NonEmpty because 'make invalid state unrepresentable' and all that :P
| groupActionsByLocation xs = | ||
| xs | ||
| & List.groupMap (\(p, act) -> (pathLocation p, (p, act))) | ||
| <&> second Foldable.toList |
There was a problem hiding this comment.
Just a tweak to work with the new NonEmpty
| & List.sortOn (\(_n, (_a, _v, typ)) -> typ) | ||
| -- Now we group by type, we need to leave identical types in their constructor order to avoid things like | ||
| -- swapping identical constructors, e.g. False turning into True and vice versa. | ||
| & List.groupMap (\con@(_, (_, _, typ)) -> (typ, con)) | ||
| -- Then we can sort those _groups_ by the name of the first constructor in the group. | ||
| & List.sortOn | ||
| ( \(_typ, group) -> | ||
| group | ||
| & NEL.sortOn fst | ||
| & \case | ||
| (n, (_, _, _)) :| _rest -> | ||
| PPE.termName ppe (Referent.Con (ConstructorReference r n) ctype) |
There was a problem hiding this comment.
Here's the tweak Paul suggested :)
Overview
We've noticed that since let-rec bindings, constructors, and ability lists are all ordered by hash, they sometimes jump around when you make changes; this means the diffs also get much worse since the bindings might have completely re-ordered in-between changes.
closes #5893
internal ticket
There's no real need for these to be printed in hash-order, since the parser will re-order them according to hash on ingestion anyways.
This changes the printer to put things in a consistent order by name rather than hash.
Implementation approach and notes
Add some
sorts in the printer modules.Test coverage
print-ordering.mdtranscript tests; as well as all the re-rendered existing transcripts