eternal icon indicating copy to clipboard operation
eternal copied to clipboard

Change supported inputs from `Table.t()` to `atom()`

Open HansGlimmerfors opened this issue 7 months ago • 1 comments

I was originally playing around with dialyzer in cachex to try to help fix some typespecs, but after stumbling upon Cachex.Services.Locksmith.start_link/0 which appeared to have no local return, I dove into eternal to find out why.

After some digging, I found that dialyzer complains a lot about eternal too - mostly due to the opaque Eternal.Table.t() (which I think is due to it being referenced in ways that opaque isn't designed for). But after experimenting a bit to try to deal with this, I realized that although the type hints at the support for numbers (@opaque t :: number | atom), there's actually not full support for a Table.t()... Looking at Eternal.Supervisor.start_link/3, the typespec says that one of the possible return types is {:ok, pid, Table.t()}, and from the code this suggests that Table.t() should correspond to the return type of :ets.new/2. However, the typespec of :ets.new/2 doesn't match that of Table.t()... (At least as far back as OTP 17, not sure about before that...). When testing, the non-atom type returned appears as a reference() not a number():

iex(2)> t :ets.table
@type table() :: atom() | tid()

iex(3)> t :ets.tid
@opaque tid()

A table identifier, as returned by new/2.

iex(4)> :ets.new(:test, [:public])
#Reference<0.516442820.2558394371.104295>

This means that calling Eternal.Supervisor.start_link/3 without passing :named_table in the ETS-options, will result in an error: Screenshot 2025-06-14 at 20 28 42

But since a refence() isn't a number(), the proposed changes won't remove support from any inputs that were previously supported (as a reference() would not have passed Eternal.Table.is_table/1, and it won't pass is_atom/1 either). Removing Eternal.Table is of course potentially breaking for users referencing it, but I believe it was part of the private API?

This PR:

  • Fixes the typespecs for dialyzer (0 warnings using flags dialyzer: [flags: [:error_handling, :underspecs]])
  • Forces Eternal.Supervisor.start_link/3 to create a :named_table, as that is what it supports

(My git diff highlights some comments that are unrelated to the changes without any changes to the text, so there might be a whitespace change too...)

HansGlimmerfors avatar Jun 14 '25 19:06 HansGlimmerfors