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

Use strip_{prefix,suffix} in rustdoc #74094

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions src/bootstrap/doc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,22 +461,21 @@ impl Step for Std {

builder.run(&mut cargo.into());
};
let krates = ["alloc", "core", "std", "proc_macro", "test"];
for krate in &krates {
static KRATES: &[&str] = &["alloc", "core", "std", "proc_macro", "test"];
for krate in KRATES {
run_cargo_rustdoc_for(krate);
}
builder.cp_r(&my_out, &out);

// Look for src/libstd, src/libcore etc in the `x.py doc` arguments and
// open the corresponding rendered docs.
for path in builder.paths.iter().map(components_simplified) {
if path.get(0) == Some(&"src")
&& path.get(1).map_or(false, |dir| dir.starts_with("lib"))
{
let requested_crate = &path[1][3..];
if krates.contains(&requested_crate) {
let index = out.join(requested_crate).join("index.html");
open(builder, &index);
if let ["src", path, ..] = path.as_slice() {
if let Some(krate) = path.strip_prefix("lib") {
if KRATES.contains(&krate) {
let index = out.join(krate).join("index.html");
open(builder, &index);
}
}
}
}
Expand Down
60 changes: 34 additions & 26 deletions src/librustc_ast/util/comments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,27 +29,31 @@ pub struct Comment {
}

pub fn is_line_doc_comment(s: &str) -> bool {
let res = (s.starts_with("///") && *s.as_bytes().get(3).unwrap_or(&b' ') != b'/')
|| s.starts_with("//!");
debug!("is {:?} a doc comment? {}", s, res);
res
let yes = match s.as_bytes() {
[b'/', b'/', b'/', c, ..] => *c != b'/',
[b'/', b'/', b'/', ..] => true,
[b'/', b'/', b'!', ..] => true,
_ => false,
};
debug!("is {:?} a line doc comment? {}", s, yes);
yes
}

pub fn is_block_doc_comment(s: &str) -> bool {
// Prevent `/**/` from being parsed as a doc comment
let res = ((s.starts_with("/**") && *s.as_bytes().get(3).unwrap_or(&b' ') != b'*')
|| s.starts_with("/*!"))
&& s.len() >= 5;
debug!("is {:?} a doc comment? {}", s, res);
res
// Previously, `/**/` was incorrectly regarded as a doc comment because it
// 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'*',
Copy link
Member

@GuillaumeGomez GuillaumeGomez Jul 6, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

TIL. :)

[b'/', b'*', b'!', _, _, ..] => true,
_ => false,
};
debug!("is {:?} a block doc comment? {}", s, yes);
yes
}

// FIXME(#64197): Try to privatize this again.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

pub fn is_doc_comment(s: &str) -> bool {
(s.starts_with("///") && is_line_doc_comment(s))
|| s.starts_with("//!")
|| (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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

  • /// ...
  • //! ..
  • /** ... */
  • /*! ... */
  • /**
  • /*!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also there are no testcases cover this function?

}

pub fn doc_comment_style(comment: &str) -> ast::AttrStyle {
Expand Down Expand Up @@ -127,22 +131,26 @@ pub fn strip_doc_comment_decoration(comment: &str) -> String {
const ONELINERS: &[&str] = &["///!", "///", "//!", "//"];

for prefix in ONELINERS {
if comment.starts_with(*prefix) {
return (&comment[prefix.len()..]).to_string();
if let Some(tail) = comment.strip_prefix(*prefix) {
return tail.to_owned();
}
}

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("*/"))
Comment on lines -135 to +142
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
Some(doc) => {
let lines = doc.lines().map(|s| s.to_string()).collect::<Vec<String>>();

let lines = vertical_trim(lines);
let lines = horizontal_trim(lines);
let lines = vertical_trim(lines);
let lines = horizontal_trim(lines);

return lines.join("\n");
lines.join("\n")
}
_ => panic!("not a doc-comment: {}", comment),
}

panic!("not a doc-comment: {}", comment);
}

/// Returns `None` if the first `col` chars of `s` contain a non-whitespace char.
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_expand/parse/lexer/tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use rustc_ast::token::{self, Token, TokenKind};
use rustc_ast::util::comments::is_doc_comment;
use rustc_ast::util::comments::is_line_doc_comment;
use rustc_ast::with_default_globals;
use rustc_data_structures::sync::Lrc;
use rustc_errors::{emitter::EmitterWriter, Handler};
Expand Down Expand Up @@ -225,9 +225,9 @@ fn literal_suffixes() {

#[test]
fn line_doc_comments() {
assert!(is_doc_comment("///"));
assert!(is_doc_comment("/// blah"));
assert!(!is_doc_comment("////"));
assert!(is_line_doc_comment("///"));
assert!(is_line_doc_comment("/// blah"));
assert!(!is_line_doc_comment("////"));
}

#[test]
Expand Down
14 changes: 8 additions & 6 deletions src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -481,12 +481,14 @@ pub fn print_const(cx: &DocContext<'_>, n: &'tcx ty::Const<'_>) -> String {
_ => {
let mut s = n.to_string();
// array lengths are obviously usize
if s.ends_with("_usize") {
let n = s.len() - "_usize".len();
s.truncate(n);
if s.ends_with(": ") {
let n = s.len() - ": ".len();
s.truncate(n);
if let Some(head) = s.strip_suffix("_usize") {
let new_len = match head.strip_suffix(": ") {
None => head.len(),
Some(hhead) => hhead.len(),
};
// SAFETY: `new_len` should be in between char boundary
unsafe {
s.as_mut_vec().set_len(new_len);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}
s
Expand Down
36 changes: 22 additions & 14 deletions src/librustdoc/html/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,16 +133,24 @@ impl<'a> Line<'a> {
// then reallocate to remove it; which would make us return a String.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fn map_line(s: &str) -> Line<'_> {
let trimmed = s.trim();
if trimmed.starts_with("##") {
Line::Shown(Cow::Owned(s.replacen("##", "#", 1)))
} else if trimmed.starts_with("# ") {
// # text
Line::Hidden(&trimmed[2..])
} else if trimmed == "#" {
// We cannot handle '#text' because it could be #[attr].
Line::Hidden("")
} else {
Line::Shown(Cow::Borrowed(s))
match trimmed.strip_prefix("#") {
Some(tail) => match tail.strip_prefix("#") {
// `##text` rendered as `#text`.
Some(_) => Line::Shown(tail.into()),
None => {
if tail.is_empty() {
// `#` will be hidden.
Line::Hidden("")
} else if let Some(text) = tail.strip_prefix(' ') {
// `# text` will be hidden.
Line::Hidden(text)
} else {
// `#text` will be shown as it could be `#[attr]`
Line::Shown(s.into())
}
}
},
None => Line::Shown(s.into()),
}
}

Expand Down Expand Up @@ -754,7 +762,7 @@ impl LangString {
}
x if x.starts_with("ignore-") => {
if enable_per_target_ignores {
ignores.push(x.trim_start_matches("ignore-").to_owned());
ignores.push(x.strip_prefix("ignore-").unwrap().to_owned());
seen_rust_tags = !seen_other_tags;
}
}
Expand All @@ -776,10 +784,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();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
x if allow_error_code_check && x.starts_with('E') && x.len() == 5 => {
if x[1..].parse::<u32>().is_ok() {
x if allow_error_code_check && x.len() == 5 && x.starts_with('E') => {
if x.strip_prefix('E').unwrap().parse::<u32>().is_ok() {
data.error_codes.push(x.to_owned());
seen_rust_tags = !seen_other_tags || seen_rust_tags;
} else {
Expand Down
8 changes: 3 additions & 5 deletions src/librustdoc/html/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -810,11 +810,9 @@ themePicker.onblur = handleThemeButtonsBlur;
if line.starts_with(&format!("\"{}\"", krate)) {
continue;
}
if line.ends_with(",\\") {
ret.push(line[..line.len() - 2].to_string());
} else {
// 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("\\")) {
Copy link
Member

Choose a reason for hiding this comment

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

Like this seems fine to me, thanks!

ret.push(head.to_string());
}
krates.push(
line.split('"')
Expand Down
3 changes: 1 addition & 2 deletions src/librustdoc/html/sources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ impl<'a> SourceCollector<'a> {
};

// Remove the utf-8 BOM if any
let contents =
if contents.starts_with("\u{feff}") { &contents[3..] } else { &contents[..] };
let contents = contents.strip_prefix("\u{feff}").unwrap_or(&contents[..]);

// Create the intermediate directories
let mut cur = self.dst.clone();
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ fn extract_leading_metadata(s: &str) -> (Vec<&str>, &str) {
let mut count = 0;

for line in s.lines() {
if line.starts_with("# ") || line.starts_with('%') {
if let Some(tail) = line.strip_prefix("# ").or_else(|| line.strip_prefix('%')) {
// trim the whitespace after the symbol
metadata.push(line[1..].trim_start());
metadata.push(tail.trim_start());
count += line.len() + 1;
} else {
return (metadata, &s[count..]);
Expand Down
92 changes: 42 additions & 50 deletions src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -541,38 +541,31 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {

let link = ori_link.replace("`", "");
let parts = link.split('#').collect::<Vec<_>>();
let (link, extra_fragment) = if parts.len() > 2 {
build_diagnostic(
cx,
&item,
&link,
&dox,
link_range,
"has an issue with the link anchor.",
"only one `#` is allowed in a link",
None,
);
continue;
} else if parts.len() == 2 {
if parts[0].trim().is_empty() {
// This is an anchor to an element of the current page, nothing to do in here!
let (link, extra_fragment) = match *parts {
[] => unreachable!("`str::split` always returns a non-empty list"),
[a] => (a.to_owned(), None),
// This is an anchor to an element of the current page, nothing to do in here!
[a, _] if a.trim().is_empty() => continue,
[a, b] => (a.to_owned(), Some(b.to_owned())),
[_, _, _, ..] => {
build_diagnostic(
cx,
&item,
&link,
&dox,
link_range,
"has an issue with the link anchor.",
"only one `#` is allowed in a link",
None,
);
continue;
}
(parts[0].to_owned(), Some(parts[1].to_owned()))
} else {
(parts[0].to_owned(), None)
};
let resolved_self;
let mut path_str;
let (res, fragment) = {
let mut kind = None;
path_str = if let Some(prefix) = ["struct@", "enum@", "type@", "trait@", "union@"]
.iter()
.find(|p| link.starts_with(**p))
{
kind = Some(TypeNS);
link.trim_start_matches(prefix)
} else if let Some(prefix) = [
static TYPES: &[&str] = &["struct@", "enum@", "type@", "trait@", "union@"];
static TY_KINDS: &[&str] = &[
"const@",
"static@",
"value@",
Expand All @@ -581,28 +574,27 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
"fn@",
"module@",
"method@",
]
.iter()
.find(|p| link.starts_with(**p))
{
kind = Some(ValueNS);
link.trim_start_matches(prefix)
} else if link.ends_with("()") {
kind = Some(ValueNS);
link.trim_end_matches("()")
} else if link.starts_with("macro@") {
kind = Some(MacroNS);
link.trim_start_matches("macro@")
} else if link.starts_with("derive@") {
kind = Some(MacroNS);
link.trim_start_matches("derive@")
} else if link.ends_with('!') {
kind = Some(MacroNS);
link.trim_end_matches('!')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Only a single one

} else {
&link[..]
}
.trim();
];
let (kind, path) =
if let Some(tail) = TYPES.iter().filter_map(|&p| link.strip_prefix(p)).next() {
(Some(TypeNS), tail)
} else if let Some(tail) =
TY_KINDS.iter().filter_map(|&p| link.strip_prefix(p)).next()
{
(Some(ValueNS), tail)
} else if let Some(head) = link.strip_suffix("()") {
(Some(ValueNS), head)
} else if let Some(tail) =
link.strip_prefix("macro@").or_else(|| link.strip_prefix("derive@"))
{
(Some(MacroNS), tail)
} else if let Some(head) = link.strip_suffix('!') {
(Some(MacroNS), head)
} else {
(None, &link[..])
};

path_str = path.trim();

if path_str.contains(|ch: char| !(ch.is_alphanumeric() || ch == ':' || ch == '_')) {
continue;
Expand All @@ -623,9 +615,9 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
if item.is_mod() && item.attrs.inner_docs { None } else { parent_node };

// replace `Self` with suitable item's parent name
if path_str.starts_with("Self::") {
if let Some(item) = path_str.strip_prefix("Self::") {
if let Some(ref name) = parent_name {
resolved_self = format!("{}::{}", name, &path_str[6..]);
resolved_self = format!("{}::{}", name, item);
path_str = &resolved_self;
}
}
Expand Down
Loading