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

time: func Parse should return day out of range error for Feb. 31 #7268

Closed
gopherbot opened this issue Feb 5, 2014 · 15 comments

Comments

Projects
None yet
5 participants
@gopherbot
Copy link

commented Feb 5, 2014

by dgnorton:

What steps will reproduce the problem?

http://play.golang.org/p/x6Efm26yQu

What is the expected output?

"...day out of range" error

What do you see instead?

"2014-03-03 00:00:00 +0000 UTC"

Which compiler are you using (5g, 6g, 8g, gccgo)?

6g

Which operating system are you using?

Linux 3.11-2-amd64 #1 SMP Debian 3.11.8-1 (2013-11-13) x86_64 GNU/Linux

Which version are you using?  (run 'go version')

go version go1.1.2 linux/amd64

Discussion:

https://groups.google.com/forum/#!topic/golang-nuts/eAtolHlyGVQ
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2014

Comment 1:

Labels changed: added repo-main, release-go1.3maybe.

Status changed to Accepted.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2014

Comment 2:

I don't believe it should error on February 31. The issue is how to phrase the
guarantee. If it errors on February 31, it must error on February 29, but only in
non-leap years. And then there are times in the past with gaps, like when the switch
happened from Gregorian to Julian times, and the location of those gaps depends on where
you are - the switch happened in different locations on different days. Russia was very
late, if I remember right.
And all of that assumes Parse even has the information. It might not know the time zone,
or the year, or the location, or even the month.
So instead, I propose that we document the current behavior.
@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2014

Comment 3:

Owner changed to @robpike.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Feb 7, 2014

Comment 4 by dgnorton:

How about returning an error for dates that can never happen (i.e., February 30, June
31, etc.) and documenting the leap year behavior?
@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2014

Comment 5:

As I said, the code might not know the month when the day is parsed.
@gopherbot

This comment has been minimized.

Copy link
Author

commented Feb 7, 2014

Comment 6 by dgnorton:

It might not know the month when the day is parsed but it might know it eventually...
diff -r ae14bde9ce3c src/pkg/time/format.go
--- a/src/pkg/time/format.go    Wed Feb 05 07:32:16 2014 -0800
+++ b/src/pkg/time/format.go    Fri Feb 07 14:19:38 2014 -0500
@@ -721,6 +721,20 @@
        return parse(layout, value, loc, loc)
 }
 
+func isValidDay(month, day int) bool {
+   max := 31
+   switch month {
+      case  4, 6, 9, 11:
+         max = 30
+      case 2:
+         max = 29
+   }
+   if day < 0 || max < day {
+      return false
+   }
+   return true
+}
+
 func parse(layout, value string, defaultLocation, local *Location) (Time, error) {
        alayout, avalue := layout, value
        rangeErrString := "" // set if a value is out of range
@@ -786,7 +800,9 @@
                        month, value, err = getnum(value, std == stdZeroMonth)
                        if month <= 0 || 12 < month {
                                rangeErrString = "month"
-                       }
+                       } else if !isValidDay(month, day) {
+                                rangeErrString = "day"
+                        }
                case stdWeekDay:
                        // Ignore weekday except for error checking.
                        _, value, err = lookup(shortDayNames, value)
@@ -797,7 +813,7 @@
                                value = value[1:]
                        }
                        day, value, err = getnum(value, std == stdZeroDay)
-                       if day < 0 || 31 < day {
+                       if !isValidDay(month, day) {
                                rangeErrString = "day"
                        }
                case stdHour:
@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2014

Comment 7:

This is not the medium for code submissions. Please see
http://golang.org/doc/contribute.html
But yes, you do have a point: it is often possible to verify the date.
But your code does not handle leap years correctly, nor should it, and it does not
handle the nearly-impossible-to-characterize other gaps in the flow of days.
As I said earlier, a real fix is impractical. I also think a poor fix is not an
improvement. I would rather document the behavior, which is simple to understand. It's
easy, if a little more expensive, to validate the returned string (except, again, for
the killer cases) for those very few systems that require rigorous date validation.
@gopherbot

This comment has been minimized.

Copy link
Author

commented Feb 7, 2014

Comment 8 by dgnorton:

My apologies for the misplaced code.  I agree that a poor fix is not an improvement and
rigorous validation isn't worth the expense here.  However, I think an inexpensive check
to make sure the date is at least possible is a good fix.  February 29 is possible but
February 30 will never happen (except in the corporate world, where it might be
convenient to extend a fiscal year beyond its usual boundary??).
No response necessary if you still disagree.  Otherwise, I'm willing to submit a patch
and documentation.
@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2014

Comment 9:

We agree that documentation is lacking, but I think your code makes it significantly
harder to explain what the function does.
Let things stew for a while. There's no rush.
@rsc

This comment has been minimized.

Copy link
Contributor

commented May 9, 2014

Comment 10:

Labels changed: added release-none, removed release-go1.3maybe.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Aug 31, 2015

CL https://golang.org/cl/14123 mentions this issue.

@robpike robpike closed this in 8b96be1 Sep 10, 2015

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2015

I suggested accepting any two-digit day, but I think that was a mistake. I have now seen it break real code that expected time.Parse to reject, for example, January 99. The analogy with time.Date may not hold up because in time.Date, the separate arguments effectively precludes the possibility of malformed input, while malformed input is a fundamental part of time.Parse.

I now believe we should do as the original report suggested, namely keep rejecting January 32 and January 99, as Go 1.5 did, and also reject February 30, April 31, June 31, September 31, November 31, and February 29 in non-leap years.

It should suffice, at the end of the parse, to check that day <= daysIn(month, year).
If the year has not been parsed but the month is february, year 0 is a leap year, so standalone "Feb 29" is accepted.
If the month and year have not been parsed but a day has been parsed, that's January year 0, so standalone "31" would be accepted.
So the check does not restrict any plausibly valid partial parse.

@rsc rsc modified the milestones: Go1.6, Unplanned Dec 10, 2015

@rsc rsc reopened this Dec 10, 2015

@gopherbot

This comment has been minimized.

Copy link
Author

commented Dec 10, 2015

CL https://golang.org/cl/17710 mentions this issue.

@robpike robpike closed this in 5ef8991 Dec 10, 2015

@ironiridis

This comment has been minimized.

Copy link

commented Jan 10, 2016

Alarmingly, this patch breaks compatibility with Rockchip's new calendaring system.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2016

I'm not alarmed.

Package time provides functionality for measuring and displaying time.
The calendrical calculations always assume a Gregorian calendar.

@golang golang locked and limited conversation to collaborators Jan 13, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.