-
Notifications
You must be signed in to change notification settings - Fork 13
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
A strongly-kinded nonce generator #160
base: master
Are you sure you want to change the base?
A strongly-kinded nonce generator #160
Conversation
5a2d4d3
to
d3b419c
Compare
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 of two minds on this. On the one hand, this is a clever trick that lets you catch certain kinds of mistakes more easily. On the other hand, you have to accept that type variables of kind NonceBranch
can't be passed to ST
. This matters for this PR as all of the functions that involve ST
(e.g., withGlobalSTNonceGenerator
) require that the variable that you pass to ST
is different from the variable you pass to NonceGenerator
. As such, I don't think that any of the ST-related functions in this PR could be used in practice, as you almost always want these variables to be the same.
Moreover, wouldn't you need to rewrite what4
's Expr
in order to make its t
type variable be of kind NonceBrand
instead of Type
? As such, this isn't really a drop-in replacement for Data.Parameterized.Nonce
(unless I'm missing something).
I think they wouldn't be as convenient to use in practice, because you'd have to track two type variables (one for the ST monad and one for the nonces), not that they'd be completely unusable, correct? In any case, I've pushed a patch that fixes this. I think it's still sound, as it yields a
Certainly, it's far too late for |
Ah, you've pushed a new version which unifies the two kinds of type variables. Very well. This patch still makes me a bit uneasy, however—likely because this adds quite a lot of additional scaffolding. All of this scaffolding is for the sake of producing better error messages, but it's not immediately obvious to me that the error messages you'd get with this interface actually would be more obvious than the old interface. Moreover, I wonder what is stopping users from writing, say, |
They changed in the way I expected when I made this PR: data Foo (t :: NonceBrandKind) = Foo
test :: forall a. a
test = let f = Foo :: Foo a in () yields
To me, this indicates that I should probably:
IMO, this is better than the
Great point. This is certainly possible, but IMO less likely than the mistake I made. I think it'd be odd for someone to write It would be interesting to see if we could also make a more strongly-kinded version of I don't feel strongly about this PR one way or another and would be fine with just closing it at this point. |
I've pushed a commit that eliminates type family UnwrapNonceBrand (nk :: NonceBrandKind) :: Type where
UnwrapNonceBrand ('NonceBrand s) = s
-- snip --
runSTNonceGenerator :: (forall s . NonceGenerator (ST (UnwrapNonceBrand s)) s -> ST (UnwrapNonceBrand s) a)
-> a This would need more documentation before merging if we decide we like it now, but I at least wanted to see if it was technically feasible to forbid this kind of error as well. |
I just spent a bunch of time fixing up some confusion I had about type variables. The problem was that I was writing
Expr sym
when I should have been writingExpr t
, fort
a phantom type parameter related to nonces. I only uncovered this mistake late in the development process, when trying to string together the end-to-end functionality I had in mind.This confusion was possible because we tend to use the kind
Type
for a wide variety of type-level variables throughout What4 and Crucible. This PR suggests an wrapper forNonceGenerator
that would avoid this pitfall. The primary downside is that we can no longer offerrunSTNonceGenerator
, as theST
monad phantom parameter does have kindType
. FWIW, this function appears unused in our Github repos, so this seems like a minor drawback.