Opened 13 months ago

Closed 11 months ago

Last modified 11 months ago

#15559 closed feature request (fixed)

fromJust has no HasCallStack

Reported by: nomeata Owned by:
Priority: normal Milestone: 8.8.1
Component: Core Libraries Version: 8.4.3
Keywords: newcomer Cc: gridaphobe
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D5256
Wiki Page:

Description

The function Data.Maybe.fromJust does not have a HasCallStack constraint. I wonder why – is that intentional, or did just nobody bother yet?

Change History (12)

comment:1 Changed 13 months ago by bgamari

Cc: gridaphobe added

When HasCallStack was introduced we intentionally did not introduce constraints to partial functions, no matter how small. IIRC this was in part because we expected third-party libraries to fill this need and in part it was out of concern for performance (since you would incur the cost of an additional argument, and potentially allocation, if the function isn't inlined). However, at this point it seems like HasCallStack is pretty well-accepted. It seems to me that we could start considering slowly adding HasCallStacks around in functions that are either (a) nearly certain to inline, or (b) not performance critical.

comment:2 Changed 13 months ago by gridaphobe

There were also concerns about API stability, particularly since the original implementation exposed the implicit parameter. Now that we have the HasCallStack alias and a decent set of API functions around it, we could swap out the implementation, if necessary, with less potential for client breakage.

I'd support starting to add HasCallStack constraints to common partial functions like fromJust, head, etc. Though, is this something that should be discussed with the Core Libraries Committee?

comment:3 Changed 13 months ago by ulysses4ever

Does it make sense to mark this one newcomer?

Last edited 13 months ago by ulysses4ever (previous) (diff)

comment:4 Changed 13 months ago by bgamari

Keywords: newcomer added

Sure, this seems like a good task for a newcomer.

However, I should emphasize to that we don't want to simply scatter HasCallStacks on every partial function. We should be thoughtful and deliberate (and probably consult with the CLC) when making changes like this.

comment:5 Changed 13 months ago by goldfire

I really like GHC's internal HasDebugCallStack, which disappears (that is, becomes ()) when DEBUG is not defined via CPP and becomes HasCallStack when DEBUG is defined. As far as I know, there's no standard way to mark a build meant for debugging, but perhaps there should be. Then, we could do the same for all applications instead of just GHC.

comment:6 Changed 12 months ago by chessai

I really like GHC's internal HasDebugCallStack, which disappears (that is, becomes ()) when DEBUG is not defined via CPP and becomes HasCallStack when DEBUG is defined. As far as I know, there's no standard way to mark a build meant for debugging, but perhaps there should be. Then, we could do the same for all applications instead of just GHC.

In addition to making this available to users of GHC, there should probably be an additional side-effect to the DEBUG flag, or a separate flag, that turns HasDebugCallStack on for all functions

comment:7 Changed 12 months ago by bgamari

We already have a very similar mechanism for Control.Exception.assert occurrences at compile-time. It would be reasonably straightforward to add a similar flag for a magic synonym of HasCallStack.

comment:8 Changed 11 months ago by fangyizhou

Differential Rev(s): D5256
Status: newpatch

comment:9 Changed 11 months ago by potato44

Differential Rev(s): D5256Phab:D5256

comment:10 Changed 11 months ago by Ben Gamari <ben@…>

In 614028e/ghc:

Data.Maybe: add callstack for fromJust (Trac #15559)

Per feature request, add `HasCallStack` to `fromJust` in `Data.Maybe`
and use `error` instead of `errorWithoutStackTrace`. This allows
`fromJust` to print call stacks when throwing the error.

Also add a new test case for the behaviour, modify existing test cases
for new signature

Test Plan: New test cases

Reviewers: hvr, bgamari

Reviewed By: bgamari

Subscribers: ulysses4ever, rwbarton, carter

GHC Trac Issues: #15559

Differential Revision: https://phabricator.haskell.org/D5256

comment:11 Changed 11 months ago by bgamari

Resolution: fixed
Status: patchclosed

fangyizhou may look into proposing adding more HasCallStacks elsewhere but this one is finished.

comment:12 Changed 11 months ago by bgamari

Milestone: 8.6.18.8.1
Note: See TracTickets for help on using tickets.