Rollup merge of #121373 - Zalathar:test-revision, r=oli-obk

Consistently refer to a test's `revision` instead of `cfg`

Compiletest allows a test file to specify multiple “revisions” (`//@ revisions: foo bar`), with each revision running as a separate test, and having the ability to define revision-specific headers (`//`@[foo]` ignore-blah`) and revision-specific code (`#[cfg(foo)]`).

The code that implements this feature sometimes uses the term “cfg” instead of “revision”. This results in two confusingly-different names for the same concept, one of which is ambiguous with other kinds of configuration (such as compiletest's own config).

This PR replaces those occurrences of `cfg` with `revision`, so that one name is used consistently.

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
+/// If revision is not None, then we look
-/// for `//[X]~` instead, where `X` is the current `cfg`.
+/// for `//[X]~` instead, where `X` is the current revision.
-pub fn load_errors(testfile: &Path, cfg: Option<&str>) -> Vec<Error> {
+pub fn load_errors(testfile: &Path, revision: Option<&str>) -> Vec<Error> {
     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<Error> {
     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<usize>,
     line_num: usize,
     line: &str,
-    cfg: Option<&str>,
+    test_revision: Option<&str>,
 ) -> Option<(WhichLine, Error)> {
     // Matches comments like:
     //     //~
     //     //~|
     //     //~^
     //     //~^^^^^
-    //     //[cfg1]~
+    //     //[rev1]~
-    //     //[cfg1,cfg2]~^^
+    //     //[rev1,rev2]~^^
     static RE: Lazy<Regex> =
-        Lazy::new(|| Regex::new(r"//(?:\[(?P<cfgs>[\w,]+)])?~(?P<adjust>\||\^*)").unwrap());
+        Lazy::new(|| Regex::new(r"//(?:\[(?P<revs>[\w,]+)])?~(?P<adjust>\||\^*)").unwrap());
 
     let captures = RE.captures(line)?;
 
-    match (cfg, captures.name("cfgs")) {
+    match (test_revision, captures.name("revs")) {
-        // Only error messages that contain our `cfg` between the square brackets apply to us.
+        // Only error messages that contain our revision between the square brackets apply to us.
-        (Some(cfg), Some(filter)) if !filter.as_str().split(',').any(|s| s == cfg) => return None,
+        (Some(test_revision), Some(revision_filters)) => {
-        (Some(_), Some(_)) => {}
+            if !revision_filters.as_str().split(',').any(|r| r == test_revision) {
+                return None;
+            }
+        }
 
         (None, Some(_)) => panic!("Only tests with revisions should use `//[X]~`"),
 

src/tools/compiletest/src/header.rs

@@ -289,20 +289,20 @@ impl TestProps {
         }
     }
 
-    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 @@ impl TestProps {
 
     /// 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 @@ impl TestProps {
                 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 @@ impl TestProps {
                         &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,30 +645,27 @@ impl TestProps {
     }
 }
 
-/// Extract a `(Option<line_config>, directive)` directive from a line if comment is present.
+/// Extract an `(Option<line_revision>, directive)` directive from a line if comment is present.
+///
+/// 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();
+    // Ignore lines that don't start with the comment prefix.
-    if ln.starts_with(comment) {
+    let after_comment = original_line.trim_start().strip_prefix(comment)?.trim_start();
-        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
-                );
-            };
 
-            let lncfg = &ln[1..close_brace];
+    if let Some(after_open_bracket) = after_comment.strip_prefix('[') {
-            Some((Some(lncfg), ln[(close_brace + 1)..].trim_start()))
+        // A comment like `//@[foo]` only applies to revision `foo`.
-        } else {
+        let Some((line_revision, directive)) = after_open_bracket.split_once(']') else {
-            Some((None, ln))
+            panic!(
-        }
+                "malformed condition directive: expected `{comment}[foo]`, found `{original_line}`"
+            )
+        };
+
+        Some((Some(line_revision), directive.trim_start()))
     } else {
-        None
+        Some((None, after_comment))
     }
 }
 
@@ -790,16 +787,32 @@ const DIAGNOSTICS_DIRECTIVE_NAMES: &[&str] = &[
     "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.
+    line_number: usize,
-    header_revision: Option<&'ln str>,
     /// 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
+    /// Some header directives start with a revision name in square brackets
-    /// (`//` or `//@` or `#`) and revision (if any) have been stripped.
+    /// (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 +844,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 +878,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;
@@ -1158,7 +1171,7 @@ pub fn make_test_description<R: Read>(
     name: test::TestName,
     path: &Path,
     src: R,
-    cfg: Option<&str>,
+    test_revision: Option<&str>,
     poisoned: &mut bool,
 ) -> test::TestDesc {
     let mut ignore = false;
@@ -1174,7 +1187,7 @@ pub fn make_test_description<R: Read>(
         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;
             }
 

src/tools/compiletest/src/header/tests.rs

@@ -10,12 +10,19 @@ fn make_test_description<R: Read>(
     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 =
+    let test = crate::header::make_test_description(
-        crate::header::make_test_description(config, &cache, name, path, src, cfg, &mut poisoned);
+        config,
+        &cache,
+        name,
+        path,
+        src,
+        revision,
+        &mut poisoned,
+    );
     if poisoned {
         panic!("poisoned!");
     }

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(
+                desc.ignore |= is_up_to_date(&config, testpaths, &early_props, revision, inputs);
-                    &config,
-                    testpaths,
-                    &early_props,
-                    revision.map(|s| s.as_str()),
-                    inputs,
-                );
             }
             test::TestDescAndFn {
                 desc,
@@ -879,7 +872,7 @@ impl Stamp {
 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<Config>,
     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(())