Rollup merge of rust-lang#147522 - Zalathar:directive, r=jieyouxu

compiletest: Use the same directive lines for EarlyProps and ignore/only/needs

Currently we load each discovered test file to scan it for directives once for EarlyProps parsing, then reload and scan it once *per revision* for ignore processing. If a revision is not ignored, we then reload and scan it again during actual execution.

That's a bit silly, so this PR tries to reduce the number of unnecessary file loads and line scans for directive parsing, by reusing the same collection of `DirectiveLine` values for EarlyProps and for each revision's ignores.

Each individual directive still needs to be re-parsed a bunch of times, but those steps can at least avoid scanning the whole file, or having to split out names from values.

---

There's more that could be done after this, such as only doing known-directive checks once per file, or embedding file paths in each `DirectiveLine`, but I decided to stop here to allow review in modest chunks.

r? jieyouxu

Author: dianqk

src/tools/compiletest/src/directives.rs

@@ -12,6 +12,7 @@ use crate::directives::auxiliary::{AuxProps, parse_and_update_aux};
 use crate::directives::directive_names::{
     KNOWN_DIRECTIVE_NAMES, KNOWN_HTMLDOCCK_DIRECTIVE_NAMES, KNOWN_JSONDOCCK_DIRECTIVE_NAMES,
 };
+pub(crate) use crate::directives::file::FileDirectives;
 use crate::directives::line::{DirectiveLine, line_directive};
 use crate::directives::needs::CachedNeedsConditions;
 use crate::edition::{Edition, parse_edition};
@@ -23,6 +24,7 @@ use crate::{fatal, help};
 pub(crate) mod auxiliary;
 mod cfg;
 mod directive_names;
+mod file;
 mod line;
 mod needs;
 #[cfg(test)]
@@ -41,31 +43,29 @@ impl DirectivesCache {
 /// Properties which must be known very early, before actually running
 /// the test.
 #[derive(Default)]
-pub struct EarlyProps {
+pub(crate) struct EarlyProps {
     /// Auxiliary crates that should be built and made available to this test.
     /// Included in [`EarlyProps`] so that the indicated files can participate
     /// in up-to-date checking. Building happens via [`TestProps::aux`] instead.
     pub(crate) aux: AuxProps,
-    pub revisions: Vec<String>,
+    pub(crate) revisions: Vec<String>,
 }
 
 impl EarlyProps {
-    pub fn from_file(config: &Config, testfile: &Utf8Path) -> Self {
-        let file_contents =
-            fs::read_to_string(testfile).expect("read test file to parse earlyprops");
-        Self::from_file_contents(config, testfile, &file_contents)
-    }
-
-    pub fn from_file_contents(config: &Config, testfile: &Utf8Path, file_contents: &str) -> Self {
+    pub(crate) fn from_file_directives(
+        config: &Config,
+        file_directives: &FileDirectives<'_>,
+    ) -> Self {
+        let testfile = file_directives.path;
         let mut props = EarlyProps::default();
         let mut poisoned = false;
+
         iter_directives(
             config.mode,
             &mut poisoned,
-            testfile,
-            file_contents,
+            file_directives,
             // (dummy comment to force args into vertical layout)
-            &mut |ref ln: DirectiveLine<'_>| {
+            &mut |ln: &DirectiveLine<'_>| {
                 parse_and_update_aux(config, ln, testfile, &mut props.aux);
                 config.parse_and_update_revisions(testfile, ln, &mut props.revisions);
             },
@@ -358,15 +358,15 @@ impl TestProps {
         let mut has_edition = false;
         if !testfile.is_dir() {
             let file_contents = fs::read_to_string(testfile).unwrap();
+            let file_directives = FileDirectives::from_file_contents(testfile, &file_contents);
 
             let mut poisoned = false;
 
             iter_directives(
                 config.mode,
                 &mut poisoned,
-                testfile,
-                &file_contents,
-                &mut |ref ln: DirectiveLine<'_>| {
+                &file_directives,
+                &mut |ln: &DirectiveLine<'_>| {
                     if !ln.applies_to_test_revision(test_revision) {
                         return;
                     }
@@ -851,13 +851,10 @@ fn check_directive<'a>(
 fn iter_directives(
     mode: TestMode,
     poisoned: &mut bool,
-    testfile: &Utf8Path,
-    file_contents: &str,
-    it: &mut dyn FnMut(DirectiveLine<'_>),
+    file_directives: &FileDirectives<'_>,
+    it: &mut dyn FnMut(&DirectiveLine<'_>),
 ) {
-    if testfile.is_dir() {
-        return;
-    }
+    let testfile = file_directives.path;
 
     // Coverage tests in coverage-run mode always have these extra directives, without needing to
     // specify them manually in every test file.
@@ -875,21 +872,15 @@ fn iter_directives(
         for directive_str in extra_directives {
             let directive_line = line_directive(0, directive_str)
                 .unwrap_or_else(|| panic!("bad extra-directive line: {directive_str:?}"));
-            it(directive_line);
+            it(&directive_line);
         }
     }
 
-    for (line_number, ln) in (1..).zip(file_contents.lines()) {
-        let ln = ln.trim();
-
-        let Some(directive_line) = line_directive(line_number, ln) else {
-            continue;
-        };
-
+    for directive_line @ &DirectiveLine { line_number, .. } in &file_directives.lines {
         // Perform unknown directive check on Rust files.
         if testfile.extension() == Some("rs") {
             let CheckDirectiveResult { is_known_directive, trailing_directive } =
-                check_directive(&directive_line, mode);
+                check_directive(directive_line, mode);
 
             if !is_known_directive {
                 *poisoned = true;
@@ -1349,7 +1340,7 @@ pub(crate) fn make_test_description(
     name: String,
     path: &Utf8Path,
     filterable_path: &Utf8Path,
-    file_contents: &str,
+    file_directives: &FileDirectives<'_>,
     test_revision: Option<&str>,
     poisoned: &mut bool,
 ) -> CollectedTestDesc {
@@ -1363,9 +1354,8 @@ pub(crate) fn make_test_description(
     iter_directives(
         config.mode,
         &mut local_poisoned,
-        path,
-        file_contents,
-        &mut |ref ln @ DirectiveLine { line_number, .. }| {
+        file_directives,
+        &mut |ln @ &DirectiveLine { line_number, .. }| {
             if !ln.applies_to_test_revision(test_revision) {
                 return;
             }

src/tools/compiletest/src/directives/file.rs

@@ -0,0 +1,24 @@
+use camino::Utf8Path;
+
+use crate::directives::line::{DirectiveLine, line_directive};
+
+pub(crate) struct FileDirectives<'a> {
+    pub(crate) path: &'a Utf8Path,
+    pub(crate) lines: Vec<DirectiveLine<'a>>,
+}
+
+impl<'a> FileDirectives<'a> {
+    pub(crate) fn from_file_contents(path: &'a Utf8Path, file_contents: &'a str) -> Self {
+        let mut lines = vec![];
+
+        for (line_number, ln) in (1..).zip(file_contents.lines()) {
+            let ln = ln.trim();
+
+            if let Some(directive_line) = line_directive(line_number, ln) {
+                lines.push(directive_line);
+            }
+        }
+
+        Self { path, lines }
+    }
+}

src/tools/compiletest/src/directives/tests.rs

@@ -3,7 +3,7 @@ use semver::Version;
 
 use crate::common::{Config, Debugger, TestMode};
 use crate::directives::{
-    DirectivesCache, EarlyProps, Edition, EditionRange, extract_llvm_version,
+    DirectivesCache, EarlyProps, Edition, EditionRange, FileDirectives, extract_llvm_version,
     extract_version_range, iter_directives, line_directive, parse_edition, parse_normalize_rule,
 };
 use crate::executor::{CollectedTestDesc, ShouldPanic};
@@ -18,13 +18,15 @@ fn make_test_description(
 ) -> CollectedTestDesc {
     let cache = DirectivesCache::load(config);
     let mut poisoned = false;
+    let file_directives = FileDirectives::from_file_contents(path, file_contents);
+
     let test = crate::directives::make_test_description(
         config,
         &cache,
         name,
         path,
         filterable_path,
-        file_contents,
+        &file_directives,
         revision,
         &mut poisoned,
     );
@@ -224,7 +226,8 @@ fn cfg() -> ConfigBuilder {
 }
 
 fn parse_rs(config: &Config, contents: &str) -> EarlyProps {
-    EarlyProps::from_file_contents(config, Utf8Path::new("a.rs"), contents)
+    let file_directives = FileDirectives::from_file_contents(Utf8Path::new("a.rs"), contents);
+    EarlyProps::from_file_directives(config, &file_directives)
 }
 
 fn check_ignore(config: &Config, contents: &str) -> bool {
@@ -776,7 +779,8 @@ fn threads_support() {
 }
 
 fn run_path(poisoned: &mut bool, path: &Utf8Path, file_contents: &str) {
-    iter_directives(TestMode::Ui, poisoned, path, file_contents, &mut |_| {});
+    let file_directives = FileDirectives::from_file_contents(path, file_contents);
+    iter_directives(TestMode::Ui, poisoned, &file_directives, &mut |_| {});
 }
 
 #[test]

src/tools/compiletest/src/lib.rs

@@ -41,7 +41,7 @@ use crate::common::{
     CodegenBackend, CompareMode, Config, Debugger, PassMode, TestMode, TestPaths, UI_EXTENSIONS,
     expected_output_path, output_base_dir, output_relative_path,
 };
-use crate::directives::DirectivesCache;
+use crate::directives::{DirectivesCache, FileDirectives};
 use crate::edition::parse_edition;
 use crate::executor::{CollectedTest, ColorConfig};
 
@@ -868,7 +868,10 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
     };
 
     // Scan the test file to discover its revisions, if any.
-    let early_props = EarlyProps::from_file(&cx.config, &test_path);
+    let file_contents =
+        fs::read_to_string(&test_path).expect("reading test file for directives should succeed");
+    let file_directives = FileDirectives::from_file_contents(&test_path, &file_contents);
+    let early_props = EarlyProps::from_file_directives(&cx.config, &file_directives);
 
     // Normally we create one structure per revision, with two exceptions:
     // - If a test doesn't use revisions, create a dummy revision (None) so that
@@ -886,8 +889,6 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
     // `CollectedTest` that can be handed over to the test executor.
     collector.tests.extend(revisions.into_iter().map(|revision| {
         // Create a test name and description to hand over to the executor.
-        let file_contents =
-            fs::read_to_string(&test_path).expect("read test file to parse ignores");
         let (test_name, filterable_path) =
             make_test_name_and_filterable_path(&cx.config, testpaths, revision);
         // Create a description struct for the test/revision.
@@ -899,7 +900,7 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
             test_name,
             &test_path,
             &filterable_path,
-            &file_contents,
+            &file_directives,
             revision,
             &mut collector.poisoned,
         );