simpler, more robust bypass

[rogpeppe]

We make the bypass implementation a little simpler by inferring the flag field position from available reflect information and more robust by checking that the flags that are set actually match the semantics we expect.

We can restrict the use of unsafe to a single function: flagField.

[davecgh]

I tested this on Go 1.3 and it fails with: panic: reflect.Value read-only flag has changed semantics

I printed the flagRO and flagAddr values and they are 1, and 6, respectively.

[rogpeppe]

Interesting. I was unable to test this on Go 1.3 because that release doesn't build on my machine any more. I looked at the code though (commit 3dbc53ae6ad4e3b93f31d35d98b38f6dda25f4ee) and I see:

type flag uintptr

const (
    flagRO flag = 1 << iota
    flagIndir
    flagAddr
    flagMethod
    flagKindShift        = iota
    flagKindWidth        = 5 // there are 27 kinds
    flagKindMask    flag = 1<<flagKindWidth - 1
    flagMethodShift      = flagKindShift + flagKindWidth
)

which seems correct for the constants I put in the code. https://play.golang.org/p/2LawlGgGssY

Perhaps there was another flag combination that I missed?

[rogpeppe]

Ah, I see what's going on - flagIndir is being set too. Will fix.

[dmitshur]

If Go 1.3 is meant to be supported, you should consider adding an entry for in .travis.yml:

https://github.com/davecgh/go-spew/blob/db69d09d2c587e9b9677f991dfcab1fc24d9086e/.travis.yml#L4-L8

[rogpeppe]

OK, so I got the tests running, but they fail because in Go 1.3, pointer-sized value types were held directly in the interface, so it's not always possible to get a pointer to a value just by setting flagAddr, so we see failures like:

(spew_test.Flag) (PANIC=value method spew_test.Flag.String called using nil *Flag pointer)

At least I'm fairly sure that's the reason for the failure (and probably for the somewhat complex current implementation of unsafeReflectValue that prompted me to write this PR).

It's been quite a while since Go 1.3. How important is it that go-spew still support it?

[davecgh]

Right. The reason for the current implementation was because of how things moved around and sometimes it stored the value directly in the pointer field (for small values), while other times it was actually a pointer to the value.

I generally like to keep it backwards compatible, but realistically the tests already don't run on Go 1.3 without minor tweaks due to syntax changes in struct declaration, so I think it's probably acceptable to update the README/doc.go to specifically call it it requires Go 1.4 or higher. I already tested it with Go 1.4 and it worked properly there. I'll test with the rest as well.

I would suggest removing the Go 1.3 parameters from the table of acceptable values though since they're not correct and would be misleading in terms of its ability to run on 1.3.

[rogpeppe]

I just added Go 1.4 build tags, so it will still work under pre-Go 1.4 but you won't get the unsafe semantics. PTAL.

[rogpeppe]

I'm curious though, was the FieldByName function available back in the pre Go 1.3 days?

Yes it was, FWIW. FieldByName was there before Go was first made public.

[rogpeppe]

Aside:

BTW I'm starting to seriously wonder whether bypassing CanAddr is a good idea. Bypassing read-only is one thing (theoretically some package can get access to a read-only value, so it's not compromising the runtime), but by bypassing CanAddr, we run the risk of changing the value inside an interface which really should be sacrosanct.

One possibility might be to bypass CanAddr by copying the interface value into a new addressable value and using that. It does run the risk of copying things that should not be copied, such as mutexes, but perhaps that's the lesser weevil.

I suspect the right answer is to avoid bypassing CanAddr in order to call methods. Is there a compelling use case for when we really need to call a pointer method on an unaddressable value?