Skip to content
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

EEP 65: -import_type directive #7618

Open
wants to merge 2 commits into
base: maint
Choose a base branch
from

Conversation

ilya-klyuchnikov
Copy link
Contributor

@ilya-klyuchnikov ilya-klyuchnikov commented Sep 1, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

CT Test Results

       4 files     122 suites   55m 51s ⏱️
2 347 tests 2 295 ✔️ 52 💤 0
2 730 runs  2 675 ✔️ 55 💤 0

Results for commit 51889c6.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@mikpe
Copy link
Contributor

mikpe commented Sep 1, 2023

How is -import_type(m, [t/0]). different from -type t() :: m:t(). which works today?

@ilya-klyuchnikov
Copy link
Contributor Author

How is -import_type(m, [t/0]). different from -type t() :: m:t(). which works today?

The EEP erlang/eep#51 provides the motivation.

Let me try to explain the motivation from the EEP using the example, assuming that the local type alias -type t() :: m:t(). is used with the intent of reducing boilerplate in specs (and probably other type aliases) in the module. Then the differences would be:

  1. Clarity of the intent. -import_type(m, [t/0]). clearly communicates that the developer wants to use the type m:t() by its short name. The purpose of -type t() :: m:t(). may not be so clear. () What if it further evolves into -type t() :: m:t() | p:t() | z:t(). which can be encountered in large code bases. For example, in erl_lint the purpose is communicated through an additional comment:
    -type anno() :: erl_anno:anno(). % a convenient alias
  2. Code navigation and readability. Once -import_type is part of the language, - it's natural for tooling/IDE (Erlang LS, IntelliJ Erlang plugin, ...) to support it similar to how -import(m, [my_fun/0]) is supported now. So, with -import_type navigation from a usage t() would go directly to the definition of m:t() inside the module m, while with the local type alias navigation from a usage t() would go to the local type alias first. - Which creates an extra hop for someone reading, navigating and understanding the code.
  3. Straightforward renaming/refactoring with tooling. Consider renaming m:t() inside the module m to something else - eg m:t1(). With import_type all the imported occurrences (in other modules) would be renamed. While for the local type alias the renaming would stop at -type t() :: m:t1().
  4. Less boilerplate when referencing parameterized types. Consider -import_type(gb_trees, [tree/2]). vs -type tree(Key, Value) :: gb_trees:tree(Key, Value).

To summarize: the differences are mostly about consistency and ergonomics when working with code.

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Sep 4, 2023
@kikofernandez
Copy link
Contributor

you can expect some updates regarding this PR by the end of the week

@garazdawi
Copy link
Contributor

There are a couple of tools that I think would need updating (or at least test cases added) before this PR is merged. A none-complete list is:

  • edoc
  • erl_docgen (possibly updating edoc is enough)
  • syntax_tools
  • emacs-mode?

Non-OTP tools that will also be effected (just listed here for reference):

  • ErlangLS
  • ELP
  • ExDoc

@IngelaAndin IngelaAndin added the Planned Focus issue added in sprint planning label Oct 4, 2023
@kikofernandez kikofernandez added the stalled waiting for input by the Erlang/OTP team label Oct 5, 2023
Copy link
Contributor

@kikofernandez kikofernandez left a comment

Choose a reason for hiding this comment

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

These are small nitpicks to consider.

I can review the PR again once the changes requested by @garazdawi have been addressed.

lib/stdlib/src/erl_expand_records.erl Show resolved Hide resolved
lib/stdlib/src/erl_expand_records.erl Outdated Show resolved Hide resolved
lib/stdlib/src/erl_lint.erl Show resolved Hide resolved
@ilya-klyuchnikov ilya-klyuchnikov force-pushed the import_type_pr branch 2 times, most recently from 5ba14cc to cd7fcc4 Compare October 13, 2023 14:04
@ilya-klyuchnikov ilya-klyuchnikov force-pushed the import_type_pr branch 2 times, most recently from b568c79 to bd340fb Compare October 16, 2023 10:19
@ilya-klyuchnikov
Copy link
Contributor Author

@kikofernandez - I have added edoc integration, - it works through keeping the mapping of imported types and using them in edoc_spec:d2e function. A simple test case has been added checking that an imported type in a spec is unfolded.

as @garazdawi mentioned - erl_docgen works then out of the box, - I have tested it with the string module - that extensively uses unicode:chardata() type: importing it as -import_type(unicode,[chardata/0]). and replacing all the references unicode:chardata() -> unicode:chardata(). The generated documentation contains unfolded unicode:chardata() in specs.

@ilya-klyuchnikov
Copy link
Contributor Author

I am not sure what to do about the following items:

  • syntax_tools
  • emacs-mode
  • edoc chunks (EEP 48)

Any advice would be much appreciated!

@garazdawi
Copy link
Contributor

I am not sure what to do about the following items:

  • syntax_tools

I would expect erl_syntax, erl_syntax_lib and prettypr to need functionality similar to what they have for import.

  • emacs-mode

Check that the syntax highlighting and flymake works the same for import_type as import.

  • edoc chunks (EEP 48)

It may just work with your updates in edoc. Add a testcase to the eep48_SUITE that checks that an imported type behaves the same as a remote type.

@rickard-green rickard-green added waiting waiting for changes/input from author and removed stalled waiting for input by the Erlang/OTP team labels Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Planned Focus issue added in sprint planning team:VM Assigned to OTP team VM waiting waiting for changes/input from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants