erlang / eep

Erlang Enhancement Proposals
http://www.erlang.org/erlang-enhancement-proposals/
264 stars 66 forks source link

EEP: internal export #58

Closed MarkoMin closed 7 months ago

MarkoMin commented 8 months ago

EEP for introducing the concept of internal exports. For details, look in the EEP itself and in the PR of reference implementation.

P.S. Do I need to send it on the mailing list?

josevalim commented 8 months ago

Given this EEP proposes to add a new module attribute that is considered by xref, it looks quite straightforward to me compared to the previous EEP, which wanted to bring this knowledge to the compiler and the loader, and therefore had wider ranging implications. With this in mind, I wonder if internal_export is the best name or if the attribute should have xref in its name. The downside is that internal exports would have to be listed twice, one as a regular export, and another, with its visibility rule.

Another question brought up by the EEP is if exporting to applications is indeed the best practice. For those working on large applications (a.k.a. monoliths), You may still want to introduce some sort of boundary between different parts of our system without breaking into smaller applications. A potential alternative here is to allow developers to define the visibility using regular expressions, prefixes/suffixes, or combination of rules, such as:

-xref_app_export {some_app, […]}. -xref_mod_export {“myapp*”, […]}.

I am not proposing to add all those possibilities upfront, but consider them in case more flexibility is required in the future.

MarkoMin commented 8 months ago

Given this EEP proposes to add a new module attribute that is considered by xref, it looks quite straightforward to me compared to the previous EEP, which wanted to bring this knowledge to the compiler and the loader, and therefore had wider ranging implications.

It's currently considered only by xref, but it's not limited to. Documentation tools could use it, shell could provide better completion based on it, language servers, . I'm not sure that internal_export is the best name, but I'm sure I don't want to bind it directly to xref. Furthermore, the plan for the future is to have internal_export_type, because a lot of types are used only from within the app itself - but those are checked by dialyzer (or any other tool). Notice also that nothing restricts us from doing the check in the loader/runtime in the far future (not saying that we should add it, but if we do, it could be on top of the current implementation)

With this in mind, I wonder if internal_export is the best name or if the attribute should have xref in its name. The downside is that internal exports would have to be listed twice, one as a regular export, and another, with its visibility rule.

Yes, but I solved this with internal_export feature with expands -internal_export([f/a, ...]). to both internal export and export. There are probably other ways too.

Another question brought up by the EEP is if exporting to applications is indeed the best practice. For those working on large applications (a.k.a. monoliths), You may still want to introduce some sort of boundary between different parts of our system without breaking into smaller applications. A potential alternative here is to allow developers to define the visibility using regular expressions, prefixes/suffixes, or combination of rules, such as:

In the EEP I assumed that misusage comes from the app's users, not from within the app itself. We could possibly extend the attribute to take 2 arguments, where second one is the list of modules, that would enable more intra-app control. In which use case you want to export something to another app (except for callbacks, which are not statically called anyways)?

kikofernandez commented 8 months ago

@MarkoMin Thanks for your contribution! We will discuss this EEP at the end of week 5, so you should hear from us at the end of week 5 or on week 6.

In the meantime, would it be possible for you to fix the markdown syntax as per the EEP guidelines [1, 2] so that we can add this EEP to the list of proposed EEPs regardless of the decision?

kikofernandez commented 7 months ago

We thank you for your contribution.

In this case, we have decided to reject EEP 67 on the basis that documentation attributes, introduced in OTP-27, is expressive enough to cover internal exports. Anything that has the documentation attribute -doc false. or -doc hidden. does not produce the documentation and can be understood as an internal function.

One can throw warnings using xref or using Dialyzer, by adding a new option that throws a warning when a hidden module is used externally.

TD5 commented 7 months ago

One can throw warnings using xref or using Dialyzer, by adding a new option that throws a warning when a hidden module is used externally.

Good to see the problem being solved, even if it wasn't in precisely the way this EEP proposed it. I think Erlang will really benefit from having a way to do this.

kikofernandez commented 7 months ago

Yes, we really wanted a way to express internal functions, and we think this is a good way to do it (not the only one 😃 )

kuna-invariant commented 7 months ago

Thank you everyone for the review and time spent on this.

But reasoning behind this rejection is abysmal and disappointing!