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

Update attributes in EEP 48 #8945

Merged
merged 2 commits into from
Oct 21, 2024
Merged

Update attributes in EEP 48 #8945

merged 2 commits into from
Oct 21, 2024

Conversation

josevalim
Copy link
Contributor

Currently, editors and documentation tooling must
parse metadata from several chunks in order to
function properly. The source is in the compilation chunk.
The behaviours a module implements is in the attributes
chunk. And the annotation of any definition requires
traversing abstract code.

With this commit, we introduce three new attributes,
:behaviours, :source, and :source_anno, so
this additional data can be standardized and easily located.

We also remove edit_url from the standard. As it is not
used by any of the official tooling, languages, or libraries.

Currently, editors and documentation tooling must
parse metadata from several chunks in order to
function properly. The source is in the compilation
chunk. The behaviours a module implements is in the
attributes chunk. And the annotation of any definition
requires traversing abstract code.

With this commit, we introduce three new attributes,
`:behaviours`, `:source`, and `:source_anno`, so
this additional data can be standardized and easily
located.

We also remove `edit_url` from the standard. As it is
not used by any of the official tooling, languages, or
libraries.
@josevalim
Copy link
Contributor Author

@garazdawi please let me know if you are happy with these changes and we can also provide a PR to OTP to attach this informtation to its Docs chunk.

Copy link
Contributor

github-actions bot commented Oct 16, 2024

CT Test Results

    2 files     70 suites   1h 5m 43s ⏱️
1 551 tests 1 309 ✅ 242 💤 0 ❌
1 767 runs  1 493 ✅ 274 💤 0 ❌

Results for commit 6e9c07f.

♻️ 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

@garazdawi
Copy link
Contributor

Could you give an/some examples of how source and source_anno would look like? I'm especially wondering what paths in source and source_anno -> file are relative to?

@garazdawi garazdawi self-assigned this Oct 16, 2024
@josevalim
Copy link
Contributor Author

josevalim commented Oct 16, 2024

Could you give an/some examples of how source and source_anno would look like? I'm especially wondering what paths in source and source_anno -> file are relative to?

@garazdawi it seems both Erlang and Elixir keep source as an absolute path, so that's how we should proceed?

iex(1)> :lists.module_info(:compile)
[
  version: ~c"8.4.3",
  options: [
    :debug_info,
    {:i, ~c"/buildroot/otp/lib/stdlib/src/../include"},
    {:i, ~c"/buildroot/otp/lib/stdlib/src/../../kernel/include"},
    :warn_missing_doc_function,
    :warn_missing_doc_callback,
    :warn_missing_spec_documented
  ],
  source: ~c"/buildroot/otp/lib/stdlib/src/lists.erl"
]
iex(2)> String.module_info(:compile)
[
  version: ~c"8.5",
  options: [
    :no_spawn_compiler_process,
    :from_core,
    :no_core_prepare,
    :no_auto_import,
    {:inline, [append_unless_empty: 2, prepend_unless_empty: 2]},
    {:inline, [duplicate: 2]},
    {:inline, [graphemes: 1]},
    {:inline, [next_grapheme: 1]},
    {:inline, [starts_with_string?: 3]},
    {:inline, [ends_with_string?: 3]},
    {:inline,
     [
       reverse_characters_to_binary: 1,
       grapheme_to_binary: 1,
       grapheme_byte_size: 1,
       codepoint_byte_size: 1
     ]}
  ],
  source: ~c"/Users/jose/OSS/elixir/lib/elixir/lib/string.ex"
]

The source_anno is going to be whatever is the function definition. The goal is to store the the second line in the example below:

-doc "Example".
example() -> ok.

So a literal copy of whatever is in the AST. For the module definition, probably the annotation of -module(foo).

@garazdawi
Copy link
Contributor

Sounds good, feel free to open a PR adding these. Just make sure that +deterministic works.

@garazdawi
Copy link
Contributor

I wonder if it might also be a good idea to move the spec into the documentation metadata? So that all the docs can be rendered without looking into anything but the doc chunk?

@josevalim
Copy link
Contributor Author

I wonder if it might also be a good idea to move the spec into the documentation metadata? So that all the docs can be rendered without looking into anything but the doc chunk?

I thought about this, back when the EEP was written, and I could not decide, because if we are storing the whole spec as an AST, then we are effectively duplicating potentially large chunks of it. Could it be a problem? We could store only the string format but ExDoc will want to parse it to generate autolinks, and we would need to be able to parse both Elixir and Erlang versions. So I didn't see much benefit. Thoughts?

@garazdawi
Copy link
Contributor

For application/erlang+html I stored the spec in the doc chunk because a single entry could refer to only part of a spec. So there it made sense. Also I just encountered that in a testcase that I would like to render the shell docs of a doc chunk of a different version than the currently running system, which is not possible right now as the spec is missing/incorrect.

When considering that doc chunks can also be external to the .beam file it makes even more sense as then you can build only the .chunk files and use those for rendering the complete docs.

For systems that are concerned about the file size, I think most strip doc chunks anyway? Or if they don't, then they can strip the debug info and still get complete documentation if the spec is included.

So in conclusion I think I would like to see the spec in there. Maybe the format should be as the AST with a function that can be called to render it into a string? For example

specification => {fun erl_pp:attribute/1, AST}

@josevalim
Copy link
Contributor Author

Those are good points.

Storing the function is tricky because the function may not be available. For example, I may be looking at a Elixir module without fully loading Elixir itself (or gleam which does not have a runtime component after compilation). So I guess I just argued that, if we want to store specs, it should be textual?

@lukaszsamson
Copy link
Contributor

For application/erlang+html I stored the spec in the doc chunk

On elixir side that wasn't very useful as the spec was erlang code. A spec in typespec AST on the other hand can be easily translated back to elixir code

@garazdawi
Copy link
Contributor

Any tool will most likely want want to use their own rendering functions for things anyway, for example in the shell we want to provide the current terminal width. So yes, a string will work just as well. We will have to rely on documentation for the tools to know how to parse the string.

@josevalim
Copy link
Contributor Author

Any tool will most likely want want to use their own rendering functions for things anyway, for example in the shell we want to provide the current terminal width. So yes, a string will work just as well.

I realized another possibility is to store the specs themselves as their own entry in the chunk:

{spec, Function, Arity}

This comes with the benefit of them having their own metadata (although the docs field itself would always be none).

That said, are you ok with postponing this discussion? I have been postponing this on the ExDoc side as well because both Erlang and Elixir uses typespecs, so we may have assumptions there that can be harmful if a language uses something else. Given Elixir may have set-theoretic types in about a year, that will force us to exercise the tooling and come up with a better understanding of what is needed here.


Btw, how do you want to proceed with this? Are we ok with merging this as a spec-only change and then send a PR for Erlang to add the new information? Or do you want a PR with everything? Although this is not ready yet for merging.

@garazdawi
Copy link
Contributor

I'm fine with postponing the spec discussion and merging this PR as is if you are happy with the names of the fields.

@josevalim
Copy link
Contributor Author

For those following this, PR for end_location is here: #8966

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Oct 21, 2024
@garazdawi garazdawi merged commit 0b60904 into erlang:master Oct 21, 2024
20 checks passed
@garazdawi
Copy link
Contributor

Thanks!

@jonatanklosko
Copy link
Contributor

FTR PR attaching the new information to the docs chunk: #8975.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants