A better error message for misplaced doc comments #22547

Closed
steveklabnik opened this Issue Feb 19, 2015 · 20 comments

Comments

Projects
None yet
@steveklabnik
Member

steveklabnik commented Feb 19, 2015

This code:

enum Option<T> {
    None,    /// No value
    Some(T),    /// Some value `T`
}

yields this error:

 error: expected ident, found `}`
 }
 ^

I was writing an example to show this erorr, and still didn't realize this was the correct error for this problem.

@qwertie

This comment has been minimized.

Show comment
Hide comment
@qwertie

qwertie Apr 10, 2015

Can't we use an alternate syntax to document things on the right? e.g. //< Description. And yes, very surprising that what appears to be a comment would cause a syntax error.

qwertie commented Apr 10, 2015

Can't we use an alternate syntax to document things on the right? e.g. //< Description. And yes, very surprising that what appears to be a comment would cause a syntax error.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 10, 2015

Member

I would not be in favor of adding yet another kind of comment, personally.

Member

steveklabnik commented Apr 10, 2015

I would not be in favor of adding yet another kind of comment, personally.

@bjeanes

This comment has been minimized.

Show comment
Hide comment
@bjeanes

bjeanes Apr 11, 2015

New syntax doesn't feel needed for differentiating the types of comments. Either /// is preceded by whitespace or by content. It seems like a reasonable rule that comments following content apply to that content, not to something on the next line.

bjeanes commented Apr 11, 2015

New syntax doesn't feel needed for differentiating the types of comments. Either /// is preceded by whitespace or by content. It seems like a reasonable rule that comments following content apply to that content, not to something on the next line.

@tshepang

This comment has been minimized.

Show comment
Hide comment
@tshepang

tshepang Jun 22, 2015

Contributor

@bjeanes in your proposal, what would happen if we had this:

enum Option<T> {
    /// No value
    None,    /// I mean there really is No value
    Some(T), /// Some value `T`
}
Contributor

tshepang commented Jun 22, 2015

@bjeanes in your proposal, what would happen if we had this:

enum Option<T> {
    /// No value
    None,    /// I mean there really is No value
    Some(T), /// Some value `T`
}
@jtianling

This comment has been minimized.

Show comment
Hide comment
@jtianling

jtianling Sep 9, 2015

  1. I can't agree with @qwertie more, what's a surprise that a comment would cause a syntax error.
  2. If we count the percent how we comment the enum, the comments followed content would be much more than the comments occupy one line, because if you got a lot of enum, you didn't want to let a lot of comment bother you which occupy one line alone.
  1. I can't agree with @qwertie more, what's a surprise that a comment would cause a syntax error.
  2. If we count the percent how we comment the enum, the comments followed content would be much more than the comments occupy one line, because if you got a lot of enum, you didn't want to let a lot of comment bother you which occupy one line alone.
@swsch

This comment has been minimized.

Show comment
Hide comment
@swsch

swsch Sep 10, 2015

How about calling /// documentation instead of documentation comment?

/// provides documentation for the following identifier. If you provide documentation for the following identifier and that item is missing, it is a) an error and b) the message makes perfect sense.

"Comments" are ignored, "Documentation" is not, just like other source code.

swsch commented Sep 10, 2015

How about calling /// documentation instead of documentation comment?

/// provides documentation for the following identifier. If you provide documentation for the following identifier and that item is missing, it is a) an error and b) the message makes perfect sense.

"Comments" are ignored, "Documentation" is not, just like other source code.

@habemus-papadum

This comment has been minimized.

Show comment
Hide comment
@habemus-papadum

habemus-papadum Sep 19, 2015

I wholeheartedly agree with @swsch... More radically, it seems to me a nice feature for any new language would be to provide a way to stop conflating the various overloaded usages of old-school comments and instead provide dedicated syntax for 1) documentation directed toward end users, 2) documentation directed at maintainers (e.g. a comment next to a tricky piece of logic) and 3) syntax to have the parser ignore arbitrary fragments of code.

I wholeheartedly agree with @swsch... More radically, it seems to me a nice feature for any new language would be to provide a way to stop conflating the various overloaded usages of old-school comments and instead provide dedicated syntax for 1) documentation directed toward end users, 2) documentation directed at maintainers (e.g. a comment next to a tricky piece of logic) and 3) syntax to have the parser ignore arbitrary fragments of code.

@malnormalulo

This comment has been minimized.

Show comment
Hide comment
@malnormalulo

malnormalulo Oct 12, 2015

I think @swsch and @lilinjn are on the right track here. Comments are, traditionally, dead text; they do nothing. Rust's doc comments not only have semantic rules coupling them to code objects, but their contents are even compiled in some cases. What's more, their usefulness is not, like a traditional comment, found in marking up code as you look at it -- they are instead a wholly separate product, existing mostly so that you don't have to look at the code.

They are, in short, an entirely different thing than the comments denoted by that ubiquitous C syntax, and should not be shackled to that syntax or terminology simply because their Javadoc forebears are.

As for the immediate issue, I think @bjeanes's solution is elegant.

I think @swsch and @lilinjn are on the right track here. Comments are, traditionally, dead text; they do nothing. Rust's doc comments not only have semantic rules coupling them to code objects, but their contents are even compiled in some cases. What's more, their usefulness is not, like a traditional comment, found in marking up code as you look at it -- they are instead a wholly separate product, existing mostly so that you don't have to look at the code.

They are, in short, an entirely different thing than the comments denoted by that ubiquitous C syntax, and should not be shackled to that syntax or terminology simply because their Javadoc forebears are.

As for the immediate issue, I think @bjeanes's solution is elegant.

@andyg0808

This comment has been minimized.

Show comment
Hide comment
@andyg0808

andyg0808 Oct 24, 2015

@tshepang What would you think of treating documentation on the same line as code as if it was written immediately above the code? That is,

enum Option<T> {
    /// No value
    None,    /// I mean there really is No value
    Some(T), /// Some value `T`
}

would be treated as if it were

enum Option<T> {
    /// No value
    /// I mean there really is No value
    None,
    /// Some value `T`
    Some(T),
}

@tshepang What would you think of treating documentation on the same line as code as if it was written immediately above the code? That is,

enum Option<T> {
    /// No value
    None,    /// I mean there really is No value
    Some(T), /// Some value `T`
}

would be treated as if it were

enum Option<T> {
    /// No value
    /// I mean there really is No value
    None,
    /// Some value `T`
    Some(T),
}
@sleeparrow

This comment has been minimized.

Show comment
Hide comment
@sleeparrow

sleeparrow Nov 4, 2015

Contributor

@andyg0808 What then would happen with this?:

/// This is documentation for `Foo`
enum Foo { /// And I guess more docs for `Foo`?
    A,
    B,
}

It's not a hard issue to resolve, but I'm pointing out that it looks weird. Not just after a brace, but in your original example, too, in my opinion.

Contributor

sleeparrow commented Nov 4, 2015

@andyg0808 What then would happen with this?:

/// This is documentation for `Foo`
enum Foo { /// And I guess more docs for `Foo`?
    A,
    B,
}

It's not a hard issue to resolve, but I'm pointing out that it looks weird. Not just after a brace, but in your original example, too, in my opinion.

@vitoreiji

This comment has been minimized.

Show comment
Hide comment
@vitoreiji

vitoreiji Nov 18, 2015

@adambadawy, it does look weird, I agree. But it also solves a corner case, it's predictable and sort of makes sense.

An alternative would be a syntax error saying you can have either one or the other style, but not both. This would prevent this weirdness much more effectively than simply frowning upon the undesired behavior. Come to think of it, this is my favorite option.

@adambadawy, it does look weird, I agree. But it also solves a corner case, it's predictable and sort of makes sense.

An alternative would be a syntax error saying you can have either one or the other style, but not both. This would prevent this weirdness much more effectively than simply frowning upon the undesired behavior. Come to think of it, this is my favorite option.

@sleeparrow

This comment has been minimized.

Show comment
Hide comment
@sleeparrow

sleeparrow Nov 18, 2015

Contributor

An alternative would be a syntax error saying you can have either one or the other style, but not both. This would prevent this weirdness much more effectively than simply frowning upon the undesired behavior. Come to think of it, this is my favorite option.

The reason this issue was opened up is because there is a poor error message for a misplaced doc comment. Now we're discussing a design with more kinds of doc comments and more ambiguity, which could lead to

  • poorer docs, in the case of a doc comment that should have been a normal comment now being implicitly appended to the docs above it
  • poorer error messages, since if a doc comment could have documenting an item before or after it, it's more complex to tell the user where we expected an item
  • and more error messages, since we can't use both styles

depending, of course, on the chosen design.

I am strongly against adding another kind of documentation comment; it will buy us very little (choosing the position of the documentation in source code) and complicate the involved tooling needlessly.

Contributor

sleeparrow commented Nov 18, 2015

An alternative would be a syntax error saying you can have either one or the other style, but not both. This would prevent this weirdness much more effectively than simply frowning upon the undesired behavior. Come to think of it, this is my favorite option.

The reason this issue was opened up is because there is a poor error message for a misplaced doc comment. Now we're discussing a design with more kinds of doc comments and more ambiguity, which could lead to

  • poorer docs, in the case of a doc comment that should have been a normal comment now being implicitly appended to the docs above it
  • poorer error messages, since if a doc comment could have documenting an item before or after it, it's more complex to tell the user where we expected an item
  • and more error messages, since we can't use both styles

depending, of course, on the chosen design.

I am strongly against adding another kind of documentation comment; it will buy us very little (choosing the position of the documentation in source code) and complicate the involved tooling needlessly.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 18, 2015

Member

My feelings have basically entirely changed since opening this bug. Or rather, while it's an unfortunate error, I don't see how we can do any better without introducing more problems.

Member

steveklabnik commented Nov 18, 2015

My feelings have basically entirely changed since opening this bug. Or rather, while it's an unfortunate error, I don't see how we can do any better without introducing more problems.

@sleeparrow

This comment has been minimized.

Show comment
Hide comment
@sleeparrow

sleeparrow Nov 18, 2015

Contributor

My feelings have basically entirely changed since opening this bug. Or rather, while it's an unfortunate error, I don't see how we can do any better without introducing more problems.

It seems simple enough to me? If we find a doc comment and then something unexpected, the error message can be changed to something like "expected documentable item after doc comment".

Contributor

sleeparrow commented Nov 18, 2015

My feelings have basically entirely changed since opening this bug. Or rather, while it's an unfortunate error, I don't see how we can do any better without introducing more problems.

It seems simple enough to me? If we find a doc comment and then something unexpected, the error message can be changed to something like "expected documentable item after doc comment".

@heyimalex

This comment has been minimized.

Show comment
Hide comment
@heyimalex

heyimalex Dec 29, 2015

Maybe to avoid ambiguity we could make it so that a doc comment can't share a line with anything but whitespace? A grep (^.*[^[:space:]\/].*\/\/\/.*$) on this repo shows that it's not being done anywhere right now, and I can't think of a good reason why it would be, but maybe I'm missing something. Right now this compiles but is incorrect:

enum Option<T> {
    Some(T), /// Some value `T`
    None,
}

It's kind of contrived but I can imagine it happening in the wild. Sorry if there's some other reason this can't be done, I just noticed a reference to this issue in the rust book!

Maybe to avoid ambiguity we could make it so that a doc comment can't share a line with anything but whitespace? A grep (^.*[^[:space:]\/].*\/\/\/.*$) on this repo shows that it's not being done anywhere right now, and I can't think of a good reason why it would be, but maybe I'm missing something. Right now this compiles but is incorrect:

enum Option<T> {
    Some(T), /// Some value `T`
    None,
}

It's kind of contrived but I can imagine it happening in the wild. Sorry if there's some other reason this can't be done, I just noticed a reference to this issue in the rust book!

@cooperra

This comment has been minimized.

Show comment
Hide comment
@cooperra

cooperra Dec 30, 2015

👍

I like forcing documentation comments to be on their own line to eliminate ambiguity. Right now end-of-line ones are unintuitive: they cause an error or, worse, describe the wrong item. I think we should either get rid of them or fix them.

In the short term, we can just write a better error message (as @adambadawy suggested) and force documentation comments to be on their own line. It would avoid a lot of complexity. But it would be nice to ultimately allow end-of-line documentation comments. Sometimes it's more convenient to describe something on the same line. Plus it's intuitive; people will probably attempt to do it anyway.

I like @vitoreiji's suggestion if we decide to keep end-of-line documentation comments:

  1. Documentation comments on their own line describe what appears on the next line.
  2. Documentation comments that share a line with anything else describe that content.
  3. It's illegal to describe something by using both forms.

In the error message, complain that multiple documentation comments describe the content and suggest that one of them should become a normal comment.

It's another error message, but it's one that people will rarely encounter if ever. Is there any reason one would want to use both forms at once?

Examples:

OK:

enum Option<T> {
    /// Describes Some(T)
    Some(T),
    None,
}

OK:

enum Option<T> {
    Some(T), /// Describes Some(T)
    None,
}

Bad:

enum Option<T> {
    /// Describes Some(T)
    Some(T), /// Also attempts to describe Some(T)
    None,
}

The only downside I can think of is it would silently break comments that rely on the weird existing behavior.

If we force them to stand alone first, then they would break loudly. Then, once everyone has fixed those errors and moved the comments to their own lines, we could add the proper end of line comments. This way, nothing would quietly break.

👍

I like forcing documentation comments to be on their own line to eliminate ambiguity. Right now end-of-line ones are unintuitive: they cause an error or, worse, describe the wrong item. I think we should either get rid of them or fix them.

In the short term, we can just write a better error message (as @adambadawy suggested) and force documentation comments to be on their own line. It would avoid a lot of complexity. But it would be nice to ultimately allow end-of-line documentation comments. Sometimes it's more convenient to describe something on the same line. Plus it's intuitive; people will probably attempt to do it anyway.

I like @vitoreiji's suggestion if we decide to keep end-of-line documentation comments:

  1. Documentation comments on their own line describe what appears on the next line.
  2. Documentation comments that share a line with anything else describe that content.
  3. It's illegal to describe something by using both forms.

In the error message, complain that multiple documentation comments describe the content and suggest that one of them should become a normal comment.

It's another error message, but it's one that people will rarely encounter if ever. Is there any reason one would want to use both forms at once?

Examples:

OK:

enum Option<T> {
    /// Describes Some(T)
    Some(T),
    None,
}

OK:

enum Option<T> {
    Some(T), /// Describes Some(T)
    None,
}

Bad:

enum Option<T> {
    /// Describes Some(T)
    Some(T), /// Also attempts to describe Some(T)
    None,
}

The only downside I can think of is it would silently break comments that rely on the weird existing behavior.

If we force them to stand alone first, then they would break loudly. Then, once everyone has fixed those errors and moved the comments to their own lines, we could add the proper end of line comments. This way, nothing would quietly break.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Dec 30, 2015

Member

Yes, this would be a breaking change; though I'm not sure there's any code that is correct that it would actually break.

That path would certainly require an RFC though.

Member

steveklabnik commented Dec 30, 2015

Yes, this would be a breaking change; though I'm not sure there's any code that is correct that it would actually break.

That path would certainly require an RFC though.

@AlexKnauth

This comment has been minimized.

Show comment
Hide comment
@AlexKnauth

AlexKnauth Jan 11, 2016

But changing the error message like @adambadawy suggested in #22547 (comment) wouldn't be a breaking change. An error message like "expected documentable item after doc comment" is much easier to understand.

But changing the error message like @adambadawy suggested in #22547 (comment) wouldn't be a breaking change. An error message like "expected documentable item after doc comment" is much easier to understand.

@Timmmm

This comment has been minimized.

Show comment
Hide comment
@Timmmm

Timmmm Dec 28, 2016

I have actually read this issue twice before and it still took me around 10 minutes to figure out why I was getting this error. Specifically I had this code:

struct Foo {
};

impl Foo {

    /// bar does something
    fn bar(&self) {
        // ...
    }

    /// baz does another thing
    /* fn baz(&self) {
        // ...
    } */
}

That is, normal code but I had commented out one function. It gives this error:

error: expected one of `const`, `default`, `extern`, `fn`, `pub`, `type`, or `unsafe`, found `}`

This is really confusing. Would it really be that hard to have the error say something like this?

error: expected documentable item (one of `const`, `default`, `extern`, `fn`, `pub`, `type`, or `unsafe`) after documentation comment, found `}`

And then point to the /// comment that triggered it. This would be a huge improvement and I guarantee it will save many people a lot of time. And Happy Christmas!

Edit/PS: Obviously in this example code it is vaguely clear what the cause is... ish. But of course my real code is significantly more complex and it was extremely far from obvious what was causing the problem. Especially as one does not normally consider comments as affecting whether compilation succeeds or not. In fact I might go so far as to say compilation should still succeed in this case. I.e. it should be a warning.

Timmmm commented Dec 28, 2016

I have actually read this issue twice before and it still took me around 10 minutes to figure out why I was getting this error. Specifically I had this code:

struct Foo {
};

impl Foo {

    /// bar does something
    fn bar(&self) {
        // ...
    }

    /// baz does another thing
    /* fn baz(&self) {
        // ...
    } */
}

That is, normal code but I had commented out one function. It gives this error:

error: expected one of `const`, `default`, `extern`, `fn`, `pub`, `type`, or `unsafe`, found `}`

This is really confusing. Would it really be that hard to have the error say something like this?

error: expected documentable item (one of `const`, `default`, `extern`, `fn`, `pub`, `type`, or `unsafe`) after documentation comment, found `}`

And then point to the /// comment that triggered it. This would be a huge improvement and I guarantee it will save many people a lot of time. And Happy Christmas!

Edit/PS: Obviously in this example code it is vaguely clear what the cause is... ish. But of course my real code is significantly more complex and it was extremely far from obvious what was causing the problem. Especially as one does not normally consider comments as affecting whether compilation succeeds or not. In fact I might go so far as to say compilation should still succeed in this case. I.e. it should be a warning.

@mgeisler

This comment has been minimized.

Show comment
Hide comment
@mgeisler

mgeisler Jun 5, 2017

For completeness, let me add that the error message has been updated at some point. You now see:

error[E0585]: found a documentation comment that doesn't document anything
  --> src/lib.rs:10:17
   |
10 |     Some(T),    /// Some value `T`
   |                 ^^^^^^^^^^^^^^^^^^
   |
   |
   = help: doc comments must come before what they document, maybe a comment
     was intended with `//`?

for the example given in the first comment. This is with Rust 1.17.

Also, it was suggested a couple of times above to allow documentation following the item it's documenting. I think that is a bad idea because it doesn't scale very well. Basically, you quickly end up with very long lines. Go uses that system for the so-called struct tags and in my experience, it's been awkward to use.

Vertical space is cheaper than horizontal space, so I think it's good if Rust at least makes that the default way to format things.

mgeisler commented Jun 5, 2017

For completeness, let me add that the error message has been updated at some point. You now see:

error[E0585]: found a documentation comment that doesn't document anything
  --> src/lib.rs:10:17
   |
10 |     Some(T),    /// Some value `T`
   |                 ^^^^^^^^^^^^^^^^^^
   |
   |
   = help: doc comments must come before what they document, maybe a comment
     was intended with `//`?

for the example given in the first comment. This is with Rust 1.17.

Also, it was suggested a couple of times above to allow documentation following the item it's documenting. I think that is a bad idea because it doesn't scale very well. Basically, you quickly end up with very long lines. Go uses that system for the so-called struct tags and in my experience, it's been awkward to use.

Vertical space is cheaper than horizontal space, so I think it's good if Rust at least makes that the default way to format things.

@hngnaig hngnaig referenced this issue in hngnaig/notes Jun 22, 2017

Open

Tạo document ở Rust #1

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