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

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

Comments

@kortschak
Copy link
Contributor

@kortschak kortschak commented Sep 12, 2018

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 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 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 label Sep 12, 2018
@mvdan
Copy link
Member

@mvdan 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

@kortschak kortschak commented Sep 12, 2018

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 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

@kortschak kortschak commented Sep 12, 2018

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.

@kortschak kortschak closed this Sep 12, 2018
@bcmills
Copy link
Member

@bcmills 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)

@rsc
Copy link
Contributor

@rsc 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 label Oct 16, 2018
@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Oct 16, 2018

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 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

@gopherbot gopherbot commented Oct 16, 2018

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

@gopherbot gopherbot closed this in a52289e Oct 16, 2018
@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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.