Thanks! I will review after the other roles PRs are merged.

The grouping code you have here feels like a bit of a code smell to me, since we're already doing grouping based on which declarations refer to each other in the BindingGroups desugaring step. It feels to me like these checks ought to be relatively easy to implement alongside the existing BindingGroups desugaring step:

• Role declarations must refer to declarations which exist in the same module
• Role declarations must refer to data declarations
• Role declarations may not be duplicated

so perhaps we should have all of the above in the BindingGroups step and only have the arity check in the type checker? I think if we were to go ahead and implement the first refactoring I discussed in #3910 (comment) that might simplify this code as well. Would you mind if I took a stab at that refactoring first?

Sure, go ahead!

I had a go at doing that refactoring and had some thoughts:

The graph in the BindingGroups step is already very subtle in terms of what the vertices are and which vertices should depend on which, especially in the presence of kind declarations and type synonyms, and I think introducing roles into the mix as well is likely to make that code quite brittle and difficult to change. On reflection, the current approach of putting all the role declarations before any data declarations at the very end of the desugaring phase and then storing roles temporarily in the Environment when we encounter those role declarations during type checking isn't really all that bad.

I think if I was going to try to refactor this to remove the need to put role declarations in the environment, then I'd modify the AST so that we can store role annotation data directly in the DataDeclaration constructor with a new constructor argument. However, there may be drawbacks to that I'm not aware of, and there's also the issue that these are positional arguments which we pattern match on everywhere, so there would be a ton of places to update, which probably wouldn't be fun.

I also realised that the three checks for role declarations which I recommended adding to the BindingGroups step (must refer to a declaration in the same module, must refer to a data or foreign import data declaration, must not be duplicated) are really more analogous to the checks in the Sugar.TypeDeclarations step, which perform those same checks for type and kind signatures, so I think they would be best off going there. I think the simplest way of doing that would be to require that a role annotation directly follows the data type it describes, like how we require that type and kind signatures directly precede the declarations they describe. That does make things more restrictive than they are now, but I suspect most of the code which has been written already follows this rule, and I think it's a sensible rule to impose.

I moved the checks back to Language.PureScript.Sugar.TypeDeclarations and strengthened the orphan check to throw unless role declarations directly follow their type. This is pleasantly simpler than it used to, thank you for the suggestion!

I applied your suggestions.

Sorry about that, I’m trying to get into the habit of pushing fixup commits instead. In the case of whole commits, do you prefer them to be removed from the history or reverted by new commits?

No worries - it's not a big deal, so don't worry if you forget. I think pushing new commits which revert the old unwanted ones is preferable, as that makes it easier to see what changed since the last time you looked.