Use `strip_{prefix,suffix}` in rustdoc #74094

tesuji · 2020-07-06T10:45:37Z

cc @Mark-Simulacrum @jyn514 for rustdoc part

rust-highfive · 2020-07-06T10:45:40Z

(rust_highfive has picked a reviewer for you, use r? to override)

GuillaumeGomez · 2020-07-06T11:08:09Z

src/librustc_ast/util/comments.rs

+    // starts with `/**` and ends with `*/`. However, this caused an ICE
+    // because some code assumed that the length of a doc comment is at least 5.
+    let yes = match s.as_bytes() {
+        [b'/', b'*', b'*', c, _, ..] => *c != b'*',


I'm surprised that we still allow /** to be a doc comment. Is it something allowed officially? I never saw it anywhere.

Here is one: https://github.com/facebook/relay/blob/07ccab7cc637f51f2f15fc75ed824d1de8ede72f/compiler/crates/schema/src/parser.rs#L23-L26

GuillaumeGomez · 2020-07-06T11:11:01Z

Also, what case is it fixing? Because if it's fixing anything, please add a test. :)

Mark-Simulacrum

I am concerned that it seems like at least some of my comments from the previous PR have not been addressed. I would prefer not to have to review again or try to copy my comments over myself...

Mark-Simulacrum · 2020-07-06T11:36:25Z

src/librustc_ast/util/comments.rs

-        || (s.starts_with("/**") && is_block_doc_comment(s))
-        || s.starts_with("/*!")
+fn is_doc_comment(s: &str) -> bool {
+    is_line_doc_comment(s) || is_block_doc_comment(s)


I left a review on the previous PR here - this is a change in behavior due to checking the length for e.g. /*!. I would ask that any changes in behavior are at least split into their own commit in this PR and as such clearly identifiable, ideally, they would be made in a separate PR. This one for example seems to have backwards compatibility concerns.

tesuji · 2020-07-06T10:50:10Z

src/librustc_ast/util/comments.rs

 }

-// FIXME(#64197): Try to privatize this again.


cc https://github.com/rust-lang/rust/pull/74048/files#r449877369
I made it private now by using is_line_doc_comment in test.

I continue to prefer that we not conflate the move to using strip_{prefix,suffix} with other changes, just because it makes it harder to review and increases the chances of missing something. At the very least combining them in the same commit is quite frustrating for things like git blame later on.

tesuji · 2020-07-06T13:53:19Z

src/librustc_ast/util/comments.rs

-        || (s.starts_with("/**") && is_block_doc_comment(s))
-        || s.starts_with("/*!")
+fn is_doc_comment(s: &str) -> bool {
+    is_line_doc_comment(s) || is_block_doc_comment(s)


re https://github.com/rust-lang/rust/pull/74048/files#r449877504
It is not clear to me what is the input of is_doc_comment?
Does it like the followings?

/// ...

//! ..

/** ... */

/*! ... */

/**

/*!

Also there are no testcases cover this function?

tesuji · 2020-07-06T14:05:23Z

src/librustc_ast/util/comments.rs

+    // starts with `/**` and ends with `*/`. However, this caused an ICE
+    // because some code assumed that the length of a doc comment is at least 5.
+    let yes = match s.as_bytes() {
+        [b'/', b'*', b'*', c, _, ..] => *c != b'*',


Here is one: https://github.com/facebook/relay/blob/07ccab7cc637f51f2f15fc75ed824d1de8ede72f/compiler/crates/schema/src/parser.rs#L23-L26

tesuji · 2020-07-06T14:16:19Z

src/librustc_ast/util/comments.rs

-    if comment.starts_with("/*") {
-        let lines =
-            comment[3..comment.len() - 2].lines().map(|s| s.to_string()).collect::<Vec<String>>();
+    match comment
+        .strip_prefix("/**")
+        .or_else(|| comment.strip_prefix("/*!"))
+        .and_then(|s| s.strip_suffix("*/"))


re https://github.com/rust-lang/rust/pull/74048/files#r449877722

I am quite sure that with block doc comment must start with either /** or /*! and must end with */
as the reference states: https://doc.rust-lang.org/nightly/reference/comments.html#doc-comments

tesuji · 2020-07-06T14:19:09Z

src/librustdoc/clean/utils.rs

+                };
+                // SAFETY: `new_len` should be in between char boundary
+                unsafe {
+                    s.as_mut_vec().set_len(new_len);


re https://github.com/rust-lang/rust/pull/74048/files#r449880719

String::truncate is cheap but there is panicking branch not elided: https://rust.godbolt.org/z/VTZ3Le
Also miri confirms that this is safe.

tesuji · 2020-07-06T14:31:56Z

src/librustdoc/html/markdown.rs

-// have no easy way of removing a potential single space after the hashes, which
-// is done in the single # case. This inconsistency seems okay, if non-ideal. In
-// order to fix it we'd have to iterate to find the first non-# character, and
-// then reallocate to remove it; which would make us return a String.


re https://github.com/rust-lang/rust/pull/74048/files#r449881015

I have no idea what this comment is talking about.
Does it replace "##" with "#" as the old code did?
So should I keep the old FIXME?

Oh you wrote this code: ffe12b1 and has a test here: https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/issue-41783.rs

tesuji · 2020-07-06T14:40:00Z

src/librustdoc/html/markdown.rs

@@ -776,10 +775,10 @@ impl LangString {
                    data.no_run = true;
                }
                x if x.starts_with("edition") => {
-                    data.edition = x[7..].parse::<Edition>().ok();
+                    data.edition = x.strip_prefix("edition").unwrap().parse::<Edition>().ok();


re https://github.com/rust-lang/rust/pull/74048/files#r449881344

I changed the code to use strip_prefix to avoid panicking code caused by slicing.

tesuji · 2020-07-06T14:45:28Z

src/librustdoc/html/render.rs

                    // Ends with "\\" (it's the case for the last added crate line)
-                    ret.push(line[..line.len() - 1].to_string());
+                    ret.push(head.to_string());


re https://github.com/rust-lang/rust/pull/74048/files#r449881783

The comment above this statement confirms that the else case line ends with \\.
cc @GuillaumeGomez who wrote this code in b4fb306

Normally, all lines should end with ",\" (except the last one which ends with "\").

So does I reserve old behavior here?

tesuji · 2020-07-06T14:50:06Z

src/librustdoc/passes/collect_intra_doc_links.rs

-                    link.trim_start_matches("derive@")
-                } else if link.ends_with('!') {
-                    kind = Some(MacroNS);
-                    link.trim_end_matches('!')


re https://github.com/rust-lang/rust/pull/74048/files#r449882381

Quoting intra doc link RFC:

In links to macros,
the link label can end with a !,
e.g., Look at the [FOO!] macro. You can alternatively use a macro@ prefix,
e.g. [macro@foo]

I don't know if multiple ! is allowed in that RFC. cc @Manishearth

Only a single one

tesuji · 2020-07-06T14:57:18Z

I replied to most review comments about rustdoc (#74048).

GuillaumeGomez · 2020-07-06T15:00:35Z

src/librustdoc/html/render.rs

-                    // Ends with "\\" (it's the case for the last added crate line)
-                    ret.push(line[..line.len() - 1].to_string());
+                // Ends with "\\" (it's the case for the last added crate line)
+                if let Some(head) = line.strip_suffix(",\\").or_else(|| line.strip_suffix("\\")) {


Like this seems fine to me, thanks!

Mark-Simulacrum · 2020-07-06T15:38:27Z

I continue to be pretty strongly opposed to making both functional changes, regardless of whether we think they're fine, and "pure cleanup" of using the new functions from std in the same PR. At the very least, I am not confident that my original review caught all such cases -- and without separate commits at least for them, it makes it hard to evaluate whether the breaking changes being made are acceptable or not.

I will try to reply to your comments shortly. (Edit: it looks like all of the ones on this PR are trying to justify making unrelated changes, and I'd just prefer that we don't make them at all in this PR).

tesuji · 2020-07-06T15:50:57Z

Many behavior changes are either:

replacing trim_*_matches with strip_*?
make it clear what we strip: Use strip_{prefix,suffix} in rustdoc #74094 (comment)

The first case, most of them I think the intent is really clear as there is an if condition check if str starts/ends with pattern only once.
The second case is needed if we want to use str::strip_* methods and leverage its usefulness.

Mark-Simulacrum · 2020-07-06T16:00:19Z

I don't disagree that it's likely that the behavior changes are benign, but I am personally against landing a PR that has a bunch of (very minor) breaking changes to various parts of the codebase without any evaluation of what those changes are exactly. I also don't really trust myself to have caught every case where there has been a behavior change, especially when some of those are being made intentionally it seems (so it's not just "accidents" that review needs to catch).

I think it's generally a good idea to have a breaking change "set" at least split out into its own PR and then we can evaluate whether the code readability of strip_prefix is worth potentially breaking users, even if we unintentionally were accepting behavior of some kind before. Making that evaluation without a list of the changes being made is quite difficult.

Manishearth · 2020-07-06T16:25:05Z

I agree with @Mark-Simulacrum , we should not mix behavior changes with refactors like this.

The changes being benign is already false: you introduced a bug in #74094 (comment)

bors · 2020-07-07T05:22:46Z

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

Muirrum · 2020-07-24T00:39:23Z

@lzutao This is a triage bump

tesuji · 2020-07-24T00:59:21Z

This is blocked on other PR. Closing for now!

rust-highfive assigned GuillaumeGomez Jul 6, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2020

GuillaumeGomez reviewed Jul 6, 2020

View reviewed changes

Mark-Simulacrum requested changes Jul 6, 2020

View reviewed changes

tesuji commented Jul 6, 2020

View reviewed changes

tesuji force-pushed the strip-rustdoc branch from bddd983 to 061d83a Compare July 6, 2020 14:52

GuillaumeGomez reviewed Jul 6, 2020

View reviewed changes

tesuji force-pushed the strip-rustdoc branch from 061d83a to 7901876 Compare July 6, 2020 15:24

Use strip_{prefix,suffix} in rustdoc

9382f07

tesuji force-pushed the strip-rustdoc branch from 7901876 to 9382f07 Compare July 6, 2020 16:00

GuillaumeGomez mentioned this pull request Jul 9, 2020

rustdoc incorrectly handles ## #73623

Closed

tesuji mentioned this pull request Jul 10, 2020

Refactor doc comment parsing #74209

Closed

Muirrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2020

tesuji closed this Jul 24, 2020

tesuji deleted the strip-rustdoc branch June 8, 2024 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `strip_{prefix,suffix}` in rustdoc #74094

Use `strip_{prefix,suffix}` in rustdoc #74094

tesuji commented Jul 6, 2020

rust-highfive commented Jul 6, 2020

GuillaumeGomez Jul 6, 2020 •

edited

Loading

tesuji Jul 6, 2020

GuillaumeGomez Jul 9, 2020

GuillaumeGomez commented Jul 6, 2020

Mark-Simulacrum left a comment

Mark-Simulacrum Jul 6, 2020

tesuji Jul 6, 2020

Mark-Simulacrum Jul 6, 2020

tesuji Jul 6, 2020

tesuji Jul 6, 2020

tesuji Jul 6, 2020

tesuji Jul 6, 2020

tesuji Jul 6, 2020

tesuji Jul 6, 2020

tesuji Jul 6, 2020

tesuji Jul 6, 2020

tesuji Jul 6, 2020 •

edited

Loading

GuillaumeGomez Jul 6, 2020

tesuji Jul 6, 2020

Manishearth Jul 6, 2020

tesuji Jul 6, 2020

Manishearth Jul 6, 2020

tesuji commented Jul 6, 2020

GuillaumeGomez Jul 6, 2020

Mark-Simulacrum commented Jul 6, 2020 •

edited

Loading

tesuji commented Jul 6, 2020

Mark-Simulacrum commented Jul 6, 2020

Manishearth commented Jul 6, 2020

bors commented Jul 7, 2020

Muirrum commented Jul 24, 2020

tesuji commented Jul 24, 2020

Use strip_{prefix,suffix} in rustdoc #74094

Use strip_{prefix,suffix} in rustdoc #74094

Conversation

tesuji commented Jul 6, 2020

rust-highfive commented Jul 6, 2020

GuillaumeGomez Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 6, 2020

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tesuji Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tesuji commented Jul 6, 2020

Choose a reason for hiding this comment

Mark-Simulacrum commented Jul 6, 2020 • edited Loading

tesuji commented Jul 6, 2020

Mark-Simulacrum commented Jul 6, 2020

Manishearth commented Jul 6, 2020

bors commented Jul 7, 2020

Muirrum commented Jul 24, 2020

tesuji commented Jul 24, 2020

Use `strip_{prefix,suffix}` in rustdoc #74094

Use `strip_{prefix,suffix}` in rustdoc #74094

GuillaumeGomez Jul 6, 2020 •

edited

Loading

tesuji Jul 6, 2020 •

edited

Loading

Mark-Simulacrum commented Jul 6, 2020 •

edited

Loading