-
Notifications
You must be signed in to change notification settings - Fork 3k
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
stdlib: fix exported types in supervisor, gen_server, gen_event #7205
base: master
Are you sure you want to change the base?
stdlib: fix exported types in supervisor, gen_server, gen_event #7205
Conversation
This feels kind of backwards! Should we not have a generic type that used by gen_statem, gen_server and supervisors? Supervisors actually are implemented as gen_servers. |
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.
I think this needs to be taken up at an OTB since it has quite much impact on the public API.
If types are exported they should have well thought names that we are happy with and would like to keep.
When exporting types they become part of the public API that we have to keep compatible, When the types where only mentioned in the doc but not exported they where only for manual use and we could change them whenever we wanted.
What do you mean with this? that the types from gen_servers are the ones that everyone references? or that the types from gen_server are the ones that everyone references under an alias (similar to what we have now but I chose supervisor to be the "global" type to be shared and every other module creates an alias towards it? |
@kikofernandez I mean that not all gen_statems or gen_servers are supervisors, rather they are mostly workers and supervisors are implemented as gen_servers. So the generic process behaviors should not reference types of the specific supervisor behavior rather the other way around, or probably we should export a gen-type that can be referenced by all the behaviors. |
@IngelaAndin @KennethL I have made changes as suggested in the OTB thread, where proc_lib is the one that exports those types, which are consumed by |
81bd801
to
3eb6e8e
Compare
ade2f5c
to
ad49f80
Compare
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.
I think proc_lib:sup_name should be proc_lib:process_name, or maybe shorter proc_name. Otherwise I like it now.
ab7170d
to
944ce98
Compare
@IngelaAndin for consistency, I think that if we have not ever exported |
@kikofernandez I agree with you. I think it makes much more sense to use the names from the main building blocks to represent common types. |
@IngelaAndin I have updated the |
@@ -142,8 +142,12 @@ gen_event:stop -----> Module:terminate/2 | |||
</datatype> | |||
|
|||
<datatype> | |||
<name name="emgr_ref"/> | |||
</datatype> | |||
<name name="process_ref"/> |
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.
emgr is not a clear name I think, I guess it stands for event manager. But in this particular place I am not sure if it is obvious if we should use the generalized type or if we should have a type for this abstraction?! I think the same goes for gen_server and supervisor module. I will make a comment in those place too, so that we can have comments from the @erlang/team-ps
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.
Or maybe it is the case that we should not have new types with the same name but let the documentation refer to
proc_lib:process_name and so on!
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.
The point on making aliases an exporting the aliases is to provide a type abstraction over it, so that theoretically, in a common type system, gen_event:process_ref()
is not the same as other type that happens to match its internal representation. That said, the current type checker (Dialyzer) will expand the types to its constituents, so for Dialyzer gen_event:process_ref()
and proc_lib:process_ref()
are the same.
(AFAIK, Dialyzer is a structural type system and will establish the equality of types as mentioned above)
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.
Regarding the generalisation of the type, I know think you mean that emgr_ref()
is more concrete than process_ref()
.
I would like to favour uniformity, so that we can create a "type" understanding without having to know that an emgr_ref()
is equivalent to what is known in other modules as process_ref()
</p> | ||
</item> | ||
</taglist> | ||
</desc> | ||
</datatype> | ||
|
||
<datatype> | ||
<name name="server_ref"/> | ||
<name name="process_ref"/> |
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.
Here is one place
@@ -122,7 +122,7 @@ gen_server:abcast -----> Module:handle_cast/2 | |||
|
|||
<datatypes> | |||
<datatype> | |||
<name name="server_name"/> | |||
<name name="process_name"/> | |||
<desc> |
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.
Here is one place
@@ -402,7 +402,10 @@ child_spec() = #{id => child_id(), % mandatory | |||
<seeerl marker="#sup_flags">above</seeerl>.</p></desc> | |||
</datatype> | |||
<datatype> | |||
<name name="sup_ref"/> | |||
<name name="process_ref"/> | |||
</datatype> |
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.
Here is one place
@@ -606,7 +609,7 @@ child_spec() = #{id => child_id(), % mandatory | |||
<fsummary>Create a supervisor process.</fsummary> | |||
<type name="startlink_ret"/> | |||
<type name="startlink_err"/> | |||
<type name="sup_name"/> | |||
<type name="process_name"/> | |||
<desc> |
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.
Here is one place
@@ -512,26 +512,26 @@ handle_event(_, _, State, Data) -> | |||
|
|||
<datatypes> | |||
<datatype> | |||
<name name="server_name"/> | |||
<name name="process_name"/> |
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.
Here is one place
5965f42
to
e2043c8
Compare
@rickard-green Could you take a look at this PR and let us know what you think of the types? |
Exports types mentioned in the documentation of supervisor, gen_server, and gen_event
Closes #6893