From d6baf3875cb9a3bbf3ebd2f7b33ab7f204b5908e Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Mon, 7 Dec 2020 12:54:55 -0700 Subject: Dogfood 'str_split_once() with Tidy --- src/tools/tidy/src/cargo.rs | 9 ++-- src/tools/tidy/src/error_codes_check.rs | 95 ++++++++++++++++++--------------- src/tools/tidy/src/extdeps.rs | 2 +- src/tools/tidy/src/features.rs | 1 + src/tools/tidy/src/lib.rs | 2 + src/tools/tidy/src/ui_tests.rs | 10 +--- 6 files changed, 63 insertions(+), 56 deletions(-) (limited to 'src') diff --git a/src/tools/tidy/src/cargo.rs b/src/tools/tidy/src/cargo.rs index 7bdd78a91e7..e06616a59f3 100644 --- a/src/tools/tidy/src/cargo.rs +++ b/src/tools/tidy/src/cargo.rs @@ -59,11 +59,10 @@ fn verify(tomlfile: &Path, libfile: &Path, bad: &mut bool) { break; } - let mut parts = line.splitn(2, '='); - let krate = parts.next().unwrap().trim(); - if parts.next().is_none() { - continue; - } + let krate = match line.split_once('=') { + None => continue, + Some((krate, _)) => krate.trim(), + }; // Don't worry about depending on core/std while not writing `extern crate // core/std` -- that's intentional. diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index 82a5234ac5b..1c175901be8 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -85,47 +85,55 @@ fn extract_error_codes( for line in f.lines() { let s = line.trim(); if !reached_no_explanation && s.starts_with('E') && s.contains("include_str!(\"") { - if let Some(err_code) = s.splitn(2, ':').next() { - let err_code = err_code.to_owned(); - if !error_codes.contains_key(&err_code) { - error_codes.insert(err_code.clone(), false); - } - // Now we extract the tests from the markdown file! - let md = some_or_continue!(s.splitn(2, "include_str!(\"").nth(1)); - let md_file_name = some_or_continue!(md.splitn(2, "\")").next()); - let path = some_or_continue!(path.parent()) - .join(md_file_name) - .canonicalize() - .expect("failed to canonicalize error explanation file path"); - match read_to_string(&path) { - Ok(content) => { - if !IGNORE_EXPLANATION_CHECK.contains(&err_code.as_str()) - && !check_if_error_code_is_test_in_explanation(&content, &err_code) - { - errors.push(format!( - "`{}` doesn't use its own error code in compile_fail example", - path.display(), - )); - } - if check_error_code_explanation(&content, error_codes, err_code) { - errors.push(format!( - "`{}` uses invalid tag `compile-fail` instead of `compile_fail`", - path.display(), - )); - } + let err_code = match s.split_once(':') { + None => continue, + Some((err_code, _)) => err_code.to_owned(), + }; + if !error_codes.contains_key(&err_code) { + error_codes.insert(err_code.clone(), false); + } + // Now we extract the tests from the markdown file! + let md_file_name = match s.split_once("include_str!(\"") { + None => continue, + Some((_, md)) => match md.split_once("\")") { + None => continue, + Some((file_name, _)) => file_name, + }, + }; + let path = some_or_continue!(path.parent()) + .join(md_file_name) + .canonicalize() + .expect("failed to canonicalize error explanation file path"); + match read_to_string(&path) { + Ok(content) => { + if !IGNORE_EXPLANATION_CHECK.contains(&err_code.as_str()) + && !check_if_error_code_is_test_in_explanation(&content, &err_code) + { + errors.push(format!( + "`{}` doesn't use its own error code in compile_fail example", + path.display(), + )); } - Err(e) => { - eprintln!("Couldn't read `{}`: {}", path.display(), e); + if check_error_code_explanation(&content, error_codes, err_code) { + errors.push(format!( + "`{}` uses invalid tag `compile-fail` instead of `compile_fail`", + path.display(), + )); } } + Err(e) => { + eprintln!("Couldn't read `{}`: {}", path.display(), e); + } } } else if reached_no_explanation && s.starts_with('E') { - if let Some(err_code) = s.splitn(2, ',').next() { - let err_code = err_code.to_owned(); - if !error_codes.contains_key(&err_code) { - // this check should *never* fail! - error_codes.insert(err_code, false); - } + let err_code = match s.split_once(',') { + None => s, + Some((err_code, _)) => err_code, + } + .to_string(); + if !error_codes.contains_key(&err_code) { + // this check should *never* fail! + error_codes.insert(err_code, false); } } else if s == ";" { reached_no_explanation = true; @@ -137,12 +145,15 @@ fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap continue, + Some((err_code, _)) => match err_code.split_once('[') { + None => continue, + Some((_, err_code)) => err_code, + }, + }; + let nb = error_codes.entry(err_code.to_owned()).or_insert(false); + *nb = true; } } } diff --git a/src/tools/tidy/src/extdeps.rs b/src/tools/tidy/src/extdeps.rs index 1cf0d24e26f..93d4d3d8047 100644 --- a/src/tools/tidy/src/extdeps.rs +++ b/src/tools/tidy/src/extdeps.rs @@ -23,7 +23,7 @@ pub fn check(root: &Path, bad: &mut bool) { } // Extract source value. - let source = line.splitn(2, '=').nth(1).unwrap().trim(); + let source = line.split_once('=').unwrap().1.trim(); // Ensure source is allowed. if !ALLOWED_SOURCES.contains(&&*source) { diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index d8029ea04f0..3c2880d0d5e 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -112,6 +112,7 @@ pub fn check( let gate_test_str = "gate-test-"; let feature_name = match line.find(gate_test_str) { + // NB: the `splitn` always succeeds, even if the delimiter is not present. Some(i) => line[i + gate_test_str.len()..].splitn(2, ' ').next().unwrap(), None => continue, }; diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index cc4c43f0468..e11d293210b 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -3,6 +3,8 @@ //! This library contains the tidy lints and exposes it //! to be used by tools. +#![feature(str_split_once)] + use std::fs::File; use std::io::Read; use walkdir::{DirEntry, WalkDir}; diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 47b328dae47..4c8c1d58ed7 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -19,14 +19,8 @@ pub fn check(path: &Path, bad: &mut bool) { // // For now, just make sure that there is a corresponding // `$testname.rs` file. - let testname = file_path - .file_name() - .unwrap() - .to_str() - .unwrap() - .splitn(2, '.') - .next() - .unwrap(); + let testname = + file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0; if !file_path.with_file_name(testname).with_extension("rs").exists() { println!("Stray file with UI testing output: {:?}", file_path); *bad = true; -- cgit 1.4.1-3-g733a5 From 7bd47bd7a14a32be27382677a98d7a031785fc6e Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Mon, 7 Dec 2020 13:39:30 -0700 Subject: Dogfood 'str_split_once() with linkchecker --- src/tools/linkchecker/main.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/tools/linkchecker/main.rs b/src/tools/linkchecker/main.rs index f213944e0ab..dcfe1bb803f 100644 --- a/src/tools/linkchecker/main.rs +++ b/src/tools/linkchecker/main.rs @@ -14,6 +14,8 @@ //! A few exceptions are allowed as there's known bugs in rustdoc, but this //! should catch the majority of "broken link" cases. +#![feature(str_split_once)] + use std::collections::hash_map::Entry; use std::collections::{HashMap, HashSet}; use std::env; @@ -232,11 +234,12 @@ fn check(cache: &mut Cache, root: &Path, file: &Path, errors: &mut bool) -> Opti { return; } - let mut parts = url.splitn(2, '#'); - let url = parts.next().unwrap(); - let fragment = parts.next(); - let mut parts = url.splitn(2, '?'); - let url = parts.next().unwrap(); + 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. -- cgit 1.4.1-3-g733a5 From 85e9ea015220cc74dc54873974ed7138ea22eced Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Mon, 7 Dec 2020 14:00:31 -0700 Subject: Dogfood 'str_split_once() with librustdoc --- src/librustdoc/config.rs | 15 +++++---------- src/librustdoc/html/render/mod.rs | 6 ++---- src/librustdoc/lib.rs | 1 + src/librustdoc/passes/collect_intra_doc_links.rs | 7 ++++--- 4 files changed, 12 insertions(+), 17 deletions(-) (limited to 'src') diff --git a/src/librustdoc/config.rs b/src/librustdoc/config.rs index e60970af0d3..2d58614b139 100644 --- a/src/librustdoc/config.rs +++ b/src/librustdoc/config.rs @@ -397,12 +397,9 @@ impl Options { matches .opt_strs("default-setting") .iter() - .map(|s| { - let mut kv = s.splitn(2, '='); - // never panics because `splitn` always returns at least one element - let k = kv.next().unwrap().to_string(); - let v = kv.next().unwrap_or("true").to_string(); - (k, v) + .map(|s| match s.split_once('=') { + None => (s.clone(), "true".to_string()), + Some((k, v)) => (k.to_string(), v.to_string()), }) .collect(), ]; @@ -707,11 +704,9 @@ fn parse_extern_html_roots( ) -> Result, &'static str> { let mut externs = BTreeMap::new(); for arg in &matches.opt_strs("extern-html-root-url") { - let mut parts = arg.splitn(2, '='); - let name = parts.next().ok_or("--extern-html-root-url must not be empty")?; - let url = parts.next().ok_or("--extern-html-root-url must be of the form name=url")?; + let (name, url) = + arg.split_once('=').ok_or("--extern-html-root-url must be of the form name=url")?; externs.insert(name.to_string(), url.to_string()); } - Ok(externs) } diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index 901f00b21da..efee4c0be06 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -167,10 +167,8 @@ impl Context { // `style-suffix.min.css`. Path::extension would just return `css` // which would result in `style.min-suffix.css` which isn't what we // want. - let mut iter = filename.splitn(2, '.'); - let base = iter.next().unwrap(); - let ext = iter.next().unwrap(); - let filename = format!("{}{}.{}", base, self.shared.resource_suffix, ext,); + let (base, ext) = filename.split_once('.').unwrap(); + let filename = format!("{}{}.{}", base, self.shared.resource_suffix, ext); self.dst.join(&filename) } } diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index 26bf4b569ff..f851d1a2372 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -16,6 +16,7 @@ #![feature(once_cell)] #![feature(type_ascription)] #![feature(split_inclusive)] +#![feature(str_split_once)] #![recursion_limit = "256"] #[macro_use] diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 551c086a8d4..fdbab74be50 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -435,8 +435,9 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { // Try looking for methods and associated items. let mut split = path_str.rsplitn(2, "::"); - // this can be an `unwrap()` because we ensure the link is never empty - let (item_str, item_name) = split.next().map(|i| (i, Symbol::intern(i))).unwrap(); + // NB: the `splitn`'s first element is always defined, even if the delimiter is not present. + let item_str = split.next().unwrap(); + let item_name = Symbol::intern(item_str); let path_root = split .next() .map(|f| f.to_owned()) @@ -447,7 +448,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ResolutionFailure::NotResolved { module_id, partial_res: None, - unresolved: item_str.into(), + unresolved: path_str.into(), } })?; -- cgit 1.4.1-3-g733a5 From f68cc68e798edfb1f5cc1913e97b34b10ee8791f Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Mon, 7 Dec 2020 15:07:24 -0700 Subject: Review feedback for collect_intra_doc_links.rs * Add assertion value is defined. * Simplify comment. * Fix bad change in err message. --- src/librustdoc/passes/collect_intra_doc_links.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index fdbab74be50..5ce64c4cd83 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -435,8 +435,9 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { // Try looking for methods and associated items. let mut split = path_str.rsplitn(2, "::"); - // NB: the `splitn`'s first element is always defined, even if the delimiter is not present. + // NB: `split`'s first element is always defined, even if the delimiter was not present. let item_str = split.next().unwrap(); + assert!(!item_str.is_empty()); let item_name = Symbol::intern(item_str); let path_root = split .next() @@ -448,7 +449,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ResolutionFailure::NotResolved { module_id, partial_res: None, - unresolved: path_str.into(), + unresolved: item_str.into(), } })?; -- cgit 1.4.1-3-g733a5 From 989edf4a5ffb0944e173ec23cb5614c252e8082e Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Tue, 8 Dec 2020 12:50:52 -0700 Subject: Review feedback * Use a match statement. * Clarify why we can't use `file_stem()`. * Error if the `:` is missing for Tidy error codes, rather than no-oping. --- compiler/rustc_mir/src/transform/coverage/debug.rs | 68 ++++++++++++---------- src/tools/tidy/src/error_codes_check.rs | 14 +++-- src/tools/tidy/src/ui_tests.rs | 3 + 3 files changed, 49 insertions(+), 36 deletions(-) (limited to 'src') diff --git a/compiler/rustc_mir/src/transform/coverage/debug.rs b/compiler/rustc_mir/src/transform/coverage/debug.rs index 1347656c23e..af81d9af0e2 100644 --- a/compiler/rustc_mir/src/transform/coverage/debug.rs +++ b/compiler/rustc_mir/src/transform/coverage/debug.rs @@ -152,38 +152,42 @@ impl DebugOptions { None => (setting_str, None), Some((k, v)) => (k, Some(v)), }; - if option == "allow_unused_expressions" { - allow_unused_expressions = bool_option_val(option, value); - debug!( - "{} env option `allow_unused_expressions` is set to {}", - RUSTC_COVERAGE_DEBUG_OPTIONS, allow_unused_expressions - ); - } else if option == "counter_format" { - match value { - None => { - bug!( - "`{}` option in environment variable {} requires one or more \ - plus-separated choices (a non-empty subset of \ - `id+block+operation`)", - option, - RUSTC_COVERAGE_DEBUG_OPTIONS - ); - } - Some(val) => { - counter_format = counter_format_option_val(val); - debug!( - "{} env option `counter_format` is set to {:?}", - RUSTC_COVERAGE_DEBUG_OPTIONS, counter_format - ); - } - }; - } else { - bug!( - "Unsupported setting `{}` in environment variable {}", - option, - RUSTC_COVERAGE_DEBUG_OPTIONS - ) - } + match option { + "allow_unused_expressions" => { + allow_unused_expressions = bool_option_val(option, value); + debug!( + "{} env option `allow_unused_expressions` is set to {}", + RUSTC_COVERAGE_DEBUG_OPTIONS, allow_unused_expressions + ); + } + "counter_format" => { + match value { + None => { + bug!( + "`{}` option in environment variable {} requires one or more \ + plus-separated choices (a non-empty subset of \ + `id+block+operation`)", + option, + RUSTC_COVERAGE_DEBUG_OPTIONS + ); + } + Some(val) => { + counter_format = counter_format_option_val(val); + debug!( + "{} env option `counter_format` is set to {:?}", + RUSTC_COVERAGE_DEBUG_OPTIONS, counter_format + ); + } + }; + } + _ => { + bug!( + "Unsupported setting `{}` in environment variable {}", + option, + RUSTC_COVERAGE_DEBUG_OPTIONS + ) + } + }; } } diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index 1c175901be8..a7199fdfce6 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -85,10 +85,16 @@ fn extract_error_codes( for line in f.lines() { let s = line.trim(); if !reached_no_explanation && s.starts_with('E') && s.contains("include_str!(\"") { - let err_code = match s.split_once(':') { - None => continue, - Some((err_code, _)) => err_code.to_owned(), - }; + let err_code = s + .split_once(':') + .expect( + format!( + "Expected a line with the format `E0xxx: include_str!(\"..\")`, but got {} without a `:` delimiter", + s, + ).as_str() + ) + .0 + .to_owned(); if !error_codes.contains_key(&err_code) { error_codes.insert(err_code.clone(), false); } diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 4c8c1d58ed7..03f4efea983 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -19,6 +19,9 @@ pub fn check(path: &Path, bad: &mut bool) { // // For now, just make sure that there is a corresponding // `$testname.rs` file. + // + // NB: We do not use file_stem() as some file names have multiple `.`s and we + // must strip all of them. let testname = file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0; if !file_path.with_file_name(testname).with_extension("rs").exists() { -- cgit 1.4.1-3-g733a5