rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.2k stars 12.7k forks source link

rustdoc support for per-parameter documentation #57525

Open kvark opened 5 years ago

kvark commented 5 years ago

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:

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.

steveklabnik commented 5 years ago

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

kvark commented 5 years ago

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

mathijshenquet commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

QuietMisdreavus commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

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

rklaehn commented 5 years ago

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 commented 4 years ago

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)

Dragonrun1 commented 4 years ago

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.

camelid commented 3 years ago

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 commented 3 years ago

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

jyn514 commented 3 years ago

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: https://github.com/rust-lang/rust/issues/57525#issuecomment-454940616

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 commented 3 years ago

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.

personalizedrefrigerator commented 3 years ago

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 commented 2 years ago

Would love this.

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

jyn514 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Related issue on the RFCs repo: https://github.com/rust-lang/rfcs/issues/2639#issuecomment-464423048

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 commented 1 year ago

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.

cheater commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

tigregalis commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

/// 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 commented 6 months ago

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.

note: only recently re-discovered this comment, completely forgot about it. i'll still try to do it though

MultisampledNight commented 6 months ago

(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)

ghost commented 5 months ago

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 commented 5 months ago

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).

geo-ant commented 1 week ago

Please feel free to delete this comment if this is not the right place to talk about this. I have released the roxygen crate which exposes an attribute macro to document function parameters and generic arguments. It can maybe act as a polyfill until this lands upstream.