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

Remove false COMPLETE statement #248

Closed
wants to merge 1 commit into from

Conversation

NorfairKing
Copy link

Given that it looks like #134 is not going to happen, and it looks like #246 and #247 won't make it in, the least we can do is remove a false COMPLETE pragma.

@AshleyYakeley
Copy link
Member

AshleyYakeley commented Nov 3, 2023

I think it's useful tbh? Anyone going to the trouble of making a case statement with all twelve month names is likely already working with a MonthOfYear that they have already validated.

For example, any MonthOfYear that comes from a YearMonthDay pattern is already validated.

@NorfairKing
Copy link
Author

already validated.

Parse, don't validate. That's what the sum type would have been for, but if you don't want it then we should at least not lie to the compiler by saying that Int has only (these) twelve values.

@AshleyYakeley
Copy link
Member

For the time library I've broadly prioritised usefulness and simplicity over type-safety. But I'll consider this PR if you have examples of the COMPLETE pragma causing an actual real-life code defect.

...or can even plausibly imagine someone being tripped up by it.

@NorfairKing NorfairKing closed this Nov 3, 2023
@AshleyYakeley
Copy link
Member

As an example of the cost in simplicity, consider two scenarios:

  1. You have year, month, day, (y,m,d) :: (Int,Int,Int) which you already know is valid, and you need to convert that to a Day.

    Currently:
    YearMonthDay (toInteger y) m d

    With WIP: Month of year type #246:
    YearMonthDay (toInteger y) (fromJust (parseMonthOfYearIndex m)) d
    note use of partial function fromJust

  2. You have year, month, day, (y,m,d) :: (Int,Int,Int) which you need to validate, converting to Maybe Day.

    Currently:
    fromGregorianValid (toInteger y) m d

    With WIP: Month of year type #246:
    parseMonthOfYearIndex m >>= \m' -> fromGregorianValid (toInteger y) m' d
    has to validate twice

@georgefst
Copy link

georgefst commented Jan 11, 2024

Please re-open this. While I have some sympathies for the arguments against #134, this makes no sense to me at all. I can't think of any other Haskell library where pattern-matching can fail at runtime with no warning and without using any obviously unsafe functions. Yet here we have, for example, the innocuous-looking:

f = case minBound of
    January -> ()
    February -> ()
    March -> ()
    April -> ()
    May -> ()
    June -> ()
    July -> ()
    August -> ()
    September -> ()
    October -> ()
    November -> ()
    December -> ()
λ> f
*** Exception: Scratch.hs:(236,5)-(248,18): Non-exhaustive patterns in case

If you're not convinced, I'm happy to run a poll on Haskell Discourse or something. I'm pretty certain that the vast majority of the community would consider 313bd8f a horrifying abuse of the COMPLETE pragma system.

@AshleyYakeley
Copy link
Member

AshleyYakeley commented Jan 11, 2024

That doesn't look at all innocuous to me, I can't imagine anyone actually writing that in code.

As I said, I'll consider this PR if you have examples of the COMPLETE pragma causing an actual real-life code defect.

@kephas
Copy link

kephas commented Jan 17, 2024

When I ask GHC to make any non-exhaustive pattern an error, I expect that I'm safe on that front when there is no error. I would be pretty pissed to have made that effort and to have a runtime error blow up in the face of my users anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants