[#222] CallStack attach

[DK318]

Description

Problem

Functions from Universum.Unsafe don't carry CallStack.
Sometimes it is hard to debug when you don't have stack trace.

Solution

Attached CallStack to all functions from Universum.Unsafe
by copying them from base and replacing errorWithoutStackTrace with
error function.

Related issues(s)

• Fixes Consider attaching CallStack to functions from Unsafe  #222

✓ Checklist for your Pull Request

Ideally a PR has all of the checkmarks set.

If something in this list is irrelevant to your PR, you should still set this
checkmark indicating that you are sure it is dealt with (be that by irrelevance).

• I made sure my PR addresses a single concern, or multiple concerns which
  are inextricably linked. Otherwise I should open multiple PR's.
• If your PR fixes/relates to an open issue then the description should
  reference this issue. See also auto linking on
  github.

Related changes (conditional)

• Tests

  • If I added new functionality, I added tests covering it.
  • If I fixed a bug, I added a regression test to prevent the bug from
    silently reappearing again.

• Documentation

  I checked whether I should update the docs and did so if necessary:

  • README
  • Haddock

• Record your changes

  • I added an entry to the changelog if my changes are visible to the users
    and
  • provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

• My commit history is clean (only contains changes relating to my
  issue/pull request and no reverted-my-earlier-commit changes) and commit
  messages start with identifiers of related issues in square brackets.

  Example: [#42] Short commit description

  If necessary both of these can be achieved even after the commits have been
  made/pushed using rebase and squash.

[gromakovsky]

What if we define

head :: HasCallStack => [a] -> a
head = Prelude.head

?
(probably with INLINE)

AFAIU the call stack will contain a reference to Universum.Unsafe, which is what we want, and at the same time we won't have to copy-paste code from base.

[gromakovsky]

I see that all files use 7-space indentation for exports, let's keep it consistent throughout the repo and not change it.

[DK318]

I've looked through Prelude sources and figured out that all these unsafe functions use errorWithoutStackTrace

[gromakovsky]

Ah, ok, I get it. Well, looks sad, it's something that wasn't considered in the issue description, I mean the fact of copy-paste wasn't mentioned in "Cons".

[dcastro]

I mean the fact of copy-paste wasn't mentioned in "Cons".

That's a good point :/ Would you still say this is worth pursuing, despite the cons? Or do you think we shouldn't do this?

[gromakovsky]

I personally don't remember much suffering from lack of CallStack in Unsafe functions. And I quite heavily don't like copy-paste. So I would refraing from making this change, but it's highly opinionated and if there are enough people who need call stacks I won't reject it. cc @Martoon-00 maybe

[Martoon-00]

Fair, I didn't mention that this issue will require copy-paste.

And the fact that we have to copy stuff like INLINE pragmas is pretty annoying (fortunately not the rewrite rules, but who knows, maybe they will appear in the future 🙈).

---

Though nevertheless, I think this change is important. Not having a callstack seems innocent until the moment when it actually fires, and then it would be extremely hard to debug given a large project. Imagine, I used Unsafe.head 10 times in different parts of the project - that would be sad to investigate.

Coming to think about it, I would consider 3 alternatives:

1. Add callstacks
2. Don't have callstacks, leave everything as-is
3. Remove the Unsafe module altogether

And 2nd option seems the worst here, because having an unsafe function without a callstack, if I understand it correctly, can be treated as an anti-pattern. We better enforce the user to use a safe function + explicit fromMaybe (error ...) (3rd option) than allow using Unsafe.head in its current form.

[Martoon-00]

Btw, written this way we will see at least two entries in each callstack - the one containing the target function (like head) and the one with errorEmptyList. I think there is no reason to include the latter, and probably freezeCallStack will help here.

Though this is subject to discussion too.