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

Simplify logic for unindenting doc comments #125396

Closed
wants to merge 1 commit into from

Conversation

camelid
Copy link
Member

@camelid camelid commented May 22, 2024

Related to #65802 (but does not resolve it).

This has almost the same behavior, except that it completely ignores raw
doc comments (i.e., #[doc = "..."]) for the purposes of computing
indentation.

@rustbot
Copy link
Collaborator

rustbot commented May 22, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 22, 2024
@camelid camelid marked this pull request as ready for review May 22, 2024 06:38
@camelid
Copy link
Member Author

camelid commented May 22, 2024

r? @GuillaumeGomez

@camelid camelid added the A-markdown-parsing Area: Markdown parsing for doc-comments label May 22, 2024
@camelid
Copy link
Member Author

camelid commented May 22, 2024

We should decide if this change makes sense and also if it could cause stability issues.

@camelid
Copy link
Member Author

camelid commented May 22, 2024

I ran a diff of the stdlib docs files between this version and HEAD rustdoc and found no differences.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

This has almost the same behavior, except that it completely ignores raw
doc comments (i.e., `#[doc = "..."]`) for the purposes of computing
indentation.
@@ -280,11 +280,11 @@ LL | | /// level changes.
| |______________________^
|
= help: the opening backtick of a previous inline code may be missing
change: The Subscriber` may be accessed by calling [`WeakDispatch::upgrade`],
to this: The `Subscriber` may be accessed by calling [`WeakDispatch::upgrade`],
change: The Subscriber` may be accessed by calling [`WeakDispatch::upgrade`],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems surprising, no?

Copy link
Member Author

@camelid camelid May 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange indeed, especially because (based on the logging I'm looking at), the indent is correctly assessed to be 1. Looking into this more... EDIT: Never mind, that was a different line of code, but still weird.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so here's the reason. It's because this code is wrapped in a macro (which is just the identity function, returning exactly its input). However, this turns the sugared doc attrs into raw doc attrs; thus, the indent is computed as 0. This also makes the lint's suggestion go into the "uglier" path because source_span_for_markdown_range requires all inputs to be sugared doc attrs.

I don't know if there's something we want to change here. It does seem unfortunate that the indentation rules will change if the docs are passed through a macro, although that does seem like a rare occurrence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you prefer then! r=me if it sounds good to you as is. ;)

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the ui test change, looks good to me!

@camelid
Copy link
Member Author

camelid commented May 27, 2024

I wonder if we should go through FCP since this changes rustdoc's handling of doc comments? We also should decide if these are truly the rules we want rustdoc to follow for unindenting. E.g., now, if rustdoc sees

///     assert!(true);
fn foo() {}

It will render as

assert!(true);

while previously it would be

assert!(true);

However,

/// Foo
///
///     assert!(true);
fn foo() {}

will render as

Foo

assert!(true);

I think this is a reasonable behavior, the summary being that we take the minimum indentation across all sugar-doc lines and consider that to be the indentation of the whole block. Alternatively, we could use the first line, or we could limit the amount of indentation we take off to be at most 1 space. Thoughts?

@rfcbot merge

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 27, 2024
@camelid

This comment has been minimized.

@camelid camelid removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 27, 2024
@camelid

This comment has been minimized.

1 similar comment
@camelid

This comment has been minimized.

@camelid camelid closed this May 28, 2024
@camelid camelid reopened this May 28, 2024
@camelid

This comment has been minimized.

@compiler-errors

This comment has been minimized.

@fmease

This comment has been minimized.

@rfcbot

This comment has been minimized.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 28, 2024
@camelid
Copy link
Member Author

camelid commented May 28, 2024

@rfcbot merge

@rfcbot
Copy link

rfcbot commented May 28, 2024

Team member @camelid has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 28, 2024
@camelid
Copy link
Member Author

camelid commented May 28, 2024

To summarize the new behavior: We only consider sugared doc comments (///, rather than #[doc] or /// that's passed through a macro) when calculating indentation. We pick the least-indented line and remove that amount of indentation from every line coming from sugared doc comments (raw doc comments are left alone).

@GuillaumeGomez
Copy link
Member

I don't think removing indent when the only indent present can be formatted as a codeblock is a good idea or a desired behaviour. If the indent is < 4, then I think it's ok, but otherwise it enters in conflict with the markdown spec and I don't think we should do that.

@notriddle
Copy link
Contributor

I'm not sure if we should be changing this. It's trading one weird corner case (I admit it's weird that non-sugared doc comments have indentation stripped) for a different, but equally weird corner case (sugared doc comments passed through macros aren't having their indentation stripped).

The table parsing bug is solved by the newer version of pulldown-cmark. Once we do the upgrade and pulldown-cmark/pulldown-cmark#665 gets deployed in rustdoc, it'll be fixed without having to change this anyway.

@camelid
Copy link
Member Author

camelid commented May 29, 2024

I understand. This PR doesn't even fix the table parsing issue. My goal with it was to simplify the logic so we know what exactly we're guaranteeing with respect to unindenting. The current code is somewhat obscure. Instead of making this change, should we discuss what exactly we expect the behavior to be and make sure the code doesn't have edge cases?

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented May 29, 2024

@camelid proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 29, 2024
@bors
Copy link
Contributor

bors commented Jun 24, 2024

☔ The latest upstream changes (presumably #126788) made this pull request unmergeable. Please resolve the merge conflicts.

@camelid
Copy link
Member Author

camelid commented Jul 8, 2024

Original bug was fixed in pulldown and then by #127127. I still think we should figure out a more principled approach for unindenting, but it's not a big deal.

@camelid camelid closed this Jul 8, 2024
@camelid camelid deleted the simplify-unindent branch July 8, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-markdown-parsing Area: Markdown parsing for doc-comments S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants