In a couple of places you use Phantom as a default for a lookup. Can you clarify why this is desirable?

The phantom role is used as a default because it is the least restrictive, so later inference passes can only refine it to a more precise role. We always prefer the more restrictive role inferred for the same parameter in the Semigroup instance for RoleMap and updateRoles.

For instance, given these definitions:

data D1 a = D1 a
data D2 a = D2 (D1 a)

When inferring D2 roles we discover they depend on those of D1, but we don’t know them yet so we pick phantom for a and store that in our role environment. Then we infer the type parameter of D1 is representational, store that in our role environment and go back to D2. This time we know D1 roles so we can infer a is representational and update the role environment by taking the more restrictive of phantom and representational for a, which is representational.

Rather than keeping this function "pure", I think it might be preferable (or at least better aligned with the way the rest of the type checker is written) for it to operate in MonadState CheckState m, and read and write directly from the actual type-checking environment, rather than doing its own thing at first and then later updating the environment after inferring/checking roles. I don't feel particularly strongly about this, though. @natefaubion do you have any thoughts on this?

I don't really have much of an opinion. Is there some currying advantage to how the functions are arranged, and how it returns an additional lambda, that's exploited elsewhere?

The explicit floating of initialRoleEnv and inferredRoleEnv outside of the do block in checkDataBindingGroupRoles is supposed to allow the roles of the data declarations in the binding group to be inferred only once and then looked up repeatedly in the DataBindingGroupDeclaration case of Language.PureScript.TypeChecker.typeCheckAll. This isn’t something I’ve verified because I’m not familiar with Core inspection but I’m not sure GHC would be able to share the work otherwise.

I can try to store the inferred roles environment in the typechecker monad state and separate roles inference from roles checking in binding groups. This should be moree performant than computing a new roles map from scratch each time we need to check roles.

Since neither @natefaubion nor I feel strongly I’m tempted to say let’s leave this as is for now. I’m happy to merge once there’s a comment about what the Any in inferDataDeclarationRoles means.

Ah, I guess it’s that we want to keep walking over all of the data declarations in the binding group until we reach a point where walking doesn’t change anything in the roleEnv, and the Any part of the result indicates whether something changed?

I lifted the role inference to the typechecker monad!

Could you add some documentation about the meaning of the Any in the return type please?

I added a comment and aliased the Any to DidRolesChange to make things explicit.

This change seems a little excessive. Whenever you want to lookup a role, instead of looking it up directly, you are mapping over the entire type environment to build another environment that is a subset of the original. I understand that there may be some clever laziness involved (it isn't clear to me that you are getting that with Map and mapMaybe vs something like List), but this seems like an easy way to also accidentally introduce poor asymptotics. What was the motivation for this change? If it's only to share code, I think you could extract out the predicate for reuse rather than try to reuse getRoleEnv directly.

I wanted to share logic and thought the whole map wouldn’t actually be built thanks to laziness but I should have verified this assumption. I’ve reused a typeKindRoles function in both getRoleEnv and lookupRoles.