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

Prior doc comments #2374

Closed
wants to merge 1 commit into
base: master
from

Conversation

@ishitatsuyuki
Member

ishitatsuyuki commented Mar 27, 2018

Rendered

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Mar 27, 2018

Thanks @ishitatsuyuki

I have often wanted to be able to doc comment struct fields and enums like that, but I really don't want anything else to be commentable from below.

One thing that annoys me when I use python or c is that there is no standard place to put documentation (just weak convention), so you end with different styles of documentation comments in different projects. In contrast, I especially like the uniformity of where doc comments are placed in current Rust.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Mar 27, 2018

I might be ok with enabling this only for struct fields and enums, though...

enum E {
/// doc comments used currently
A,
B, //! doc comments with this RFC

This comment has been minimized.

@Centril

Centril Mar 27, 2018

Contributor

It's possible to do this in Haskell with haddock like so (which you can mention in the Prior art if you like):

data Maybe a
  = Nothing -- ^ There is nothing here.
  | Just a  -- ^ We have a measly 'a'.

But I'm skeptical that this enhances readability for Rust. At least, if this is possible, I would like it to not be the recommended style by rustfmt.


This sparked an orthogonal question in my mind... What if we allowed the following:

fn foo(x: usize,  //! Some comments about x
       y: String, //! Some comments about y
     ) -> Result  //! Comments about return values.
{
    unimplemented!()
}

or in Haskell:

foo :: Int    -- ^ Some comments about x
    -> String -- ^ Some comments about y
    -> Result -- ^ Comments about return values.
foo x y = undefined

I guess using //! in this way makes sense in that case.

This comment has been minimized.

@ExpHP

ExpHP Mar 28, 2018

I'm not too keen on the return value. Mind the way this would most likely work is:

fn foo(x: usize,  //! Some comments about x
       y: String, //! Some comments about y
     ) -> Result  //! Comments about return values.
{ }

fn foo(x: usize,  //! Some comments about x
       y: String, //! Some comments about y
     ) -> Result;  //! Comments about the... hey, wait a second, you
                   //! pesky semicolon! This is actually documenting the
                   //! entire fn item, isn't it?!

unless you want to propose serious changes (I think) to how rust tokenizes things.

This comment has been minimized.

@ishitatsuyuki

ishitatsuyuki Mar 28, 2018

Member

I don't think we would be introducing param documents immediately. Instead, I imagine if this will even be a part of this RFC, it will be just concatenated and we'll leave parameter description boilerplates up to the document author.

@JustAPerson

This comment has been minimized.

JustAPerson commented Mar 27, 2018

The repurposing of //! strikes me as confusing. So does allowing interleaving. What about so:

enum Foo {
  A, //< This documents A
  B, //< This documents B
}

I mention //< because it sort of points in the correct direction of attention. Vaguely? This should probably be limited to enums and maybe Centril's idea regarding function prototypes.

@ExpHP

This comment has been minimized.

ExpHP commented Mar 28, 2018

Repurposing //! is much better IMO because it's already forbidden in these locations.

Also note: It might be awkward if this is not extended to all attributes. You see, doc comments are actually attributes. In fact, they are actually turned into attributes before the token stream is given to a macro, and macros are capable of doing things like move them around and duplicate them.

macro_rules! the_amazing_docstring {
    (
        #[doc = $doc_a:expr]
        #[doc = $doc_b:expr]
        pub struct $A:ident;
    ) => {
        #[doc = $doc_a]
        #[doc = $doc_b]
        #[doc = $doc_b]
        #[doc = $doc_b]
        pub struct $A;
    };
}

the_amazing_docstring! {
    /// Is there an
    /// echo in here?
    pub struct A;
}

screenshot_20180327_212005

@tanriol

This comment has been minimized.

tanriol commented Mar 30, 2018

It feels confusing to me when, in a comma-separated list, a comment after the comma refers to the item before it, especially if it works with the block and/or attribute syntax.

enum E {
    A, /*! I'm a comment for A. Surprise! */ B
}
@clarcharr

This comment has been minimized.

Contributor

clarcharr commented Apr 2, 2018

I don't like how this coincides with the format for outer doc comments, and perhaps a syntax like //< or //^ might be better.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Apr 3, 2018

See also #920 for prior suggestions of using //< for consistency with Doxygen.

@kornelski

This comment has been minimized.

Contributor

kornelski commented Apr 8, 2018

I'd prefer to /// "just work" (line comment after a token applying to the token).

The root cause of the problem is that the parser doesn't see the comments the same way people do. We should change the parser, not people :)

@kennytm

This comment has been minimized.

Member

kennytm commented Apr 8, 2018

@kornelski

In this code segment:

simple!{/// Reasons why an `u32` is not a valid UTF codepoint.
    InvalidCodepoint {

Should "just work" apply the doc-comment to the simple! { block or InvalidCodepoint { block?

@kornelski

This comment has been minimized.

Contributor

kornelski commented Apr 8, 2018

@kennytm Wording in your example implies a specific interpretation, but I think the case is at best ambiguous, and it could logically be interpreted either way.

The rules could be tweaked to handle edge cases (maybe ignore more "useless" tokens), but I think even the simplest rule works reasonably well:

  1. if /// is the first non-whitespace token in a line, it applies to the item following it (as it is currently)
  2. otherwise, it applies as if it was at the beginning of that line, before all other tokens on that line.

I've implemented a variant of that in Citrus on clang tokens & Rustc's Spans, and it was easy to implement and seemed to match real-world comment usage well.

so with the above two rules this:

simple!{/// Comment
    InvalidCodepoint {

would be interpreted as:

/// Comment
simple!{
    InvalidCodepoint {

I think the interpretation is entirely reasonable. I've seen if (foo) { // handle foo case in C a lot. Your example had different wording, but I think it could as well be:

simple!{ /// this macro defines simple enums
    InvalidCodepoint {

And of course this edge case can be disambiguated either way:

/// a simple macro
simple!{
    /// Reasons why an `u32` is not a valid UTF codepoint.
    InvalidCodepoint {

If you'd like your case interpreted your way, then "the first non-whitespace token in a line" rule could be changed to "the first non-whitespace token after start-of-line or {". It would still work as intended for struct and enum fields.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Jul 5, 2018

I have often wanted these, but ultimately I prefer not supporting them and have found them annoying in practice in other environments.

Here are my reasons:

  1. It is nice to have a uniform convention.
  2. Something like //< encourages shorter comments so that things fit on one line. When using comments like this, I often find myself tweaking wording and trying to keep things "short and pithy" which I think is ultimately an antipattern.
@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Jul 5, 2018

@rfcbot fcp close

There hasn't been a lot of activity on this RFC since April and it seems like enough people have concerns about this feature that is not a "shoe-in" -- it also doesn't particularly fit the roadmap. Therefore, I move that we close the RFC.

Thank you very much @ishitatsuyuki for the suggestion, in any case. =)

@rfcbot

This comment has been minimized.

rfcbot commented Jul 5, 2018

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

rfcbot commented Sep 27, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

rfcbot commented Oct 7, 2018

The final comment period, with a disposition to close, as per the review above, is now complete.

By the power vested in me by Rust, I hereby close this RFC.

@rfcbot rfcbot added closed and removed disposition-close labels Oct 7, 2018

@rfcbot rfcbot closed this Oct 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment