Check that instance declarations don't overlap#3129
Check that instance declarations don't overlap#3129LiamGoodacre merged 1 commit intopurescript:masterfrom LiamGoodacre:feature/overlapping-inst-decl
Conversation
| :: Qualified (ProperName 'ClassName) | ||
| -> TypeClassData | ||
| -> [Type] | ||
| -> m (S.Set ModuleName) |
There was a problem hiding this comment.
Oh right, I was thinking that internalError required it.
| typesApart :: Int -> Bool | ||
| typesApart i = typeHeadsApart (lhs !! i) (rhs !! i) | ||
|
|
||
| typeHeadsApart :: Type -> Type -> Bool |
There was a problem hiding this comment.
Maybe this should live alongside the equality test we have elsewhere?
| -> S.Set ModuleName | ||
| -> m () | ||
| checkOverlappingInstance ch dictName className typeClass tys' nonOrphanModules = do | ||
| for_ nonOrphanModules $ \m -> do |
There was a problem hiding this comment.
I think there is a possible issue with MPTCs here, where we can miss an instance in an unrelated module, but that's fine. We should probably just document it.
There was a problem hiding this comment.
Unless I've misunderstood you, I'm not sure that's correct. The modules in nonOrphanModules are the only modules that the instance is allowed to appear based on the type class's covering sets (we'd get an orphan instance error if one were in a different module). So we are definitely seeing all the possible instances that could overlap. Right?
There was a problem hiding this comment.
Wait, I understand, we're only looking for modules that the instance talks about.
There was a problem hiding this comment.
Right, I can always define an instance for an unrelated data type in another module. This is only an issue with FlexibleInstances or MPTCs though.
| nonOrphanModules' = foldl1 S.intersection (foldMap lookupModule `S.map` typeClassCoveringSets typeClass) | ||
| findNonOrphanModules _ _ _ = internalError "Unqualified class name in findNonOrphanModules" | ||
|
|
||
| checkOverlappingInstance |
There was a problem hiding this comment.
Could you please add a comment here?
paf31
left a comment
There was a problem hiding this comment.
Looks good at first glance, thanks for working on this! I'll review again once there is a failing example though.
|
The build will fail due to the lack of purescript/purescript-psci-support#7 |
|
Did you want to take another look at this? @paf31 (or anyone else interested). |
|
I feel like I've reviewed this in quite a lot of detail already, and it's been tested quite a lot, so I think it's more than good enough for a first release. |
|
(btw, I can't approve my own PRs) 😸 |
|
Thanks @hdgarrood |
Fixes #2957
I plan to add a failing example which demonstrates the covering sets aspect of this.