Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HasCallStack to fail, empty, mzero, asum and msum #301

Open
arybczak opened this issue Oct 26, 2024 · 45 comments
Open

Add HasCallStack to fail, empty, mzero, asum and msum #301

arybczak opened this issue Oct 26, 2024 · 45 comments

Comments

@arybczak
Copy link

I propose we include HasCallStack in the context of the following functions:

  • fail from MonadFail
  • empty from Alternative
  • mzero from MonadPlus
  • asum and msum from Data.Foldable

fail, empty and mzero all signal failure of some sort, but it's currently impossible to retrieve a meaningful call stack when it happens, making debugging harder than it should be.

Moreover, fail is used implicitly in pattern match failures in do notation and while the error message includes location of the failing pattern match, it would be much better if there was a possibility of including the call stack.

At the moment if one wants to have Alternative/MonadPlus interfaces that report meaningful call stacks for failures, it's necessary to provide specialized versions of empty and asum, kinda defeating the point of the generic interface.

asum and msum would also need HasCallStack since they themselves call empty/mzero if all computations on the list fail.


Hopefully it's less controversial than #115, because fail/empty/mzero unconditionally signal failure and are generally small, so during compilation either they are specialized and inlined, or they are not specialized and the overhead of the HasCallStack is insignificant when compared to the overhead of being in a polymorphic Applicative/Monad and calling dictionary functions all the time.

asum and msum are small and both already have INLINE pragmas on them.

PoC: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/13491 (failing tests are due to expected output changes).

This change is fully backwards compatible.

@phadej
Copy link

phadej commented Oct 27, 2024

Adding HasCallStack to mzero doesn't feel right. Would you also advocate adding HasCallStack to Monoid.mempty? I barely see a difference.

@arybczak
Copy link
Author

arybczak commented Oct 27, 2024

While both mempty and mzero are technically identities, they are IMO very different because of their kinds. mempty :: m is a value that represents empty something, while mzero :: m a is a monadic action that can't produce a, they are used in different context (even for types for which these two definitions happen to coincide, like Maybe and []).

Also, from hackage docs (description of mzero):

The identity of mplus. It should also satisfy the equations

mzero >>= f  =  mzero
v >> mzero   =  mzero

which means it's a propagating failure in a monadic context.

Also, from description of fail:

Instances of MonadFail should satisfy the following law: fail s should be a left zero for >>=,

fail s >>= f  =  fail s

If your Monad is also MonadPlus, a popular definition is

fail _ = mzero

@parsonsmatt
Copy link

parsonsmatt commented Oct 27, 2024

+1

The exception message with mzero :: IO a is terrible:

λ> mzero :: IO ()
*** Exception: user error (mzero)

Having even a limited CallStack here would make a huge difference for diagnosing errors that arise from this.

@tomjaguarpaw
Copy link
Member

Maybe then HasCallStack should be a constraint on instance MonadPlus IO? I don't see why we should force it on all instances of MonadPlus. It doesn't add much to instance MonadPlus [], for example.

@parsonsmatt
Copy link

Maybe then HasCallStack should be a constraint on instance MonadPlus IO? I don't see why we should force it on all instances of MonadPlus. It doesn't add much to instance MonadPlus [], for example.

That's not possible. You can't do instance (HasCallStack) => MonadPlus IO because implicit parameters are not allowed in constraints. The only way to get a callstack on this is to add it in the class function signature. Likewise you can't use an InstanceSig because instance signatures aren't allowed to have more constraints than the definition - they can only be specialized to the type you're defining the instance for.

@arybczak
Copy link
Author

It doesn't add much to instance MonadPlus [], for example.

FWIW if it's not used, GHC will optimize it away:

unknown@electronics haskell $ ./ghc/_build/stage1/bin/ghc --interactive
GHCi, version 9.13.20241024: https://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/unknown/.ghci
>>> import Control.Monad
>>> :t mzero
mzero
  :: (MonadPlus m, GHC.Internal.Stack.Types.HasCallStack) => m a
>>> 
Leaving GHCi.
unknown@electronics haskell $ cat call.hs
module Call where

import Control.Monad

test :: [Int] -> [Int]
test xs = do
  x <- xs
  if even x
    then mzero
    else pure x
unknown@electronics haskell $ ./ghc/_build/stage1/bin/ghc -fforce-recomp -ddump-simpl -dsuppress-all -O call.hs
[1 of 1] Compiling Call             ( call.hs, call.o )

==================== Tidy Core ====================
Result size of Tidy Core
  = {terms: 36, types: 21, coercions: 0, joins: 0/0}

$trModule4 = "main"#

$trModule3 = TrNameS $trModule4

$trModule2 = "Call"#

$trModule1 = TrNameS $trModule2

$trModule = Module $trModule3 $trModule1

Rec {
test
  = \ ds_a12X ->
      case ds_a12X of {
        [] -> [];
        : y_a130 ys_a131 ->
          case y_a130 of a1_a12k { I# ipv_a12l ->
          case remInt# ipv_a12l 2# of {
            __DEFAULT -> : a1_a12k (test ys_a131);
            0# -> test ys_a131
          }
          }
      }
end Rec }

@ocharles
Copy link

Are there any practical reasons why addingHasCallStack would matter here? Does it carry a performance penalty?

@phadej
Copy link

phadej commented Oct 27, 2024

While both mempty and mzero are technically identities, they are IMO very different because of their kinds. mempty :: m is a value that represents empty something, while mzero :: m a is a monadic action that can't produce a, they are used in different context (even for types for which these two definitions happen to coincide, like Maybe and []).

But someone may argue that the asum definition using foldr:

asum :: (Foldable t, Alternative f) => t (f a) -> f a
{-# INLINE asum #-}
asum = foldr (<|>) empty

is not correct, and instead it should be defined using foldMap (c.f. sum):

    sum :: Num a => t a -> a
    sum = getSum #. foldMap' Sum

i.e.

asum :: (Foldable t, Alternative f) => t ( f a) -> f a
asum = getAlt #. foldMap Alt

and then, you'd need HasCallStack on mempty too.

@phadej
Copy link

phadej commented Oct 27, 2024

Are there any practical reasons why addingHasCallStack would matter here?

It sets precedent. It's easy to argue that fromInteger should also have HasCallStack, and fromIntegral. minimum and maxium in Foldable. Probably a lot more type-class members and generic functions would benefit from HasCallStack. Some may have partial implementation, some may convey some other kind of error. Should they all have HasCallStack, it's up to CLC to decide.

@parsonsmatt
Copy link

I'm in favor of HasCallStack being added to class signatures for any method where there are instances in base that throw exceptions (and, additionally, adding it to every top-level function that throws).

@tomjaguarpaw
Copy link
Member

I'm non-commitally supportive of adding HasCallStack to any functions in base that throw. I'm not yet convinced about extending to types of all class methods whose implementation can throw, especially purely algebraic constructions like empty, mzero, asum and msum. Since fail is specifically for failure, it's more justifiable.

@arybczak
Copy link
Author

arybczak commented Oct 28, 2024

I'm not yet convinced about extending to types of all class methods whose implementation can throw, especially purely algebraic constructions like empty, mzero, asum and msum.

HasCallStack constraint and runtime exceptions are only loosely related. You can use HasCallStack productively in code that doesn't touch IO. For example, if you define "enhanced" newtype MaybeT m a = MaybeT (m (Either CallStack a)) and mzero propagates Left CallStack instead of Nothing, you can have runMaybeT :: MaybeT m a -> m (Either CallStack a) that will tell you the call site of mzero.

@tomjaguarpaw
Copy link
Member

tomjaguarpaw commented Oct 28, 2024

Sure, and you could also define a new type that has an "enhanced" behaviour for mplus too. It might be quite useful to know where we combined two possibilities. Is there a clear criterion that specifies when HasCallStack should be added to a method and when not?

@arybczak
Copy link
Author

arybczak commented Oct 28, 2024

Sure, and you could also define a new type that has an "enhanced" behaviour for MonadPlus too.

I don't know what that means.

Is there a clear criterion that specifies when HasCallStack should be added to a method and when not?

I don't know :)

@tomjaguarpaw
Copy link
Member

tomjaguarpaw commented Oct 28, 2024

I don't know what that means.

Theoretically, something like this could be useful. Does that mean we should add HasCallStack to mplus too? If not, why not?

data MaybeT m a = MaybeT CallStack (m a)

mplus ::
  MonadPlus m =>
  HasCallStack =>
  MaybeT m a ->
  MaybeT m a ->
  MaybeT m a
mplus (MaybeT _ ma1) (MaybeT _ ma2) =
  MaybeT callStack (ma1 `Control.Monad.mplus` ma2)

@arybczak
Copy link
Author

arybczak commented Oct 28, 2024

Theoretically, something like this could be useful.

Would it? How come? Why would you ever want to have a call stack of the last invocation of mplus?

@parsonsmatt
Copy link

parsonsmatt commented Oct 28, 2024

Adding HasCallStack to mzero etc will immediately and obviously improve the awful diagnostic that the IO instance gives. The benefit is clear and significant. In the absence of any demonstrated cost, I think we should just go for it.

As for "should we add HasCallStack elsewhere?" - I think we can take that on a case-by-case basis. If someone requests it and demonstrates benefit, the cost to adding it is really quite small. If someone reports a bad diagnostic experience from fromInteger being partial, then yes let's add HasCallStack to that - after they make an appropriate CLC ticket, as is done here.

@tomjaguarpaw
Copy link
Member

Theoretically, something like this could be useful.

Would it? How come? Why would you ever want to have a call stack of the last invocation of mplus?

It could also be all invocations of mplus, not just the last one. Why wouldn't it be useful? It's useful to know the invocation of mzero, so it seems it could be useful to know the invocation of mplus, right?

Perhaps someone knows a reason it's strictly more important to know the invocation of mzero than mplus. If so then they should state it!

Adding HasCallStack to mzero etc will immediately and obviously improve the awful diagnostic that the IO instance gives. The benefit is clear and significant. In the absence of any demonstrated cost, I think we should just go for it.

I agree, except with the "just going for it" part. There's a bad instance (instance MonadPlus IO) and to fix it we're suggesting changing the class definition, not the instance. But there's nothing wrong with the class!

@mpickering
Copy link

Big -1 from me. Proliferating HasCallStack to functions in base is an anti-pattern.

  • Behaviour of these functions could branch on the calling location if the callstack is inspected.
  • As a point of principle, backtrace collection mechanisms should not affect the type of a function. Please invest time in improving the other backtrace mechanisms if they are not sufficient.

For me, I'm not sure how adding HasCallStack is much different to the motivation for having fail :: String -> m a as part of the definition of Monad. The only difference seems to be that one implicitly passes a structured string and the other an explicit user-supplied String.

@arybczak
Copy link
Author

arybczak commented Oct 29, 2024

Why wouldn't it be useful? It's useful to know the invocation of mzero, so it seems it could be useful to know the invocation of mplus, right?

I demonstrated clearly why having a HasCallStack on mzero improves the existing code (namely, you get the location of failure which makes debugging vastly easier), you're talking about some sort of theoretical scenario that might or might not exist where someone for some reason (that you can't articulate) would want call stacks of mplus that generally in existing code doesn't fail and you're asking ME to provide a counter-argument why wouldn't it be useful. No man, it doesn't work like that.

The proposal is small, well-defined and its benefits clearly demonstrated. It's a strict improvement over the status quo in terms of easing debugging pains when dealing with a few kinds of errors. If someone wants me to provide more information in the context of the proposal, please do so. I'm not going to engage in any further discussions regarding head-in-the-sky musings in this thread.


Behaviour of these functions could branch on the calling location if the callstack is inspected.

Behaviour of any function can branch out based on cleverly placed unsafePerformIO that checks the current time 🤷

As a point of principle, backtrace collection mechanisms should not affect the type of a function.

This ship has sailed the moment GHC got HasCallStack.

Please invest time in improving the other backtrace mechanisms if they are not sufficient.

In fact, multiple people tried hard to improve over the status quo (https://gitlab.haskell.org/ghc/ghc/-/merge_requests/3236, https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6797) and it eventually got superseded by https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8869 which implements ghc-proposals/ghc-proposals#330 and utilizes HasCallStack as the only mechanism that works out of the box:

unknown@electronics ghc $ cat test_ex.hs 
module Main where

import Control.Exception.Backtrace

main :: IO ()
main = do
  setBacktraceMechanismState CostCentreBacktrace True
  setBacktraceMechanismState HasCallStackBacktrace True
  setBacktraceMechanismState ExecutionBacktrace True
  setBacktraceMechanismState IPEBacktrace True
  error "test error"
unknown@electronics ghc $ _build/stage1/bin/ghc test_ex.hs 
[1 of 2] Compiling Main             ( test_ex.hs, test_ex.o )
[2 of 2] Linking test_ex
unknown@electronics ghc $ ./test_ex 
test_ex: Exception:

test error
CallStack (from HasCallStack):
  error, called at test_ex.hs:11:3 in main:Main

Package: ghc-internal
Module: GHC.Internal.Exception
Type: ErrorCall

Cost-centre stack backtrace:
IPE backtrace:
HasCallStack backtrace:
  collectBacktraces, called at libraries/ghc-internal/src/GHC/Internal/Exception.hs:169:13 in ghc-internal:GHC.Internal.Exception
  toExceptionWithBacktrace, called at libraries/ghc-internal/src/GHC/Internal/Exception.hs:204:5 in ghc-internal:GHC.Internal.Exception
  error, called at test_ex.hs:11:3 in main:Main

So it doesn't look like an easy problem to fix and HasCallStack seems to be the best approach we had for multiple years now.

@mpickering
Copy link

@arybczak

I am sure you can understand there is a difference between using an unsafe compiler function and using a value advertised in the type signature. Perhaps we could also have mzero :: HasCallStack => String -> m a so that the user can annotate each call site easily with the reason for the failure as well as getting a call stack?

You also raise a very good point about it taking over 4 years to navigate the maze of committee bureaucracy in order to attempt to improve the situation. I think after all that, motivation to keep working on improvements grew a little bit thin. However I don't think that it gives the excuse of not doing things in a principled manner, as these small fixes accumulate over time and lead to a messy interface. This seems to me an argument to reduce the amount of oversight and barriers to improvement than any argument in favour of the current proposal.

@tomjaguarpaw
Copy link
Member

I'm not going to engage in any further discussions regarding head-in-the-sky musings in this thread.

You're welcome to engage however you wish. Yet, what you call "head-in-the-sky musings" are an important part of my deliberative process, especially when it comes to the more "mathematical" or "algebraic" designs in base. Without progress on those head-in-the-sky musings, my vote will be a "no". But you should advocate for your proposal as you prefer, of course.

I am not, in general, inclined to put purity above pragmatism, nor vice versa. I approach the tension on a case-base-case basis. The case in question, of Applicative, Alternative, Monad, MonadPlus and family, is of well-defined, principled, mathematical abstractions. I am extremely reluctant to "adjust" them to incorporate pragmatism, for the sake of a small number of unfortunate instances.

I'm open to being persuaded otherwise, but that would require engaging with my "headi-in-the-sky musings".

MonadFail, on the other hand, is not principled. I have no objection to adding HasCallStack to fail.

@TeofilC
Copy link

TeofilC commented Oct 29, 2024

HasCallStack as the only mechanism that works out of the box:
but it's currently impossible to retrieve a meaningful call stack when it happens, making debugging harder than it should be.

I'm not sure if this is fair. Sure you need to compile your program with IPE information to get native backtraces, but that seems less invasive than having to modify base to me. What's blocking users from using IPE backtraces in GHC-9.14 (other than it not being on by default)? I think this is a very important question for our community know in general.

Putting aside concerns around principles, what are the concrete costs to this proposal?

  • Do we know how much this will increase the size of binaries/libraries (if at all)?
  • Have we checked that this will definitely not break any code?

In the original post you say these are not issues, but it would be good to see some numbers to back that up.

It might be helpful to commit the updated test files on the MR, I'm not sure if we can start a head.hackage build if those jobs are failing.

@hasufell
Copy link
Member

I tend to agree with @mpickering about HasCallStack being somewhat of a hack.


Wrt the process:

This seems to me an argument to reduce the amount of oversight and barriers to improvement than any argument in favour of the current proposal.

I think that's not a fair assessment.

The exception proposals were voted on one by one. Part one took 3 months to vote (which I still consider an ok timeframe). The following votes could only happen after that. Part 4 took longer because GHC devs were unsure about the design.

I also think that we discussed the interplay between CLC and GHC proposals... the major difference being that GHC proposals don't require an up-front implementation, while CLC proposals do. Afair the consensus was that CLC can give non-binding opinions for GHC proposals that touch base (and don't have an implementation yet).

Is there anything else you think that could be improved?

@mixphix
Copy link
Collaborator

mixphix commented Oct 29, 2024

I'm also in the "algebraic typeclasses" boat. Should we carry around the extra information when working with mzero :: Maybe a? empty :: [a]? These are not "failure" states in the same way that fail from MonadFail (where "fail" is in the name) suggests. They are agnostic multipurpose mathematical abstractions that shouldn't leak compiler data. I'm inclined to agree that instance Alternative/MonadPlus IO is a footgun that should be avoided. From the documentation:

{- from GHC.Internal.Base -}
-- | Takes the first non-throwing 'IO' action\'s result.
-- 'empty' throws an exception.
--
-- @since base-4.9.0.0
instance Alternative IO where
    empty = failIO "mzero"
    (<|>) = mplusIO

{- from GHC.Internal.IO -}
-- Using catchException here means that if `m` throws an
-- 'IOError' /as an imprecise exception/, we will not catch
-- it. No one should really be doing that anyway.
mplusIO :: IO a -> IO a -> IO a
mplusIO m n = m `catchException` \ (_ :: IOError) -> n

There have always been ways to misuse the instance, and I personally think that rather than infect the rest of my Alternatives with HasCallStack, I'd prefer to remove instance Alternative/MonadPlus IO.

@parsonsmatt
Copy link

As a point of principle, backtrace collection mechanisms should not affect the type of a function. Please invest time in improving the other backtrace mechanisms if they are not sufficient.

This ship sailed a long time ago. HasCallStack is "the way" to do callstacks in Haskell. I am glad that we're working on improvements, but we cannot ignore the bad state of the status quo for improvements that aren't here yet and may not materialize for years.

I am sure you can understand there is a difference between using an unsafe compiler function and using a value advertised in the type signature.

This feels a little disingenuous. HasCallStack => m a is a signature that says "I may throw an exception, and you want the diagnostic information in the GHC-provided HasCallStack when I do." No one sees HasCallStack as a means of branching on the caller, and doing so would be at least as dubious as unsafePerformIO in any code base I've worked on.

There have always been ways to misuse the instance, and I personally think that rather than infect the rest of my Alternatives with HasCallStack, I'd prefer to remove instance Alternative/MonadPlus IO.

I would be pretty likely to vote in favor of removing these instances. They are footguns. The impact assessment would have to come back pretty strong for me to feel otherwise.

However, we could improve people's lives today at virtually no cost. Adding HasCallStack isn't aesthetically nice, but it solves a very real pain point and a very real problem in Haskell development. Every other language has implicit callstacks, all the time, for everything, and the lack of these callstacks make debugging and diagnosing Haskell issues a huge pain in the ass. It's one of the biggest problems we have at work, and that's after I've spent an enormous amount of time and effort on patterns and libraries like annotated-exception and require-callstack.

What's blocking users from using IPE backtraces in GHC-9.14 (other than it not being on by default)?

Honestly, I don't know anything about IPE backtraces, except that they're a "new and upcoming" replacement for HasCallStack. There's almost no documentation on how to use them. The -finfo-table-map flag was apparently introduced in GHC 9.2, but it's unclear to me how to actually get a stack trace here - are we waiting on GHC 9.14 (?!) before stack traces from this can generally be available?


We should be focusing on solving real problems. The bad diagnostic information in IO-derived instances of Alternative and MonadPlus is a real problem. We can solve it now with this proposal.

Please consider the concrete experience of the person who is trying to fix a production system or implement a feature, and they are staring down this message:

*** Exception: user error (mzero)

Now please imagine telling them: "We could have put a HasCallStack on this function, but I decided against it. It wouldn't have been very clean or elegant. Besides, at some point in the distant future, there may be several compiler flags and runtime options you can select to get a full stack trace. I hope you can understand."

@TeofilC
Copy link

TeofilC commented Oct 29, 2024

Honestly, I don't know anything about IPE backtraces, except that they're a "new and upcoming" replacement for HasCallStack. There's almost no documentation on how to use them. The -finfo-table-map flag was apparently introduced in GHC 9.2, but it's unclear to me how to actually get a stack trace here - are we waiting on GHC 9.14 (?!) before stack traces from this can generally be available?

My understanding is that IPE backtraces should work in GHC 9.10. I mention 9.14 because that's when the proposed base change (to add HasCallStack) would first appear to users, right?

My understanding is that IPE backtraces have been around since GHC 9.4, but the annotated exception proposal was required to make them ergonomic, and that was released in 9.10.

So it sounds like there is at least a documentation gap. That's good to know!

@tbidne
Copy link

tbidne commented Oct 29, 2024

I share the unease over HasCallStack showing up in signatures. Details like that really shouldn't be part of the type. It's worse on typeclasses since you'll have HasCallStack on instances that have nothing to do with stacktraces, which is really gross. But what is the alternative? Suffer until/if IPE backtraces are improved?

Even the (excellent!) backtraces proprosal doesn't solve this problem, namely, that HasCallStack is AFAICT the best general exception reporting mechanism:

  1. DWARF is only available on linux, and stacktraces are hard to understand.
  2. Profiling is a non-option for many use-cases e.g. production servers.
  3. IPE is seemingly unpredictable.

That leaves HasCallStack, as far as producing reliable, accurate, and portable stacktraces.

I hope this doesn't sound like I am complaining, since I know it's hard and I really appreciate all of the work that has gone into it. The current state is much better than even just a couple years ago. But, as a user, I still don't know what I should be doing other than using HasCallStack as often as possible.

I don't know what the Right ™️ solution is, since -- as many users have pointed out -- there are plenty of functions in base with this same problem. Annotating everything that could possibly be involved with stacktraces is probably not viable. Yet here's the bloodied patient, and here we are in the emergency room.

@tomjaguarpaw
Copy link
Member

Please consider the concrete experience of the person who is trying to fix a production system or implement a feature, and they are staring down this message:

*** Exception: user error (mzero)

Now please imagine telling them: "We could have put a HasCallStack on this function, but I decided against it. It wouldn't have been very clean or elegant. Besides, at some point in the distant future, there may be several compiler flags and runtime options you can select to get a full stack trace. I hope you can understand."

This is indeed a sad story and I sympathize with the poor soul in this situation. However, it's not the only story we could tell.

For example, did the mzero occur in our own code? Then the story would proceed with me telling them "We could have avoided mzero, for example with an hlint or stan rule. We could have chosen to write and use our own alternative, that captures as much of the surrounding context as we liked, but we decided to use mzero anyway, in the hope that we could persuade the CLC to add a HasCallStack constraint to it, which would give us at least a single call stack entry. I hope you understand.".

Or did the mzero occur in client code that we have no control over? But luckily, in this story, we had already persuaded the CLC to amend mzero? Then our colleague is staring at the message

*** Exception: use error (mzero)
CallStack (from HasCallStack):
  mzero, called at <foo>:xyz:1 in bar

Then I would proudly explain "Yes, I managed to get that single-level call stack, by persuading the CLC to essentially retrospectively patch someone else's code." My colleague looks back blankly, wondering how they're going to use that single-level call stack in the debugging process, since it barely gives more clue than the mzero message itself. It fails to explain where the caller of mzero is itself called!


To be clear, I do not think the status quo is good. But nor do I think that slapping the HasCallStack bandaid on things that might error is a good solution in general. I particularly do not think it's a good solution when it's applied to type classes that, firstly, are algebraic constructions and, secondly, will have very few instances that use HasCallStack at all.

@Bodigrim
Copy link
Collaborator

Then I would proudly explain "Yes, I managed to get that single-level call stack, by persuading the CLC to essentially retrospectively patch someone else's code." My colleague looks back blankly, wondering how they're going to use that single-level call stack in the debugging process, since it barely gives more clue than the mzero message itself. It fails to explain where the caller of mzero is itself called!

I've been through this in practice with head and such, and I can vouch that even a single level of stack trace is extremely helpful. At the very least it tells clearly in which library the issue occured, considerably narrowing the search for the culprit. Otherwise you have no choice but to grep the entire dependency tree!

@Kleidukos
Copy link
Member

This is also my experience when all I have are the logs of a web application in production: Even a single line of backtrace remains very useful and saves a lot of time.

@hasufell
Copy link
Member

This ship sailed a long time ago. HasCallStack is "the way" to do callstacks in Haskell. I am glad that we're working on improvements, but we cannot ignore the bad state of the status quo for improvements that aren't here yet and may not materialize for years.

I'm not super deep into error handling like @parsonsmatt ...but I'd like to understand what is missing from the current proposals from @bgamari and co to get us there (without using HaskCallStack in type sigs).

@MaxGabriel
Copy link

Strongly agree with @Bodigrim and @Kleidukos that even a minimal stacktrace is a huge help.

We could have avoided mzero, for example with an hlint or stan rule

I think ideally people use hlint to ban bad functions, but they might not do that until they've already been bitten by a bad experience with the function. There are definitely times where you inherit a codebase and it's not up to your standards, and it's also common that you eg ban new uses of a function going forward but grandfather in old uses (to stop the bleeding before you can take on the larger project).

@avieth
Copy link

avieth commented Oct 31, 2024

How about this alternative proposal?

  1. Add HasCallStack to fail from MonadFail
  2. Remove the following instances
    • Alternative IO
    • MonadPlus IO
  3. Make a new module that exports the former minimal typeclass members from the above two instances, but with HasCallStack
    • empty = failIO "mzero" :: HasCallStack => IO a
    • (<|>) = mplusIO :: HasCallStack => IO a -> IO a -> IO a

There would be some breakage from 2, but everywhere that a missing instance appears is a place where the maintainer must decide what to do about traceability. I'd argue that's a good thing, because using Alternative IO or MonadPlus IO was already a problem (or else we wouldn't have this proposal).

They might choose the least-effort route and take what's exported from 3.

Or they might choose to give a meaningful non-IO exception Left err or Nothing value instead, by using the widely-available and well-known Alternative and MonadPlus instances from ExceptT or MaybeT.

This would improve the library ecosystem overall.

I know it's a tough sell because of the breakage, but in my view the proliferation of HasCallStack and the lack of traceability are each worthy problems that must be dealt with. Dropping these problematic instances makes them (the problems) no longer appear mutually-exclusive and starts us on a path to solving both.

@hasufell
Copy link
Member

Remove the following instances

So far my intuition is

  • -1 to this proposal
  • -1 to removing instances

@Bodigrim
Copy link
Collaborator

Bodigrim commented Nov 1, 2024

With the modern GHC we can deprecate individual instances and instance {Alternative,MonadPlus} IO seems to be a good use case for this feature.

I'm generally supporting of adding HasCallStack whenever possible, it remains to be the most robust method to obtain call stacks. If in future other methods get productionalized, we can drop HasCallStack, luckily it is not a breaking change.

@9999years
Copy link

@parsonsmatt: Every other language has implicit callstacks, all the time, for everything, and the lack of these callstacks make debugging and diagnosing Haskell issues a huge pain in the ass.

It's difficult to emphasize how foolish I feel telling new hires coming from any other language that if they don't write HasCallStack on every single function they write that they will not have access to stack traces. Especially given that there does not exist tooling to add these constraints automatically (and haskell-language-server is unusable on industrial-scale codebases), this is an important issue to solve.

@tbidne: Annotating everything that could possibly be involved with stacktraces is probably not viable. Yet here's the bloodied patient, and here we are in the emergency room.

I think this is well put. The idea that we need to write HasCallStack on every single function in every single codebase to access a feature that every other language has by default is completely absurd. And yet HasCallStack is the only reliable mechanism we have for this today, and it's largely transparent to users.

Let's add HasCallStack to stop the bleeding and then (please!) add some mechanism to get complete, reliable, implicit call stacks in unoptimized builds mode. GHC development is not like Rust development — GHC does not cut a new release every six weeks with strong backwards- and forwards-compatibility guarantees — and we cannot expect a feature like this to materialize immediately or even in the next year.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Nov 1, 2024

The case in question, of Applicative, Alternative, Monad, MonadPlus and family, is of well-defined, principled, mathematical abstractions.

I don't think this is the case for Alternative / MonadPlus. Documentation does not say a word about semantics or laws for (<|>) / mplus beyond that they are associative. Wiki says "The precise set of rules that MonadPlus should obey is not agreed upon". In my opinion they both are ad-hoc mechanisms.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Nov 1, 2024

It's useful to know the invocation of mzero, so it seems it could be useful to know the invocation of mplus, right?

mplus is supposed to be associative. So leaves of the tree (mzero or return) are potentially interesting, but the exact sequence in which they were combined is irrelevant. And mzero leaves are more interesting than return because they more likely represent exceptional / boundary conditions.

(I don't find the example of newtype MaybeT m a = MaybeT (m (Either CallStack a)) particularly motivating though)

@tomjaguarpaw
Copy link
Member

A point of information: instance Alternative IO is law-breaking. empty is supposed to be the identity of <|>, but it isn't in that instance:

> let err = throwIO (userError "Important error message")
> err <|> empty
*** Exception: user error (mzero)

We can quibble about whether err and mzero are theoretically "observably distinct", but in practice they indeed are: empty @IO can swallow important error messages!
The instance could be fixed by changing mzero/empty to throw an exception that only they can construct. But regardless, it does seem to suggest that mzeros that throw exceptions are dubious.

Documentation does not say a word about semantics or laws for (<|>) / mplus beyond that they are associative.

Indeed not, but firstly there are laws for the interaction of mzero with Monad operations, and secondly, being monoidal is not a weak condition! After all, the only laws of Applicative and Monad are that pure id/<.>1 and pure/>=> are monoidal, respectively. (The typical presentation of the Applicative laws is more complicated, but, by parametricity, it's equivalent to simply monoidicity.)

So I don't consider "it's simply monoidal" to be a sufficient refutation of being a "well-defined, principled, mathematical abstraction".

mplus is supposed to be associative. So leaves of the tree (mzero or return) are potentially interesting, but the exact sequence in which they were combined is irrelevant. And mzero leaves are more interesting than return because they more likely represent exceptional / boundary conditions.

I agree that in practice mzero is more likely to throw an exception that we want to track down to its origin. But in principle mplus could also throw an exception, especially in a badly-written instance, like the one for MonadPlus IO.

So the question boils down to: is it better for the community overall to partially remediate the consequences of a badly-written instance, or to preserve the principled nature of a mathematical type class, despite the fact that someone wrote a bad instance for it?

Let's do a thought-experiment. Say that we didn't have the instance Monoid a => Monoid (IO a) but instead

instance Monoid (IO a) where
    mempty = empty
    mconcat = (<|>)

This version of history is at least plausible. Or alternatively, suppose a popular library defined newtype MyIO a = MkMyIO (IO a) with the above instance. Could I justifiably come to CLC and say "let's put HasCallStack => on mempty, because there are real world users who are getting exceptions from it"? I don't think so. That would be the fault of the instance, not the fault of the class, and it seems completely inappropriate to change the definition of a principled, mathematical class because of one or more bad instances.

I'm generally supporting of adding HasCallStack whenever possible, it remains to be the most robust method to obtain call stacks. If in future other methods get productionalized, we can drop HasCallStack, luckily it is not a breaking change.

I acknowledge this as a fairly strong argument, and on that basis I would be willing to vote in favour, in return for:

  1. deprecation of Alternative/MonadPlus of IO becoming part of the proposal
  2. the supporters of this proposal agreeing to make a concerted effort to remove uses of those instances from the ecosystem, and equivalent ones, such as OP's own
  3. subsequent removal of HasCallStack => from mzero in, say, 24 months

mzero causes bad error messages in some cases. There are two proposed solutions: add more information to mzero, or stop using mzero in those cases. Some people want one, some people want the other. Let's compromise and do both.

Footnotes

  1. (<.>) = liftA2 (.) – see Applicative Archery.

@arybczak
Copy link
Author

arybczak commented Nov 21, 2024

I'm generally supporting of adding HasCallStack whenever possible, it remains to be the most robust method to obtain call stacks. If in future other methods get productionalized, we can drop HasCallStack, luckily it is not a breaking change.

Exactly. The proposed change is both backwards-compatible and forwards-compatible. If IPE ever becomes a reliable way to obtain stack traces, then HasCallStack constraints in base can be deprecated and dropped. For the record, I don't particularly like HasCallStack either for all of its quirks, but it's the best we have at the moment.

This version of history is at least plausible.

Yet it's not reality.

it seems completely inappropriate to change the definition of a principled, mathematical class

I care more about UX than ideals. It would be nice if more people in the Haskell community started doing so, maybe the language would become more popular and less rough around the edges.

the supporters of this proposal agreeing to make a concerted effort to remove uses of those instances from the ecosystem, and equivalent ones, such as OP's own

If I wanted to do this, I would just provide overrides of empty and <|> in Effectful.NonDet and be done with it, so this proposal wouldn't exist. It's done anyway for now as a workaround, but re-using a known interface from base seems like a better idea, it's a less confusing option and IMO it (i.e. Alternative/MonadPlus) should accommodate these use-cases (i.e. provide HasCallStack where it makes sense) if the cost of doing so it negligible AND it also improves UX of the existing code. We'll see if I'm in a minority when it comes to voting I guess.

subsequent removal of HasCallStack => from mzero in, say, 24 months

The constraint should be removed if (and only if) there's another seamless and reliable way of getting call stacks like IPE.

@tomjaguarpaw
Copy link
Member

tomjaguarpaw commented Nov 21, 2024

subsequent removal of HasCallStack => from mzero in, say, 24 months

The constraint should be removed if (and only if) there's another seamless and reliable way of getting call stacks like IPE.

Perhaps I didn't explain my point clearly enough: I'm suggesting removing the HasCallStack constraint in (say) 24 months because in this putative future no one will be using empty at a type where they need it any more.

Anyway, I've explained my point of view and offered a compromise. If you want to stick to your original position then so be it; that is your prerogative as a proposer. My vote will be no.

I think it's highly debatable that Haskell will be "less rough around the edges" if we add ad hoc constraints to mathematical type classes. I personally would say that makes it more rough. One could equally say Haskell will become less rough around the edges if people who prefer practicality over ideals don't use mathematical type classes.

This version of history is at least plausible.

Yet it's not reality.

Reasoning principles ought to remain valid in plausible alternative realities. Your reasoning principles here seems to suggest that you would add HasCallStack => to mempty in such a reality.

If I wanted to do this, I would just provide overrides of empty and <|>

Right, that would be my suggestion to someone who finds themselves in that position.

re-using a known interface from base seems like a better idea

But the interface from base doesn't satisfy your requirements! Why then try to change base rather than just defining your own interface that satisfies your need?

@Kleidukos
Copy link
Member

@tomjaguarpaw

That would be the fault of the instance, not the fault of the class, and it seems completely inappropriate to change the definition of a principled, mathematical class because of one or more bad instances.

I think there are several things on which we can compromise and still reach a point where the UX of end-users is improved while not tainting our principles.

  1. This does not change anything in the mathematical principles of the typeclass. There is no change in the algebraic behaviour of its operations. Maybe I'm wrong however, but from my end-user point of view, we're discussing the ways an error report can be improved when looking at the logs.

  2. Since the IO instance are provided by base I think there's a duty of making people's life easier. Now, if these were rogue instance from third-party libraries, they'd be entirely responsible for the poor UX, and I don't think the CLC would be under as much moral obligation to do anything.

@tomjaguarpaw
Copy link
Member

Since the IO instance are provided by base I think there's a duty of making people's life easier.

I agree there is a duty of considering possibilities to make people's lives easier, but not at all costs, especially not at making other people's lives harder.

I don't think the CLC would be under as much moral obligation to do anything.

I suggest not bringing morality into this decision.

This does not change anything in the mathematical principles of the typeclass. There is no change in the algebraic behaviour of its operations. Maybe I'm wrong however, but from my end-user point of view, we're discussing the ways an error report can be improved when looking at the logs.

I completely disagree. This proposed change makes it possible for problems to creep in that are much harder to debug. HasCallStack allows its CallStacks to be observed in pure code. I think this is a design flaw, but that's where we are. For example, the definition of badEmpty below changes depending upon where it's called. That's really, really bad, and terrible to debug. This proposal aims to get a better dynamic debugging story and in doing so weakens the static debugging story. It becomes harder to design bugs out of out of the program to start with.

You might say "well, no one will use CallStack like that in practice". Maybe you're right, but that's the kind of thing they say in the Python world ("we're all adults here, we won't access _-prefixed private methods"), and I moved from Python to Haskell for a good reason.

import GHC.Stack
import Data.Char

data MyAlt f a = MkMyAlt String (f a)
  deriving stock (Functor, Show)

instance Applicative f => Applicative (MyAlt f) where
  pure x = MkMyAlt [] (pure x)
  MkMyAlt s1 f <*> MkMyAlt s2 x =
    MkMyAlt (s1 <> s2) (f <*> x)

instance Alternative f => Alternative (MyAlt f) where
  empty = MkMyAlt [] empty
  MkMyAlt s1 x1 <|> MkMyAlt s2 x2 =
    MkMyAlt (s1 <> s2) (x1 <|> x2)

badEmpty :: (HasCallStack, Alternative f) => MyAlt f a
badEmpty = MkMyAlt s empty
  where
    s = if even (sum (map ord (prettyCallStack callStack)))
        then "Was even"
        else "Was odd"

-- ghci> example
-- MkMyAlt "Was odd" []
-- MkMyAlt "Was even" []
example :: IO ()
example = do
  print (badEmpty :: MyAlt [] ())
  print (badEmpty :: MyAlt [] ())

@Kleidukos
Copy link
Member

@tomjaguarpaw

I suggest not bringing morality into this decision.

Sorry for the misunderstanding, I said "moral obligation" to distinguish it from a legal obligation, or even being held at gunpoint.

You might say "well, no one will use CallStack like that in practice".

I wouldn't dare being so dismissive, Hyrum's Law teaches us that this would be bound to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests