Stream: rustdoc

Topic: block doc comments broken?


view this post on Zulip Jane Lusby (Mar 12 2021 at 18:18):

I just noticed in a recent PR on displaydoc that doc attributes from block comments seem to have changed how they're parsed or somethng

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:18):

https://github.com/yaahc/displaydoc/pull/22/checks?check_run_id=2091047604

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:18):

    /**
     * Variant4 wants to have a lot of lines
     *
     * Lets see how this works out for it
     */
    Variant4,

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:18):

This used to pass with this assert

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:18):

    assert_display(
        Happy::Variant4,
        "Variant4 wants to have a lot of lines\n\n Lets see how this works out for it",
    );

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:19):

but now we get this

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:19):

---- does_it_print stdout ----
thread 'does_it_print' panicked at 'assertion failed: `(left == right)`
  left: `"Variant4 wants to have a lot of lines\n\n Lets see how this works out for it"`,
 right: `"* Variant4 wants to have a lot of lines\n     *\n     * Lets see how this works out for it"`', tests/happy.rs:37:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:20):

the right and left are swapped because of how I implemented assert_display but the "right" side here is the output of the display method for Variant4

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:20):

and it is including the * and whitespace before the * on the doc comment

view this post on Zulip Jane Lusby (Mar 12 2021 at 18:21):

Is this the intended behaviour?

view this post on Zulip Joshua Nelson (Mar 12 2021 at 20:23):

Jane Lusby said:

Is this the intended behaviour?

most likely this is because of https://blog.guillaume-gomez.fr/articles/2020-11-11+New+doc+comment+handling+in+rustdoc

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:44):

makes sense

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:44):

looks like it wasn't intended then

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:44):

cc @GuillaumeGomez

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:45):

should I go ahead and open an issue for this?

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:45):

What wasn't intended?

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:45):

Let me check quickly

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:46):

Ah the "js-like" comments

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:47):

I don't remember what I did for them but I think that there isn't supposed to have "*" at the start of every line. I can't find a test about it though...

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:50):

So yes, simply remove the '*' at the start and it should work just fine.

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:50):

or you can use #[doc = "..."] too. Under-used but works very nicely. :)

view this post on Zulip Joshua Nelson (Mar 12 2021 at 21:51):

GuillaumeGomez said:

So yes, simply remove the '*' at the start and it should work just fine.

I think we should change rustdoc to do this automatically

view this post on Zulip Joshua Nelson (Mar 12 2021 at 21:52):

adding * at the start is the most common way to write these comments

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:58):

Not really. And also, those comments are pretty rare (I saw them "in the wild" only twice including today). I removed all the dark magic and I'm not sure it's a good idea to bring it back.

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:58):

It used to do so, right?

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:58):

older versions of rustc remove these despite it showing up as one attribute

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:58):

either way we're going to have to manually strip some of these on our side

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:58):

so its not super urgent

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:58):

That's a good question, I don't remember and I'm too tired to check now

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:58):

because we didn't catch this before it went into stable

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:59):

GuillaumeGomez said:

That's a good question, I don't remember and I'm too tired to check now

no i mean, I know for certain it did

view this post on Zulip Jane Lusby (Mar 12 2021 at 21:59):

our testsuite verifies this

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 21:59):

rustdoc was doing weird things on the doc comments before, so it's possible that it was doing something to handle this case too.

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:01):

Weirdly enough, my PR doesn't seem like it removed it: https://github.com/rust-lang/rust/pull/78400/files

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:02):

But the fact that there weren't tests to enforce this behavior is a bit annoying...

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:03):

So, back to this, I don't think we should handle this in rustdoc. I don't like having dark magic in doc comments (and in my mind, doc comments are very different than code comments), but since @Joshua Nelson disagrees, it seems worthwhile to ask to the other rustdoc team members what they think about it.

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:07):

(and in my mind, doc comments are very different than code comments),

I don't see how this is super relevant - the reason I think we should support this is because other documentation systems do, like javadoc and doxygen

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:07):

it seems weird to remove a feature and require the user to use a new syntax just because it's a little harder to maintain

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:08):

And I don't thinkgthat taking other systems as reference is always relevant. The problem here is that it'll allow stuff like this:

/// * hello
/// * aloa
/// * toudoum

And there, instead of a list you'll get the '*' removed.

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:08):

ok, so only do this for /** style comments

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:09):

One change in how we handle doc comments is always impacting the other ones unfortunately

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:09):

GuillaumeGomez said:

One chance in how we doc comments is always impacting the other ones unfortunately

I don't see why that has to be true? we could encode the original syntax in the metadata or just parse it on-demand

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:10):

The problem is that we can mix different kind of doc comments. That would mean that we should handle all kinds on their own instead of handling all of them as one

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:10):

I think you should see how it used to be done before the regression

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:13):

I see how it can be done, it's just that I'm not a big fan at all

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:13):

It brought many issues

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:13):

for example:

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:14):

/// hello
/**  this
  *  is
  * weid!
  */
///next

In your opinion, what happens there?

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:15):

how many spaces should we remove from the start of each line if we don't keep the "context" in all comment kinds?

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:16):

I think it should use the same rules as normal /// comments, but with the * treated as the start of the line

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:16):

Why should it? :3

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:16):

That creates inconsistency in itself

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:16):

now you're just being difficult lol

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:17):

A bit sorry :p

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:17):

It's just that I already had this conversation with Misdreavus before they implemented it. At the time I didn't see any reason to oppose because it seemed like a good improvement. In the end it was a nightmare.

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:18):

What we're discussing is less "advanced" but remains the same kind of handling. So before going full in, I really want to be sure it won't be something that we need to remove once again

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:18):

I really don't think we should be removing stable features ever

view this post on Zulip Joshua Nelson (Mar 12 2021 at 22:19):

whether or not this was a good idea the first time, we can't just throw our hands up and refuse to deal with it

view this post on Zulip GuillaumeGomez (Mar 12 2021 at 22:19):

I agree with you on that point. It's very surprising that we had no test for that though (which is why we only hear about it now)

view this post on Zulip GuillaumeGomez (Mar 13 2021 at 20:34):

Sorry for not understanding what you said... Way too tired to code I think. Things might get better in two weeks. It'll impact rustdoc I think so I keep you up to date if there is anything.

Anyway, you seem to be tired too. Try to take some time off too!

view this post on Zulip Joshua Nelson (Mar 13 2021 at 21:03):

I think you meant to send that as a DM :P but I appreciate it :heart:

view this post on Zulip GuillaumeGomez (Mar 15 2021 at 08:58):

Well, like I said: very tired. XD

view this post on Zulip Jane Lusby (Apr 07 2021 at 19:57):

following up on this, I'm opening an issue on this regression and it appears to have been introduced in rust 1.47

view this post on Zulip Jane Lusby (Apr 07 2021 at 19:57):

so a while ago now

view this post on Zulip Jane Lusby (Apr 07 2021 at 19:58):

looks like this blog post https://blog.guillaume-gomez.fr/articles/2020-11-11+New+doc+comment+handling+in+rustdoc is from 4 months ago so that maybe checks out??

view this post on Zulip Jane Lusby (Apr 07 2021 at 19:58):

at this point I'm not positive if this is the source of the regression since it happened further back than expected

view this post on Zulip Jane Lusby (Apr 07 2021 at 20:03):

follow up issue

view this post on Zulip Jane Lusby (Apr 07 2021 at 20:03):

https://github.com/rust-lang/rust/issues/83982

view this post on Zulip GuillaumeGomez (Apr 07 2021 at 20:12):

I honestly don't know how we can handle that... It seems to be the expected behaviour to me but if it worked before, it's a breaking change. Don't know why we ever supported the js-like doc comments. :-/

view this post on Zulip Manish Goregaokar (Apr 07 2021 at 20:14):

Wait I feel like we discussed breaking the js-comments and decided not to

view this post on Zulip Manish Goregaokar (Apr 07 2021 at 20:14):

but maybe i'm wrong

view this post on Zulip Manish Goregaokar (Apr 07 2021 at 20:14):

i feel like i recall discussing js comments

view this post on Zulip Joshua Nelson (Apr 07 2021 at 20:15):

GuillaumeGomez said:

I honestly don't know how we can handle that... It seems to be the expected behaviour to me but if it worked before, it's a breaking change. Don't know why we ever supported the js-like doc comments. :-/

I keep telling you you can just look at how we did it before

view this post on Zulip GuillaumeGomez (Apr 07 2021 at 20:27):

We changed the way we handle doc comments at least three times. So which before are you referring to? (and what we handle too)

view this post on Zulip Joshua Nelson (Apr 07 2021 at 20:29):

ok well it turns out this is completely unrelated to rustdoc so it's a moot point https://github.com/rust-lang/rust/issues/83982#issuecomment-815240014

view this post on Zulip GuillaumeGomez (Apr 07 2021 at 20:29):

And apparently I didn't break it from @Joshua Nelson 's comment. Phew, great work everyone! Time to sleep now :)

view this post on Zulip GuillaumeGomez (Apr 07 2021 at 20:30):

Manish Goregaokar said:

Wait I feel like we discussed breaking the js-comments and decided not to

I remember having this discussion too but don't remember when or the outcome


Last updated: Oct 11 2021 at 22:34 UTC