Remove Eq constraint and other large improvements

[parsonsmatt]

Before submitting your PR, check that you've:

• Bumped the version number.
• Documented new APIs with Haddock markup.
• Added @since declarations to the Haddock.
• Ran stylish-haskell and otherwise adhered to the style guide.

After submitting your PR:

• Update the Changelog.md file with a link to your PR.
• Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts).

[hdgarrood]

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?

[parsonsmatt]

The only difference should be in withFrozenCallStack which prevenst the callstack from having ["throw", "throwWIthCallStack"].

[hdgarrood]

Ah right I see - in that case can we get rid of one of them?

[friedbrice]

Everyone: "Haskell code has no side effects."

HasCallStack: "Hold my beer."

[parsonsmatt]

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 🤔

[friedbrice]

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.

[hdgarrood]

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.

[friedbrice]

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.

[friedbrice]

Wouldn't it make more sense for AnnotatedException to have a Maybe CallStack field, instead of extracting out the callstacks and treating them specially?

[friedbrice]

Sorry if this question is kinda obtuse. I know I'm missing a lot of context.

[parsonsmatt]

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 mergeFooBar

Which also seems weird - seems weird for an exception to know how to handle it's own annotations.

[friedbrice]

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.

[parsonsmatt]

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.

[friedbrice]

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?

[friedbrice]

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.

[friedbrice]

also wouldn't catch loops in mutual recursion.

[friedbrice]

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]

FWIW, I think these changes are overall truly good, right and salutary.