Adding HasCallStack to the methods of the Ix typeclass

[Kleidukos]

Context

While working on a codebase that makes use of Data.Ix methods like index, one can get such feedback:

haddock: internal error: Ix{Int}.index: Index (4441) out of range ((0,122))

(haddock here is the program being working on)

I believe we deserve better traces when working with functions that throw exceptions like this.

Proposal

I'd like to add HasCallStack constraints to the methods of Data.Ix so that our life becomes less miserable when encountering such issues.

Implementation

A draft implementation is up on the GHC GitLab at: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9694

[simonmar]

The proliferation of HasCallStack is making me a bit sad.

1. Once a function is decorated with HasCallStack callers take the performance hit whether they like it or not
2. Operational/debugging details like HasCallStack just don't belong in the types. It's not only ugly, it's also another thing to confuse people with. (Partial is only slightly better: since a lot of functions are partial and we are never going to be consistent about tagging them all, I don't think this is a good direction either)

Is there a core libraries policy on this?

[mpickering]

I am in agreement with @simonmar, HasCallStack shouldn't be used for this purpose. There are other methods of obtaining full callstacks on errors which don't interfere with the types of expressions.

[Kleidukos]

Well, what makes me sad is that my development experience is more painful than it should be. I've been promised revolutionary debuggers for lazy functional programming languages in the form of 30-years old PhD theses that would rock my world if only someone would implement them.

Python, Java, Ruby and Erlang have reasonable behaviours in the presence of exception. In Haskell we have promises from a time when I wasn't even born. Sorry for the salt in this message but the magic solutions that were promised when Oleg tackled the problem with Data.List functions still haven't shown up, and I have to work on Haddock in the meantime.

Unless you're offering your help to work on Haddock @simonmar? In which case I'd be delighted.

[Kleidukos]

I am in agreement with @simonmar, HasCallStack shouldn't be used for this purpose. There are other methods of obtaining full callstacks on errors which don't interfere with the types of expressions.

@mpickering Are these methods real? Do they have an entry in a manual somewhere? Is this something that can be used today? What are the drawbacks?

[simonmar]

Does profiling not work for you?

[JakobBruenker]

HasCallStack shouldn't be used for this purpose

What should HasCallStack be used for?

[ocharles]

@mpickering Are these methods real?

I've had good luck with +RTS -xc, so that one is at least real.

[dpwiz]

The amount of times I've seen the internal error: Ix{Int}.index: Index (4441) out of range ((0,122)) is rather uncomfortable.

Usually it's some heisenbug that is a PITA to replicate to even have a guess at what gone wrong. So offline options like profiling wouldn't solve that.

I don't care if that would be fixed with HasCallStack exactly, but if that's the case, I think I'd rather pay the price to know what exactly happened where.

[Kleidukos]

I've had good luck with +RTS -xc, so that one is at least real.

@ocharles And I'm glad that you've had good luck, I really am. But it doesn't seem to be what is needed here, from the results: https://paste.tomsmeding.com/zXemIGRu
(the most relevant bit is at line 2651).

[ocharles]

Hmm yes, it seems the actual error doesn't even show up there - that's no good! I think line 2651 is just your normal stderr, right? If so, I think that provides some motivation to this very issue.

[Kleidukos]

Hmm yes, it seems the actual error doesn't even show up there - that's no good! I think line 2651 is just your normal stderr, right? If so, I think that provides some motivation to this very issue.

@ocharles 🎯 bullseye.

[simonmar]

What I'm really asking here is whether we've decided as a community that what we want to do is add HasCallStack to any function that might throw, or that might call something that might throw, and so on. That is, let's figure out what the overall approach for the core libraries is, so there's a policy against which to judge proposals like this one.

[ocharles]

It might be worth moving that out of this issue, even if it means this issue's discussion is blocked until that discussion settles. I only suggest this because I think there is a discussion to be had about exactly what @Kleidukos is proposed.

One thought I had is in response to @simonmar

Once a function is decorated with HasCallStack callers take the performance hit whether they like it or not

Yes, but in the context of this issue we're discussing index which already does bounds checking and sacrifices performance. A bounds check that produces a rubbish error seems a bit pointless. Either do a bounds check and give me something useful (that is "I want to pay the performance cost for a good error/safety") or omit the check entirely and give me screaming performance (so I'll use unsafeIndex).

[cdsmith]

I'd really like to see +RTS -xc be a useful alternative to HasCallStack, and I agree it's a wart that HasCallStack is expressed in the type system!

If +RTS -xc worked without -prof, and included the info in the exception instead of dumping it to stderr, that would be great! But -prof is a far greater performance cost than HasCallStack, and it's just not feasible to build Haskell programs with -prof as a matter of general practice. And @Kleidukos posted a trace that I trust amply demonstrates why dumping to stderr when an exception is thrown is supremely unhelpful: somewhere in those thousands of lines of garbage output, there might be something helpful, but good luck finding it. (It's actually apparently not there at all, which is a third way in which -xc is unhelpful, but I don't understand what exactly is going on there...)

Currently, HasCallStack is what we have that basically works, despite the warts.

[treeowl]

@cdsmith Including it in the SomeException itself is impossible; we'd need to attach it outside somehow. A side benefit of that is that "while we're at it" we can tack on a bit indicating whether the exception is synchronous or asynchronous, which we currently don't have.

The problem with enabling stack traces by default is that I believe they're fairly expensive. I'd certainly want to be able to turn that off.

[bgamari]

In my opinion we should rather focus our efforts on making sure that the backtrace provenance proposal makes it into 9.8 rather than continue to paper around the deeper issue of lacking backtrace support with more HasCallStack usage. HasCallStack can be a very useful tool, but it can cause considerable performance cost when it is introduced into an inner loop (where Ix methods tend to reside). Including it unconditionally in Ix seems like it is asking for trouble.

I would invite anyone interested in this area to refer to the proposal. Those interested in further detail on some of the lower-cost alternatives to HasCallStack that this proposal will unlock are invited to refer to my Munihac 2022 talk

[Kleidukos]

@bgamari I'm glad to see that there are pragmatic solutions in the works.

[Bodigrim]

Does profiling not work for you?

We've been through this argument multiple times, starting from https://mail.haskell.org/pipermail/libraries/2021-May/031246.html. No, profiling does not work: it's generally not possible to provide clients with a debug build and it is generally impossible to reproduce their environment reliably.

That is, let's figure out what the overall approach for the core libraries is, so there's a policy against which to judge proposals like this one.

Core libraries have already converged to the opinion that indexing requires HasCallStack:

• Prelude.!!
• Data.ByteString.index
• Data.Text.index
• Data.Vector.Generic.!

HasCallStack can be a very useful tool, but it can cause considerable performance cost when it is introduced into an inner loop (where Ix methods tend to reside). Including it unconditionally in Ix seems like it is asking for trouble.

I support adding HasCallStack to index (which performs bounds checks anyway), but not to unsafeIndex (which indeed is performance-critical and, well, unsafe).

@Kleidukos did you try to annotate instances with HasCallStack instead of annotating the type class? I don't remember if it's permitted.

[cartazio]

I support the stances of Ben and Simon Marlowe here. Bounds checking merely uses up branch predictor resources and can be a mostly hidden cost, whereas has call stack increases register resources, in a largely invisible way. And will also I suppose create a heap allocation and check! There’s some unfathomably bad regressions this will have on real code out there. the extra allocations from building the stack up would trigger minor gcs in zero allocation checked index code.

[Bodigrim]

I'm not convinced that adding HasCallStack to index has measurable performance impact. If index gets inlined in a monomorphic context (which it usually does, being a very small function), there is no extra cost to pay. If index does not inline and/or remains polymorphic, then, honestly, your performance will be abysmal any way.

[cartazio]

It’s a shame I missed the vector discussion, because vector historically had an excellent solution: cabal build flags. Which are super easy to toggle with cabal.project files. But I’ve been busy with personal health matters much of the last 2-3 years.

There should never be a “silent memory corruption ” vs “robust inner loop performance” in how operations are implemented.

[cartazio]

I'm not convinced that adding HasCallStack to index has measurable performance impact. If index gets inlined in a monomorphic context (which it usually does, being a very small function), there is no extra cost to pay. If index does not inline and/or remains polymorphic, then, honestly, your performance will be abysmal any way.

wait, does hascallstack stay an implicit parameter with associated heap allocations are runtime though? that can still be an issue there. That said, if you can show me core with an example that hoists that computation out of the innerloop (and robustly so), i'm super cool with all of this.

[ocharles]

Bounds checking merely uses up branch predictor resources and can be a mostly hidden cost

Ok, but this is somewhat moving the goal posts. We can't have HasCallStack because of performance, but "oh, that performance cost is OK". I know this is Simon's point, but we really need more precision here.

[konsumlamm]

The unsafe... functions definitely shouldm't have a HasCallStack constraint (they don't do any checks, so there is no error to throw, that's their entire point).

EDIT: I see you just changed that.

[simonmar]

Yes, but in the context of this issue we're discussing index which already does bounds checking and sacrifices performance. A bounds check that produces a rubbish error seems a bit pointless. Either do a bounds check and give me something useful (that is "I want to pay the performance cost for a good error/safety") or omit the check entirely and give me screaming performance (so I'll use unsafeIndex).

The error that index produces is not pointless though: it runs all your exception handlers and is recoverable, whereas unsafeIndex just segfaults.

[ocharles]

Ok, that's fair - read my point as more about the error message itself.

[cartazio]

its worth remembering on some platforms we already have Stack Walking. Which only has a performance cost in the case of an error. The lack of dwarf stack traces on Darwin platforms is an issue of having the libraries for the RTS to correctly stack walk on MachO dwarf, rather than a fundamental limit. (I'm not sure what the corresponding story is on windows though)

[cartazio]

additionally, we can totally in the @Ericson2314 future, have Cabal Flags for build flavors of base that allow application builders / developers to choose which tradeoffs matter to them via some innocent cabal-flags about these very nuanced topics.

[tbidne]

@Kleidukos did you try to annotate instances with HasCallStack instead of annotating the type class? I don't remember if it's permitted.

@Bodigrim Somewhat shockingly (imo), InstanceSigs does in fact impact HasCallStack.

1. HasCallStack needs be on the typeclass decl to prevent the callstack chain from breaking, full stop.
2. Removing HasCallStack via InstanceSigs actually works.
3. On the other hand, adding HasCallStack will add an extra a type signature in an instance, called at ... line, but it otherwise does not impact 1. This won't save you if HasCallStack is not on the typeclass decl.

I experimented here: https://github.com/tbidne/hcs-class/blob/main/src/Lib.hs.

[Kleidukos]

Foo: Ix{Char}.index: Index ('d') out of range (('a','c'))
CallStack (from HasCallStack):
  error, called at libraries/base/GHC/Ix.hs:171:5 in base:GHC.Ix
  indexError, called at libraries/base/GHC/Ix.hs:192:32 in base:GHC.Ix
  a type signature in an instance, called at libraries/base/GHC/Ix.hs:190:14 in base:GHC.Ix
  index, called at /home/hecate/Foo.hs:23:7 in main:Main
  baz, called at /home/hecate/Foo.hs:18:16 in main:Main
  operation, called at /home/hecate/Foo.hs:13:3 in main:Main
  foo, called at /home/hecate/Foo.hs:8:8 in main:Main

Annotating both method declaration and implementation is perfect! Thanks @tbidne

[Kleidukos]

Just to conclude on my thoughts regarding @bgamari's work on ghc-proposals/ghc-proposals#330: As soon as we can migrate from HasCallStack to this, I'll be in the front-lines to submit migration patches.

[adamgundry]

@tbidne You may be interested in https://gitlab.haskell.org/ghc/ghc/-/issues/22103, a discussion related to InstanceSigs and HasCallStack that resulted in some documentation improvements (though perhaps more are possible).

[tbidne]

@tbidne You may be interested in https://gitlab.haskell.org/ghc/ghc/-/issues/22103, a discussion related to InstanceSigs and HasCallStack that resulted in some documentation improvements (though perhaps more are possible).

@adamgundry Thanks for the link; that's helpful. I was aware instances could provide more polymorphic signatures, but did not consider how this could interact with HasCallStack.

[bgamari]

@Bodigrim says,

I'm not convinced that adding HasCallStack to index has measurable performance impact. If index gets inlined in a monomorphic context (which it usually does, being a very small function), there is no extra cost to pay.

Sadly, this isn't quite true. If there is a CallStack from the call-site of the index then we will incur another 4 words of (likely garbage) allocation every time index is called.

[cartazio]

Indeed. Which is exactly I’m im a tad sad to hear that’s not hidden behind a build flag In vector. (I’m willing to slap together a patch for vector in that direction if there’s appetite). Allocations within inner loops can be problematic :(

…

On Thu, Jan 19, 2023 at 9:08 AM Ben Gamari ***@***.***> wrote: @Bodigrim <https://github.com/Bodigrim> says, I'm not convinced that adding HasCallStack to index has measurable performance impact. If index gets inlined in a monomorphic context (which it usually does, being a very small function), there is no extra cost to pay. Sadly, this isn't quite true. If there is a CallStack from the call-site of the index then we will incur another 4 words of (likely garbage) allocation every time index is called. — Reply to this email directly, view it on GitHub <#115 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAABBQWSP44OVKTC3FNDVA3WTFDGJANCNFSM6AAAAAATZFGVWU> . You are receiving this because you commented.Message ID: ***@***.***>

[Bodigrim]

@Kleidukos what's the conclusion on this? Would you like to proceed with a vote?

[Kleidukos]

After a chat with @bgamari I have come to the decision to close the process. The inability to backport such a change (as it would create a context due to the nature of HasCallStack) does not offer much, as it would then be released starting with GHC 9.8, which is also going to have the implementation of ghc-proposals/ghc-proposals#330.

My thanks to everyone who helped with various suggestions, this has really helped crystallising the difficulties that we encounter as a community to teach and transmit our best practices when using Haskell in anger.