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

runtime: misleading error when reflect type names clash #17283

Closed
mvdan opened this issue Sep 29, 2016 · 19 comments

Comments

Projects
None yet
7 participants
@mvdan
Copy link
Member

commented Sep 29, 2016

Please answer these questions before submitting your issue. Thanks!

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

go version devel +456a01a Wed Sep 28 16:20:41 2016 +0000 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/mvdan/go"
GORACE=""
GOROOT="/home/mvdan/tip"
GOTOOLDIR="/home/mvdan/tip/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build526272435=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

Run https://github.com/mvdan/reflect-names (play.golang.org does not do since I need multiple packages to trigger this).

What did you expect to see?

An error message that made me understand what was wrong.

What did you see instead?

panic: interface conversion: interface is foo.Bar, not foo.Bar

The first solution that comes to mind is that if the two names clash, the full import path should be included. For example:

panic: interface conversion: interface is github.com/mvdan/reflect-names/p1/foo.Bar, not github.com/mvdan/reflect-names/p2/foo.Bar
@mvdan

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2016

Alternatively, to avoid unnecessarily long lines a common prefix could be stripped:

panic: interface conversion: interface is p1/foo.Bar, not p2/foo.Bar

@quentinmit quentinmit added the NeedsFix label Oct 3, 2016

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

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

I could come up with a patch for this, but first I want input on whether this makes sense at all. @quentinmit I assume that's what NeedsFix means?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

It's fine to send a patch for this (see https://golang.org/doc/contribute.html). Thanks.

Note that the compiler is supposed to already do this. See the code in Sym.symfmt in cmd/compile/internal/gc/fmt.go. I don't know why it is not working for your test case.

@rsc rsc modified the milestones: Go1.8Maybe, Go1.8 Oct 21, 2016

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2016

@ianlancetaylor thanks for the pointer. I've been playing around with debug prints. I assume you were referring to:

// If the name was used by multiple packages, display the full path,
if s.Pkg.Name != "" && numImport[s.Pkg.Name] > 1 {
        return fmt.Sprintf("%q.%s", s.Pkg.Path, s.Name)
}

But that code will not run when my error is being prepared - when the two _type.string() calls are made in the runtime - as Sym.symfmt sees fmtmode == FTypeId. To my understanding, it should be FErr, as the switch case clearly says case FErr: // This is for the user and the code that we want is in its body.

The way in which fmtmode is set is very confusing, and I haven't been able to track this down. This is also a dance between runtime and gc, two packages I've never touched before. Trying to tinker with this function at all mostly just results in cmd/compile breaking and me having to redo a full make.bash.

Do you have any pointers? Any idea why fmtmode could be wrong? Any way to debug this better than searching via prints?

Thanks!

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2016

@mvdan I'm sorry, I don't know the answers to your questions. You could try asking on golang-dev.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2016

@rsc you seem to have touched this code more than anyone else, any idea? I'd rather keep golang-dev as a last resource.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 1, 2016

@mvdan I'm happy to help you look into fixing this.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

Thanks @mdempsky! See #17283 (comment). If that's going in the wrong direction, how would you go about figuring this out without breaking the compiler at every step?

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 1, 2016

Okay, familiarized myself a little with the code.

So to summarize the problem, we're storing short package-name-qualified names for types like "foo.Bar", which we use in runtime.TypeAssertionError panics (and a couple other places). The problem is if we have two different package paths "x/foo" and "y/foo" that both use the package name "foo", the resulting error message is ambiguous.

I don't think simply using package path for ambiguous package names will work for a couple reasons:

  1. We currently emit the "foo.Bar" strings when compiling the "x/foo" and "y/foo" packages, at which time we don't know whether "foo" will be ambiguous in the resulting program.
  2. Because package names and package paths are separate namespaces (though in practice related), mixing the two can introduces their own ambiguities.

I suspect to really fix this, we would need to change TypeAssertionError to always use package path qualification, rather than package name qualification.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

Sounds fair. If the path up to the package name ends up being the same, perhaps TypeAssertionError.Error() could omit that part from the output string, to keep the current behaviour when the types are not ambiguous.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 1, 2016

That may work, but to reiterate: package name is not guaranteed to be a suffix of package path (or necessarily related at all). For example https://godoc.org/google.golang.org/api/plus/v1 has package path "google.golang.org/api/plus/v1" but package name "plus".

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

Ah, right. Feel free to take over on the patch, as this seems non-trivial to fix.

@rsc rsc modified the milestones: Go1.9Early, Go1.8Maybe Nov 2, 2016

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.9Early May 3, 2017

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017

@mvdan mvdan marked this as a duplicate of #21156 Jul 25, 2017

@mvdan mvdan self-assigned this Nov 11, 2017

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2017

I'm having a second crack at this, and finally made a bit of progress. Will send a work-in-progress CL soon, and will ask for pointers from there.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 11, 2017

Change https://golang.org/cl/77210 mentions this issue: runtime: use full package paths for

@mdempsky mdempsky changed the title cmd/compile: misleading error when reflect type names clash runtime: misleading error when reflect type names clash Nov 28, 2017

@mdempsky mdempsky modified the milestones: Go1.10, Go1.11 Nov 28, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

The Go compiler/runtime team discussed this and decided that we should change

interface is foo.Bar, not foo.Baz

to instead say

interface is foo.Bar (x/y/z), not foo.Baz (a/b/c)

I.e., if the package names are the same (even if the type name is different), then we include the package path afterwards.

Also that this isn't a blocking issue for 1.10.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

@mdempsky thank you - that seems like a good decision. I'll update my CL shortly before the 1.11 tree opens to implement this.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2018

I just realised that this issue is a dup of #11634. I wasn't able to find that previous issue when filing this one, and just stumbled across it now. Which should we keep? I'm leaning towards this newer one, as it has more discussion towards a solution, and it is already milestoned and assigned.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

Unfortunately, #11634 points out this problem is more general than we'd been considering here: we might need to distinguish complex types like func(chan *foo.Bar). Moreover, because of map, struct, and interface type literals, there can be an arbitrary number of named types within a type descriptor.

I think let's close this in favor of the older issue.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2018

@mdempsky makes sense, thank you. I'm glad that at least we noticed early, before we sunk more time into a flawed solution and/or the wrong problem.

@golang golang locked and limited conversation to collaborators Jan 6, 2019

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.