core: Remove String() methods from advanced types

[stevenroose]

Most of these methods did not contain all the relevant information
inside the object and were not using a similar formatting type.

Moreover, the existence of a suboptimal String() method breaks usage
with more advanced data dumping tools like go-spew.

Here is a comparison of the current obj.String(), fmt.Printf("%+v", obj) and spew.Dump(obj) formats for the types in question: https://gist.github.com/stevenroose/baae8a21dc25648e40b59b27e5246fba

[karalabe]

Converting these base types to string is a general debug help. I'm not seeing go spew as a cleaner alternative that I'd enjoy using during a debug session. Any opinions @fjl ?

[stevenroose]

@karalabe I agree that String() can help with debugging. But it doesn't if it (1) has a confusing format and most importantly (2) leaves out information. If I have to print the string, and all missing values manually after that, that's cumbersome.

I expect spew.Dump to always give all values for all fields. Which it doesn't, f.e. for Receipt. (Because spew.Dump uses %v, it prefers the String() method.)

IMO String should only used when a simple one-line string can catch the entire meaning of the object. Like with addresses, hashes, amounts, ... Not for complex structs.

[fjl]

I agree with @stevenroose. I use spew all the time and it's a lot better than what String returns.

[stevenroose]

One thing I wasn't entirely sure about though: https://github.com/ethereum/go-ethereum/pull/16205/files#diff-9e6283ea4cfb34b85a7f4aa9b3452fb0R320

There is no function to force recalculation of the hash value. So I hope that comparison makes sense and it's not some kind of cached value. I tried comparing the spew outputs, but they contain pointer addresses, so that doesn't work.

[fjl]

Did you try reflect.DeepEqual for the comparison?
Using Hash should also work, the cache is invalidated when data inside the transaction changes.

[stevenroose]

--- FAIL: TestLookupStorage (0.00s)
	database_util_test.go:323: tx #0 [28e681d2b3c130374a292d008dae6de26dd5a1220b4a97046a6ef44e1ba9e765]: transaction mismatch: have &{{1 0xc422505b80 1111 0xc420a1b960 0xc422505c20 [17 17 17] 0xc422505d20 0xc422505d40 0xc422505d60 <nil>} {<nil>} {37} {<nil>}}, want &{{1 0xc422504240 1111 0xc420a1b340 0xc422504220 [17 17 17] 0xc4225042c0 0xc4225042e0 0xc422504300 <nil>} {[40 230 129 210 179 193 48 55 74 41 45 0 141 174 109 226 109 213 161 34 11 74 151 4 106 110 244 78 27 169 231 101]} {<nil>} {<nil>}}
FAIL

Aligned, it looks like one of them has an explicit value somewhere where the other has a nil. It looks like it could be the hash. Perhaps in one of them it's cached already and in the other not. Also, there are some values different that look like pointers.

&{{1 0xc422505b80 1111 0xc420a1b960 0xc422505c20 [17 17 17] 0xc422505d20 0xc422505d40 0xc422505d60 <nil>} {<nil>} {37} {<nil>}}
&{{1 0xc422504240 1111 0xc420a1b340 0xc422504220 [17 17 17] 0xc4225042c0 0xc4225042e0 0xc422504300 <nil>} {[40 230 129 210 179 193 48 55 74 41 45 0 141 174 109 226 109 213 161 34 11 74 151 4 106 110 244 78 27 169 231 101]} {<nil>} {<nil>}}


Doesn't work. I guess it somehow checks the atomic.Store's for the hash which just keep an address, no?

[rjl493456442]

Maybe you can use *spew.ConfigState and set DisableMethods as True. So that all Stringer interfaces will be disabled.

[stevenroose]

@rjl493456442 That's a possibility, but a lot of String() methods do make sense. Like hashes, addresses and *big.Int. You really don't want hashes and addresses to be shown as a list of digits.