basho / riak_dt

Convergent replicated datatypes in Erlang
Apache License 2.0
353 stars 70 forks source link

Strange Type-Spec of riak_dt_map #140

Closed Menkir closed 2 years ago

Menkir commented 2 years ago

Hey Guys,

i recently took a look into the type spec of riak_dt_map and found something strange.

According to the riak_dt_map.update/3 function the first parameter Op is defined as map_op():

riak_dt_map.erl

...
-type map_op() :: {update, [map_field_update() | map_field_op()]}.
-type map_field_op() ::  {remove, field()}.
-type map_field_update() :: {update, field(), crdt_op()}.
...
-spec update(map_op(), riak_dt:actor() | riak_dt:dot(), riak_dt_map()) ->
                    {ok, riak_dt_map()} | precondition_error().
...

The field() type is resolved to:

...
-type field() :: {field_name(), field_value()}.
-type field_name() :: {Name :: binary(), CRDTModule :: crdt_mod()}.
-type field_value() :: {crdts(), tombstone()}.
...

So field is basically a tuple of two tuples etc.

But in riak_dt_map.update/3 docs there is the following explanation:

...
%% `{update, field_name(), Op} where `Op' is a valid update operation for a
%% CRDT of type `Mod' from the `Key' pair `{Name, Mod}' If there is no
%% local value for `Key' a new CRDT is created, the operation applied
%% and the result inserted otherwise, the operation is applied to the
%% local value.
...

{update, field_name(), Op} is not the correct type spec of map_op(), its {update, field(), crdt_op()}.

This causes a compile warning.

martinsumner commented 2 years ago

This isn't causing a compile warning for me - what warning are you seeing?

My understanding is that the type spec you describe {update, field(), crdt_op()} is as as defined for map_field_update, the type of an individual map update.

The map_op perhaps should be pluralised, as it is a list of these updates (i.e. map_op is the container for the Ops referred to in the doc string, where the map_field_update is a potential type for an individual Op).

i.e. map_op is the type of "Ops", and each Op can be type map_field_update or map_field_op.

It isn't necessarily a mistake that the type map_op, which refers to multiple Ops, is not pluralised. As all the Ops in the list will be applied in one operation - so it is a singular "Op" which contains multiple "Ops".

martinsumner commented 2 years ago

Just to be clear, I do get this compile warning:

... riak_dt/_build/default/lib/riak_dt/ebin/riak_dt.app" is missing description entry - but this isn't related, and I will fix this later today.

Menkir commented 2 years ago

Ok i think i as on the wrong track. My Projekt uses an older fork where the type specs are still wrong. @martinsumner even fixed that three years ago: https://github.com/basho/riak_dt/commit/81d88764e4cc9acd66807fc8bbe1ded375fbe44e

But anyway thanks to @martinsumner !! :smile: