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.

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

Jump to bottom

EDoc: EEP-48 doc chunk generation #2803

Merged
merged 27 commits into from
Feb 12, 2021
Merged

EDoc: EEP-48 doc chunk generation #2803

merged 27 commits into from
Feb 12, 2021

Conversation

erszcz
Copy link
Contributor

@erszcz erszcz commented Oct 14, 2020 •
edited
Loading

This PR enables EDoc to output EEP-48 style doc chunks. The work leading to this PR was so far discussed and documented at erlef/documentation-wg#4, whereas developed at https://github.com/erszcz/edoc.

I tried to address all points raised in the discussions so far:

  1. Chunks are written to disk as standalone files, not embedded into .beam files.
  2. It's possible to generate chunks from the command line by running a script.
  3. The chunks are as close to erl_docgen chunks as possible (with the caveat of type links described below).
  4. All @spec and @type tags are rewritten to -spec and -type attributes, respectively.

We discussed the type link caveat with @garazdawi - erl_docgen chunk links to types do not use type arities. This means links to t/0 and t/1 have the same form. EDoc chunks do not follow this principle, since it seemed more robust to allow linking to arbitrary types. Until this is fixed for erl_docgen chunks, measures have to be taken in 3rd party tools (e.g. ExDoc) to watch for this difference.

Some features stemming from EDoc design and implementation, but not necessarily intuitive, are not addressed in this PR:

  1. Function doc comments can precede or follow -spec attributes, but type doc comments must follow -type attributes.
  2. @since and @deprecated tags are supported on functions, but are not supported on types or callbacks.

This PR also adds test/eep48_SUITE.erl which tests the majority of the newly added functionality, as well as some of the above points which are currently not supported (these tests are skipped by default).

@erszcz erszcz mentioned this pull request Oct 14, 2020
Open
16 tasks
@josevalim
Copy link
Contributor

This is amazing work @erszcz! ❤️

One of the things to discuss, probably for after this PR is merged, is to deprecate the tags @spec and @type in EDoc. Possibly stop parsing them altogether and emit a warning, since they won't be used by chunks and, afaik, the offer not benefit (only downsides?) over the existing -type and -spec.

@KennethL
Copy link
Contributor

KennethL commented Oct 15, 2020 via email

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Oct 19, 2020
@erszcz erszcz mentioned this pull request Nov 13, 2020
Closed
@garazdawi garazdawi self-assigned this Nov 23, 2020
garazdawi
garazdawi reviewed Nov 23, 2020
View reviewed changes
Copy link
Contributor

@garazdawi garazdawi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've done a cursory review and I think all in it it looks very good. Most of the comments are about backward-compatibility and documentation.

It would be nice if the changes of @spec to -spec would be a separate commit from everything else so that it is easier to see that it does not change any functionality.

Does @richcarl have any comments about this PR?

I also looked through some rendered functions and found some small irregularities that I'm not sure if you have seen or not:

erl_parse:tree/2

In the edoc html generate this function looks like this:

tree(Type::atom(),Data::term()) -> tree()

however, in the shell docs it looks like this:

-spec tree(atom(),term()) -> tree().

This because edoc for html reads the variable names and inserts them
into the spec. Information is lost when this is not done as the reader
does not know which argument is Type and which is Data in the text.

-spec tree(atom(), term()) -> tree().

For special purposes only. Creates an abstract syntax tree node
with type tag *Type* and associated data *Data*.

See also

See also data/1.

See also is_tree/1.

See also type/1.

These should probably be grouped into one:

See also data/1, is_tree/1, type/1

as that is done both in the edoc html and shell_docs.

Problem in shell_docs

Seems like shell_docs does not render <pre> correctly, or rather
it does not insert a newline before itself so that things like merl
look like this:

  Quick start

  To enable the full power of Merl, your module needs to include the
  Merl header file:
         -include_lib("syntax_tools/include/merl.hrl").

  Then, you can use the ?Q(Text) macros in your code to create
  ASTs or match on existing ASTs. For example:
         Tuple = ?Q("{foo, 42}"),
         ?Q("{foo, _@Number}") = Tuple,
         Call = ?Q("foo:bar(_@Number)")

  Calling merl:print(Call) will then print the following code:
         foo:bar(42)

  The ?Q macros turn the quoted code fragments into ASTs, and

I'll take a look at fixing that and try not to break any of the erl_docgen renderings.

lib/edoc/bin/edoc.escript Outdated Show resolved Hide resolved
lib/edoc/doc/overview.edoc Outdated Show resolved Hide resolved
lib/edoc/doc/overview.edoc Outdated Show resolved Hide resolved
lib/edoc/bin/edoc.escript Outdated Show resolved Hide resolved
lib/edoc/doc/overview.edoc Show resolved Hide resolved
lib/edoc/src/edoc_layout.erl Show resolved Hide resolved
lib/edoc/src/edoc_run.erl Outdated Show resolved Hide resolved
lib/edoc/test/eep48_SUITE.erl Show resolved Hide resolved
lib/stdlib/src/shell_docs.erl Show resolved Hide resolved
lib/edoc/src/edoc.erl Outdated Show resolved Hide resolved
@garazdawi garazdawi mentioned this pull request Nov 23, 2020
Merged
@erszcz
Copy link
Contributor Author

erszcz commented Dec 8, 2020

Thanks for the review, @garazdawi!

I think it makes the most sense to start with the spec/type changes. I'm sending a separate PR shortly, to make it easy to review. Then, I'll rebase this as appropriate.

@erszcz erszcz mentioned this pull request Dec 8, 2020
Merged
@erszcz erszcz changed the title EEP-48 doc chunk support for EDoc EDoc: EEP-48 doc chunk generation Dec 9, 2020
@erszcz
Copy link
Contributor Author

erszcz commented Dec 11, 2020

With regard to erl_syntax:tree/1,2, I've added code which inserts argument names into the specs stored in the chunk. This means the spec in the chunk will not reflect the spec in the source file 1-to-1 anymore, but for the example from erl_syntax.erl:

-spec tree(atom()) -> tree().

tree(Type) ->
    tree(Type, []).

-spec tree(atom(), term()) -> tree().

tree(Type, Data) ->
    ...

We get properly named types in erl:

1> h(erl_syntax, tree).

  -spec tree(Type :: atom()) -> tree().

  Equivalent to tree(Type, []).

  -spec tree(Type :: atom(), Data :: term()) -> tree().

  For special purposes only. Creates an abstract syntax tree node with type tag Type and
  associated data Data.

It does not yet support 'bounded_fun' syntax tree nodes (-spec f(A) -> A when A :: type()) - for now, these are just passed as is.

@erszcz
Copy link
Contributor Author

erszcz commented Dec 18, 2020

These should probably be grouped into one:

See also data/1, is_tree/1, type/1

Done in the newest batch of changes.

Moreover, arg names are now properly added to bounded_fun kind of specs.

@garazdawi
Copy link
Contributor

Would you mind rebasing on top of latest master to get the #2914 changes removed from this PR?

@erszcz
Copy link
Contributor Author

erszcz commented Jan 7, 2021

@garazdawi Sure, I should be able to rebase this tomorrow. I also have some WIP on the documentation for the escript - I'll attach on top of the rebase.

@garazdawi
Copy link
Contributor

Great, let me know when you are ready for me to review the PR again.

@garazdawi garazdawi added the waiting waiting for changes/input from author label Jan 8, 2021
@erszcz erszcz force-pushed the edoc-chunk-support branch 2 times, most recently from e10d3cb to db33aff Compare January 8, 2021 12:34
@erszcz
Copy link
Contributor Author

erszcz commented Jan 8, 2021 •
edited
Loading

I've rebased this on top of the current master. I think I've addressed all points of the first review - please let me know if there's something missing. The current status is as follows:

  • edoc.escript is renamed to just edoc - I think it's simpler and better, if it's intended for public use. However, in the HTML table of contents it is listed as edoc_cmd, since otherwise it would clash with edoc.erl docs. I think it's fine this way, but let me know if you think otherwise.
  • Specs stored in chunks don't reflect 1-to-1 the specs in source files - they're extended to contain arg names apart from types. I think that you, @garazdawi, intended to make the specs in source files and in the chunks to be 100% identical. Maybe EDoc should just warn "Hey, your spec looks like this: ..., but it'd look better like that: .... Would you mind fixing your source?" That would only require the user to copy-n-paste the rewritten example, but I'm afraid even that might be too much. This is borderline with EDoc being a linter. Opinions are welcome.
  • @josevalim's good point about warnings for @type and @spec. This is slightly connected with the above point. Should this warning be part of this PR?
  • If @spec and @type are considered deprecated, then what about deprecating @deprecated (pun intended ;) )? There's xref which supports a -deprecated attribute, so this case is analogous to dialyzer and @spec/@type. I think it's worth considering, but not necessarily as part of this PR. This would also solve the problem of EEP-48 defining the deprecated field as just a flat binary(), while current EDoc allowing it to contain markup.

TODOs:

  • When testing this rebased code on Recon, I found out that user_type nodes (that is all non-builtin types) are not handled in the spec rewriting code - I'll work on this next.

@garazdawi
Copy link
Contributor

Hello,

I did lib/edoc/bin/edoc -chunks -app syntax_tools and got a bunch of crashes looking like this:

edoc: error in layout 'edoc_layout_chunks': {'EXIT',
                                       {function_clause,
                                        [{edoc_layout_chunks,annotate_spec,
                                          [['Name'],undefined],
                                          [{file,"edoc_layout_chunks.erl"},
                                           {line,250}]},

Is that because they have both a -spec and @spec?

@garazdawi
Copy link
Contributor

Specs stored in chunks don't reflect 1-to-1 the specs in source files - they're extended to contain arg names apart from types. I think that you, @garazdawi, intended to make the specs in source files and in the chunks to be 100% identical.

I think it is fine that they do not match 100%. This is an oddity in the way that edoc behaves, but I don't think it is worth forcing users to rewrite their specs.

@garazdawi
Copy link
Contributor

garazdawi commented Jan 11, 2021 •
edited
Loading

warnings for @type and @spec. This is slightly connected with the above point. Should this warning be part of this PR?

I think it is better to do this in another PR.

garazdawi
garazdawi reviewed Jan 11, 2021
View reviewed changes
Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

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

I did a quick look to see if it would be easy to add support in the make system to use edoc instead of erl_docgen to build the doc chunks for Erlang/OTP and it turns out that it is not. So I think we will leave that for later.

My only remaining question from the original review is whether we should document the edoc_doclet_chunks and edoc_layout_chunks modules? The way it is implemented now it is only possible to generate chunks via the script without relying on undocumented API's.

lib/edoc/src/edoc_layout_chunks.erl Outdated Show resolved Hide resolved
lib/edoc/src/edoc_layout_chunks.erl Outdated Show resolved Hide resolved
lib/edoc/src/edoc_layout_chunks.erl Outdated Show resolved Hide resolved
lib/edoc/doc/overview.edoc Outdated Show resolved Hide resolved
@erszcz
Copy link
Contributor Author

erszcz commented Jan 20, 2021

Status update:

When testing this rebased code on Recon, I found out that user_type nodes (that is all non-builtin types) are not handled in the spec rewriting code - I'll work on this next.

Done.

got a bunch of crashes ... because they have both a -spec and @SPEC?

Indeed, that turns out to be the reason. It seems that in such a case EDoc doesn't pass the -spec attr form down to the layout :| I might be able to find out why and fix it in EDoc core, but for now a warning will be printed that just @spec or redundant -spec and @spec are not supported when generating chunks. At least we have a clear answer to the "should we print a warning or not" question :)

@garazdawi garazdawi mentioned this pull request Jan 20, 2021
Merged
erszcz and others added 11 commits February 5, 2021 21:06
@erszcz
Add edoc/.gitignore
4001143
@erszcz
Allow all header ranks in the chunks
2092cc2
@erszcz
Improve spec formatter completeness, diagnostics and robustness
f0f14ef
@erszcz
Make the type tag lookup more robust
10de0f8 
In order to gather all info about a type that EEP-48 requires,
we refer to EDoc #tag{} records.
These records do not contain the type name at the top level,
so this lookup was done just by line number.

When a header file is included into another file,
it might happen that the position information of two distinct type
definitions is the same - they are still different types,
but the lookup by line will return the wrong one.

This lead to the following error (on kernel/src/disk_log.erl):

    name: dlog_size
    arity: 0
    type attr: {attribute,82,type,{file_error,{type,82,term,[]},[]}}

    edoc: error in layout 'edoc_layout_chunks': {'EXIT',
                                           {{badmatch,{file_error,[]}},
                                            [{edoc_layout_chunks,meta_type_sig,4,
                                              [{file,"edoc_layout_chunks.erl"},
                                               {line,170}]},

This change introduces a more sophisticated and robust lookup,
which fixes the problem of types defined in include files.
@erszcz
Fix various small things
4a58810 
- Fix unused variables and lookup functions
- Remove edoc-orig-tag
- Remove name attribute from <a> elements
- Be less verbose if not debugging
@erszcz
Add a script to batch test libraries
921436e
@erszcz
Allow only HTML tags allowed by shell_docs
461ef2b
@erszcz
Document EEP-48 support, making doc chunks, new doclet/layout
1343415
@erszcz @garazdawi
Depend on stdlib with shell_docs
722328c 
Co-authored-by: Lukas Larsson <[email protected]>
@erszcz
Change the description
3728fac
@erszcz
Adjust function signature formatting to better comply with EEP-48
52dc947
@garazdawi
Copy link
Contributor

github actions doc build has started to fail with the latest changes, do you know why?

@erszcz
Copy link
Contributor Author

erszcz commented Feb 8, 2021

ce77e68 and c5581d2 introduce the priority of attrs over tags. However, as this changes EDoc core, it's not limited to chunk generation, but all paths, so static HTML or erl_docgen spec extraction, too.

Specs were missing in OTP docs after these changes -
93c34bb fixes that.

I'm now troubleshooting some types which used to be exported, but currently are not.

@erszcz
Copy link
Contributor Author

erszcz commented Feb 8, 2021

I’ve reverted the PR to the last commit passing all checks. I’ll carry on in a separate branch.

@erszcz
Add a fixture of a spec prefix with a module name
dd975bb 
Apparently, these are not handled properly by EDoc,
therefore types mentioned in them aren't properly exported to docs.
@erszcz
Handle specs prefixed by a module name
60f7634
@erszcz
Do not drop module name from MFA-style specs
a347941
@erszcz
Copy link
Contributor Author

erszcz commented Feb 8, 2021

The latest commits invert the priorities of tags and attributes. -spec and -type now take precedence over @spec and @type. Warnings will be emitted for the tags.

This priority inversion means that if a module has both a @type and a -type of the same name, but only the former carries a doc comment, then this comment will not appear anymore in either the HTML or chunk output. See @type array in stdlib/src/array.erl for an example. This is a backward incompatible change, but should not affect OTP docs.

In case of specs, the problem of consecutive -spec attributes is solved - EDoc can now accept specs in any order and not necessarily directly adjacent to a function they describe. This solves some errors in OTP files, e.g. stdlib/src/otp_internal.erl.

I finally think there's nothing more to add to this PR, unless some bugs pop up. @garazdawi @richcarl what do you think?

@garazdawi
Copy link
Contributor

I finally think there's nothing more to add to this PR, unless some bugs pop up.

I agree.

@garazdawi garazdawi merged commit a347941 into erlang:master Feb 12, 2021
@garazdawi
Copy link
Contributor

Merged! Thanks for all your work!

@erszcz
Copy link
Contributor Author

erszcz commented Feb 15, 2021

Woohoo! And thank you for the reviews!

@erszcz erszcz mentioned this pull request Mar 31, 2021
Closed
@garazdawi garazdawi mentioned this pull request Oct 4, 2021
Merged
9 tasks
@erszcz erszcz mentioned this pull request Sep 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richcarl richcarl richcarl requested changes

@garazdawi garazdawi garazdawi approved these changes

Assignees

@garazdawi garazdawi

Labels
feature team:VM Assigned to OTP team VM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@erszcz @josevalim @KennethL @garazdawi @richcarl @rickard-green