-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add gradualizer #11
Add gradualizer #11
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
==========================================
+ Coverage 90.69% 91.03% +0.33%
==========================================
Files 8 8
Lines 430 435 +5
==========================================
+ Hits 390 396 +6
+ Misses 40 39 -1
☔ View full report in Codecov by Sentry. |
902a19e
to
e257ca0
Compare
src/cets.erl
Outdated
ack_pid => AckPid, | ||
opts => Opts | ||
}. | ||
|
||
assert_integer(X) when is_integer(X) -> X. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_integer(X) when is_integer(X) -> X. | |
-spec assert_integer(any()) -> integer(). | |
assert_integer(X) when is_integer(X) -> X. |
We have to add a spec to let Gradualizer now that after the guard succeeds the return value is actually an integer()
. Otherwise, it would assume -spec assert_integer(any()) -> any()
and therefore treat the return value as any()
further on, which significantly relaxes any further checks against it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, interesting, dialyzer would still know that assert_integer returns an integer :)
But not gradualizer
Line 604 Column 2: Invalid type specification for function cets:handle_get_info/1. The success typing is (#{'ack_pid':=_, 'opts':=_, 'other_servers':=[pid()], 'tab':=atom() | ets:tid(), _=>_}) -> #{'ack_pid':=_, 'memory':=integer(), 'nodes':=[atom()], 'opts':=_, 'size':=integer(), 'table':=atom() | ets:tid()}
Will add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, we don't really need to add assert, just spec is enough.
src/cets.erl
Outdated
size => returns_non_neg_integer(ets:info(Tab, size)), | ||
memory => returns_non_neg_integer(ets:info(Tab, memory)), | ||
ack_pid => AckPid, | ||
opts => Opts | ||
}. | ||
|
||
%% We are sure that the ETS table exists when running handle_get_info | ||
%% But gradualizer does not know that, so let us tell it with the spec | ||
-spec returns_non_neg_integer(any()) -> non_neg_integer(). | ||
returns_non_neg_integer(X) -> X. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't want/need the guard, i.e. the dynamic check (which I think is better to have in general, unless we're 100% the result cannot be anything else), we can also do:
-include_lib("gradualizer/include/gradualizer.hrl").
...
size => ?assert_type(ets:info(Tab, size), integer())
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, actually runtime check
-spec assert_non_neg_integer(any()) -> non_neg_integer().
assert_non_neg_integer(X) when is_integer(X), X >= 0 -> X.
makes dialyzer unhappy
src/cets.erl
Line 620 Column 2: Type specification cets:assert_non_neg_integer(any()) -> non_neg_integer() is a supertype of the success typing: cets:assert_non_neg_integer(non_neg_integer()) -> non_neg_integer()
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, don't wanna add gradualizer as a build dep just for the header, to make compilation faster :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we might create a gradualizer_support
package at some point to address the compilation issue.
src/cets_call.erl
Outdated
where({via, Module, Name}) -> Module:whereis_name(Name). | ||
|
||
%% whereis/1 could return Port, so ignore Ports (makes Gradualizer happy) | ||
pid_or_undefined(undefined) -> undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same story, if there's no spec then Gradualizer will assume -spec pid_or_port(_) -> any()
.
caf8f44
to
c790b21
Compare
Use assert_type_non_neg_integer function for gradualizer
c790b21
to
b872029
Compare
Added gradualizer, tweaked types a bit to pass the checks.
Mainly we have to constrain the result from a function before returning it (if we don't use the same return type)
It also adds more dialyzer checks (which produce similar warnings).
MIM-1981