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

Batch constraints to within maxTupleSize in deriveArgDict #48

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jonathanlking
Copy link
Contributor

This PR fixes #47 by batching constraints to never exceed maxTupleSize.

@Ericson2314
Copy link
Member

Can you add a test with a large sum type?

Comment on lines 29 to 27
batchConstraints :: [Type] -> Type
batchConstraints cs
| length cs == 0 = TupleT 0
| otherwise = AppT (AppT (TupleT 2) initialBatch) (batchConstraints rest)
where
(initial, rest) = splitAt (maxTupleSize - 1) cs
initialBatch = foldl AppT (TupleT (length initial)) initial

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to do something like

Suggested change
batchConstraints :: [Type] -> Type
batchConstraints cs
| length cs == 0 = TupleT 0
| otherwise = AppT (AppT (TupleT 2) initialBatch) (batchConstraints rest)
where
(initial, rest) = splitAt (maxTupleSize - 1) cs
initialBatch = foldl AppT (TupleT (length initial)) initial
batchConstraints :: [Type] -> Type
batchConstraints cs
| length cs <= maxTupleSize = foldl AppT (TupleT (length cs)) cs
| otherwise = batchConstraints $ batchConstraints <$> chunkBy maxTupleSize cs

maybe there is an issue, but that seems about right. I am out of time to think about this ATM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seemed to work and I'm very happy to make the change. I couldn't find a chunkBy, or any function :: Int -> [a] -> [[a]], in base or an existing dependency, so I just wrote an implementation in this library rather than add a new library dependency.

@jonathanlking jonathanlking force-pushed the fix/deriving-large-types branch from a21dc28 to c9fdfd9 Compare November 1, 2022 19:24
@jonathanlking
Copy link
Contributor Author

This appears to have been fixed by #35.
I have rebased this branch, just keeping the test. There might be value in merging the test to prevent a future regression, but I wouldn't be too disappointed otherwise.

@jonathanlking jonathanlking force-pushed the fix/deriving-large-types branch from c9fdfd9 to 895cc79 Compare November 18, 2022 12:19
@ali-abrar
Copy link
Member

Looks like the test doesn't pass

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

Successfully merging this pull request may close these issues.

deriveArgDict fails for types with many constructors
3 participants