parsonsmatt / annotated-exception

Machinery for throwing and catching exceptions with some annotation.
BSD 3-Clause "New" or "Revised" License
32 stars 3 forks source link

add withFrozenCallStack to checkpointMany #15

Closed avieth closed 2 years ago

avieth commented 2 years ago

checkpointMany is part of the intended user-facing API, so just like checkpoint, we don't want it to appear in the stack trace.

Before submitting your PR, check that you've:

After submitting your PR:

avieth commented 2 years ago

I think I left the callstack in this function intentionally - otherwise we won't get a CallStack entry for any checkpoints if the calling function does not have a HasCallStack constraint

Could be confusing since checkpoint does use withFrozenCallStack. As a user I'd think that checkpoint and checkpointMany would have the same behaviour w.r.t. the call stack.

That said, it may not be the behaviour we want anyway... What I personally would expect from checkpoint/checkpointMany is that the call stack is not frozen but that checkpoint/checkpointMany itself does not appear in the call stack. I think that can be done by having

checkpoint :: (MonadCatch m) => Annotation -> (HasCallStack => m a) -> m a
checkpoint ann = checkpointMany [ann]

checkpointMany :: (MonadCatch m) => [Annotation] -> (HasCallStack => m a) -> m a
checkpointMany anns action = action `catch` ...
parsonsmatt commented 2 years ago

The withFrozenCallStack for checkpoint should mean we don't see checkpointMany and checkpoint in the CallStack.

I don't think we should see any CallStack that references source code in the library. But I do think we should see a CallStack that references the call site of checkpoint or checkpointMany. That's the intent of the current withFrozenCallStack use, which may itself be buggy, which is why I think some tests demonstrating expectations and current behavior would be good to see.

avieth commented 2 years ago

The withFrozenCallStack for checkpoint should mean we don't see checkpointMany and checkpoint in the CallStack.

Yup, but it also means we don't see any frames from the action given to it, which seems to not be what we want. Omitting the HasCallStack from checkpoint will mean that checkpoint does not appear.

avieth commented 2 years ago

I think some tests demonstrating expectations and current behavior would be good to see.

I'll write something up

parsonsmatt commented 2 years ago

Yup, but it also means we don't see any frames from the action given to it, which seems to not be what we want.

I'm not sure about that - my understanding of checkpoint:

checkpoint :: (HasCallStack, MonadCatch m) => Annotation -> m a -> m a
checkpoint ann = withFrozenCallStack (checkpointMany [ann])

We can eta expand and parenthesize for clarity:

checkpoint ann action = 
    (withFrozenCallStack (checkpointMany [ann])) action

So we are freezing the callstack that checkpointMany sees (such that the last entry is checkpoint), but action's CallStack is not frozen.

Omitting the HasCallStack from checkpoint will mean that checkpoint does not appear.

I want checkpoint to appear in the CallStack, though. If I write:

checkpoint "hello" do
  checkpoint "goodbye" do
    error "oh no"

then I want to see both checkpoint call-sites on the resulting error. I don't want to see a CallStack that references code in the annotated-exception library, though.

avieth commented 2 years ago

We can eta expand and parenthesize for clarity:

checkpoint ann action = 
    (withFrozenCallStack (checkpointMany [ann])) action

Ah! I see it now. Thanks

avieth commented 2 years ago

Also did not realize that the intention of checkpoint was to add a stack frame. Makes sense now.