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.

Sign up for GitHub

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

Jump to bottom

fmt: 7c7cecc should be rolled back #27634

Closed
kortschak opened this issue Sep 12, 2018 · 11 comments
Closed

fmt: 7c7cecc should be rolled back #27634

kortschak opened this issue Sep 12, 2018 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.12

Comments

@kortschak
Copy link
Contributor

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

Any child of 7c7cecc.

Does this issue reproduce with the latest release?

No.

What operating system and processor architecture are you using (go env)?

amd64/linux

What did you do?

Run tests for Gonum's mat package.

What did you expect to see?

Pass.

What did you see instead?

Failure because %#v now formats floats with a trailing ".0" if the value is integral.

For example here.

Test failure 
=== RUN   TestFormat
--- FAIL: TestFormat (0.00s)
    format_test.go:146: unexpected format result test 0 part 2:
        got:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:3, Data:[]float64{0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0}, Stride:3}, capRows:3, capCols:3}
        want:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:3, Data:[]float64{0, 0, 0, 0, 0, 0, 0, 0, 0}, Stride:3}, capRows:3, capCols:3}
    format_test.go:146: unexpected format result test 1 part 2:
        got:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:3, Data:[]float64{1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0}, Stride:3}, capRows:3, capCols:3}
        want:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:3, Data:[]float64{1, 1, 1, 1, 1, 1, 1, 1, 1}, Stride:3}, capRows:3, capCols:3}
    format_test.go:146: unexpected format result test 2 part 2:
        got:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:3, Data:[]float64{1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0}, Stride:3}, capRows:3, capCols:3}
        want:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:3, Data:[]float64{1, 1, 1, 1, 1, 1, 1, 1, 1}, Stride:3}, capRows:3, capCols:3}
    format_test.go:146: unexpected format result test 3 part 2:
        got:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:3, Data:[]float64{1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0}, Stride:3}, capRows:3, capCols:3}
        want:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:3, Data:[]float64{1, 0, 0, 0, 1, 0, 0, 0, 1}, Stride:3}, capRows:3, capCols:3}
    format_test.go:146: unexpected format result test 4 part 2:
        got:
        &mat.Dense{mat:blas64.General{Rows:2, Cols:3, Data:[]float64{1.0, 2.0, 3.0, 4.0, 5.0, 6.0}, Stride:3}, capRows:2, capCols:3}
        want:
        &mat.Dense{mat:blas64.General{Rows:2, Cols:3, Data:[]float64{1, 2, 3, 4, 5, 6}, Stride:3}, capRows:2, capCols:3}
    format_test.go:146: unexpected format result test 5 part 2:
        got:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:2, Data:[]float64{1.0, 2.0, 3.0, 4.0, 5.0, 6.0}, Stride:2}, capRows:3, capCols:2}
        want:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:2, Data:[]float64{1, 2, 3, 4, 5, 6}, Stride:2}, capRows:3, capCols:2}
    format_test.go:146: unexpected format result test 6 part 3:
        got:
        &mat.Dense{mat:blas64.General{Rows:2, Cols:3, Data:[]float64{0.0, 1.0, 1.4142135623730951, 1.7320508075688772, 2.0, 2.23606797749979}, Stride:3}, capRows:2, capCols:3}
        want:
        &mat.Dense{mat:blas64.General{Rows:2, Cols:3, Data:[]float64{0, 1, 1.4142135623730951, 1.7320508075688772, 2, 2.23606797749979}, Stride:3}, capRows:2, capCols:3}
    format_test.go:146: unexpected format result test 7 part 3:
        got:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:2, Data:[]float64{0.0, 1.0, 1.4142135623730951, 1.7320508075688772, 2.0, 2.23606797749979}, Stride:2}, capRows:3, capCols:2}
        want:
        &mat.Dense{mat:blas64.General{Rows:3, Cols:2, Data:[]float64{0, 1, 1.4142135623730951, 1.7320508075688772, 2, 2.23606797749979}, Stride:2}, capRows:3, capCols:2}
    format_test.go:146: unexpected format result test 8 part 3:
        got:
        &mat.Dense{mat:blas64.General{Rows:2, Cols:3, Data:[]float64{0.0, 1.0, 1.4142135623730951, 1.7320508075688772, 2.0, 2.23606797749979}, Stride:3}, capRows:2, capCols:3}
        want:
        &mat.Dense{mat:blas64.General{Rows:2, Cols:3, Data:[]float64{0, 1, 1.4142135623730951, 1.7320508075688772, 2, 2.23606797749979}, Stride:3}, capRows:2, capCols:3}

Additional information

I believe this breaks the Go1 compatibility promise, and that it does not do so to fix a bug.

According to the fmt documentation, the %#v verb/flag specifies that "a Go-syntax representation of the value" being printed will be output. However, a string without a trailing ".0" is valid Go syntax for float values and so there was no bug.

It's true that it is not possible to infer the kind from the representation of the value when the suffix is omitted, but the fmt documentation does not say that this should possible. And already, already type information is lost (for example, even within the builtin types, signed integers are indistinguishable, unsigned integers are indistinguishable, and float types are indistinguishable).
@zegl
Copy link
Contributor

zegl commented Sep 12, 2018

The Go 1 compability does not guarantee that all programs will work the same way with all versions of Go 1, the compatibility promise gives Go the right to fix bugs. In the issue #26363 it was decided to fix this bug, even if it would change the behavior of existing programs.

@dsnet
Copy link
Member

dsnet commented Sep 12, 2018

\cc @robpike for decision. I won't be surprised if we keep the new behavior.

@dsnet dsnet added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 12, 2018
@mvdan
Copy link
Member

mvdan commented Sep 12, 2018

This was already discussed in #27598, and in the original issue where this particular behavior was changed.

In general, it's okay if tests that lock in internal behavior break between Go releases. For example, if you depend on exactly what os.File.Readdir does to handle very large directories - that logic has been rewritten twice in the past few releases, if I remember correctly.

You could say that %#v isn't internal behavior, but the new output is valid and fmt makes no promises on its exact output. Writing tests that precisely depend on its byte-by-byte output are bound to break from time to time.

@kortschak
Copy link
Contributor Author

If it were a bug, I'd be very happy for the bug to be fixed, but this looks to me merely like a choice of aesthetic changing.

I'd argue strongly that %#v is not internal behaviour. I'd also argue that the go1compat does make claims that fmt will remain consistent unless a bug or security issue is found.

@robpike
Copy link
Contributor

robpike commented Sep 12, 2018

It was decided it was a bug, as the value "1" as a Go number is not automatically a floating-point constant and code could care. You can argue about the severity of the bug, and whether it deserves addressing, but it has been addressed, discussed, acted upon, and resolved. I believe the old behavior was incorrect and the new behavior is correct. Yes, it can break some programs. Almost any change can. But this one affects very few. I would prefer we leave things as they are now. If we roll it back, we will have just one release with different behavior and that's even stranger.

There have been a number of relatively minor bug fixes like this made over time. They are considered carefully. I assure you that they are never an "aesthetic decision".

@kortschak
Copy link
Contributor Author

If we roll it back, we will have just one release with different behavior and that's even stranger.

This change was not released.

I have found an elegant way to work around the change, but for the record, I think the decision here is wrong.

@bcmills
Copy link
Contributor

bcmills commented Oct 10, 2018

0 is a valid float64 value. So is 1. Consider this program:
https://play.golang.org/p/GHO4-2G3lpv

And the analogous program using a non-default integer type, which also doesn't print any distinguishing type information:
https://play.golang.org/p/tnc2xGx252M

Even if the behavior is not what was originally intended, this change breaks existing tests (including a fair number within Google's internal codebase) for what seems like a marginal benefit.

I think we should roll this back. At the very least, a breaking change to fmt should go through the proposal process.

(CC @FiloSottile @andybons @rsc)

@bcmills bcmills reopened this Oct 10, 2018
@bcmills bcmills mentioned this issue Oct 10, 2018
Closed
@bcmills bcmills added this to the Go1.12 milestone Oct 10, 2018
@rsc
Copy link
Contributor

rsc commented Oct 16, 2018

It's true that we do need to be able to make fixes, but this one doesn't seem important enough to force the issue. Rob is OK with rolling it back. Please do so.

@rsc rsc changed the title fmt: 7c7cecc breaks the Go1 promise fmt: 7c7cecc should be rolled back Oct 16, 2018
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 16, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 16, 2018
@kortschak
Copy link
Contributor Author

Interestingly, I ran into a bug in writing some tests just the other day that arose exactly because the old behaviour exists (assigning a code-generated literal 0 to an element of a map[string]interface{} where a float64 was expected). I am happy to recant the last sentence of my comment above.

@rsc
Copy link
Contributor

rsc commented Oct 16, 2018

If you really need to get the right type out of the print, you should be using %T(%#v) not just %#v.

@gopherbot
Copy link

Change https://golang.org/cl/142597 mentions this issue: Revert "fmt: fix incorrect format of whole-number floats when using %#v"

@golang golang locked and limited conversation to collaborators Oct 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
8 participants
@zegl @rsc @kortschak @mvdan @robpike @bcmills @dsnet @gopherbot