-
Notifications
You must be signed in to change notification settings - Fork 211
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
Add new record completion operator #1375
Conversation
... as standardized in dhall-lang/dhall-lang#767
Note that this requires a few other changes to be merged so that this can be updated to use the corresponding |
@@ -689,6 +689,8 @@ eval !env t0 = | |||
vCombineTypes (eval env t) (eval env u) | |||
Prefer t u -> | |||
vPrefer env (eval env t) (eval env u) | |||
RecordCompletion t u -> | |||
eval env (Annot (Prefer (Field t "default") u) (Field t "Type")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we know that eval
ignores the type annotation, can we just skip adding it here?
eval env (Annot (Prefer (Field t "default") u) (Field t "Type")) | |
eval env (Prefer (Field t "default") u) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I prefer to pedantically follow the standard given that we don't have any benchmarks showing that this is a measurable optimization
@@ -178,7 +178,10 @@ instance Arbitrary Directory where | |||
instance (Arbitrary s, Arbitrary a) => Arbitrary (Expr s a) where | |||
arbitrary = | |||
Test.QuickCheck.suchThat | |||
(Generic.Random.genericArbitraryRecG customGens weights) | |||
(Generic.Random.withBaseCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is good, but I don't actually quite understand why. Can you help me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes an infinite loop. Adding one more constructor caused the weights to generate an infinitely growing expression tree. Using withBaseCase
ensures that the random generator can still cap the recursion depth when this happens so that we don't have to be so diligent with weights.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! This sounds like a bug in generic-random
. Should we report this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it is a bug. The documentation from withBaseCase
seems to suggest that this behavior is expected if you don't use withBaseCase
... as standardized in dhall-lang/dhall-lang#767