Open DK318 opened 2 years ago
I've looked through Prelude
sources and figured out that all these unsafe functions use errorWithoutStackTrace
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".
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?
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
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 :see_no_evil:).
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:
Unsafe
module altogetherAnd 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.
Description
Problem
Functions from
Universum.Unsafe
don't carryCallStack
. Sometimes it is hard to debug when you don't have stack trace.Solution
Attached
CallStack
to all functions fromUniversum.Unsafe
by copying them frombase
and replacingerrorWithoutStackTrace
witherror
function.Related issues(s)
✓ 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).
Related changes (conditional)
Tests
Documentation
I checked whether I should update the docs and did so if necessary:
Record your changes
Stylistic guide (mandatory)
[x] 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.