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: Zero value of time.Month fails to print #17720

Closed
kokes opened this issue Nov 1, 2016 · 6 comments

Comments

Projects
None yet
5 participants
@kokes
Copy link

commented Nov 1, 2016

What version of Go are you using (go version)?

go version go1.7 windows/amd64

What did you do?

I set a variable I was to reuse, var m time.Month, but forgot to set it. When printing everything to check my inputs, fmt.Println panicked.

A trivial replication: https://play.golang.org/p/lGKrw6nBb3

What did you expect to see?

A printed zero value for my month.

What did you see instead?

%!v(PANIC=runtime error: index out of range) 0

Discussion

If one initiates time.Month without setting it to a specific month, it fails to print, because int defaults to zero, but months are expected to be within [1, 12], so the String() method goes out of bounds on the slice of prepared strings.

I don't think the zero value can feasibly be January, because while that would solve the String() issue, comparisons to time.January would not work (since time.January is time.Month(1)). Perhaps prepending the months array with something (not sure if there's a precedent for this) and changing the String() method from func (m Month) String() string { return months[m-1] } to func (m Month) String() string { return months[m] } might be enough.

@martisch

This comment has been minimized.

Copy link
Member

commented Nov 1, 2016

I do not think changing the array and using months[m] is general enough and a good solution since month=99 will return a similar error. If we change behaviour we should make sure all invalid values are handled.

Open question would be what an invalid month (including the zero value) should print instead. If there is a better agreed answer than a printing a runtime error then we can change the String method to print that for invalid values. Also should take into account that month.String() is called directly from other methods in time and those just truncate the returned string. So a panic there might be better preferable over some truncated invalid string that produces confusing output. Code calling String() might also depend on getting a panic for invalid values.

The same question can be asked for the weekdays String method too.

@kokes

This comment has been minimized.

Copy link
Author

commented Nov 1, 2016

Good point. Shall we perhaps use stringer to generate all the months and weekdays? (Weekday's zero value prints just fine, because it's Sunday, but no other invalid values do.)

Running stringer within the time package for these two types results in these generated files. I presume these would need to get checked in, since stringer is not within the standard toolkit. Also, existing String() methods need to be deleted then.

@martisch

This comment has been minimized.

Copy link
Member

commented Nov 1, 2016

I do not think Stringer helps or does make the code better. I rather keep the string array. It is more readable and simpler at least to me. I think the issue is not with the string array as such but what to print or do when the value String should print is outside the valid range.

We can then do something like:

if i := m-1; 0 =< i && i < len(months) {
    // valid value
    return[i]
}
// invalid value
?

Your proposed solution for ? seems to be to return fmt.Sprintf("Month(%d)", i+1).

However as outlined above this will print "Mon" when truncated by methods that call String() and just truncate the returned string s[:3]. Which can even be confused for Monday in a date format.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 1, 2016

No need for stringer. Just add a simple if m < 1 || m > 12. I agree that s[:3] returning Mon for Month(0) or Month(15) could be confusing. Maybe %!Month(0), similar to the bogus fmt Printf style.

@quentinmit quentinmit added the NeedsFix label Nov 3, 2016

@quentinmit quentinmit added this to the Go1.8Maybe milestone Nov 3, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Nov 11, 2016

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

@gopherbot gopherbot closed this in a18b4b3 Nov 11, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Nov 12, 2016

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

gopherbot pushed a commit that referenced this issue Nov 12, 2016

time: simplify stringification of Month
Simplifies https://golang.org/cl/33145
which fixed #17720.

Change-Id: Ib922d493cdc5920832dc95b55094796baca7243e
Reviewed-on: https://go-review.googlesource.com/33194
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

@golang golang locked and limited conversation to collaborators Nov 12, 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.