Change supported inputs from Table.t() to atom()#6
Open
HansGlimmerfors wants to merge 3 commits into
Open
Conversation
Author
|
I suppose that in theory removing the But other functions such as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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/0which 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 aTable.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 thatTable.t()should correspond to the return type of:ets.new/2. However, the typespec of:ets.new/2doesn't match that ofTable.t()... (At least as far back as OTP 17, not sure about before that...). When testing, the non-atom type returned appears as areference()not anumber():This means that calling

Eternal.Supervisor.start_link/3without passing:named_tablein the ETS-options, will result in an error:But since a
refence()isn't anumber(), the proposed changes won't remove support from any inputs that were previously supported (as areference()would not have passedEternal.Table.is_table/1, and it won't passis_atom/1either). RemovingEternal.Tableis of course potentially breaking for users referencing it, but I believe it was part of the private API?This PR:
dialyzer: [flags: [:error_handling, :underspecs]])Eternal.Supervisor.start_link/3to 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...)