From 9e11b61e8de1ffb6a1ea8fb1f8c42cfe30b6b3e4 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 11 Jan 2021 10:13:08 -0800 Subject: [PATCH 1/3] linkchecker: Organize state into a struct, and add report. Moves all the state into a struct so it doesn't need to be passed around as much. Also adds a report showing how long it took and what it found. This includes a minor change: a failure to load a file is now an error, instead of being ignored. This should only happen if there is a permission error or some other shenanigans going on. --- src/tools/linkchecker/main.rs | 473 +++++++++++++++++++--------------- 1 file changed, 261 insertions(+), 212 deletions(-) diff --git a/src/tools/linkchecker/main.rs b/src/tools/linkchecker/main.rs index c677d04917e..7df4f5a9c46 100644 --- a/src/tools/linkchecker/main.rs +++ b/src/tools/linkchecker/main.rs @@ -20,6 +20,7 @@ use std::fs; use std::path::{Component, Path, PathBuf}; use std::rc::Rc; +use std::time::Instant; use once_cell::sync::Lazy; use regex::Regex; @@ -89,16 +90,41 @@ macro_rules! t { fn main() { let docs = env::args_os().nth(1).unwrap(); let docs = env::current_dir().unwrap().join(docs); - let mut errors = false; - walk(&mut HashMap::new(), &docs, &docs, &mut errors); - if errors { - panic!("found some broken links"); + let mut checker = Checker { + root: docs.clone(), + cache: HashMap::new(), + errors: 0, + start: Instant::now(), + html_files: 0, + html_redirects: 0, + links_checked: 0, + links_ignored_external: 0, + links_ignored_exception: 0, + intra_doc_exceptions: 0, + }; + checker.walk(&docs); + checker.report(); + if checker.errors != 0 { + println!("found some broken links"); + std::process::exit(1); } } +struct Checker { + root: PathBuf, + cache: Cache, + errors: u32, + start: Instant, + html_files: u32, + html_redirects: u32, + links_checked: u32, + links_ignored_external: u32, + links_ignored_exception: u32, + intra_doc_exceptions: u32, +} + #[derive(Debug)] pub enum LoadError { - IOError(std::io::Error), BrokenRedirect(PathBuf, std::io::Error), IsRedirect, } @@ -131,13 +157,13 @@ fn small_url_encode(s: &str) -> String { } impl FileEntry { - fn parse_ids(&mut self, file: &Path, contents: &str, errors: &mut bool) { + fn parse_ids(&mut self, file: &Path, contents: &str, errors: &mut u32) { if self.ids.is_empty() { with_attrs_in_source(contents, " id", |fragment, i, _| { let frag = fragment.trim_start_matches("#").to_owned(); let encoded = small_url_encode(&frag); if !self.ids.insert(frag) { - *errors = true; + *errors += 1; println!("{}:{}: id is not unique: `{}`", file.display(), i, fragment); } // Just in case, we also add the encoded id. @@ -147,22 +173,237 @@ fn parse_ids(&mut self, file: &Path, contents: &str, errors: &mut bool) { } } -fn walk(cache: &mut Cache, root: &Path, dir: &Path, errors: &mut bool) { - for entry in t!(dir.read_dir()).map(|e| t!(e)) { - let path = entry.path(); - let kind = t!(entry.file_type()); - if kind.is_dir() { - walk(cache, root, &path, errors); - } else { - let pretty_path = check(cache, root, &path, errors); - if let Some(pretty_path) = pretty_path { - let entry = cache.get_mut(&pretty_path).unwrap(); - // we don't need the source anymore, - // so drop to reduce memory-usage - entry.source = Rc::new(String::new()); +impl Checker { + fn walk(&mut self, dir: &Path) { + for entry in t!(dir.read_dir()).map(|e| t!(e)) { + let path = entry.path(); + let kind = t!(entry.file_type()); + if kind.is_dir() { + self.walk(&path); + } else { + let pretty_path = self.check(&path); + if let Some(pretty_path) = pretty_path { + let entry = self.cache.get_mut(&pretty_path).unwrap(); + // we don't need the source anymore, + // so drop to reduce memory-usage + entry.source = Rc::new(String::new()); + } } } } + + fn check(&mut self, file: &Path) -> Option { + // Ignore non-HTML files. + if file.extension().and_then(|s| s.to_str()) != Some("html") { + return None; + } + self.html_files += 1; + + let res = self.load_file(file, SkipRedirect); + let (pretty_file, contents) = match res { + Ok(res) => res, + Err(_) => return None, + }; + self.cache.get_mut(&pretty_file).unwrap().parse_ids( + &pretty_file, + &contents, + &mut self.errors, + ); + + // Search for anything that's the regex 'href[ ]*=[ ]*".*?"' + with_attrs_in_source(&contents, " href", |url, i, base| { + // Ignore external URLs + if url.starts_with("http:") + || url.starts_with("https:") + || url.starts_with("javascript:") + || url.starts_with("ftp:") + || url.starts_with("irc:") + || url.starts_with("data:") + { + self.links_ignored_external += 1; + return; + } + self.links_checked += 1; + let (url, fragment) = match url.split_once('#') { + None => (url, None), + Some((url, fragment)) => (url, Some(fragment)), + }; + // NB: the `splitn` always succeeds, even if the delimiter is not present. + let url = url.splitn(2, '?').next().unwrap(); + + // Once we've plucked out the URL, parse it using our base url and + // then try to extract a file path. + let mut path = file.to_path_buf(); + if !base.is_empty() || !url.is_empty() { + path.pop(); + for part in Path::new(base).join(url).components() { + match part { + Component::Prefix(_) | Component::RootDir => { + // Avoid absolute paths as they make the docs not + // relocatable by making assumptions on where the docs + // are hosted relative to the site root. + self.errors += 1; + println!( + "{}:{}: absolute path - {}", + pretty_file.display(), + i + 1, + Path::new(base).join(url).display() + ); + return; + } + Component::CurDir => {} + Component::ParentDir => { + path.pop(); + } + Component::Normal(s) => { + path.push(s); + } + } + } + } + + // Alright, if we've found a file name then this file had better + // exist! If it doesn't then we register and print an error. + if path.exists() { + if path.is_dir() { + // Links to directories show as directory listings when viewing + // the docs offline so it's best to avoid them. + self.errors += 1; + let pretty_path = path.strip_prefix(&self.root).unwrap_or(&path); + println!( + "{}:{}: directory link - {}", + pretty_file.display(), + i + 1, + pretty_path.display() + ); + return; + } + if let Some(extension) = path.extension() { + // Ignore none HTML files. + if extension != "html" { + return; + } + } + let res = self.load_file(&path, FromRedirect(false)); + let (pretty_path, contents) = match res { + Ok(res) => res, + Err(LoadError::BrokenRedirect(target, _)) => { + self.errors += 1; + println!( + "{}:{}: broken redirect to {}", + pretty_file.display(), + i + 1, + target.display() + ); + return; + } + Err(LoadError::IsRedirect) => unreachable!(), + }; + + if let Some(ref fragment) = fragment { + // Fragments like `#1-6` are most likely line numbers to be + // interpreted by javascript, so we're ignoring these + if fragment.splitn(2, '-').all(|f| f.chars().all(|c| c.is_numeric())) { + return; + } + + // These appear to be broken in mdbook right now? + if fragment.starts_with('-') { + return; + } + + let entry = self.cache.get_mut(&pretty_path).unwrap(); + entry.parse_ids(&pretty_path, &contents, &mut self.errors); + + if entry.ids.contains(*fragment) { + return; + } + + if is_exception(file, &format!("#{}", fragment)) { + self.links_ignored_exception += 1; + } else { + self.errors += 1; + print!("{}:{}: broken link fragment ", pretty_file.display(), i + 1); + println!("`#{}` pointing to `{}`", fragment, pretty_path.display()); + }; + } + } else { + let pretty_path = path.strip_prefix(&self.root).unwrap_or(&path); + if is_exception(file, pretty_path.to_str().unwrap()) { + } else { + self.errors += 1; + print!("{}:{}: broken link - ", pretty_file.display(), i + 1); + println!("{}", pretty_path.display()); + } + } + }); + + // Search for intra-doc links that rustdoc didn't warn about + // FIXME(#77199, 77200) Rustdoc should just warn about these directly. + // NOTE: only looks at one line at a time; in practice this should find most links + for (i, line) in contents.lines().enumerate() { + for broken_link in BROKEN_INTRA_DOC_LINK.captures_iter(line) { + if is_intra_doc_exception(file, &broken_link[1]) { + self.intra_doc_exceptions += 1; + } else { + self.errors += 1; + print!("{}:{}: broken intra-doc link - ", pretty_file.display(), i + 1); + println!("{}", &broken_link[0]); + } + } + } + Some(pretty_file) + } + + fn load_file( + &mut self, + file: &Path, + redirect: Redirect, + ) -> Result<(PathBuf, Rc), LoadError> { + let pretty_file = PathBuf::from(file.strip_prefix(&self.root).unwrap_or(&file)); + + let (maybe_redirect, contents) = match self.cache.entry(pretty_file.clone()) { + Entry::Occupied(entry) => (None, entry.get().source.clone()), + Entry::Vacant(entry) => { + let contents = match fs::read_to_string(file) { + Ok(s) => Rc::new(s), + Err(err) => { + return Err(if let FromRedirect(true) = redirect { + LoadError::BrokenRedirect(file.to_path_buf(), err) + } else { + panic!("error loading {}: {}", file.display(), err); + }); + } + }; + + let maybe = maybe_redirect(&contents); + if maybe.is_some() { + self.html_redirects += 1; + if let SkipRedirect = redirect { + return Err(LoadError::IsRedirect); + } + } else { + entry.insert(FileEntry { source: contents.clone(), ids: HashSet::new() }); + } + (maybe, contents) + } + }; + match maybe_redirect.map(|url| file.parent().unwrap().join(url)) { + Some(redirect_file) => self.load_file(&redirect_file, FromRedirect(true)), + None => Ok((pretty_file, contents)), + } + } + + fn report(&self) { + println!("checked links in: {:.1}s", self.start.elapsed().as_secs_f64()); + println!("number of HTML files scanned: {}", self.html_files); + println!("number of HTML redirects found: {}", self.html_redirects); + println!("number of links checked: {}", self.links_checked); + println!("number of links ignored due to external: {}", self.links_ignored_external); + println!("number of links ignored due to exceptions: {}", self.links_ignored_exception); + println!("number of intra doc links ignored: {}", self.intra_doc_exceptions); + println!("errors found: {}", self.errors); + } } fn is_intra_doc_exception(file: &Path, link: &str) -> bool { @@ -191,198 +432,6 @@ fn is_exception(file: &Path, link: &str) -> bool { } } -fn check(cache: &mut Cache, root: &Path, file: &Path, errors: &mut bool) -> Option { - // Ignore non-HTML files. - if file.extension().and_then(|s| s.to_str()) != Some("html") { - return None; - } - - let res = load_file(cache, root, file, SkipRedirect); - let (pretty_file, contents) = match res { - Ok(res) => res, - Err(_) => return None, - }; - { - cache.get_mut(&pretty_file).unwrap().parse_ids(&pretty_file, &contents, errors); - } - - // Search for anything that's the regex 'href[ ]*=[ ]*".*?"' - with_attrs_in_source(&contents, " href", |url, i, base| { - // Ignore external URLs - if url.starts_with("http:") - || url.starts_with("https:") - || url.starts_with("javascript:") - || url.starts_with("ftp:") - || url.starts_with("irc:") - || url.starts_with("data:") - { - return; - } - let (url, fragment) = match url.split_once('#') { - None => (url, None), - Some((url, fragment)) => (url, Some(fragment)), - }; - // NB: the `splitn` always succeeds, even if the delimiter is not present. - let url = url.splitn(2, '?').next().unwrap(); - - // Once we've plucked out the URL, parse it using our base url and - // then try to extract a file path. - let mut path = file.to_path_buf(); - if !base.is_empty() || !url.is_empty() { - path.pop(); - for part in Path::new(base).join(url).components() { - match part { - Component::Prefix(_) | Component::RootDir => { - // Avoid absolute paths as they make the docs not - // relocatable by making assumptions on where the docs - // are hosted relative to the site root. - *errors = true; - println!( - "{}:{}: absolute path - {}", - pretty_file.display(), - i + 1, - Path::new(base).join(url).display() - ); - return; - } - Component::CurDir => {} - Component::ParentDir => { - path.pop(); - } - Component::Normal(s) => { - path.push(s); - } - } - } - } - - // Alright, if we've found a file name then this file had better - // exist! If it doesn't then we register and print an error. - if path.exists() { - if path.is_dir() { - // Links to directories show as directory listings when viewing - // the docs offline so it's best to avoid them. - *errors = true; - let pretty_path = path.strip_prefix(root).unwrap_or(&path); - println!( - "{}:{}: directory link - {}", - pretty_file.display(), - i + 1, - pretty_path.display() - ); - return; - } - if let Some(extension) = path.extension() { - // Ignore none HTML files. - if extension != "html" { - return; - } - } - let res = load_file(cache, root, &path, FromRedirect(false)); - let (pretty_path, contents) = match res { - Ok(res) => res, - Err(LoadError::IOError(err)) => { - panic!("error loading {}: {}", path.display(), err); - } - Err(LoadError::BrokenRedirect(target, _)) => { - *errors = true; - println!( - "{}:{}: broken redirect to {}", - pretty_file.display(), - i + 1, - target.display() - ); - return; - } - Err(LoadError::IsRedirect) => unreachable!(), - }; - - if let Some(ref fragment) = fragment { - // Fragments like `#1-6` are most likely line numbers to be - // interpreted by javascript, so we're ignoring these - if fragment.splitn(2, '-').all(|f| f.chars().all(|c| c.is_numeric())) { - return; - } - - // These appear to be broken in mdbook right now? - if fragment.starts_with('-') { - return; - } - - let entry = &mut cache.get_mut(&pretty_path).unwrap(); - entry.parse_ids(&pretty_path, &contents, errors); - - if !entry.ids.contains(*fragment) && !is_exception(file, &format!("#{}", fragment)) - { - *errors = true; - print!("{}:{}: broken link fragment ", pretty_file.display(), i + 1); - println!("`#{}` pointing to `{}`", fragment, pretty_path.display()); - }; - } - } else { - let pretty_path = path.strip_prefix(root).unwrap_or(&path); - if !is_exception(file, pretty_path.to_str().unwrap()) { - *errors = true; - print!("{}:{}: broken link - ", pretty_file.display(), i + 1); - println!("{}", pretty_path.display()); - } - } - }); - - // Search for intra-doc links that rustdoc didn't warn about - // FIXME(#77199, 77200) Rustdoc should just warn about these directly. - // NOTE: only looks at one line at a time; in practice this should find most links - for (i, line) in contents.lines().enumerate() { - for broken_link in BROKEN_INTRA_DOC_LINK.captures_iter(line) { - if !is_intra_doc_exception(file, &broken_link[1]) { - *errors = true; - print!("{}:{}: broken intra-doc link - ", pretty_file.display(), i + 1); - println!("{}", &broken_link[0]); - } - } - } - Some(pretty_file) -} - -fn load_file( - cache: &mut Cache, - root: &Path, - file: &Path, - redirect: Redirect, -) -> Result<(PathBuf, Rc), LoadError> { - let pretty_file = PathBuf::from(file.strip_prefix(root).unwrap_or(&file)); - - let (maybe_redirect, contents) = match cache.entry(pretty_file.clone()) { - Entry::Occupied(entry) => (None, entry.get().source.clone()), - Entry::Vacant(entry) => { - let contents = match fs::read_to_string(file) { - Ok(s) => Rc::new(s), - Err(err) => { - return Err(if let FromRedirect(true) = redirect { - LoadError::BrokenRedirect(file.to_path_buf(), err) - } else { - LoadError::IOError(err) - }); - } - }; - - let maybe = maybe_redirect(&contents); - if maybe.is_some() { - if let SkipRedirect = redirect { - return Err(LoadError::IsRedirect); - } - } else { - entry.insert(FileEntry { source: contents.clone(), ids: HashSet::new() }); - } - (maybe, contents) - } - }; - match maybe_redirect.map(|url| file.parent().unwrap().join(url)) { - Some(redirect_file) => load_file(cache, root, &redirect_file, FromRedirect(true)), - None => Ok((pretty_file, contents)), - } -} - fn maybe_redirect(source: &str) -> Option { const REDIRECT: &str = "

Redirecting to , ids: RefCell> }, + /// This file is an HTML redirect to the given local path. + Redirect { target: PathBuf }, + /// This is not an HTML file. + OtherFile, + /// This is a directory. + Dir, + /// The file doesn't exist. + Missing, } -enum Redirect { - SkipRedirect, - FromRedirect(bool), -} - -struct FileEntry { - source: Rc, - ids: HashSet, -} - -type Cache = HashMap; +/// A cache to speed up file access. +type Cache = HashMap; fn small_url_encode(s: &str) -> String { s.replace("<", "%3C") @@ -156,62 +161,36 @@ fn small_url_encode(s: &str) -> String { .replace("\"", "%22") } -impl FileEntry { - fn parse_ids(&mut self, file: &Path, contents: &str, errors: &mut u32) { - if self.ids.is_empty() { - with_attrs_in_source(contents, " id", |fragment, i, _| { - let frag = fragment.trim_start_matches("#").to_owned(); - let encoded = small_url_encode(&frag); - if !self.ids.insert(frag) { - *errors += 1; - println!("{}:{}: id is not unique: `{}`", file.display(), i, fragment); - } - // Just in case, we also add the encoded id. - self.ids.insert(encoded); - }); - } - } -} - impl Checker { - fn walk(&mut self, dir: &Path) { + /// Primary entry point for walking the filesystem to find HTML files to check. + fn walk(&mut self, dir: &Path, report: &mut Report) { for entry in t!(dir.read_dir()).map(|e| t!(e)) { let path = entry.path(); let kind = t!(entry.file_type()); if kind.is_dir() { - self.walk(&path); + self.walk(&path, report); } else { - let pretty_path = self.check(&path); - if let Some(pretty_path) = pretty_path { - let entry = self.cache.get_mut(&pretty_path).unwrap(); - // we don't need the source anymore, - // so drop to reduce memory-usage - entry.source = Rc::new(String::new()); - } + self.check(&path, report); } } } - fn check(&mut self, file: &Path) -> Option { - // Ignore non-HTML files. - if file.extension().and_then(|s| s.to_str()) != Some("html") { - return None; - } - self.html_files += 1; - - let res = self.load_file(file, SkipRedirect); - let (pretty_file, contents) = match res { - Ok(res) => res, - Err(_) => return None, + /// Checks a single file. + fn check(&mut self, file: &Path, report: &mut Report) { + let (pretty_path, entry) = self.load_file(file, report); + let source = match entry { + FileEntry::Missing => panic!("missing file {:?} while walking", file), + FileEntry::Dir => unreachable!("never with `check` path"), + FileEntry::OtherFile => return, + FileEntry::Redirect { .. } => return, + FileEntry::HtmlFile { source, ids } => { + parse_ids(&mut ids.borrow_mut(), &pretty_path, source, report); + source.clone() + } }; - self.cache.get_mut(&pretty_file).unwrap().parse_ids( - &pretty_file, - &contents, - &mut self.errors, - ); // Search for anything that's the regex 'href[ ]*=[ ]*".*?"' - with_attrs_in_source(&contents, " href", |url, i, base| { + with_attrs_in_source(&source, " href", |url, i, base| { // Ignore external URLs if url.starts_with("http:") || url.starts_with("https:") @@ -220,10 +199,10 @@ fn check(&mut self, file: &Path) -> Option { || url.starts_with("irc:") || url.starts_with("data:") { - self.links_ignored_external += 1; + report.links_ignored_external += 1; return; } - self.links_checked += 1; + report.links_checked += 1; let (url, fragment) = match url.split_once('#') { None => (url, None), Some((url, fragment)) => (url, Some(fragment)), @@ -242,10 +221,10 @@ fn check(&mut self, file: &Path) -> Option { // Avoid absolute paths as they make the docs not // relocatable by making assumptions on where the docs // are hosted relative to the site root. - self.errors += 1; + report.errors += 1; println!( "{}:{}: absolute path - {}", - pretty_file.display(), + pretty_path, i + 1, Path::new(base).join(url).display() ); @@ -262,138 +241,165 @@ fn check(&mut self, file: &Path) -> Option { } } - // Alright, if we've found a file name then this file had better - // exist! If it doesn't then we register and print an error. - if path.exists() { - if path.is_dir() { + let (target_pretty_path, target_entry) = self.load_file(&path, report); + let (target_source, target_ids) = match target_entry { + FileEntry::Missing => { + if is_exception(file, &target_pretty_path) { + report.links_ignored_exception += 1; + } else { + report.errors += 1; + println!( + "{}:{}: broken link - `{}`", + pretty_path, + i + 1, + target_pretty_path + ); + } + return; + } + FileEntry::Dir => { // Links to directories show as directory listings when viewing // the docs offline so it's best to avoid them. - self.errors += 1; - let pretty_path = path.strip_prefix(&self.root).unwrap_or(&path); + report.errors += 1; println!( - "{}:{}: directory link - {}", - pretty_file.display(), + "{}:{}: directory link to `{}` \ + (directory links should use index.html instead)", + pretty_path, i + 1, - pretty_path.display() + target_pretty_path ); return; } - if let Some(extension) = path.extension() { - // Ignore none HTML files. - if extension != "html" { - return; + FileEntry::OtherFile => return, + FileEntry::Redirect { target } => { + let t = target.clone(); + drop(target); + let (target, redir_entry) = self.load_file(&t, report); + match redir_entry { + FileEntry::Missing => { + report.errors += 1; + println!( + "{}:{}: broken redirect from `{}` to `{}`", + pretty_path, + i + 1, + target_pretty_path, + target + ); + return; + } + FileEntry::Redirect { target } => { + // Redirect to a redirect, this link checker + // currently doesn't support this, since it would + // require cycle checking, etc. + report.errors += 1; + println!( + "{}:{}: redirect from `{}` to `{}` \ + which is also a redirect (not supported)", + pretty_path, + i + 1, + target_pretty_path, + target.display() + ); + return; + } + FileEntry::Dir => { + report.errors += 1; + println!( + "{}:{}: redirect from `{}` to `{}` \ + which is a directory \ + (directory links should use index.html instead)", + pretty_path, + i + 1, + target_pretty_path, + target + ); + return; + } + FileEntry::OtherFile => return, + FileEntry::HtmlFile { source, ids } => (source, ids), } } - let res = self.load_file(&path, FromRedirect(false)); - let (pretty_path, contents) = match res { - Ok(res) => res, - Err(LoadError::BrokenRedirect(target, _)) => { - self.errors += 1; - println!( - "{}:{}: broken redirect to {}", - pretty_file.display(), - i + 1, - target.display() - ); - return; - } - Err(LoadError::IsRedirect) => unreachable!(), - }; + FileEntry::HtmlFile { source, ids } => (source, ids), + }; - if let Some(ref fragment) = fragment { - // Fragments like `#1-6` are most likely line numbers to be - // interpreted by javascript, so we're ignoring these - if fragment.splitn(2, '-').all(|f| f.chars().all(|c| c.is_numeric())) { - return; - } - - // These appear to be broken in mdbook right now? - if fragment.starts_with('-') { - return; - } - - let entry = self.cache.get_mut(&pretty_path).unwrap(); - entry.parse_ids(&pretty_path, &contents, &mut self.errors); - - if entry.ids.contains(*fragment) { - return; - } - - if is_exception(file, &format!("#{}", fragment)) { - self.links_ignored_exception += 1; - } else { - self.errors += 1; - print!("{}:{}: broken link fragment ", pretty_file.display(), i + 1); - println!("`#{}` pointing to `{}`", fragment, pretty_path.display()); - }; + // Alright, if we've found an HTML file for the target link. If + // this is a fragment link, also check that the `id` exists. + if let Some(ref fragment) = fragment { + // Fragments like `#1-6` are most likely line numbers to be + // interpreted by javascript, so we're ignoring these + if fragment.splitn(2, '-').all(|f| f.chars().all(|c| c.is_numeric())) { + return; } - } else { - let pretty_path = path.strip_prefix(&self.root).unwrap_or(&path); - if is_exception(file, pretty_path.to_str().unwrap()) { + + // These appear to be broken in mdbook right now? + if fragment.starts_with('-') { + return; + } + + parse_ids(&mut target_ids.borrow_mut(), &pretty_path, target_source, report); + + if target_ids.borrow().contains(*fragment) { + return; + } + + if is_exception(file, &format!("#{}", fragment)) { + report.links_ignored_exception += 1; } else { - self.errors += 1; - print!("{}:{}: broken link - ", pretty_file.display(), i + 1); - println!("{}", pretty_path.display()); - } + report.errors += 1; + print!("{}:{}: broken link fragment ", pretty_path, i + 1); + println!("`#{}` pointing to `{}`", fragment, pretty_path); + }; } }); // Search for intra-doc links that rustdoc didn't warn about // FIXME(#77199, 77200) Rustdoc should just warn about these directly. // NOTE: only looks at one line at a time; in practice this should find most links - for (i, line) in contents.lines().enumerate() { + for (i, line) in source.lines().enumerate() { for broken_link in BROKEN_INTRA_DOC_LINK.captures_iter(line) { if is_intra_doc_exception(file, &broken_link[1]) { - self.intra_doc_exceptions += 1; + report.intra_doc_exceptions += 1; } else { - self.errors += 1; - print!("{}:{}: broken intra-doc link - ", pretty_file.display(), i + 1); + report.errors += 1; + print!("{}:{}: broken intra-doc link - ", pretty_path, i + 1); println!("{}", &broken_link[0]); } } } - Some(pretty_file) - } - - fn load_file( - &mut self, - file: &Path, - redirect: Redirect, - ) -> Result<(PathBuf, Rc), LoadError> { - let pretty_file = PathBuf::from(file.strip_prefix(&self.root).unwrap_or(&file)); - - let (maybe_redirect, contents) = match self.cache.entry(pretty_file.clone()) { - Entry::Occupied(entry) => (None, entry.get().source.clone()), - Entry::Vacant(entry) => { - let contents = match fs::read_to_string(file) { - Ok(s) => Rc::new(s), - Err(err) => { - return Err(if let FromRedirect(true) = redirect { - LoadError::BrokenRedirect(file.to_path_buf(), err) - } else { - panic!("error loading {}: {}", file.display(), err); - }); - } - }; - - let maybe = maybe_redirect(&contents); - if maybe.is_some() { - self.html_redirects += 1; - if let SkipRedirect = redirect { - return Err(LoadError::IsRedirect); - } - } else { - entry.insert(FileEntry { source: contents.clone(), ids: HashSet::new() }); - } - (maybe, contents) - } - }; - match maybe_redirect.map(|url| file.parent().unwrap().join(url)) { - Some(redirect_file) => self.load_file(&redirect_file, FromRedirect(true)), - None => Ok((pretty_file, contents)), + // we don't need the source anymore, + // so drop to reduce memory-usage + match self.cache.get_mut(&pretty_path).unwrap() { + FileEntry::HtmlFile { source, .. } => *source = Rc::new(String::new()), + _ => unreachable!("must be html file"), } } + /// Load a file from disk, or from the cache if available. + fn load_file(&mut self, file: &Path, report: &mut Report) -> (String, &FileEntry) { + let pretty_path = + file.strip_prefix(&self.root).unwrap_or(&file).to_str().unwrap().to_string(); + + let entry = + self.cache.entry(pretty_path.clone()).or_insert_with(|| match fs::metadata(file) { + Ok(metadata) if metadata.is_dir() => FileEntry::Dir, + Ok(_) => { + if file.extension().and_then(|s| s.to_str()) != Some("html") { + FileEntry::OtherFile + } else { + report.html_files += 1; + load_html_file(file, report) + } + } + Err(e) if e.kind() == ErrorKind::NotFound => FileEntry::Missing, + Err(e) => { + panic!("unexpected read error for {}: {}", file.display(), e); + } + }); + (pretty_path, entry) + } +} + +impl Report { fn report(&self) { println!("checked links in: {:.1}s", self.start.elapsed().as_secs_f64()); println!("number of HTML files scanned: {}", self.html_files); @@ -406,6 +412,25 @@ fn report(&self) { } } +fn load_html_file(file: &Path, report: &mut Report) -> FileEntry { + let source = match fs::read_to_string(file) { + Ok(s) => Rc::new(s), + Err(err) => { + // This usually should not fail since `metadata` was already + // called successfully on this file. + panic!("unexpected read error for {}: {}", file.display(), err); + } + }; + match maybe_redirect(&source) { + Some(target) => { + report.html_redirects += 1; + let target = file.parent().unwrap().join(target); + FileEntry::Redirect { target } + } + None => FileEntry::HtmlFile { source: source.clone(), ids: RefCell::new(HashSet::new()) }, + } +} + fn is_intra_doc_exception(file: &Path, link: &str) -> bool { if let Some(entry) = INTRA_DOC_LINK_EXCEPTIONS.iter().find(|&(f, _)| file.ends_with(f)) { entry.1.is_empty() || entry.1.contains(&link) @@ -432,6 +457,8 @@ fn is_exception(file: &Path, link: &str) -> bool { } } +/// If the given HTML file contents is an HTML redirect, this returns the +/// destination path given in the redirect. fn maybe_redirect(source: &str) -> Option { const REDIRECT: &str = "

Redirecting to (contents: &str, attr: &str, } } } + +fn parse_ids(ids: &mut HashSet, file: &str, source: &str, report: &mut Report) { + if ids.is_empty() { + with_attrs_in_source(source, " id", |fragment, i, _| { + let frag = fragment.trim_start_matches("#").to_owned(); + let encoded = small_url_encode(&frag); + if !ids.insert(frag) { + report.errors += 1; + println!("{}:{}: id is not unique: `{}`", file, i, fragment); + } + // Just in case, we also add the encoded id. + ids.insert(encoded); + }); + } +} From b6532ebeeb31f3e1236e23bffbd36327979458dc Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 24 May 2021 16:25:23 -0700 Subject: [PATCH 3/3] Add some tests for the linkchecker. --- .../linkchecker/tests/basic_broken/foo.html | 5 ++ .../tests/broken_fragment_local/foo.html | 5 ++ .../tests/broken_fragment_remote/bar.html | 4 + .../broken_fragment_remote/inner/foo.html | 5 ++ .../linkchecker/tests/broken_redir/foo.html | 5 ++ .../tests/broken_redir/redir-bad.html | 10 +++ src/tools/linkchecker/tests/checks.rs | 77 +++++++++++++++++++ .../linkchecker/tests/directory_link/foo.html | 5 ++ .../tests/directory_link/somedir/index.html | 4 + .../linkchecker/tests/redirect_loop/foo.html | 5 ++ .../tests/redirect_loop/redir-bad.html | 10 +++ .../linkchecker/tests/valid/inner/bar.html | 7 ++ .../linkchecker/tests/valid/inner/foo.html | 14 ++++ .../tests/valid/inner/redir-bad.html | 11 +++ .../tests/valid/inner/redir-target.html | 5 ++ .../linkchecker/tests/valid/inner/redir.html | 10 +++ src/tools/linkchecker/tests/valid/outer.html | 5 ++ 17 files changed, 187 insertions(+) create mode 100644 src/tools/linkchecker/tests/basic_broken/foo.html create mode 100644 src/tools/linkchecker/tests/broken_fragment_local/foo.html create mode 100644 src/tools/linkchecker/tests/broken_fragment_remote/bar.html create mode 100644 src/tools/linkchecker/tests/broken_fragment_remote/inner/foo.html create mode 100644 src/tools/linkchecker/tests/broken_redir/foo.html create mode 100644 src/tools/linkchecker/tests/broken_redir/redir-bad.html create mode 100644 src/tools/linkchecker/tests/checks.rs create mode 100644 src/tools/linkchecker/tests/directory_link/foo.html create mode 100644 src/tools/linkchecker/tests/directory_link/somedir/index.html create mode 100644 src/tools/linkchecker/tests/redirect_loop/foo.html create mode 100644 src/tools/linkchecker/tests/redirect_loop/redir-bad.html create mode 100644 src/tools/linkchecker/tests/valid/inner/bar.html create mode 100644 src/tools/linkchecker/tests/valid/inner/foo.html create mode 100644 src/tools/linkchecker/tests/valid/inner/redir-bad.html create mode 100644 src/tools/linkchecker/tests/valid/inner/redir-target.html create mode 100644 src/tools/linkchecker/tests/valid/inner/redir.html create mode 100644 src/tools/linkchecker/tests/valid/outer.html diff --git a/src/tools/linkchecker/tests/basic_broken/foo.html b/src/tools/linkchecker/tests/basic_broken/foo.html new file mode 100644 index 00000000000..cb27c55c9fe --- /dev/null +++ b/src/tools/linkchecker/tests/basic_broken/foo.html @@ -0,0 +1,5 @@ + + +test + + diff --git a/src/tools/linkchecker/tests/broken_fragment_local/foo.html b/src/tools/linkchecker/tests/broken_fragment_local/foo.html new file mode 100644 index 00000000000..66c457ad01f --- /dev/null +++ b/src/tools/linkchecker/tests/broken_fragment_local/foo.html @@ -0,0 +1,5 @@ + + +test + + diff --git a/src/tools/linkchecker/tests/broken_fragment_remote/bar.html b/src/tools/linkchecker/tests/broken_fragment_remote/bar.html new file mode 100644 index 00000000000..7879e1ce9fd --- /dev/null +++ b/src/tools/linkchecker/tests/broken_fragment_remote/bar.html @@ -0,0 +1,4 @@ + + + + diff --git a/src/tools/linkchecker/tests/broken_fragment_remote/inner/foo.html b/src/tools/linkchecker/tests/broken_fragment_remote/inner/foo.html new file mode 100644 index 00000000000..7683060b3a6 --- /dev/null +++ b/src/tools/linkchecker/tests/broken_fragment_remote/inner/foo.html @@ -0,0 +1,5 @@ + + +test + + diff --git a/src/tools/linkchecker/tests/broken_redir/foo.html b/src/tools/linkchecker/tests/broken_redir/foo.html new file mode 100644 index 00000000000..bd3e3ad3343 --- /dev/null +++ b/src/tools/linkchecker/tests/broken_redir/foo.html @@ -0,0 +1,5 @@ + + + bad redir + + diff --git a/src/tools/linkchecker/tests/broken_redir/redir-bad.html b/src/tools/linkchecker/tests/broken_redir/redir-bad.html new file mode 100644 index 00000000000..3e376629f74 --- /dev/null +++ b/src/tools/linkchecker/tests/broken_redir/redir-bad.html @@ -0,0 +1,10 @@ + + + + + + +

Redirecting to sometarget...

+ + + diff --git a/src/tools/linkchecker/tests/checks.rs b/src/tools/linkchecker/tests/checks.rs new file mode 100644 index 00000000000..c6ec999e5cf --- /dev/null +++ b/src/tools/linkchecker/tests/checks.rs @@ -0,0 +1,77 @@ +use std::path::Path; +use std::process::{Command, ExitStatus}; + +fn run(dirname: &str) -> (ExitStatus, String, String) { + let output = Command::new(env!("CARGO_BIN_EXE_linkchecker")) + .current_dir(Path::new(env!("CARGO_MANIFEST_DIR")).join("tests")) + .arg(dirname) + .output() + .unwrap(); + let stdout = String::from_utf8(output.stdout).unwrap(); + let stderr = String::from_utf8(output.stderr).unwrap(); + (output.status, stdout, stderr) +} + +fn broken_test(dirname: &str, expected: &str) { + let (status, stdout, stderr) = run(dirname); + assert!(!status.success()); + if !stdout.contains(expected) { + panic!( + "stdout did not contain expected text: {}\n\ + --- stdout:\n\ + {}\n\ + --- stderr:\n\ + {}\n", + expected, stdout, stderr + ); + } +} + +fn valid_test(dirname: &str) { + let (status, stdout, stderr) = run(dirname); + if !status.success() { + panic!( + "test did not succeed as expected\n\ + --- stdout:\n\ + {}\n\ + --- stderr:\n\ + {}\n", + stdout, stderr + ); + } +} + +#[test] +fn valid() { + valid_test("valid/inner"); +} + +#[test] +fn basic_broken() { + broken_test("basic_broken", "bar.html"); +} + +#[test] +fn broken_fragment_local() { + broken_test("broken_fragment_local", "#somefrag"); +} + +#[test] +fn broken_fragment_remote() { + broken_test("broken_fragment_remote/inner", "#somefrag"); +} + +#[test] +fn broken_redir() { + broken_test("broken_redir", "sometarget"); +} + +#[test] +fn directory_link() { + broken_test("directory_link", "somedir"); +} + +#[test] +fn redirect_loop() { + broken_test("redirect_loop", "redir-bad.html"); +} diff --git a/src/tools/linkchecker/tests/directory_link/foo.html b/src/tools/linkchecker/tests/directory_link/foo.html new file mode 100644 index 00000000000..40a8461b86c --- /dev/null +++ b/src/tools/linkchecker/tests/directory_link/foo.html @@ -0,0 +1,5 @@ + + + dir link + + diff --git a/src/tools/linkchecker/tests/directory_link/somedir/index.html b/src/tools/linkchecker/tests/directory_link/somedir/index.html new file mode 100644 index 00000000000..7879e1ce9fd --- /dev/null +++ b/src/tools/linkchecker/tests/directory_link/somedir/index.html @@ -0,0 +1,4 @@ + + + + diff --git a/src/tools/linkchecker/tests/redirect_loop/foo.html b/src/tools/linkchecker/tests/redirect_loop/foo.html new file mode 100644 index 00000000000..bee58b212b5 --- /dev/null +++ b/src/tools/linkchecker/tests/redirect_loop/foo.html @@ -0,0 +1,5 @@ + + + loop link + + diff --git a/src/tools/linkchecker/tests/redirect_loop/redir-bad.html b/src/tools/linkchecker/tests/redirect_loop/redir-bad.html new file mode 100644 index 00000000000..fe7780e6739 --- /dev/null +++ b/src/tools/linkchecker/tests/redirect_loop/redir-bad.html @@ -0,0 +1,10 @@ + + + + + + +

Redirecting to redir-bad.html...

+ + + diff --git a/src/tools/linkchecker/tests/valid/inner/bar.html b/src/tools/linkchecker/tests/valid/inner/bar.html new file mode 100644 index 00000000000..4b500d78b76 --- /dev/null +++ b/src/tools/linkchecker/tests/valid/inner/bar.html @@ -0,0 +1,7 @@ + + + +

Bar

+ + + diff --git a/src/tools/linkchecker/tests/valid/inner/foo.html b/src/tools/linkchecker/tests/valid/inner/foo.html new file mode 100644 index 00000000000..3c6a7483bcd --- /dev/null +++ b/src/tools/linkchecker/tests/valid/inner/foo.html @@ -0,0 +1,14 @@ + + + test local frag + remote link + remote link with fragment + this book + this book with fragment + external links not validated + Redirect + +

Local

+ + + diff --git a/src/tools/linkchecker/tests/valid/inner/redir-bad.html b/src/tools/linkchecker/tests/valid/inner/redir-bad.html new file mode 100644 index 00000000000..d21336e7e73 --- /dev/null +++ b/src/tools/linkchecker/tests/valid/inner/redir-bad.html @@ -0,0 +1,11 @@ + + + + + + +

Redirecting to xxx...

+ + These files are skipped, but probably shouldn't be. + + diff --git a/src/tools/linkchecker/tests/valid/inner/redir-target.html b/src/tools/linkchecker/tests/valid/inner/redir-target.html new file mode 100644 index 00000000000..bd59884a01e --- /dev/null +++ b/src/tools/linkchecker/tests/valid/inner/redir-target.html @@ -0,0 +1,5 @@ + + +

Redir

+ + diff --git a/src/tools/linkchecker/tests/valid/inner/redir.html b/src/tools/linkchecker/tests/valid/inner/redir.html new file mode 100644 index 00000000000..1808b23aed8 --- /dev/null +++ b/src/tools/linkchecker/tests/valid/inner/redir.html @@ -0,0 +1,10 @@ + + + + + + +

Redirecting to redir-target.html...

+ + + diff --git a/src/tools/linkchecker/tests/valid/outer.html b/src/tools/linkchecker/tests/valid/outer.html new file mode 100644 index 00000000000..35f799f2023 --- /dev/null +++ b/src/tools/linkchecker/tests/valid/outer.html @@ -0,0 +1,5 @@ + + + + +