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

rustdoc support for per-parameter documentation #57525

Open
kvark opened this issue Jan 11, 2019 · 36 comments
Open

rustdoc support for per-parameter documentation #57525

kvark opened this issue Jan 11, 2019 · 36 comments
Labels
A-attributes Area: #[attributes(..)] C-feature-request Category: A feature request, i.e: not implemented / a PR. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@kvark
Copy link
Contributor

kvark commented Jan 11, 2019

It appears to me that this pattern of documenting function arguments has become an informal standard:

    /// Supplies a new frame to WebRender.
    ///
    /// Non-blocking, it notifies a worker process which processes the display list.
    ///
    /// Note: Scrolling doesn't require an own Frame.
    ///
    /// Arguments:
    ///
    /// * `document_id`: Target Document ID.
    /// * `epoch`: The unique Frame ID, monotonically increasing.
    /// * `background`: The background color of this pipeline.
    /// * `viewport_size`: The size of the viewport for this frame.
    /// * `pipeline_id`: The ID of the pipeline that is supplying this display list.
    /// * `content_size`: The total screen space size of this display list's display items.
    /// * `display_list`: The root Display list used in this frame.
    /// * `preserve_frame_state`: If a previous frame exists which matches this pipeline
    ///                           id, this setting determines if frame state (such as scrolling
    ///                           position) should be preserved for this new display list.
    pub fn set_display_list(
        &mut self,
        epoch: Epoch,
        background: Option<ColorF>,
        viewport_size: LayoutSize,
        (pipeline_id, content_size, display_list): (PipelineId, LayoutSize, BuiltDisplayList),
        preserve_frame_state: bool,
) {...}

It's quite sub-optimal currently for a number of reasons:

  • have to repeat argument names
  • separate block of argument docs versus arguments themselves
  • just a lot of noise

So not only it's not exactly most convenient, it's also easy to get the docs de-synchronized from the code (notice the document_id above ^). It would be much better if we could do this instead:

    /// Supplies a new frame to WebRender.
    ///
    /// Non-blocking, it notifies a worker process which processes the display list.
    ///
    /// Note: Scrolling doesn't require an own Frame.
    pub fn set_display_list(
        &mut self,
        /// Unique Frame ID, monotonically increasing.
        epoch: Epoch,
        /// Background color of this pipeline.
        background: Option<ColorF>,
        /// Size of the viewport for this frame.
        viewport_size: LayoutSize,
        /// The ID of the pipeline that is supplying this display list,
        /// the total screen space size of this display list's display items,
        /// and the root Display list used in this frame.
        (pipeline_id, content_size, display_list): (PipelineId, LayoutSize, BuiltDisplayList),
        /// If a previous frame exists which matches this pipeline id,
        /// this setting determines if frame state (such as scrolling position)
        /// should be preserved for this new display list.
        preserve_frame_state: bool,
) {...}

Does that sound remotely possible? With an assumption that the latter case would produce near-identical documentation to the former.

@kvark kvark changed the title rustdoc support for per-argument documentaion rustdoc support for per-argument documentation Jan 11, 2019
@steveklabnik
Copy link
Member

I am not aware of any sort of "standard" for documenting arguments. We don't generally do this in the standard library.

@kvark
Copy link
Contributor Author

kvark commented Jan 11, 2019

@steveklabnik I was under wrong impression then, thanks for correcting me. There is still value, I think, to support properly documented function arguments.

@estebank estebank added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 11, 2019
@mathijshenquet
Copy link

mathijshenquet commented Jan 15, 2019

Perhaps it is possible to reuse intra rustdoc links syntax for this. We could include the arguments of a function in its rustdoc scope. Using implied reference links, arg below would be resolved to the argument of the function.

/// [`arg`]: some arg
fn example(arg: u32) {

}

Similar to paramref in C# xml documentation.

@mathijshenquet
Copy link

Although this would not be per-argument documentation, it would fix the de-syncing docs issue while being a relatively lightweight addition to rustdoc.

@GuillaumeGomez
Copy link
Member

It could interesting. However a big problem remains: what should it look like, both in input and ouput? (markdown side and html side.)

@QuietMisdreavus
Copy link
Member

I'm a fan of trying to get per-argument doc comments. Having a finer-grain set of documentation than "the entire function" provides a hook for people to write more docs.

The thing we'll have to solve first is to get doc comments to actually work on function arguments:

/// function docs
fn asdf<
    /// typaram docs
    Asdf: Display
>(
    /// function arg docs
    // ~^ ERROR: expected pattern, found `/// function arg docs`
    thing: Asdf
) -> String {
    format!("{}", thing)
}

Using an unsugared #[doc = "function arg docs"] attribute also doesn't work. However, the type parameter attribute doesn't receive a complaint. However, i'm pretty sure that none of these elements have their attributes tracked in the compiler right now, so that will have to be parsed out and loaded before rustdoc can use them.


As for how to display it, that's another matter. For bare functions, it's not a stretch that we can add headings to that page (it's currently empty except for the main docs). For associated functions or trait functions, it gets hairier. We currently don't have any precedent for adding headings underneath a generated item (like a function), so anything we add will be new ground. However, it's possible to just print the "whole function" docs, then add headings for anything we have docs for (i.e. don't add argument/typaram listings for functions that haven't added fine-grain docs for them). At least, that's my initial idea, and the one i would try if/when we get the docs to load in the first place.

@QuietMisdreavus
Copy link
Member

One thing i would be interested in seeing (and this may become a lang-team question) is whether/how we can add an attribute to the return type of a function, to possibly document that separately.

@estk
Copy link
Contributor

estk commented Feb 14, 2019

The feature request as proposed by @kvark is brilliant! I think it fits very nicely with rust's pragmatic view of language design.

  • This seems like a significant reduction to the friction of writing docs.
  • A standard would benefit the reader in more quickly finding the desired section of the doc.
  • It could enhance the richness of the generated documentation.

Bottom line for me is that good docs are critical and this seems to encourage both creation and consumption.

@rklaehn
Copy link

rklaehn commented Oct 26, 2019

My two cents:

I don't mind this feature, but I just wanted to say that I love the fact that documenting every argument individually is not done frequently in the rust docs. In my experience with other languages where documenting every argument with a line of explanation is encouraged or even enforced by a linter, that leads to really pointless documentation since often it is completely obvious from the name of the argument what it is for.

I very much prefer a few sentences explaining the function and reference to the argument names using arg only where it makes sense...

@sivizius
Copy link

sivizius commented Jun 1, 2020

It could interesting. However a big problem remains: what should it look like, both in input and ouput? (markdown side and html side.)

we could use definition lists for that:

<dl>
  <dt><code>arg</code></dt><dd>Something</dd>
  <dt><code>very_long_arg</code></dt><dd>Something else</dd>
  <dt><code>long_arg</code></dt><dd>Foobar</dd>
</dl>

and with css this could be aligned like this (without bullet points and with correct alignment)

  •                   arg – Something
  • very_long_arg – Something else
  •          long_arg – Foobar

@Dragonrun1
Copy link

I think @mathijshenquet idea of using implied reference links is a good one and using <dl> list like @sivizius suggested would be the way to go in html with this once the other details are worked out. It wasn't mentioned but one of the <h2> - <h6> headers should be used before the actual <dl> list with Arguments as the title.

That's my 2 cents anyway and I hope to see this added soon.

@jyn514 jyn514 added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Aug 25, 2020
@jyn514 jyn514 added the A-attributes Area: #[attributes(..)] label Nov 5, 2020
@camelid
Copy link
Member

camelid commented Dec 17, 2020

We could of course just use the @param syntax used in JS Doc and other documentation systems.

Whatever syntax we use, personally I think the No. 1 reason to implement some kind of per-argument docs system that rustdoc understands is that we could add diagnostics so that they never go stale if an argument is removed. I just ran into this and opened a PR to fix it (#80103) but that wouldn't have ever happened if rustdoc gave an error when documentation refers to an argument that doesn't exist.

@camelid
Copy link
Member

camelid commented Dec 17, 2020

@jyn514 Why did you apply the A-attributes label?

@jyn514
Copy link
Member

jyn514 commented Dec 19, 2020

Rather than adding special syntax, I would rather just accept doc-comments on the parameters themselves:

    /// Supplies a new frame to WebRender.
    ///
    /// Non-blocking, it notifies a worker process which processes the display list.
    ///
    /// Note: Scrolling doesn't require an own Frame.
    pub fn set_display_list(
        /// Target Document ID.
        &mut self,
        /// The unique Frame ID, monotonically increasing.
        epoch: Epoch,
        /// The background color of this pipeline.
        background: Option<ColorF>,
        /// The size of the viewport for this frame.
        viewport_size: LayoutSize,
        /// * `pipeline_id`: The ID of the pipeline that is supplying this display list.
        /// * `content_size`: The total screen space size of this display list's display items.
        /// * `display_list`: The root Display list used in this frame.
        (pipeline_id, content_size, display_list): (PipelineId, LayoutSize, BuiltDisplayList),
        /// If a previous frame exists which matches this pipeline
        /// id, this setting determines if frame state (such as scrolling
        /// position) should be preserved for this new display list.
        preserve_frame_state: bool,
    )

Then there's no need for special HTML or markdown syntax, it's very clear what text applies to which parameters just by looking at it. I added A-attributes because like QuietMisdreavus mentioned, it will require support from the parser to accept attributes in those positions: #57525 (comment)

I don't mind this feature, but I just wanted to say that I love the fact that documenting every argument individually is not done frequently in the rust docs. In my experience with other languages where documenting every argument with a line of explanation is encouraged or even enforced by a linter, that leads to really pointless documentation since often it is completely obvious from the name of the argument what it is for.
I very much prefer a few sentences explaining the function and reference to the argument names using arg only where it makes sense...

I agree that this shouldn't be the encouraged form of documentation, but I think it would be nice to at least make it possible to write (for the same reason that /// is the de facto standard but it's still possible to use /**).

@jyn514
Copy link
Member

jyn514 commented Dec 19, 2020

Whatever syntax we use, personally I think the No. 1 reason to implement some kind of per-argument docs system that rustdoc understands is that we could add diagnostics so that they never go stale if an argument is removed. I just ran into this and opened a PR to fix it (#80103) but that wouldn't have ever happened if rustdoc gave an error when documentation refers to an argument that doesn't exist.

This isn't an issue if the syntax is on the argument itself - you'd know when you remove the argument to remove the documentation, because it's right there staring you in the face.

bors bot added a commit to intellij-rust/intellij-rust that referenced this issue Jun 20, 2021
7060: INT: Generate documentation stub r=mchernyavsky a=0x7CA

changelog: Added an editor intention that generates a documentation stub as demonstrated in: https://doc.rust-lang.org/beta/rust-by-example/meta/doc.html and rust-lang/rust#57525


![oKw9t3X](https://user-images.githubusercontent.com/12113428/113785446-7103c500-9737-11eb-963b-0deda81a1052.gif)

I'm not sure if the current implementation is robust enough, as the RustCommenter lacks the functionality that was used for IntelliJ Javadocs and Pycharm docstring implementations. So this implementation has to manually create the comments by inserting strings.  I was also not able to parse the function context like in intellij to see whether a function was being declared or just being referenced, or already had a doc, so I had to implement it. 
Feedback is very welcome

Co-authored-by: 0x7CA <12113428+0x7CA@users.noreply.github.com>
@personalizedrefrigerator

Perhaps it is possible to reuse intra rustdoc links syntax for this. We could include the arguments of a function in its rustdoc scope. Using implied reference links, arg below would be resolved to the argument of the function.

/// [`arg`]: some arg
fn example(arg: u32) {

}

Similar to paramref in C# xml documentation.

This is also what Dart does. I think it would be quite nice to have.

@tigregalis
Copy link

Would love this.

It is already called out as a future possibility in RFC 2565: Attributes in formal function parameter position

@jyn514
Copy link
Member

jyn514 commented Jul 24, 2022

The next step here is for someone to make a lang team MCP or RFC. It is not something rustdoc can do on its own.

@GuillaumeGomez
Copy link
Member

Just something I thought about (not a blocker): I hope it won't increase rustdoc source code complexity too much and won't have a negative impact on performance. But I still like the idea.

@camelid
Copy link
Member

camelid commented Jul 24, 2022

Yeah, we should make sure that we feel comfortable with the implementation approach. However, I think since we already have items that are not actually items (e.g., fields), it shouldn't be too different from existing stuff.

@newpavlov
Copy link
Contributor

newpavlov commented Nov 2, 2022

It could be worth to support the following more compact way to document function parameters:

pub fn f(
    a: u32, //! Parameter A
    b: u32, //! Parameter B
) { ... }

Also we should be explicit about sharing doc comment with several arguments, for example:

pub fn(
    /// Parameter
    a: u32, b: u32,
} { ... }

I think it should result in a compilation error, but currently Rust accepts:

pub struct Foo (
    /// First field doc
    u32, u32,
);

So from consistency point of view it should be accepted for function arguments as well.

@jyn514
Copy link
Member

jyn514 commented Nov 2, 2022

Again, no changes can be made here without an RFC. Posting on this issue is not a productive way to get the feature implemented.

@scottmcm
Copy link
Member

scottmcm commented Nov 2, 2022

Related issue on the RFCs repo: rust-lang/rfcs#2639 (comment)

I'd much rather see a way for `epoch` in the doc comment to mean the parameter specifically, to help catch typos (and maybe give on-hover type information, or something.

My experience with having stuff like this is that it invariably turns into a "you must do it for all the argument" in some well-meaning mandate, which just ends up being horrible noise. If there are so many parameters that you can't just mention them naturally in prose, then the problem is that there are too many parameters.

Not to mention that having them per-argument is horrible whenever they're at all related to each other. It's way nicer to be able to say "draw a rectangle from the top left corner (x0, y0) to the bottom-right corner (x1, y1)" rather than having some annoying

@param x0 the x coordinate of the top-left corner of the rectangle to draw
@param y0 the y coordinate of the top-left corner of the rectangle to draw
@param x1 the x coordinate of the bottom-right corner of the rectangle to draw
@param y1 the y coordinate of the bottom-right corner of the rectangle to draw

@thurn
Copy link

thurn commented Jan 12, 2023

The useful part about this for me in languages that support it, like C# and Java, is that you can create a linter which detects when you mention a function parameter that does not exist. This helps keep documentation up-to-date when functions change.

facebook-github-bot pushed a commit to facebook/buck2 that referenced this issue Apr 3, 2023
Summary:
Looks like function parameter docs are not supported in Rust: rust-lang/rust#57525

Move them up so that they are displayed as in our docs, would be nice to move it back once it is supported.

Differential Revision: D44629092

fbshipit-source-id: 6124f70f2e31ddc9d268e5aedbc674a42f994add
@cheater
Copy link

cheater commented Feb 10, 2024

What if instead I prefer

    pub fn set_display_list(
        &mut self,
        epoch: Epoch,                  /// Unique Frame ID, monotonically increasing.
        background: Option<ColorF>,    /// Background color of this pipeline.
        viewport_size: LayoutSize,     /// Size of the viewport for this frame.
        (pipeline_id, content_size, display_list): (PipelineId, LayoutSize, BuiltDisplayList),
          /// ^ The ID of the pipeline that is supplying this display list,
          /// the total screen space size of this display list's display items,
          /// and the root Display list used in this frame.
        preserve_frame_state: bool,
          /// ^ If a previous frame exists which matches this pipeline id,
          /// this setting determines if frame state (such as scrolling position)
          /// should be preserved for this new display list.
) {...}

Many people do comments like that

@cheater
Copy link

cheater commented Feb 10, 2024

linter

this can work with the format presented in the original post that OP doesn't like (it is, in fact, many people's preferred way of doing things)

@tigregalis
Copy link

What if instead I prefer

    pub fn set_display_list(
        &mut self,
        epoch: Epoch,                  /// Unique Frame ID, monotonically increasing.
        background: Option<ColorF>,    /// Background color of this pipeline.
        viewport_size: LayoutSize,     /// Size of the viewport for this frame.
        (pipeline_id, content_size, display_list): (PipelineId, LayoutSize, BuiltDisplayList),
          /// ^ The ID of the pipeline that is supplying this display list,
          /// the total screen space size of this display list's display items,
          /// and the root Display list used in this frame.
        preserve_frame_state: bool,
          /// ^ If a previous frame exists which matches this pipeline id,
          /// this setting determines if frame state (such as scrolling position)
          /// should be preserved for this new display list.
) {...}

Many people do comments like that

This suggestion covers your first case, I think in a more rustic way:

pub fn f(
    a: u32, //! Parameter A
    b: u32, //! Parameter B
) { ... }

As for your second case, that's the first time I've seen that, what language or ecosystem is that common in? Other than the obscurity, I think there is value in consistency within the language and between users of Rust. By that I mean there should be one good way of doing things.

@cheater
Copy link

cheater commented Feb 12, 2024

@tigregalis what suggestion do you have for argument comments that require multiple lines?

@tigregalis
Copy link

You go above the line you're commenting, as in the original post, and in perfect alignment with existing Rust documentation syntax. To be clear the suggestion is that the ///-before syntax and //!-after syntax could be compatible, and largely consistent with existing Rust syntax.

I think something that's obvious but no one has specifically mentioned here, are the parallels to documenting a struct.

You can easily imagine this:

    /// Supplies a new frame to WebRender.
    ///
    /// Non-blocking, it notifies a worker process which processes the display list.
    ///
    /// Note: Scrolling doesn't require an own Frame.
    pub struct DisplayListSetter {
        /// Unique Frame ID, monotonically increasing.
        epoch: Epoch,
        /// Background color of this pipeline.
        background: Option<ColorF>,
        /// Size of the viewport for this frame.
        viewport_size: LayoutSize,
        /// The ID of the pipeline that is supplying this display list,
        /// the total screen space size of this display list's display items,
        /// and the root Display list used in this frame.
        (pipeline_id, content_size, display_list): (PipelineId, LayoutSize, BuiltDisplayList),
        /// If a previous frame exists which matches this pipeline id,
        /// this setting determines if frame state (such as scrolling position)
        /// should be preserved for this new display list.
        preserve_frame_state: bool,
    }

Being roughly equivalent to this:

    /// Supplies a new frame to WebRender.
    ///
    /// Non-blocking, it notifies a worker process which processes the display list.
    ///
    /// Note: Scrolling doesn't require an own Frame.
    pub fn set_display_list(
        &mut self,
        /// Unique Frame ID, monotonically increasing.
        epoch: Epoch,
        /// Background color of this pipeline.
        background: Option<ColorF>,
        /// Size of the viewport for this frame.
        viewport_size: LayoutSize,
        /// The ID of the pipeline that is supplying this display list,
        /// the total screen space size of this display list's display items,
        /// and the root Display list used in this frame.
        (pipeline_id, content_size, display_list): (PipelineId, LayoutSize, BuiltDisplayList),
        /// If a previous frame exists which matches this pipeline id,
        /// this setting determines if frame state (such as scrolling position)
        /// should be preserved for this new display list.
        preserve_frame_state: bool,
) {...}

@newpavlov
Copy link
Contributor

newpavlov commented Feb 12, 2024

Note that Rust also supports inner (/*! ... */) and outer (/** ... */) block doc comments. Though they are rarely used in practice. So in theory one could write something like this:

fn foo(
    arg1: u32, /*! First arg
                   Second line
                   Third line */  
) { ... }

Though personally I would strongly prefer to use multiline /// comments instead.

@swfsql
Copy link

swfsql commented Feb 16, 2024

@tigregalis I also think that's a good model, but there are quite some corner cases, such as when there are generics on the parameters (including for impl Traits, trait items and Self references). For Self, in the struct it would refer to the new struct not to the original one, and this is specially important when some trait bounds types must be defined to Self. Finally, the function may also have generics and bounds, which would also make the struct being generic, have the same bounds and so on.

For example:

    /// Doc.
    pub fn f(
        /// Doc.
        a: Self
    ) {...}
    /// Doc.
    pub struct FSetter {
        /// Doc.
        a: Self, // <- should not refer to `FSetter`
    }

@sivizius
Copy link

sivizius commented Feb 17, 2024

/// vs. //!, like #[…] vs. #![…], is not before vs. after but rather before-a-block and inside-a-block. There is no after-a-block. Like struct Foo {} //^ This is a foo is not possible, arg: Arg, //! This is an argument, neither on the same line nor with a linebreak, is ›rustic‹. Yes, this is a somewhat arbitrary restriction, but ensures expectations. I do not want to have to figure out first to which a comment is referring to. In rust-code, I can assume that comments are always before the item the comment is about. IMHO, doc-comments on arguments must behave exactly the same as doc-comments on struct-fields.

@MultisampledNight
Copy link
Contributor

MultisampledNight commented Apr 19, 2024

Do I understand correctly that the best way forward seems to be an RFC for attributes on function arguments? If so, I'd want to write one (having never written one but needing this for other reasons).
This would be very nice for macro reasons, too, currently function arguments are the only publicly exposed named items that aren't allowed to be doc-commented or marked with other attributes.

@MultisampledNight
Copy link
Contributor

(minor nit: fwiw I guess the title should be about parameters instead, not arguments? arguments would be the actual value passed at runtime i think)

@fmease fmease changed the title rustdoc support for per-argument documentation rustdoc support for per-parameter documentation Apr 19, 2024
@ghost
Copy link

ghost commented May 19, 2024

Just ran into this myself. Does anybody know which things would have to be modified in order to implement the initial proposal? (or has it already been implemented in a nightly?)

@eggyal
Copy link
Contributor

eggyal commented May 19, 2024

Do I understand correctly that the best way forward seems to be an RFC for attributes on function arguments?

That's correct (it's not that that's "the best way" so much as it's a necessity for any change to the surface syntax of the language, which this would amount to).

If so, I'd want to write one (having never written one but needing this for other reasons).

Make sure you first read carefully and follow the instructions in the Rust RFC Book and review the discussion here (especially @scottmcm's comment above: amongst other things, they're a member of the lang team that must approve the RFC).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)] C-feature-request Category: A feature request, i.e: not implemented / a PR. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests