Remove Eq constraint and other large improvements#12
Conversation
| -- | ||
| -- The 'CallStack' will *not* be present as a 'CallStack' - it will be | ||
| -- a 'CallStackAnnotation'. | ||
| -- An alias for 'throwWithCallStack'. |
There was a problem hiding this comment.
This looks weird to me. Is there really no difference between throw and throwWithCallStack? Their implementations are different. If they do the same thing in practice, should one actually be an alias of the other, or should we just remove one of them?
There was a problem hiding this comment.
The only difference should be in withFrozenCallStack which prevenst the callstack from having ["throw", "throwWIthCallStack"].
There was a problem hiding this comment.
Ah right I see - in that case can we get rid of one of them?
There was a problem hiding this comment.
Everyone: "Haskell code has no side effects."
HasCallStack: "Hold my beer."
There was a problem hiding this comment.
I suppose we could drop one of them. I like having the alias around to keep Control.Exception.Annotated (as much as possible) a drop-in replacement for other Control.Exception code.
Really, though, there's a lot that needs to be done there to make the APIs totally compatble.
Kinda wish backpack were easier to use, seems like a natural desire 🤔
There was a problem hiding this comment.
I don't know that I fully agree with this idea of giving the functions the same name as some functions in some other module as a "drop in" replacement. It's almost like "reassignment by shadowing" or like dynamic scoping of variables ("extreme late binding in all things," right?). It just seems like a very error-prone practice, and all it really saves you from is having to do a regex replace.
There was a problem hiding this comment.
Ah, of course. Keeping it around just to make it slightly easier to switch over makes sense to me, even if it’s not currently a full drop-in replacement.
|
|
||
| - [#12](https://github.com/parsonsmatt/annotated-exception/pull/12) | ||
| - Removed the `Eq` instance for `Annotation` as well as the `Eq` constraint | ||
| in `AnnC`. These instances were only used for testing, and prevented the |
There was a problem hiding this comment.
These instances were only used for testing,
I wish there were a way to expose something like this for testing without influencing the library interface, other than like CPP.
| (callStacks, other) = | ||
| tryAnnotations (newAnnotations <> oldAnnotations) | ||
| in | ||
| foldr addCallStackToException (AnnotatedException other e) callStacks |
There was a problem hiding this comment.
Wouldn't it make more sense for AnnotatedException to have a Maybe CallStack field, instead of extracting out the callstacks and treating them specially?
There was a problem hiding this comment.
Sorry if this question is kinda obtuse. I know I'm missing a lot of context.
There was a problem hiding this comment.
No, it's a great question, and something I waffled on a bit.
It's nice to have the exposed constructor and just write AnnotatedException annotations blah and then do foldr whatever def annotations. I like that.
It's also nice to have a notion of uniformity for how annotations are handled. It's a list. I, as a library author, don't get any special tricks that you, a library consumer, don't also have access to. So if I want to treat CallStack in a special way, I do so with the same tools and techniques as anyone else would. In that way, the implementation in the library serves as a sort of "how to manual" on using the library as well, if you studied the implementation.
CallStack seems like a reasonable exception. And it would be easy to have:
data Annotations = Annotations { getAnnotations :: [Annotation], annotationsCallStack :: Maybe CallStack }But now, suppose I write this:
lmao :: HasCallStack => IO a -> IO a
lmao =
checkpoint (Annotation callStack)This is going to slam the CallStack into [Annotation], not the Maybe CallStack field. Unless I also do a type-check on all the functions which can add to the [Annotation] to defer to the CallStack instead, if the type works. Possible, sure:
annotate :: [Annotation] -> AnnotatedException e -> AnnotatedException e
annotate anns (AnnotatedException oldAnns e) = AnnotatedException newAnns e
where
newAnns = foldr updateAnnotation oldAnns anns
updateAnnotation ann acc =
case castAnnotation ann of
Just callstack ->
acc { annotationCallStack = mergeCallStack callstack (annotationCallStack acc) }
Nothing ->
acc { getAnnotations = ann : getAnnotations acc }And this logic had to be duplicated everywhere I modify Annotations, much like how it is done today.
Making it flexible/extensible would be something like, uh,
data TypeMap (xs :: [Type])
data SomeTypeMap where
SomeTypeMap :: TypeMap xs -> SomeTypeMap
data Annotations = Annotations { regularAnnotations :: [Annotation], specialAnnotations :: SomeTypeMap }
annotate :: Annotation -> Annotations -> Annotations
annotate ann anns =
case ann of
Annotation (a :: a) ->
case lookupTypeMap @a (specialAnnotations anns) of
Just existing ->
anns { specialAnnotations = mergeAnnotation a (specialAnnotations anns) }
Nothing ->
anns { regularAnnotations = ann : regularAnnotations anns } But then, the "special handling of annotations" is dependent on the specific value of the annotation in question, so you'd want to define something like:
myThrow = throwIO (AnnotatedException myEmptyAnnotations e)
myEmptyAnnotations = Annotations [] what
where
what = SomeTypeMap
$ TypeMap.handle @CallStack mergeCallStack
$ TypeMap.handle @Foobar mergeFooBarWhich also seems weird - seems weird for an exception to know how to handle it's own annotations.
There was a problem hiding this comment.
I understand your thinking. I guess I disagree in that I'm way more open to the idea of hiding the data constructors, so the lmao scenario couldn't happen. Also informing my opinion is the notion that the callstack is not (or at least should not be) a part of the semantics of the Haskell language, so hiding it to prevent people from branching on it would be an ideal feature (and treating them specially would be justified and would not then suggest a generalization). But I understand that people can have different goals, so thank you for explaining how your design does, in fact, meet your goals quite well.
There was a problem hiding this comment.
I guess I disagree in that I'm way more open to the idea of hiding the data constructors, so the lmao scenario couldn't happen.
Well, supposing I replace Annotation with toAnnotation :: (Typeable a, Show a) => a -> Annotation, that still doesn't help, because Annotation doesn't handle anything specially - AnnotationList would.
Another option would be something like:
class (Typeable a) => ToAnnotation a where
toAnnotation :: a -> AnnotationList
default toAnnotation :: (Show a, Typeable a) => a -> AnnotationList
toAnnotation a = mempty { annotations = [Annotation a] }
instance ToAnnotation CallStack where
toAnnotation cs = mempty { annotationCallStack = Just cs }
instance Semigroup AnnotationList where
Annotations anns mcs <> Annotations anns' mcs' =
Annotations (anns <> anns') (mergeCallStacks mcs mcs')
instance Monoid AnnotationList where
mempty = AnnotationList mempty Nothing🤔
But then you have to write a ToAnnotation instance for basically everything, which means you'd need to expose (at least) set/modify on AnnotationList, and you'd need the constructor for Annotation available. Which gets us back to our original problem of Annotation callStack not working how we'd want.
| -- The library maintains an internal check that a single 'CallStack' is present | ||
| -- in the list, so this only returns the first one found. If you have added | ||
| -- a 'CallStack' directly to the @['Annotation']@, then this will likely break. | ||
| -- |
There was a problem hiding this comment.
Okay, sorry for sounding like a broken record here, but this just corroborates my earlier thinking. Wouldn't it really make more sense for AnnotatedException to have a Maybe CallStack field?
| -- not a huge fan of the direct recursion, but it seems easier than trying | ||
| -- to finagle a `foldr` or something |
There was a problem hiding this comment.
it'd be neat if the compiler could verify that at least one branch/equation doesn't make a recursive call. that still wouldn't guarantee termination, though.
There was a problem hiding this comment.
also wouldn't catch loops in mutual recursion.
friedbrice
left a comment
There was a problem hiding this comment.
Okay, don't waste time on answering me if you've already thought this through, but from where I'm standing, it looks like you're continually doing a lot work traversing your annotations looking for one (or more?) that might be a callstack. I mean, it looks like it should work, and it looks like you're being very careful, but I can't help thinking that a tweak to the data model would eliminate the need to be so careful and save on a lot of this work.
friedbrice
left a comment
There was a problem hiding this comment.
FWIW, I think these changes are overall truly good, right and salutary.
Before submitting your PR, check that you've:
@sincedeclarations to the Haddock.stylish-haskelland otherwise adhered to the style guide.After submitting your PR: