From e13410225be8aad07da0a7853504e95185b0bc10 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 20 Feb 2024 13:31:57 +1100 Subject: [PATCH 1/3] Improve internal docs for the `HeaderLine` callback struct --- src/tools/compiletest/src/header.rs | 34 ++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 7f765fd86c8..0e013d64b4d 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -646,6 +646,8 @@ pub fn local_pass_mode(&self) -> Option { } /// Extract a `(Option, directive)` directive from a line if comment is present. +/// +/// See [`HeaderLine`] for a diagram. pub fn line_directive<'line>( comment: &str, ln: &'line str, @@ -790,16 +792,32 @@ pub fn line_directive<'line>( "unset-rustc-env", ]; -/// Arguments passed to the callback in [`iter_header`]. +/// The broken-down contents of a line containing a test header directive, +/// which [`iter_header`] passes to its callback function. +/// +/// For example: +/// +/// ```text +/// //@ compile-flags: -O +/// ^^^^^^^^^^^^^^^^^ directive +/// ^^^^^^^^^^^^^^^^^^^^^ original_line +/// +/// //@ [foo] compile-flags: -O +/// ^^^ header_revision +/// ^^^^^^^^^^^^^^^^^ directive +/// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ original_line +/// ``` struct HeaderLine<'ln> { - /// Contents of the square brackets preceding this header, if present. - header_revision: Option<&'ln str>, + line_number: usize, /// Raw line from the test file, including comment prefix and any revision. original_line: &'ln str, - /// Remainder of the directive line, after the initial comment prefix - /// (`//` or `//@` or `#`) and revision (if any) have been stripped. + /// Some header directives start with a revision name in square brackets + /// (e.g. `[foo]`), and only apply to that revision of the test. + /// If present, this field contains the revision name (e.g. `foo`). + header_revision: Option<&'ln str>, + /// The main part of the header directive, after removing the comment prefix + /// and the optional revision specifier. directive: &'ln str, - line_number: usize, } fn iter_header( @@ -831,7 +849,7 @@ fn iter_header( ]; // Process the extra implied directives, with a dummy line number of 0. for directive in extra_directives { - it(HeaderLine { header_revision: None, original_line: "", directive, line_number: 0 }); + it(HeaderLine { line_number: 0, original_line: "", header_revision: None, directive }); } } @@ -865,7 +883,7 @@ fn iter_header( // First try to accept `ui_test` style comments } else if let Some((header_revision, directive)) = line_directive(comment, ln) { - it(HeaderLine { header_revision, original_line, directive, line_number }); + it(HeaderLine { line_number, original_line, header_revision, directive }); } else if mode == Mode::Ui && suite == "ui" && !REVISION_MAGIC_COMMENT_RE.is_match(ln) { let Some((_, rest)) = line_directive("//", ln) else { continue; From 99fb653d1d0f79961506413de549bebd7b798ba8 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 17 Feb 2024 23:58:29 +1100 Subject: [PATCH 2/3] Consistently refer to a test's `revision` instead of `cfg` Compiletest code sometimes refers to a test's revision as its `cfg`. This results in two different names for the same thing, one of which is ambiguous with other kinds of configuration (such as compiletest's own config). This patch replaces those occurrences of `cfg` with `revision`. --- src/tools/compiletest/src/errors.rs | 27 +++++++++++++---------- src/tools/compiletest/src/header.rs | 26 +++++++++++----------- src/tools/compiletest/src/header/tests.rs | 13 ++++++++--- src/tools/compiletest/src/lib.rs | 19 +++++----------- 4 files changed, 44 insertions(+), 41 deletions(-) diff --git a/src/tools/compiletest/src/errors.rs b/src/tools/compiletest/src/errors.rs index e0ec76aa027..140c3aa0a64 100644 --- a/src/tools/compiletest/src/errors.rs +++ b/src/tools/compiletest/src/errors.rs @@ -72,9 +72,9 @@ enum WhichLine { /// and also //~^ ERROR message one for the preceding line, and /// //~| ERROR message two for that same line. /// -/// If cfg is not None (i.e., in an incremental test), then we look -/// for `//[X]~` instead, where `X` is the current `cfg`. -pub fn load_errors(testfile: &Path, cfg: Option<&str>) -> Vec { +/// If revision is not None, then we look +/// for `//[X]~` instead, where `X` is the current revision. +pub fn load_errors(testfile: &Path, revision: Option<&str>) -> Vec { let rdr = BufReader::new(File::open(testfile).unwrap()); // `last_nonfollow_error` tracks the most recently seen @@ -90,7 +90,7 @@ pub fn load_errors(testfile: &Path, cfg: Option<&str>) -> Vec { rdr.lines() .enumerate() .filter_map(|(line_num, line)| { - parse_expected(last_nonfollow_error, line_num + 1, &line.unwrap(), cfg).map( + parse_expected(last_nonfollow_error, line_num + 1, &line.unwrap(), revision).map( |(which, error)| { match which { FollowPrevious(_) => {} @@ -108,24 +108,27 @@ fn parse_expected( last_nonfollow_error: Option, line_num: usize, line: &str, - cfg: Option<&str>, + test_revision: Option<&str>, ) -> Option<(WhichLine, Error)> { // Matches comments like: // //~ // //~| // //~^ // //~^^^^^ - // //[cfg1]~ - // //[cfg1,cfg2]~^^ + // //[rev1]~ + // //[rev1,rev2]~^^ static RE: Lazy = - Lazy::new(|| Regex::new(r"//(?:\[(?P[\w,]+)])?~(?P\||\^*)").unwrap()); + Lazy::new(|| Regex::new(r"//(?:\[(?P[\w,]+)])?~(?P\||\^*)").unwrap()); let captures = RE.captures(line)?; - match (cfg, captures.name("cfgs")) { - // Only error messages that contain our `cfg` between the square brackets apply to us. - (Some(cfg), Some(filter)) if !filter.as_str().split(',').any(|s| s == cfg) => return None, - (Some(_), Some(_)) => {} + match (test_revision, captures.name("revs")) { + // Only error messages that contain our revision between the square brackets apply to us. + (Some(test_revision), Some(revision_filters)) => { + if !revision_filters.as_str().split(',').any(|r| r == test_revision) { + return None; + } + } (None, Some(_)) => panic!("Only tests with revisions should use `//[X]~`"), diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 0e013d64b4d..50e8cfd60a9 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -289,20 +289,20 @@ pub fn new() -> Self { } } - pub fn from_aux_file(&self, testfile: &Path, cfg: Option<&str>, config: &Config) -> Self { + pub fn from_aux_file(&self, testfile: &Path, revision: Option<&str>, config: &Config) -> Self { let mut props = TestProps::new(); // copy over select properties to the aux build: props.incremental_dir = self.incremental_dir.clone(); props.ignore_pass = true; - props.load_from(testfile, cfg, config); + props.load_from(testfile, revision, config); props } - pub fn from_file(testfile: &Path, cfg: Option<&str>, config: &Config) -> Self { + pub fn from_file(testfile: &Path, revision: Option<&str>, config: &Config) -> Self { let mut props = TestProps::new(); - props.load_from(testfile, cfg, config); + props.load_from(testfile, revision, config); match (props.pass_mode, props.fail_mode) { (None, None) if config.mode == Mode::Ui => props.fail_mode = Some(FailMode::Check), @@ -315,9 +315,9 @@ pub fn from_file(testfile: &Path, cfg: Option<&str>, config: &Config) -> Self { /// Loads properties from `testfile` into `props`. If a property is /// tied to a particular revision `foo` (indicated by writing - /// `//[foo]`), then the property is ignored unless `cfg` is + /// `//@[foo]`), then the property is ignored unless `test_revision` is /// `Some("foo")`. - fn load_from(&mut self, testfile: &Path, cfg: Option<&str>, config: &Config) { + fn load_from(&mut self, testfile: &Path, test_revision: Option<&str>, config: &Config) { let mut has_edition = false; if !testfile.is_dir() { let file = File::open(testfile).unwrap(); @@ -331,7 +331,7 @@ fn load_from(&mut self, testfile: &Path, cfg: Option<&str>, config: &Config) { testfile, file, &mut |HeaderLine { header_revision, directive: ln, .. }| { - if header_revision.is_some() && header_revision != cfg { + if header_revision.is_some() && header_revision != test_revision { return; } @@ -455,7 +455,7 @@ fn split_flags(flags: &str) -> Vec { &mut self.check_test_line_numbers_match, ); - self.update_pass_mode(ln, cfg, config); + self.update_pass_mode(ln, test_revision, config); self.update_fail_mode(ln, config); config.set_name_directive(ln, IGNORE_PASS, &mut self.ignore_pass); @@ -645,7 +645,7 @@ pub fn local_pass_mode(&self) -> Option { } } -/// Extract a `(Option, directive)` directive from a line if comment is present. +/// Extract an `(Option, directive)` directive from a line if comment is present. /// /// See [`HeaderLine`] for a diagram. pub fn line_directive<'line>( @@ -664,8 +664,8 @@ pub fn line_directive<'line>( ); }; - let lncfg = &ln[1..close_brace]; - Some((Some(lncfg), ln[(close_brace + 1)..].trim_start())) + let line_revision = &ln[1..close_brace]; + Some((Some(line_revision), ln[(close_brace + 1)..].trim_start())) } else { Some((None, ln)) } @@ -1176,7 +1176,7 @@ pub fn make_test_description( name: test::TestName, path: &Path, src: R, - cfg: Option<&str>, + test_revision: Option<&str>, poisoned: &mut bool, ) -> test::TestDesc { let mut ignore = false; @@ -1192,7 +1192,7 @@ pub fn make_test_description( path, src, &mut |HeaderLine { header_revision, original_line, directive: ln, line_number }| { - if header_revision.is_some() && header_revision != cfg { + if header_revision.is_some() && header_revision != test_revision { return; } diff --git a/src/tools/compiletest/src/header/tests.rs b/src/tools/compiletest/src/header/tests.rs index 274006ae8c1..f76fb406cea 100644 --- a/src/tools/compiletest/src/header/tests.rs +++ b/src/tools/compiletest/src/header/tests.rs @@ -10,12 +10,19 @@ fn make_test_description( name: test::TestName, path: &Path, src: R, - cfg: Option<&str>, + revision: Option<&str>, ) -> test::TestDesc { let cache = HeadersCache::load(config); let mut poisoned = false; - let test = - crate::header::make_test_description(config, &cache, name, path, src, cfg, &mut poisoned); + let test = crate::header::make_test_description( + config, + &cache, + name, + path, + src, + revision, + &mut poisoned, + ); if poisoned { panic!("poisoned!"); } diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 667358b1a6e..57a82eb37ed 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -745,7 +745,7 @@ fn make_test( let revisions = if early_props.revisions.is_empty() || config.mode == Mode::Incremental { vec![None] } else { - early_props.revisions.iter().map(Some).collect() + early_props.revisions.iter().map(|r| Some(r.as_str())).collect() }; revisions @@ -753,20 +753,13 @@ fn make_test( .map(|revision| { let src_file = std::fs::File::open(&test_path).expect("open test file to parse ignores"); - let cfg = revision.map(|v| &**v); let test_name = crate::make_test_name(&config, testpaths, revision); let mut desc = make_test_description( - &config, cache, test_name, &test_path, src_file, cfg, poisoned, + &config, cache, test_name, &test_path, src_file, revision, poisoned, ); // Ignore tests that already run and are up to date with respect to inputs. if !config.force_rerun { - desc.ignore |= is_up_to_date( - &config, - testpaths, - &early_props, - revision.map(|s| s.as_str()), - inputs, - ); + desc.ignore |= is_up_to_date(&config, testpaths, &early_props, revision, inputs); } test::TestDescAndFn { desc, @@ -879,7 +872,7 @@ fn add_dir(&mut self, path: &Path) { fn make_test_name( config: &Config, testpaths: &TestPaths, - revision: Option<&String>, + revision: Option<&str>, ) -> test::TestName { // Print the name of the file, relative to the repository root. // `src_base` looks like `/path/to/rust/tests/ui` @@ -907,11 +900,11 @@ fn make_test_name( fn make_test_closure( config: Arc, testpaths: &TestPaths, - revision: Option<&String>, + revision: Option<&str>, ) -> test::TestFn { let config = config.clone(); let testpaths = testpaths.clone(); - let revision = revision.cloned(); + let revision = revision.map(str::to_owned); test::DynTestFn(Box::new(move || { runtest::run(config, &testpaths, revision.as_deref()); Ok(()) From 544d09132bef3d483a0e10b4bb25e25079107e37 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 21 Feb 2024 11:49:01 +1100 Subject: [PATCH 3/3] Flatten the parse logic in `line_directive` --- src/tools/compiletest/src/header.rs | 31 ++++++++++++----------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 50e8cfd60a9..44e5d8dea7d 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -650,27 +650,22 @@ pub fn local_pass_mode(&self) -> Option { /// See [`HeaderLine`] for a diagram. pub fn line_directive<'line>( comment: &str, - ln: &'line str, + original_line: &'line str, ) -> Option<(Option<&'line str>, &'line str)> { - let ln = ln.trim_start(); - if ln.starts_with(comment) { - let ln = ln[comment.len()..].trim_start(); - if ln.starts_with('[') { - // A comment like `//[foo]` is specific to revision `foo` - let Some(close_brace) = ln.find(']') else { - panic!( - "malformed condition directive: expected `{}[foo]`, found `{}`", - comment, ln - ); - }; + // Ignore lines that don't start with the comment prefix. + let after_comment = original_line.trim_start().strip_prefix(comment)?.trim_start(); - let line_revision = &ln[1..close_brace]; - Some((Some(line_revision), ln[(close_brace + 1)..].trim_start())) - } else { - Some((None, ln)) - } + if let Some(after_open_bracket) = after_comment.strip_prefix('[') { + // A comment like `//@[foo]` only applies to revision `foo`. + let Some((line_revision, directive)) = after_open_bracket.split_once(']') else { + panic!( + "malformed condition directive: expected `{comment}[foo]`, found `{original_line}`" + ) + }; + + Some((Some(line_revision), directive.trim_start())) } else { - None + Some((None, after_comment)) } }