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

Use HasCallStack and error in GHC.List and .NonEmpty #5

Closed
Bodigrim opened this issue Oct 27, 2021 · 22 comments
Closed

Use HasCallStack and error in GHC.List and .NonEmpty #5

Bodigrim opened this issue Oct 27, 2021 · 22 comments
Labels
approved Approved by CLC vote base-4.17 Implemented in base-4.17 (GHC 9.4)

Comments

@Bodigrim
Copy link
Collaborator

The original proposal was raised on May 30 by @phadej https://mail.haskell.org/pipermail/libraries/2021-May/031246.html, accompanied by detailed impact analysis. There were extensive discussions on the proposal in June. On Jul 7 @phadej asked CLC to trigger a vote on the proposal in https://mail.haskell.org/pipermail/libraries/2021-July/031355.html.

Merge request is available at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6772. GHC developers seem to be on board with it.

Since CLC vote is long overdue, I would like to trigger it immediately. Members of CLC are encouraged to vote in replies to this issue.

@cgibbard
Copy link
Contributor

+1

5 similar comments
@emilypi
Copy link
Member

emilypi commented Oct 27, 2021

+1

@Bodigrim
Copy link
Collaborator Author

+1

@mixphix
Copy link
Collaborator

mixphix commented Oct 27, 2021

+1

@chessai
Copy link
Member

chessai commented Oct 27, 2021

+1

@gwils
Copy link
Contributor

gwils commented Oct 27, 2021

+1

@Bodigrim Bodigrim added the approved Approved by CLC vote label Oct 27, 2021
@Bodigrim
Copy link
Collaborator Author

Thanks everyone, approved unanimously.

@simonmar
Copy link
Member

I'm probably way too late to do anything about this now, but I find it very surprising that this change was approved so easily. I have argued against adding HasCallStack to standard library functions in the past (which is why it wasn't done before, I successfully beat this back at least once). The core of the objection is that call stacks are a debugging feature that should not be visible in the source code, let alone the types. Yes this feature is useful sometimes - but not useful enough to make it so visible in the types of standard library functions. In the Prelude no less! I'd much rather see us invest in making the other methods for getting call stacks more convenient and accurate, reducing the need for HasCallStack.

@tomjaguarpaw
Copy link
Member

@goldfirere proposed IsPartial as a denotational (as opposed to operational) alternative to HasCallStack. That would allow partial functions to be marked without having to leak the specific implementation detail of the marking. partialityIsOK (tweaked by me)
can intermediate between the two.

partialityIsOK :: HasCallStack => String -> (IsPartial => r) -> r

@tchoutri
Copy link

@tomjaguarpaw I would highly advise that we ask the PureScript community about their usage and experience of their own Partial denotational typeclass. @hdgarrood Is this something on which you can comment?

@tchoutri
Copy link

@simonmar During the (very) heated debates, an alternative approach was suggested which was that we would have a Partial constraint that (in GHC) would amount to a HasCallStack constraint. This was suggested to enable stack traces in a way that doesn't force alternative implementation to use GHC's callstack.

@simonmar
Copy link
Member

IsPartial is a bit better, but has the big disadvantage that it can't be enforced and would therefore end up being used inconsistently. The existence of IsPartial might lead people to assume that the absence of IsPartial implies total. Would we use IsPartial for functions that might not terminate?

During the (very) heated debates,

I explored a bit but didn't find any very heated debates, can you help me with a link or two?

@tchoutri
Copy link

IsPartial is a bit better, but has the big disadvantage that it can't be enforced and would therefore end up being used inconsistently. The existence of IsPartial might lead people to assume that the absence of IsPartial implies total. Would we use IsPartial for functions that might not terminate?

Yes, those are hard questions that the PureScript community has to deal with, which is why I highly suggest that we survey them to see how their engineering practices have evolved on their side, to deal with Partial.

@hdgarrood
Copy link

We've definitely had people wondering whether the absence of a Partial constraint implies that a function is total, although off the top of my head I am not sure I remember whether I've seen people trip over this in their actual code. What I have seen, though, is that it's hard to explain how to use the class effectively. I wrote some documentation for the Partial class in PureScript but in the wild, in my experience, it's much more common to see people just slap our version of partialityIsOK, namely unsafePartial, on at the call site of each partial function, even if the call site has no basis for doing that and really the Partial constraint should have been propagated. If most people are misunderstanding how it's meant to be used and instead using it in this way, I think its utility over just documenting functions as partial is arguably limited.

Another thing that I think is worth mentioning, although which is possibly less applicable to Haskell than it is to PureScript, is that it's worth being clear about how the String argument to partialityIsOK is used, and how a partial function should behave in case it receives an argument for which it can't return a value. In PureScript, Partial functions are not required to throw; for example, there is a partial array indexing function which returns undefined at runtime if you ask for an out-of-bounds index (for performance reasons), even though most types do not permit undefined as a valid runtime representation. For a while we had an unsafePartialBecause function whose signature was forall a. String -> (Partial => a) -> a, but it was deprecated it in favour of a version without the String argument, because its implementation would catch and rethrow with the provided String as a message, which encoded an invalid assumption that partiality was always indicated by throwing an exception. Of course, if that assumption were going to be valid in Haskell then it would be fine, and it would also be fine if the String was ignored and was only present for the benefit of future readers of the code, although I personally find that slightly off-putting.

@tomjaguarpaw
Copy link
Member

it's much more common to see people just slap our version of partialityIsOK, namely unsafePartial, on at the call site of each partial function, even if the call site has no basis for doing that and really the Partial constraint should have been propagated. If most people are misunderstanding how it's meant to be used and instead using it in this way, I think its utility over just documenting functions as partial is arguably limited.

I think my proposed partialityIsOK deals with that, by requiring HasCallStack (but I may be wrong because I don't really understand HasCallStack).

partialityIsOK :: HasCallStack => String -> (IsPartial => r) -> r

@Bodigrim
Copy link
Collaborator Author

I'm probably way too late to do anything about this now, but I find it very surprising that this change was approved so easily.

The proposal was raised in May and approved in October. This is as far from "easily" as I can imagine.

I explored a bit but didn't find any very heated debates, can you help me with a link or two?

Please refer to the links in #5 (comment).


I would like to highlight that there was plenty of time to discuss and propose alternatives (IsPartial, etc.). CLC has approved a specific design !6772.

@simonmar
Copy link
Member

I'm not complaining that I wasn't consulted, I mean sure, if I don't actively read the libraries mailing list then I'm sure I'll miss things. No, my surprise was really that nobody seemed to be worried about the aesthetics of the feature, and the impact on new users and teaching. As far as I can tell, my previous objection to this wasn't mentioned either - I just dug it up, I think this is it: https://gitlab.haskell.org/ghc/ghc/-/issues/11035#note_109305

@Bodigrim
Copy link
Collaborator Author

Bodigrim commented Nov 17, 2021

@simonmar I guess the main difference is that when you raised your objections six years ago HasCallStack was a relatively new concept, but now it is used by hundreds of packages (7512 matches across 412 packages), including base itself. It's no longer experimental, it is widely adopted and well known. Somehow people prefer it to profiling builds and DWARF, and a better tooling for call stack has not materialized.

@hdgarrood
Copy link

I think my proposed partialityIsOK deals with that, by requiring HasCallStack

Not quite. It certainly helps to have a call stack to hand if it does turn out to blow up at runtime, but what I had in mind was people misunderstanding how IsPartial propagation is supposed to be used. For example, suppose you have a function foldr1 :: IsPartial => (a -> a -> a) -> [a] -> a and you're trying to write a minimum function using foldr1. At first, you write this:

minimum :: [a] -> a
minimum = foldr1 min

This will fail because there's no IsPartial instance available. At this stage, I've seen that it's common for people to not realise that they can add an IsPartial constraint to minimum, but instead just slap a partialityIsOK on because they remember that that's how to shut the compiler up:

minimum :: [a] -> a
minimum = partialityIsOK "minimum should not be used with empty arrays" (foldr1 min)

despite the fact that the partiality has not been handled.

@tomjaguarpaw
Copy link
Member

I think my proposed partialityIsOK deals with that, by requiring HasCallStack

Not quite.

Yes, I was thinking the IsPartial constraint would just be swapped out for a HasCallStack constraint, but HasCallStack is magical and just gets satisfied if it doesn't appear in the annotated type.

@Bodigrim
Copy link
Collaborator Author

Indeed, IsPartial would be a breaking change, similar in effect to removal of head from Prelude.

@chshersh
Copy link
Member

I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all approved CLC proposals.

Field Value
Author @AndreasPK
Status merged
base version 4.17.0.0
Merge Request (MR) https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6772
Blocked by nothing
CHANGELOG entry present
Migration guide not needed

Please, let me know if you find any mistakes 🙂

@chshersh chshersh added the base-4.17 Implemented in base-4.17 (GHC 9.4) label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote base-4.17 Implemented in base-4.17 (GHC 9.4)
Projects
None yet
Development

No branches or pull requests