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

internal/reflectlite: Implements erroneously reports that map types do not implement interfaces #34486

Closed
proglottis opened this issue Sep 24, 2019 · 21 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@proglottis
Copy link

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

$ go version
go version go1.13 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/james/Library/Caches/go-build"
GOENV="/Users/james/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/james/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/james/src/proglottis/ts/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 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/nn/cm9pxx2x4pzcmc4_brdzc_9m0000gn/T/go-build569203332=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Tried to check if an error was of type schema.MultiError.

https://play.golang.org/p/eAMul1kgxXJ

What did you expect to see?

Preferably it would populate my errMulti := schema.MultiError{} variable so I can inspect the contents for better error handling.

What did you see instead?

A runtime error:

runtime: nameOff 0x74696177 out of range 0xf0000 - 0x161ba6
fatal error: runtime: name offset out of range
@cuonglm
Copy link
Member

cuonglm commented Sep 24, 2019

This is fixed in master, after https://go-review.googlesource.com/c/go/+/191198

@bcmills
Copy link
Contributor

bcmills commented Sep 24, 2019

Closing based on @cuonglm's reply.

@proglottis, please let us know if this still isn't working for you using gotip.

@bcmills bcmills closed this as completed Sep 24, 2019
@proglottis
Copy link
Author

Thought I'd have a go with gotip, and it now panics. This might be fine if maps aren't supported, but the message is not correct as this type does implement error.

$ ./go version
go version devel +1804bba Tue Sep 24 18:20:52 2019 +0000 darwin/amd64
$ ./go run ./issue.go 
panic: errors: *target must be interface or implement error

goroutine 1 [running]:
errors.As(0x10f1ec0, 0xc0000ba040, 0x10b8200, 0xc0000c2000, 0xc000088f50)
	/Users/james/sdk/gotip/src/errors/wrap.go:87 +0x5cc
main.main()
	/Users/james/sdk/gotip/bin/issue.go:17 +0xb0
exit status 2

I wanted to exclude the possibility that there's something going on in the 3rd party library, and also add a compile time error type assertion. So here's a new script that panics as above on tip:

https://play.golang.org/p/ArfXTQ2AeFc

@bcmills bcmills reopened this Sep 24, 2019
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 24, 2019
@bcmills bcmills added this to the Go1.14 milestone Sep 24, 2019
@bcmills
Copy link
Contributor

bcmills commented Sep 24, 2019

CC @randall77 @martisch @mpvl

@cuonglm
Copy link
Member

cuonglm commented Sep 25, 2019

@bcmills should cc @neild too, this is errors.As implementation details, not the compiler.

@cuonglm
Copy link
Member

cuonglm commented Sep 25, 2019

Hmm, somehow, e.Implements(errorType) returns false for a map.

@bcmills
Copy link
Contributor

bcmills commented Sep 25, 2019

Yes, this is a bug in internal/reflectlite. (It can't be demonstrated in the Playground because internal/reflectlite is internal, but the test in https://golang.org/cl/197179 clearly reproduces the bug.)

@bcmills bcmills added release-blocker NeedsFix The path to resolution is known, but the work has not been done. labels Sep 25, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 25, 2019
@bcmills bcmills changed the title errors: Cannot errors.As into map based error type internal/reflectlite: Implements erroneously reports that map types do not implement interfaces Sep 25, 2019
@gopherbot
Copy link

Change https://golang.org/cl/197179 mentions this issue: internal/reflectlite: test Implements with a defined map type

@bcmills
Copy link
Contributor

bcmills commented Sep 25, 2019

CC @bradfitz, who has also made some (probably unrelated) changes in reflectlite

@martisch
Copy link
Contributor

martisch commented Sep 25, 2019

One thing that I noticed while looking at reflectlite is that we forgot to update it (we updated reflect) in https://golang.org/cl//191198 which likely causes mismatches of struct definitions between runtime and reflectlite.

Especially the map type is now larger:

type mapType struct {

type maptype struct {

While at it we should add tests (to avoid misalignment and breakage of reflectlite) and update the comments to reference reflectlite for updates too.

Im happy work on a fix tomorrow evening UTC if no one jumps on realigning the definitions earlier.

@gopherbot
Copy link

Change https://golang.org/cl/197558 mentions this issue: runtime: add doc to remind adopting changes to reflectlite

@gopherbot
Copy link

Change https://golang.org/cl/197559 mentions this issue: internal/reflectlite: updates reflectlite to match runtime rtype/mapType

@martisch
Copy link
Contributor

@cuonglm got to it before I could finish writing tests locally and verify this fixes issues seen. Can we also add tests to https://golang.org/cl/197559 to prevent this from happening again?

@cuonglm
Copy link
Member

cuonglm commented Sep 27, 2019

@cuonglm got to it before I could finish writing tests locally and verify this fixes issues seen. Can we also add tests to https://golang.org/cl/197559 to prevent this from happening again?

Sure, but how can we test it generally? Or you mean we just test for mapType specific in this case?

@bcmills
Copy link
Contributor

bcmills commented Sep 27, 2019

At the very least, the regression test from https://golang.org/cl/197179 should be added.

Would it also be possible to, say, add a test somewhere that uses reflect to verify that the corresponding types defined in runtime, reflect, and reflectlite all have the same field names and types?

@cuonglm
Copy link
Member

cuonglm commented Sep 30, 2019

Would it also be possible to, say, add a test somewhere that uses reflect to verify that the corresponding types defined in runtime, reflect, and reflectlite all have the same field names and types?

I don' know anyway, since when all of them are un-exported IIRC.

@bcmills
Copy link
Contributor

bcmills commented Sep 30, 2019

I can think of a few possibilities to consider:

  1. Some shared internal package that does not depend on runtime, reflect, or reflectlite, containing variables of type interface{} or func() interface{}, into which instances (or accessors that return instances) of the corresponding types could be written, with the test examining the unexported fields using reflect.
  2. //go:linkname aliases, following the same structure as (1) but without the need for a separate package.
  3. A test using go/types or similar to parse the types from the source code for comparison, rather than checking them at run-time.
  4. A refactoring to move the type declarations into some shared internal package, so that instead of having three copies kept in sync we only have one copy in a more-foundational package.

@bradfitz
Copy link
Contributor

I started working on a test the other day until I realized how gross this all was (and that I was distracted from my primary task). But my conclusion was that (3) would be the least gross/invasive/objectionable.

@bcmills
Copy link
Contributor

bcmills commented Sep 30, 2019

my conclusion was that (3) would be the least gross/invasive/objectionable.

Agreed that (3) is the least invasive.

I suspect I would find (4) a lot less “gross”. (But “gross” and “objectionable” are both subjective, so I'll leave that up to the folks who are actually maintaining these packages day-to-day.)

@bradfitz
Copy link
Contributor

While my personal preference would be many small packages to minimize repetition (option (4)), historically I've seen enough resistance to such refactoring that I now try to avoid it in Go projects.

@gopherbot
Copy link

Change https://golang.org/cl/199280 mentions this issue: internal/reflectlite: add type mirror with reflect test

gopherbot pushed a commit that referenced this issue Oct 5, 2019
Updates #34486

Change-Id: Iec9a5d120013aaa287eccf2999b3f2b831be070e
Reviewed-on: https://go-review.googlesource.com/c/go/+/197558
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Oct 7, 2019
Add test to check that struct type in reflectlite is mirror of reflect.
Note that the test does not check the field types, only check for number
of fields and field name are the same.

Updates #34486

Change-Id: Id5f9b26d35faec97863dd1fe7e5eab37d4913181
Reviewed-on: https://go-review.googlesource.com/c/go/+/199280
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Oct 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants