IntersectMBO / IntersectMBO/ouroboros-consensus
Resolve Orphan Instances
- Dominant language
- Haskell
- Stars
- 67
- Forks
- 43
- Avg merge
- 5d 13h
- Merged PRs (30d)
- 43
Description
The ouroboros-network code base contains many orphan instances. The `{-# OPTIONS_GHC -Wno-orphans #-}` pragma is used in 143 modules. Removing the pragmas reveals ~1000 orphan instances. We should consider resolving these into non-orphan instances.
### Thoughts on why are orphan instances bad
* They create a maintenance burden
* If GHC complains that an instance does not exist one must now ask if there is an orphan instance somewhere. I assume this results in fragile regex search over the whole code base. If we discourage orphans then the decision is easy: place it by any of the relevant types.
* When creating an instance, the programmer must consider if to place the instance near the relevant types or to put it in some orphan instance module for more consistency. This requires a good understanding of existing orphan instances. If we discourage orphans then we don't need to consider this.
* They encourage more orphan instances.
* Often instances depend on other instances. E.g. Serialization of `data X = X A B` in `ModuleX` might uses the serialization instances for `A` and `B` in module `ModuleA` and `ModuleB` respectively. If I write the `instance Serialize X` next to `data X = ...` GHC might complain that `A` and `B` are not instances of `Serialize`. Then I do some regex to find if/where the orphan instances are and find/import them from the `Orphans` module. Now GHC complains of a module cycle! `Orphans` contains many instances for types from many modules and unsurprisingly imports `ModuleX`. Now I have to move the `Serialize X` instance into `Orphans` thereby adding a new orphan instance. In some cases you can't do this as the `Serialize X` instance requires functions or constructors not exported from `ModuleX`! In that case you can keep the instance in `ModuleX` and add constraints: `instance (Serializable A, Serializable B) => Serializable X`, but this is quite unfortunate as anything that uses this instance now also has to add those constrains or import the Orphan module (assuming that doesn't cause another module cycle!) . See https://github.com/input-output-hk/ouroboros-network/pull/3214#discussion_r666556323.
* Fundamentally they are bad as they allow multiple overlapping instances to exist without compile time errors until you create a use site with at least 2 overlapping instances are in scope.
### Thoughts on refactoring
* It'll be hard to resolve ~1000 orphan instances all in one PR. Since many instances depend on other instances, we'll likely have to start with instances for some of the simpler data types in order to avoid dragging all the other instances along with it.
* The only orphans left over should be exclusively for classes and types in external packages:
* Can we move some of those instances into one of the dependent packages?
* Perhaps we can put all such orphans in a single module without depending on many home modules. After all, the instances are exclusively for external types.
Contributor guide
Research direction
Start by inventorying the 143 modules using `{-# OPTIONS_GHC -Wno-orphans #-}` and the roughly 1,000 orphan instances they expose. Read the linked PR discussion for context, then identify a small group of simple data types and their dependent instances as a possible first step. Done means reducing orphan instances without introducing module cycles, while retaining only unavoidable instances for external types.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- haskell
- Domain
- blockchain
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100