Parameterize Split
#5523
Open
Parameterize Split
#5523
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
They’re isomorphic to `Name` and `HQ'.HashQualified Name`. There are some benefits to the alternative structure, but overall it just resulted in a lot of duplicated code. `Name` can be restructured in future, but first we need to eliminate the duplication.
All uses of `HashQualified` should now be outside the full pathy structure. There are a couple motivations for this: 1. it makes the many `Name` ↔︎ `Split` conversions simpler and 2. changing the `Path` component of a `Split` should have to pass through the `HashQualified` structure, because it can potentially invalidate the hash[^1]. [^1]: `HashQualified` shouldn’t have a `Functor` instance, because it should only be modifiable in carefully-controlled ways, but that’s a battle for another day. Types like `Split` and `AbsSplit` still exist because there aren’t `Name` equivalents to those types (and there are additional types like `(ProjectPath, NameSegment)` that are structured similarly to `Split`).
It now takes the path-like type as an argument, so we use `Split Path`, `Split Absolute`, and `Split ProjectPath` as appropriate. Some operations are also generalized over `Split p`.
This adds a type class, `Pathy`, with instances for the common path types: `Path`, `Path'`, `Absolute`, and `Relative`. The intent is to simplify the code, but also to make it easier to transition away from direct `Path` usage in future. This also renames a few operations, to make them more path-specific. E.g., instead of `snoc`, it’s `descend`[^1]. and `absoluteEmpty` is now `root`. [^1]: `snoc` and `cons` are particularly confusing names, because we store segments in different orders in different contexts, and it’s not clear whether `snoc`/`cons` should refer to the internal ordering or the conceptual ordering. Also, `Path` is likely to reverse it order at some point to match `Name`, and if `snoc`/`cons` reflects the internal ordering, then that’ll break things. Also the `Path` constructor/field are no longer exported, and `Path'` is now defined directly in terms of the `pattern`s that were used everywhere.
With the various changes since removing `Split'`, it now makes more sense to use `Split Path'` (structurally identical to `Split'`) than `Name`.
This covers `ProjectPath` and `BranchRelativePath`. It required splitting `Name` related functions into a separate class, because these types don’t convert to `Name`. I think that’s a good idea anyway, as we want to phase out those conversions.
This also tightens up path types in a few places.
|
I generally recommend reviewing PRs commit-by-commit, and I still do here, but it’s worth noting that some changes from the first commit were reverted in the sixth commit. Because of the intervening work, it was complicated to rewrite the history to avoid the reversion. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This is motivated by making changes to UCM commands that require handling many different variants of path types. This results in a lot of almost-duplicated code that needs to be updated in many places whenever there is a change.
This primarily makes two changes:
HashQualifiedoutside of theHQSplittuples andSplitby its path type.This results in the following changes to types:
(path, NameSegment)→Split pathAbsSplit→Split AbsoluteHQSplit'→HashQualified (Split Path')HQSplitAbsolute→HashQualified (Split Absolute)HQSplit→HashQualified (Split Path)Split'→Split Path'Split→Split PathThere are a number of related changes that follow from this:
PathyandNameyclasses that provide common operations across these types,BranchRelativePathandProjectPathwith these types,HQvariants ofSplittypes (these are now all simplyfmaportraverse), andSplit PathbecomingSplit Relativein some cases).And a couple other changes that were just easy to make:
snocis nowdescendandunsnocis nowsplit),Showfrom path serialization, andPath'a sum type, rather than anewtype.Interesting/controversial decisions
One thing that feels controversial is the increased use of the unsafe
Functor HashQualifiedinstance. However, I think this PR just exposes already unsafe usage. Basically, modifying the name in aHashQualifiedcan invalidate the hash, butFunctorcan’t manage that. Because of the change from(Path, HashQualified NameSegment)toHashQualified (Path, NameSegment), changes to thePathnow requireFunctor HashQualified. But it was already the case that changing that path could make the hash not match thePath </> NameSegment, it’s just more apparent now.Loose ends
There are still plenty of ways to improve the path handling in Unison. This brings things a bit closer. I think an eventual unification of
SplitandNameis worthwhile, combining the benefits of each.Split’s structure allows for distinguishing between absolute and relative paths at the type levelName’s reversed segment ordering allows for simpler code