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

reflect: ConvertibleTo returns false in go1.17 for slice values that previously returned true #47785

Closed
liggitt opened this issue Aug 18, 2021 · 15 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@liggitt
Copy link
Contributor

liggitt commented Aug 18, 2021

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

$ go version
go version go1.17 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/liggitt/Library/Caches/go-build"
GOENV="/Users/liggitt/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/liggitt/.gvm/pkgsets/go1.17/global/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/liggitt/.gvm/pkgsets/go1.17/global"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/liggitt/.gvm/gos/go1.17"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/liggitt/.gvm/gos/go1.17/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/liggitt/tmp/convertible117/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/7f/9xt_73f12xlby0w362rgk0s400kjgb/T/go-build2916420994=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Used the reflect ConvertibleTo function (via a call to https://github.com/stretchr/testify/blob/v1.7.0/assert/assertions.go#L78-L96) to compare slices of two different types with identical names/members/layouts in different packages.

This worked as expected prior to go 1.17.

Minimal reproducer at https://github.com/liggitt/convertible117/blob/master/convertible_test.go

What did you expect to see?

gvm use go1.16 && go test ./ -v -count=1
Now using version go1.16
=== RUN   TestConvertible
--- PASS: TestConvertible (0.00s)
PASS
ok  	github.com/liggitt/convertible117	0.629s

What did you see instead?

gvm use go1.17 && go test ./ -v -count=1
Now using version go1.17
=== RUN   TestConvertible
    convertible_test.go:24: !convertible
--- FAIL: TestConvertible (0.00s)
FAIL
FAIL	github.com/liggitt/convertible117	0.566s
FAIL
@liggitt liggitt changed the title reflect: IsConvertible returns false in go1.17 for values that previously returned true reflect: ConvertibleTo returns false in go1.17 for values that previously returned true Aug 18, 2021
@liggitt liggitt changed the title reflect: ConvertibleTo returns false in go1.17 for values that previously returned true reflect: ConvertibleTo returns false in go1.17 for slice values that previously returned true Aug 18, 2021
@liggitt
Copy link
Contributor Author

liggitt commented Aug 18, 2021

it looks like this was introduced in 7473a6a#diff-8d7fac7a0925d669f941654f9b1cc5ea0be70dd93bfa3ab255456e510cd98cd7R1602 which added a PkgPath equality requirement to haveIdenticalType

@randall77
Copy link
Contributor

If anything, this is a bug in 1.16.

If you add

	_ = ([]b.Reference)(actual)

The compiler complains that the conversion is not allowed (both in 1.16 and 1.17).

Converting slices with different named element types, even if their structure is the same, is not allowed.

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 18, 2021
@mknyszek mknyszek added this to the Backlog milestone Aug 18, 2021
@mknyszek
Copy link
Contributor

Based on @randall77's comment, it seems like the new behavior is technically more correct. Did the actual conversion also used to succeed? If so, this might be a breaking change (not saying we should roll it back, but maybe something to be aware of).

@liggitt
Copy link
Contributor Author

liggitt commented Aug 18, 2021

Based on @randall77's comment, it seems like the new behavior is technically more correct. Did the actual conversion also used to succeed?

yeah, it did (see the test case), which is really unfortunate

@randall77
Copy link
Contributor

The behavior changed at https://go-review.googlesource.com/c/go/+/309729
Not sure that was intentional.

@jinzhu @ianlancetaylor

@griesemer Am I right, that the new behavior is correct?

@liggitt
Copy link
Contributor Author

liggitt commented Aug 18, 2021

If so, this might be a breaking change (not saying we should roll it back, but maybe something to be aware of).

It seems like this a breaking change that is in bounds per https://golang.org/doc/go1compat ("If a compiler or library has a bug that violates the specification, a program that depends on the buggy behavior may break if the bug is fixed. We reserve the right to fix such bugs.")

At the very least this needs to be documented in the release notes, but it's worth asking if the benefits gained by the change outweigh the breakage incurred, or if there is an alternate way to fix the issue https://go-review.googlesource.com/c/go/+/309729 was addressing without breaking existing programs.

@randall77
Copy link
Contributor

It definitely seems like a bug. This code prints false:

package main

import "reflect"

type A struct {
}
type B struct {
}

func main() {
	a := []A{}
	b := []B{}

	println(reflect.ValueOf(a).Type().ConvertibleTo(reflect.ValueOf(b).Type()))
}

In 1.16, ConvertibleTo only reports true if the base types are from two different packages and are identically named. That can't be right.

@randall77
Copy link
Contributor

But I agree, we don't want to break existing programs lightly. We'll have to ponder whether/how to fix this, or document it, or whatever.

@ianlancetaylor
Copy link
Contributor

As discussed above, this change is correct. The types are not convertible and reflect.ConvertibleTo should not be reporting that they are convertible. This change seems clearly correct.

I agree that this should be in the release notes. It didn't occur to me that this would affect ConvertibleTo and AssignableTo in this way.

It's unfortunate that this breaks existing programs, but the earlier behavior was clearly a bug. The reflect package should not be reporting that types are convertible when they are not convertible in the language.

We actually got the correct answer here in Go releases up to 1.7. It broke in Go 1.8. Looks like the breaking change was https://golang.org/cl/24190.

@randall77
Copy link
Contributor

24190 was just the spec change, the actual breaking CL was https://go-review.googlesource.com/c/go/+/30191

@gopherbot
Copy link

Change https://golang.org/cl/343329 mentions this issue: reflect: add test for invalid conversion

gopherbot pushed a commit that referenced this issue Aug 19, 2021
Conversion between slices with different element types is not allowed.
Previously (1.8 <= goversion <= 1.16), this conversion was allowed
if the base types were from different packages and had identical names.

Update #47785

Change-Id: I359de5b6fe3ff35bdbf9ab5a13902a0f820cac66
Reviewed-on: https://go-review.googlesource.com/c/go/+/343329
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@randall77 randall77 modified the milestones: Backlog, Go1.17.1 Aug 19, 2021
@randall77 randall77 added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 19, 2021
@randall77
Copy link
Contributor

Remilestoned to 1.17 for a doc fix.

@randall77 randall77 added the CherryPickCandidate Used during the release process for point releases label Aug 19, 2021
@gopherbot
Copy link

Change https://golang.org/cl/343669 mentions this issue: [release-branch.go1.17] doc: mention fix of Convert/ConvertibleTo in 1.17

@gopherbot
Copy link

Change https://golang.org/cl/343690 mentions this issue: _content/doc: mention fix of Convert/ConvertibleTo in 1.17

gopherbot pushed a commit to golang/website that referenced this issue Aug 19, 2021
CL 309729 fixed a bug in Convert/ConvertibleTo that we should
mention in the release notes for 1.17.

Update golang/go#47785

Change-Id: I4490c15ddf1f52404f9def19e2c9dafac754fd95
Reviewed-on: https://go-review.googlesource.com/c/website/+/343690
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@toothrot
Copy link
Contributor

Change https://golang.org/cl/343669 can be abandoned, as we don't generally keep the release-branch's HTML up to date. The x/website change will correctly update the documentation.

@toothrot toothrot removed the CherryPickCandidate Used during the release process for point releases label Aug 25, 2021
@golang golang locked and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants