Check role declarations against inferred roles#3873
Check role declarations against inferred roles#3873hdgarrood merged 9 commits intopurescript:masterfrom
Conversation
Fixes purescript#3867. The main change here is that we are now storing checked roles in the `types` field of the Environment for `data` and `foreign import data` declarations, via the additions to the `TypeKind` type. Because we are storing roles in the environment, we no longer need to infer roles for any given type more than once. As such, `checkRoles` now works in a monad with `MonadState CheckState`, and when we come across another type while inferring roles, we can simply look its roles up from the Environment. The BindingGroups desugaring step ensures that types are in topological order, so this is safe: any mentioned types' roles will already have been checked and stored in the Environment. One case that we do not yet handle is types which cyclically depend on each other, e.g. data F a = F (G a) data G a = G (F a) Rather than crashing the compiler, these will now be incorrectly marked Phantom. I would like to address this in a future PR.
|
@kl0tl would you like to review this? I think the approach I've taken here should allow us to take advantage of existing parts of the compiler -- specifically the BindingGroups desugaring pass and the type checking Environment -- to make this a bit more performant and robust. Starting from what I have here, my hope is that handling role inference for recursive types should be relatively straightforward, since we can take advantage of the BindingGroups pass, but it will end up looking a bit different from how you did it in your other PR. |
|
I'm also not bothering to deal with type synonyms or constrained types in data declarations yet, and that's also on purpose - I'd prefer to address these things incrementally. |
kl0tl
left a comment
kl0tl
left a comment
There was a problem hiding this comment.
This looks great!
Is there a good reason for giving phantom roles to prim types and kinds? I would’ve picked nominal. I guess it doesn’t matter much since Coercible isn’t polykinded but it allows one to coerce Row Type and RowList Type to Row Symbol and RowList Symbol for instance, which doesn’t make sense to me.
My general approach was that I was trying to give the same roles as would have been inferred. We've got:
|
|
I don’t think the roles picked cause any issues but |
|
Admittedly I'm unable to come up with a situation in which those dictionaries are useful to have, but it seems reasonable to me that you could e.g. coerce an |
|
I guess you‘re right. This still feels weird to me but that‘s definitely not a strong enough argument against phantom roled |
| TypeApp _ (TypeApp _ fn k1) _k2 | eqType fn tyFunction -> | ||
| go (Nominal : acc) k1 | ||
| _k -> | ||
| Nominal : acc |
There was a problem hiding this comment.
We should recurse on the second argument of -> and return the accumulator untouched in the base case. Otherwise we return [Nominal] for Type, [Nominal, Nominal] for Type -> Type and Type -> Type -> … -> Type, [Nominal, Nominal, Nominal] for (Type -> Type) -> Type…
module Example where
import Safe.Coerce (coerce)
foreign import data Foreign3 :: Type -> Type -> Type -> Type
-- We infer `nominal nominal` instead of `nominal nominal nominal`,
-- this is equivalent to giving a phantom role to `Foreign3` last parameter:
example :: ∀ a b c d. Foreign3 a b c -> Foreign3 a b d
example = coerceAlso now that we have kind polymorphism we should recurse under ForAll:
module Example where
import Safe.Coerce (coerce)
foreign import data Foreign2 :: ∀ k. k -> k -> Type
-- We infer `nominal` instead of `nominal nominal`,
-- this is equivalent to giving a phantom role to `Foreign2` last parameter:
example :: ∀ k (a :: k) (b :: k) (c :: k). Foreign2 a b -> Foreign2 a c
example = coerceI understand this function was just renamed and extracted from Language.PureScript.TypeChecker.Roles so I can open an issue and deal with this in another pull request if you so wish.
There was a problem hiding this comment.
Dealing with this in another pull request sounds preferable to me. Thanks!
|
Thanks @natefaubion, @kl0tl! |
3 participants
Fixes #3867. The main change here is that we are now storing checked
roles in the
typesfield of the Environment fordataandforeign import datadeclarations, via the additions to theTypeKindtype. Because we are storing roles in the environment, we no longer need
to infer roles for any given type more than once. As such,
checkRolesnow works in a monad with
MonadState CheckState, and when we comeacross another type while inferring roles, we can simply look its roles
up from the Environment. The BindingGroups desugaring step ensures
that types are in topological order, so this is safe: any mentioned
types' roles will already have been checked and stored in the
Environment.
One case that we do not yet handle is types which cyclically depend on
each other, e.g.
data F a = F (G a)
data G a = G (F a)
Rather than crashing the compiler, these will now be incorrectly marked
Phantom. I would like to address this in a future PR.