haskell / time

A time library
http://hackage.haskell.org/package/time
Other
118 stars 78 forks source link

Remove false COMPLETE statement #248

Closed NorfairKing closed 9 months ago

NorfairKing commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.

AshleyYakeley commented 9 months ago

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 #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 #246: parseMonthOfYearIndex m >>= \m' -> fromGregorianValid (toInteger y) m' d has to validate twice

georgefst commented 7 months ago

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 313bd8f203ae1b0fb5c72d0e329578923a975ab3 a horrifying abuse of the COMPLETE pragma system.

AshleyYakeley commented 7 months ago

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 commented 7 months ago

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.