Restrict over', iover', and set' to traversals #475
Conversation
* The `Identity'` type was non-standard and overly complicated. The `Solo` type, now in `base`, is operationally identical and simpler to deal with. Use that instead. * Remove some surprisingly lazy matches on `Identity'`.
treeowl
force-pushed
the
solo
branch
3 times, most recently
from
7a7585d
to
4d41f41
Compare
* `over'`, `iover'`, `set'`, and associated operators previously accepted setters. However, it's impossible to actually modify strictly through a setter; a traversal is needed for that. Restrict the types to require `A_Traversal`, and remove the associated (technically correct but deceptive) `Mapping` instances. * Document the strictness behavior of `set'`. Fixes well-typed#473
|
I think it would be rather nice to add the |
| over' | ||
| :: Is k A_Setter | ||
| :: Is k A_Traversal | ||
| => Optic k is s t a b | ||
| -> (a -> b) -> s -> t | ||
| -- See [Note: Solo wrapping] | ||
| over' o = \f -> | ||
| let star = getOptic (castOptic @A_Setter o) $ Star (wrapIdentity' . f) | ||
| in unwrapIdentity' . runStar star | ||
| let star = getOptic (castOptic @A_Traversal o) $ Star ((Solo $!) . f) | ||
| in getSolo . runStar star |
There was a problem hiding this comment.
Should we move set' and over' to Optics.Traversal instead?
There was a problem hiding this comment.
They would be a better fit, probably; I was just trying to avoid breaking things unnecessarily.
| @@ -77,6 +77,7 @@ library | |||
| , indexed-profunctors >= 0.1 && <0.2 | |||
| , transformers >= 0.5 && <0.7 | |||
| , indexed-traversable >= 0.1 && <0.2 | |||
| , OneTuple >= 0.3 && <0.4 | |||
There was a problem hiding this comment.
This looks like it might pull in quite a few extra dependencies to optics-core. Is that right, and can we avoid it?
There was a problem hiding this comment.
Actually, perhaps this is true only for older GHC versions?
There was a problem hiding this comment.
It looks to me like the package is quite careful to avoid excessive dependencies when possible.
There was a problem hiding this comment.
For psychological reasons I'd prefer us to not include this dependency.
The cost of not doing so is irrelevant - Solo can be copy pasted where Identity' was along with its Applicative instance.
There was a problem hiding this comment.
@arybczak Psychological reasons? Why put the compat code here when someone else has already written it there?
There was a problem hiding this comment.
Psychological reasons?
Yes, people don't like a lot of deps in "core" packages. Whether it's rational or not is irrelevant.
There was a problem hiding this comment.
Okay ... I'll copy whatever's needed over.
| -- The new value will be forced if and only if the optic traverses at | ||
| -- least one target. If forcing the new value is inexpensive, then it | ||
| -- is cheaper to do so manually and use 'set'. |
There was a problem hiding this comment.
I wonder if we could easily test this, since we didn't before and apparently didn't get it right...
There was a problem hiding this comment.
I'm not sure what the intention is here. I'm just documenting what it actually does.
There was a problem hiding this comment.
Or were you referring to testing strictness generally? containers does some of that; I haven't checked if there's traversal strictness testing, but if so that could be copied maybe.
| @@ -102,14 +102,28 @@ over o = \f -> runFunArrow $ getOptic (castOptic @A_Setter o) (FunArrow f) | |||
| -- 'over' is used, because the first coordinate of a pair is never forced. | |||
| -- | |||
| over' | |||
There was a problem hiding this comment.
The haddocks above here have another "TODO DOC" it would be great to nail along with this.
There was a problem hiding this comment.
The intuition is that each target result is forced before producing the full result. More precisely, I believe
over' p f xs = forceElements res `seq` res
where
forceElements = foldrOf p seq ()
res = over p f xsThere was a problem hiding this comment.
That "law" only works with a non-type-changing traversal, but I think it's a good way to express the concept.
over',iover',set', and associated operators previouslyaccepted setters. However, it's impossible to actually modify strictly
through a setter; a traversal is needed for that. Restrict the types
to require
A_Traversal, and remove the associated (technicallycorrect but deceptive)
Mappinginstances.Document the strictness behavior of
set'.The internal
Identity'type was non-standard and overly complicated.The
Solotype, now inbase, is operationally identical and simpler todeal with. Use that instead.
Remove some surprisingly lazy matches on
Identity'/Solo.Fixes #473