Skip to content

Add generator for existing atoms #310

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add generator for existing atoms #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a76d552
Add generator for existing atoms
Maria-12648430 Apr 6, 2023
61041f0
Improvements and tests
Maria-12648430 Apr 17, 2023
33594d9
Improve existing_atom tests
Maria-12648430 Apr 18, 2023
fd445b8
Changes after first review
Maria-12648430 Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions include/proper.hrl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@
%% Basic types
%%------------------------------------------------------------------------------

-import(proper_types, [integer/2, float/2, atom/0, binary/0, binary/1,
bitstring/0, bitstring/1, list/1, vector/2, union/1,
weighted_union/1, tuple/1, loose_tuple/1, exactly/1,
fixed_list/1, function/2, map/2, any/0]).
-import(proper_types, [integer/2, float/2, atom/0, existing_atom/0, binary/0,
binary/1, bitstring/0, bitstring/1, list/1, vector/2,
union/1, weighted_union/1, tuple/1, loose_tuple/1,
exactly/1, fixed_list/1, function/2, map/2, any/0]).


%%------------------------------------------------------------------------------
Expand Down
13 changes: 12 additions & 1 deletion src/proper.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,10 @@
%%% <dt>`{stop_nodes, boolean()}'</dt>
%%% <dd> Specifies whether parallel PropEr should stop the nodes after running a property
%%% or not. Defaults to true.</dd>
%%% <dt>`{default_atom_generator, atom | existing_atom}'</dt>
%%% <dd> Declares the type of atom generator to use in the {@link proper_types:any/0. `any()'}
%%% generator to be either {@link proper_types:atom/0. `atom()'} or
%%% {@link proper_types:existing_atom/0. `existing_atom()'}.</dd>
%%% </dl>
%%%
%%% == Spec testing ==
Expand Down Expand Up @@ -539,6 +543,7 @@
| 'quiet'
| 'verbose'
| pos_integer()
| {'default_atom_generator', 'atom' | 'existing_atom'}
| {'constraint_tries',pos_integer()}
| {'false_positive_mfas',false_positive_mfas()}
| {'max_shrinks',non_neg_integer()}
Expand Down Expand Up @@ -569,6 +574,7 @@
constraint_tries = 50 :: pos_integer(),
expect_fail = false :: boolean(),
any_type :: {'type', proper_types:type()} | 'undefined',
default_atom_generator = atom :: 'atom' | 'existing_atom',
spec_timeout = infinity :: timeout(),
skip_mfas = [] :: [mfa()],
false_positive_mfas :: false_positive_mfas(),
Expand Down Expand Up @@ -744,8 +750,10 @@ global_state_init_size_seed(Size, Seed) ->
-spec global_state_init(opts()) -> 'ok'.
global_state_init(#opts{start_size = StartSize, constraint_tries = CTries,
search_strategy = Strategy, search_steps = SearchSteps,
any_type = AnyType, seed = Seed, numworkers = NumWorkers} = Opts) ->
any_type = AnyType, seed = Seed, numworkers = NumWorkers,
default_atom_generator = DefaultAtomGen} = Opts) ->
clean_garbage(),
put('$default_atom_generator', DefaultAtomGen),
put('$size', StartSize - 1),
put('$left', 0),
put('$search_strategy', Strategy),
Expand All @@ -772,6 +780,7 @@ global_state_reset(#opts{start_size = StartSize} = Opts) ->
global_state_erase() ->
proper_typeserver:stop(),
proper_arith:rand_stop(),
erase('$default_atom_generator'),
erase('$any_type'),
erase('$constraint_tries'),
erase('$left'),
Expand Down Expand Up @@ -1039,6 +1048,8 @@ parse_opt(UserOpt, Opts) ->
N when is_integer(N) ->
?VALIDATE_OPT(?POS_INTEGER(N), Opts#opts{numtests = N});
%% tuple options, sorted on tag
{default_atom_generator,G} ->
?VALIDATE_OPT(G =:= atom orelse G =:= existing_atom, Opts#opts{default_atom_generator = G});
{constraint_tries,N} ->
?VALIDATE_OPT(?POS_INTEGER(N), Opts#opts{constraint_tries = N});
{false_positive_mfas,F} ->
Expand Down
25 changes: 24 additions & 1 deletion src/proper_gen.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
any_gen/1, native_type_gen/2, safe_weighted_union_gen/1,
safe_union_gen/1]).

-export([existing_atom_gen/0]).

%% Public API types
-export_type([instance/0, seed/0, size/0]).
%% Internal types
Expand Down Expand Up @@ -418,6 +420,27 @@ atom_gen(Size) ->
atom_rev(Atom) ->
{'$used', atom_to_list(Atom), Atom}.

%% @private
Copy link
Collaborator

@kostis kostis Apr 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two functions, and the PR in general, should describe the main idea of the existing atom generation algorithm and the assumptions it relies on. Also its limitations.

Having written that, if I understand what the existing_atom_gen() does, it is in fact not an atom generator (in the sense that proper_gen:atom_gen/1 is), but it simply returns a random atom from those (currently) loaded in the atom table, which it stores in its own map, right?

Since there is a magic way (the <<131, 75, Index:24>> trick) to access each of these atoms given an Index, and the upper limit of this index is known ( it is given by erlang:system_info(atom_count)), what is the point of first iterating through all these atoms putting them in an internal map, rather than randomly picking one Index in the range and returning the corresponding atom? (Perhaps after a num_tries if this process is unlucky to only end up in atoms starting with $.) What I am missing?

Incidentally, I do not like the magic constants 131, 75 and 24. They should be made appropriate defines and ideally obtained by the Erlang/OTP system somehow.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having written that, if I understand what the existing_atom_gen() does, it is in fact not an atom generator (in the sense that proper_gen:atom_gen/1 is), but it simply returns a random atom from those (currently) loaded in the atom table, which it stores in its own map, right?

That is correct, yes. "Find all existing atoms, (store them in the map,) pick one at random". The term "generator" is probably not the exact term to use here (I guess this is you objection?), but what is the correct term to use?

What I am missing?

Nothing I think ;) That is a great suggestion, I'll do that. How about we use a fallback atom like '' to return if all tries turn up atoms starting with $? We need to return something, right?

-spec existing_atom_gen() -> atom().
%% We make sure we never clash with internal atoms by ignoring atoms starting with
%% the character '$'.
existing_atom_gen() ->
existing_atom_gen(10).

existing_atom_gen(0) ->
'';
existing_atom_gen(N) ->
Index = proper_arith:rand_int(0, erlang:system_info(atom_count) - 1),
Atom = get_existing_atom(Index),
case Atom =:= '' orelse hd(atom_to_list(Atom)) =/= $$ of
true -> Atom;
false -> existing_atom_gen(N - 1)
end.

-define(ATOM_TERM_BIN(Index), <<131, 75, Index:24>>).
get_existing_atom(Index) ->
binary_to_term(?ATOM_TERM_BIN(Index)).

%% @private
-spec binary_gen(size()) -> proper_types:type().
binary_gen(Size) ->
Expand Down Expand Up @@ -594,7 +617,7 @@ any_gen(Size) ->
-spec real_any_gen(size()) -> imm_instance().
real_any_gen(0) ->
SimpleTypes = [proper_types:integer(), proper_types:float(),
proper_types:atom()],
proper_types:default_atom()],
union_gen(SimpleTypes);
real_any_gen(Size) ->
FreqChoices = [{?ANY_SIMPLE_PROB,simple}, {?ANY_BINARY_PROB,binary},
Expand Down
4 changes: 2 additions & 2 deletions src/proper_gen_next.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ float_gen_sa({'$type', TypeProps}) ->

%% List
is_list_type(Type) ->
has_same_generator(Type, proper_types:list(proper_types:atom())).
has_same_generator(Type, proper_types:list(proper_types:default_atom())).

list_choice(empty, Temp) ->
C = ?RANDOM_MOD:uniform(),
Expand Down Expand Up @@ -426,7 +426,7 @@ vector_gen_sa(Type) ->

%% atom
is_atom_type(Type) ->
has_same_generator(Type, proper_types:atom()).
has_same_generator(Type, proper_types:default_atom()).

atom_gen_sa(_AtomType) ->
StringType = proper_types:list(proper_types:integer(0, 255)),
Expand Down
27 changes: 25 additions & 2 deletions src/proper_types.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@
-module(proper_types).
-export([is_inst/2, is_inst/3]).

-export([integer/2, float/2, atom/0, binary/0, binary/1, bitstring/0,
-export([integer/2, float/2, atom/0, existing_atom/0, binary/0, binary/1, bitstring/0,
bitstring/1, list/1, vector/2, union/1, weighted_union/1, tuple/1,
loose_tuple/1, exactly/1, fixed_list/1, function/2, map/0, map/2,
any/0, shrink_list/1, safe_union/1, safe_weighted_union/1]).
Expand All @@ -160,6 +160,7 @@
native_type/2, distlist/3, with_parameter/3, with_parameters/2,
parameter/1, parameter/2]).
-export([le/2]).
-export([default_atom/0]).

%% Public API types
-export_type([type/0, raw_type/0]).
Expand Down Expand Up @@ -674,6 +675,28 @@ atom() ->
{is_instance, fun atom_is_instance/1}
]).

%% @doc All existing atoms. All atoms used internally by PropEr start with a
%% '`$'', so such atoms will never be produced as instances of this type. You
%% should also refrain from using such atoms in your code, to avoid a potential
%% clash.
%% Instances do not shrink.
-spec existing_atom() -> proper_types:type().
existing_atom() ->
?BASIC([
{generator, fun proper_gen:existing_atom_gen/0},
{reverse_gen, fun proper_gen:atom_rev/1},
{is_instance, fun atom_is_instance/1},
{noshrink, true}
]).

%% @private
-spec default_atom() -> proper_types:type().
default_atom() ->
case get('$default_atom_generator') of
existing_atom -> existing_atom();
_ -> atom()
end.

atom_is_instance(X) ->
is_atom(X)
%% We return false for atoms starting with '$', since these are
Expand Down Expand Up @@ -1127,7 +1150,7 @@ map(K, V) ->
%% type if you are certain that you need it.
-spec any() -> proper_types:type().
any() ->
AllTypes = [integer(),float(),atom(),bitstring(),?LAZY(loose_tuple(any())),
AllTypes = [integer(),float(),default_atom(),bitstring(),?LAZY(loose_tuple(any())),
?LAZY(list(any())), ?LAZY(map(any(), any()))],
subtype([{generator, fun proper_gen:any_gen/1}], union(AllTypes)).

Expand Down
6 changes: 3 additions & 3 deletions src/proper_typeserver.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,9 @@

%% CAUTION: all these must be sorted
-define(STD_TYPES_0,
[any,arity,atom,binary,bitstring,bool,boolean,byte,char,float,integer,
list,neg_integer,nil,non_neg_integer,number,pos_integer,string,term,
timeout]).
[any,arity,atom,binary,bitstring,bool,boolean,byte,char,existing_atom,
float,integer,list,neg_integer,nil,non_neg_integer,number,pos_integer,
string,term,timeout]).
-define(HARD_ADTS,
%% gb_trees:iterator and gb_sets:iterator are NOT hardcoded
[{{array,0},array}, {{array,1},proper_array},
Expand Down
14 changes: 14 additions & 0 deletions test/proper_tests.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1398,6 +1398,20 @@ sampleshrink_test_() ->
?_shrinksTo([a], Gen)},
?_test(proper_gen:sampleshrink(Gen))]}].

existing_atom_test() ->
Copy link
Collaborator

@kostis kostis Apr 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very unclear what these tests really test and what their underlying assumptions are.
For example, their correctness crucially depends on not loading any other Erlang modules between the two calls to erlang:system_info(atom_count). Is this robust to e.g. executing all the PropEr tests in parallel?

Also, it's unclear why the default_atom_test is using the any() instead of the atom() generator. For example, the test trivially succeeds if any() happens to return terms other than atoms...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, the tests are brittle, meant as a first attempt =(

Executing PropEr tests in parallel is likely to break them. In fact, anything else that may be going on in the system may break them.

Also, it's unclear why the default_atom_test is using the any() instead of the atom() generator. For example, the test trivially succeeds if any() happens to return terms other than atoms...

The default_atom_generator option, by and large, decides what atom generator is used in the any generator, either atom or existing_atom (there is probably a better name for this). Anyway, this is the reason why any is used in the test. And yes, if any happens to generate only non-atoms, this test will pass. I tried with ?SUCHTHAT, but then the test fails if the generator fails to produce any atom.

I'm at a loss about what to do about this, or rather how to test it PropErly. Thankful for advice 🤷‍♀️

_ = proper_gen:pick(proper_types:existing_atom()),
N = erlang:system_info(atom_count),
{ok, Atom} = proper_gen:pick(proper_types:existing_atom()),
?assert(erlang:is_atom(Atom)),
?assertEqual(N, erlang:system_info(atom_count)).

default_atom_test() ->
_ = proper:quickcheck(?FORALL(_, any(), true),
[1, {default_atom_generator, existing_atom}]),
N = erlang:system_info(atom_count),
?assert(proper:quickcheck(?FORALL(_, any(), true),
[{default_atom_generator, existing_atom}])),
?assertEqual(N, erlang:system_info(atom_count)).

%%------------------------------------------------------------------------------
%% Performance tests
Expand Down