Use unsized, typeless, tuple to fix OTP 18 compilation

[ahf]

This patch fixes compilation for OTP 18.

There's currently no method of specifying a tuple of arbitrary size of type T.

[marianoguerra]

👍 please merge this, making it harder to use riak_core on latest ubuntu with erlang 18 by default.

[cmeiklejohn]

👎

Wouldn't it be better to introduce an actual type for this tuple and what it contains? From what I can tell, you've weakened the type such that Dialyzer will be less effective in determining the proper type -- this isn't just a tuple, it's a tuple containing something.

[ahf]

How are you going to do that? There is, to my knowledge, no way to specify a tuple of arbitrary size of type T.

The code is based around the idea that you (ab)use tuples to create O(1) lookup tables. See:
https://github.com/ahf/riak_core/blob/fix-undefined-tuple-type/src/chashbin.erl#L141-L142

Fixing the implementation is a much harder issue, but until then the code is simply broken - not just broken in the "it could be nicer" kind of broken, but the "s*** doesn't compile" kind of broken.

[jlouis]

As far as I read the source code, the tuple/N form was removed in R18 (and rightly so, it doesn't make any sense from a type level perspective). So sooner or later, you have to fix this. A possibility is to replace to tuple with a map() nowadays, which will probably be a bit slower, but not much.

[lehoff]

Changing the tuple to a map sounds fine re getting cleaner code and better help for Dialyzer.

However, there are many Riak installations on R16, so changing to a map() would make it harder to maintain one code branch.

And then we have the issue of the code being written with some performance goals in mind.
Changing would require proper measurements of the critical path in Riak for starters - if chashbin is on the critical path one has to measure the impact of the change.

[marianoguerra]

if I include a switch on the version maintaining the current type for < R18 and the new one for >= R18 will it be accepted?

if not, is there a proposed solution?

as is, riak_core doesn't compile on R18.

[jlouis]

Were I to make the call, I'd just take the patch. I'm weary about the dialyzer being able to do anything sensible (check with typer what it infers).

[lehoff]

Having looked a bit deeper into the code I agree with @jlouis since the nodes field is only used inside the chashbin module and it is limited what Dialyzer can get out of that.

Accepting this will enable us to keep the code the same without any version magic.

[lehoff]

Double-checked inside Basho and the patch is fine.

[ahf]

Thanks.

[mbbroberg]

Awesome work @ahf! If you find the time, come hang out with us at RICON this year. 👌

Thanks for merging @lehoff.