Combine clauses of record-selector function declarations #181
Conversation
There was a problem hiding this comment.
I'm happy to accept this patch. Many thanks for writing it!
But, it strikes me that any use of getRecordSelectors will be wrong in exactly this way. So I think fixing the problem at the source would be an improvement. It's also conceivable that we should refactor getRecordSelectors out of th-desugar and into singletons. It's unclear to me at the moment as to why that function is there. The new getRecordSelectors would have to work on a list of Cons.
Other designs around this are welcome.
| concatMapM (getRecordSelectors arg_ty) cons | ||
| mergeLetDecs <$> concatMapM (getRecordSelectors arg_ty) cons | ||
|
|
||
| -- After retrieving the record selectors from a data type's constructors, it |
src/Data/Singletons/Promote.hs
Outdated
| -- their clauses. | ||
| | DFunD n clauses <- x | ||
| = let (eq_n, neq_n) = partition (\case DFunD n2 _ -> n == n2; _ -> False) xs | ||
| merged_clauses = concat $ clauses:map (\(DFunD _ cls) -> cls) eq_n |
There was a problem hiding this comment.
partitionWith (stolen from GHC and put in Data.Singletons.Util) should simplify this.
There was a problem hiding this comment.
Good catch, I'll refactor it to use partitionWith.
|
I agree that making |
|
Considering |
|
Oh, it turns out there'd be one awkward aspect of removing
|
|
I suppose option 3 is the most widely helpful.... just in case some user of |
|
Closing in favor of goldfirere/th-desugar#55. |
Previously, in
promoteDataDecswe were simply callinggetRecordSelectorson each constructor of a data type and building aLetDecEnvmap of names to function declarations clauses. But if a record selector is contained in multiple constructors, then the clauses of the record selector function will be spread out across multipleDLetDs, so when theLetDecEnvis built, it only keeps the most recentDLetDfor a particular record selector name! This causes the incorrect promoted definition that appeared in #180.This adds
mergeLetDecsto post-process the results ofgetRecordSelectorsso that all of the clauses for a particular record selector are grouped together before being turned into aLetDecEnv. I've added a lengthy comment abovemergeLetDecsexplaining what it does and why it's needed.Fixes #180.