Rollup merge of #147431 - Zalathar:directive, r=jieyouxu

compiletest: Read the whole test file before parsing directives

Few tests are larger than a handful of kilobytes, and nowadays we scan the whole file for directives anyway, so there's little reason not to just read the whole thing up-front.

This avoids having to deal with I/O within `iter_directives`, which should make it easier to overhaul directive processing.

r? jieyouxu

Author: matthiaskrgr

src/tools/compiletest/src/directives.rs

@@ -1,9 +1,6 @@
 use std::collections::HashSet;
-use std::env;
-use std::fs::File;
-use std::io::BufReader;
-use std::io::prelude::*;
 use std::process::Command;
+use std::{env, fs};
 
 use camino::{Utf8Path, Utf8PathBuf};
 use semver::Version;
@@ -54,18 +51,19 @@ pub struct EarlyProps {
 
 impl EarlyProps {
     pub fn from_file(config: &Config, testfile: &Utf8Path) -> Self {
-        let file = File::open(testfile.as_std_path()).expect("open test file to parse earlyprops");
-        Self::from_reader(config, testfile, file)
+        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_reader<R: Read>(config: &Config, testfile: &Utf8Path, rdr: R) -> Self {
+    pub fn from_file_contents(config: &Config, testfile: &Utf8Path, file_contents: &str) -> Self {
         let mut props = EarlyProps::default();
         let mut poisoned = false;
         iter_directives(
             config.mode,
             &mut poisoned,
             testfile,
-            rdr,
+            file_contents,
             // (dummy comment to force args into vertical layout)
             &mut |ref ln: DirectiveLine<'_>| {
                 parse_and_update_aux(config, ln, testfile, &mut props.aux);
@@ -362,15 +360,15 @@ impl TestProps {
     fn load_from(&mut self, testfile: &Utf8Path, test_revision: Option<&str>, config: &Config) {
         let mut has_edition = false;
         if !testfile.is_dir() {
-            let file = File::open(testfile.as_std_path()).unwrap();
+            let file_contents = fs::read_to_string(testfile).unwrap();
 
             let mut poisoned = false;
 
             iter_directives(
                 config.mode,
                 &mut poisoned,
                 testfile,
-                file,
+                &file_contents,
                 &mut |ref ln: DirectiveLine<'_>| {
                     if !ln.applies_to_test_revision(test_revision) {
                         return;
@@ -859,7 +857,7 @@ fn iter_directives(
     mode: TestMode,
     poisoned: &mut bool,
     testfile: &Utf8Path,
-    rdr: impl Read,
+    file_contents: &str,
     it: &mut dyn FnMut(DirectiveLine<'_>),
 ) {
     if testfile.is_dir() {
@@ -886,16 +884,7 @@ fn iter_directives(
         }
     }
 
-    let mut rdr = BufReader::with_capacity(1024, rdr);
-    let mut ln = String::new();
-    let mut line_number = 0;
-
-    loop {
-        line_number += 1;
-        ln.clear();
-        if rdr.read_line(&mut ln).unwrap() == 0 {
-            break;
-        }
+    for (line_number, ln) in (1..).zip(file_contents.lines()) {
         let ln = ln.trim();
 
         let Some(directive_line) = line_directive(line_number, ln) else {
@@ -1359,13 +1348,13 @@ where
     Some((min, max))
 }
 
-pub(crate) fn make_test_description<R: Read>(
+pub(crate) fn make_test_description(
     config: &Config,
     cache: &DirectivesCache,
     name: String,
     path: &Utf8Path,
     filterable_path: &Utf8Path,
-    src: R,
+    file_contents: &str,
     test_revision: Option<&str>,
     poisoned: &mut bool,
 ) -> CollectedTestDesc {
@@ -1380,7 +1369,7 @@ pub(crate) fn make_test_description<R: Read>(
         config.mode,
         &mut local_poisoned,
         path,
-        src,
+        file_contents,
         &mut |ref ln @ DirectiveLine { line_number, .. }| {
             if !ln.applies_to_test_revision(test_revision) {
                 return;

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

@@ -1,5 +1,3 @@
-use std::io::Read;
-
 use camino::Utf8Path;
 use semver::Version;
 
@@ -10,12 +8,12 @@ use crate::directives::{
 };
 use crate::executor::{CollectedTestDesc, ShouldPanic};
 
-fn make_test_description<R: Read>(
+fn make_test_description(
     config: &Config,
     name: String,
     path: &Utf8Path,
     filterable_path: &Utf8Path,
-    src: R,
+    file_contents: &str,
     revision: Option<&str>,
 ) -> CollectedTestDesc {
     let cache = DirectivesCache::load(config);
@@ -26,7 +24,7 @@ fn make_test_description<R: Read>(
         name,
         path,
         filterable_path,
-        src,
+        file_contents,
         revision,
         &mut poisoned,
     );
@@ -226,14 +224,13 @@ fn cfg() -> ConfigBuilder {
 }
 
 fn parse_rs(config: &Config, contents: &str) -> EarlyProps {
-    let bytes = contents.as_bytes();
-    EarlyProps::from_reader(config, Utf8Path::new("a.rs"), bytes)
+    EarlyProps::from_file_contents(config, Utf8Path::new("a.rs"), contents)
 }
 
 fn check_ignore(config: &Config, contents: &str) -> bool {
     let tn = String::new();
     let p = Utf8Path::new("a.rs");
-    let d = make_test_description(&config, tn, p, p, std::io::Cursor::new(contents), None);
+    let d = make_test_description(&config, tn, p, p, contents, None);
     d.ignore
 }
 
@@ -243,9 +240,9 @@ fn should_fail() {
     let tn = String::new();
     let p = Utf8Path::new("a.rs");
 
-    let d = make_test_description(&config, tn.clone(), p, p, std::io::Cursor::new(""), None);
+    let d = make_test_description(&config, tn.clone(), p, p, "", None);
     assert_eq!(d.should_panic, ShouldPanic::No);
-    let d = make_test_description(&config, tn, p, p, std::io::Cursor::new("//@ should-fail"), None);
+    let d = make_test_description(&config, tn, p, p, "//@ should-fail", None);
     assert_eq!(d.should_panic, ShouldPanic::Yes);
 }
 
@@ -778,9 +775,8 @@ fn threads_support() {
     }
 }
 
-fn run_path(poisoned: &mut bool, path: &Utf8Path, buf: &[u8]) {
-    let rdr = std::io::Cursor::new(&buf);
-    iter_directives(TestMode::Ui, poisoned, path, rdr, &mut |_| {});
+fn run_path(poisoned: &mut bool, path: &Utf8Path, file_contents: &str) {
+    iter_directives(TestMode::Ui, poisoned, path, file_contents, &mut |_| {});
 }
 
 #[test]
@@ -789,7 +785,7 @@ fn test_unknown_directive_check() {
     run_path(
         &mut poisoned,
         Utf8Path::new("a.rs"),
-        include_bytes!("./test-auxillary/unknown_directive.rs"),
+        include_str!("./test-auxillary/unknown_directive.rs"),
     );
     assert!(poisoned);
 }
@@ -800,7 +796,7 @@ fn test_known_directive_check_no_error() {
     run_path(
         &mut poisoned,
         Utf8Path::new("a.rs"),
-        include_bytes!("./test-auxillary/known_directive.rs"),
+        include_str!("./test-auxillary/known_directive.rs"),
     );
     assert!(!poisoned);
 }
@@ -811,7 +807,7 @@ fn test_error_annotation_no_error() {
     run_path(
         &mut poisoned,
         Utf8Path::new("a.rs"),
-        include_bytes!("./test-auxillary/error_annotation.rs"),
+        include_str!("./test-auxillary/error_annotation.rs"),
     );
     assert!(!poisoned);
 }
@@ -822,29 +818,29 @@ fn test_non_rs_unknown_directive_not_checked() {
     run_path(
         &mut poisoned,
         Utf8Path::new("a.Makefile"),
-        include_bytes!("./test-auxillary/not_rs.Makefile"),
+        include_str!("./test-auxillary/not_rs.Makefile"),
     );
     assert!(!poisoned);
 }
 
 #[test]
 fn test_trailing_directive() {
     let mut poisoned = false;
-    run_path(&mut poisoned, Utf8Path::new("a.rs"), b"//@ only-x86 only-arm");
+    run_path(&mut poisoned, Utf8Path::new("a.rs"), "//@ only-x86 only-arm");
     assert!(poisoned);
 }
 
 #[test]
 fn test_trailing_directive_with_comment() {
     let mut poisoned = false;
-    run_path(&mut poisoned, Utf8Path::new("a.rs"), b"//@ only-x86   only-arm with comment");
+    run_path(&mut poisoned, Utf8Path::new("a.rs"), "//@ only-x86   only-arm with comment");
     assert!(poisoned);
 }
 
 #[test]
 fn test_not_trailing_directive() {
     let mut poisoned = false;
-    run_path(&mut poisoned, Utf8Path::new("a.rs"), b"//@ revisions: incremental");
+    run_path(&mut poisoned, Utf8Path::new("a.rs"), "//@ revisions: incremental");
     assert!(!poisoned);
 }
 

src/tools/compiletest/src/lib.rs

@@ -892,7 +892,8 @@ 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 src_file = fs::File::open(&test_path).expect("open test file to parse ignores");
+        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.
@@ -904,7 +905,7 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
             test_name,
             &test_path,
             &filterable_path,
-            src_file,
+            &file_contents,
             revision,
             &mut collector.poisoned,
         );