ignore some unsupported directives in checks

Author: asquared31415

src/tools/test_common/src/directives.rs

@@ -363,7 +363,7 @@ macro_rules! name_val_or_name_directive {
 // Macros are in the form (name, compiletest_name, ui_test_name).
 // If ui_test_name does not exist, ui_test does not support that directive.
 // ========================================================================
-name_value_directive!(ErrorPatternDirective, "error-in-other-file", "error-pattern");
+name_value_directive!(ErrorPatternDirective, "error-pattern", "error-in-other-file");
 name_value_directive!(CompileFlagsDirective, "compile-flags", "compile-flags");
 name_value_directive!(RunFlagsDirective, "run-flags"); // UNUSED IN UI TESTS
 name_value_directive!(PrettyModeDirective, "pretty-mode"); // UNUSED IN UI TESTS

src/tools/tidy/src/ui_tests.rs

@@ -192,18 +192,21 @@ fn check_ui_test_headers(bad: &mut bool, file_path: &Path, mode: HeaderCheckMode
     }
 }
 
+fn emit_error(file_path: &Path, action: &HeaderAction) {
+    tidy_error!(
+        &mut false,
+        "invalid test header\n    {}:{}\n    {}",
+        file_path.display(),
+        action.line_num(),
+        action.error_message()
+    );
+}
 /// Emits errors for the header lines specified. Returns whether any errors were emitted
 fn emit_header_errors(file_path: &Path, bad_lines: Vec<HeaderAction>) -> bool {
     let mut bad = false;
     for action in bad_lines {
-        let err = action.error_message();
-        tidy_error!(
-            &mut bad,
-            "invalid test header\n    {}:{}\n    {}",
-            file_path.display(),
-            action.line_num(),
-            err
-        );
+        emit_error(file_path, &action);
+        bad = true;
     }
     bad
 }
@@ -212,61 +215,65 @@ fn fix_header_errors(file_path: &Path, bad_lines: Vec<HeaderAction>) -> io::Resu
     // Process each header error into a replacement for a line.
     let line_replacements = bad_lines
         .into_iter()
-        .map(|header_action| {
-            (
-                header_action.line_num(),
-                match header_action.action() {
-                    LineAction::UseUiTestComment => {
-                        replace_compiletest_comment(header_action.line()).unwrap()
-                    }
-                    LineAction::MigrateToUiTest { compiletest_name, ui_test_name } => {
-                        // Replace comment type first, then the name range specified.
-                        let mut new_line =
-                            replace_compiletest_comment(header_action.line()).unwrap();
-                        // This is always a directive that contains the compiletest name.
-                        let name_start = new_line.find(compiletest_name.as_str()).unwrap();
-                        new_line.replace_range(
-                            name_start..(name_start + compiletest_name.len()),
-                            ui_test_name.as_str(),
-                        );
-                        new_line
-                    }
-                    LineAction::UseUITestName { compiletest_name, ui_test_name } => {
-                        // This is always a directive that contains the compiletest name.
-                        let name_start =
-                            header_action.line().find(compiletest_name.as_str()).unwrap();
-                        let mut new_line = header_action.line().to_string();
-                        new_line.replace_range(
-                            name_start..(name_start + compiletest_name.len()),
-                            ui_test_name.as_str(),
-                        );
-                        new_line
-                    }
-                    LineAction::Error { message } => todo!(),
-                },
-            )
+        .filter_map(|header_action| {
+            match header_action.action() {
+                LineAction::UseUiTestComment => Some((
+                    header_action.line_num(),
+                    replace_compiletest_comment(header_action.line()).unwrap(),
+                )),
+                LineAction::MigrateToUiTest { compiletest_name, ui_test_name } => {
+                    // Replace comment type first, then the name range specified.
+                    let mut new_line = replace_compiletest_comment(header_action.line()).unwrap();
+                    // This is always a directive that contains the compiletest name.
+                    let name_start = new_line.find(compiletest_name.as_str()).unwrap();
+                    new_line.replace_range(
+                        name_start..(name_start + compiletest_name.len()),
+                        ui_test_name.as_str(),
+                    );
+                    Some((header_action.line_num(), new_line))
+                }
+                LineAction::UseUITestName { compiletest_name, ui_test_name } => {
+                    // This is always a directive that contains the compiletest name.
+                    let name_start = header_action.line().find(compiletest_name.as_str()).unwrap();
+                    let mut new_line = header_action.line().to_string();
+                    new_line.replace_range(
+                        name_start..(name_start + compiletest_name.len()),
+                        ui_test_name.as_str(),
+                    );
+                    Some((header_action.line_num(), new_line))
+                }
+                LineAction::Error { .. } => {
+                    emit_error(file_path, &header_action);
+                    // No line to replace
+                    None
+                }
+            }
         })
         .collect::<HashMap<_, _>>();
 
-    let file_contents = fs::read_to_string(file_path)?;
+    if line_replacements.len() > 0 {
+        let file_contents = fs::read_to_string(file_path)?;
 
-    // Replace each line in the contents of the file that there is an entry for.
-    let replaced_contents = file_contents
-        // split_inclusive here because we want each line to still have its newline to be
-        // joined. The line replacements also keep their newline.
-        .split_inclusive('\n')
-        .enumerate()
-        .map(|(line_num_zero_idx, line)| {
-            // enumerate is 0-indexed, but the entries for line numbers are 1-indexed.
-            line_replacements.get(&(line_num_zero_idx + 1)).map(|s| s.as_str()).unwrap_or(line)
-        })
-        .collect::<String>();
-    // dbg!(&replaced_contents);
+        // Replace each line in the contents of the file that there is an entry for.
+        let replaced_contents = file_contents
+            // split_inclusive here because we want each line to still have its newline to be
+            // joined. The line replacements also keep their newline.
+            .split_inclusive('\n')
+            .enumerate()
+            .map(|(line_num_zero_idx, line)| {
+                // enumerate is 0-indexed, but the entries for line numbers are 1-indexed.
+                line_replacements.get(&(line_num_zero_idx + 1)).map(|s| s.as_str()).unwrap_or(line)
+            })
+            .collect::<String>();
+        // dbg!(&replaced_contents);
 
-    println!("Writing fixed file {}", file_path.display());
+        println!("Writing fixed file {}", file_path.display());
 
-    // Return whether the file was successfully written.
-    fs::write(file_path, replaced_contents)
+        // Return whether the file was successfully written.
+        fs::write(file_path, replaced_contents)
+    } else {
+        Ok(())
+    }
 }
 
 /// Replace the comment portion of a compiletest style header with a ui_test style comment.

src/tools/tidy/src/ui_tests/ui_test_headers.rs

@@ -40,29 +40,29 @@ pub(super) fn check_file_headers(file_path: &Path) -> Result<(), HeaderError> {
                 DirectiveMatchResult::UseUiTestComment => {
                     errors.push(HeaderAction {
                         line_num,
-                        line: comment.full_line().to_string(),
+                        line: comment.full_line().to_owned(),
                         action: LineAction::UseUiTestComment,
                     });
                     break;
                 }
                 DirectiveMatchResult::MigrateToUiTest => {
                     errors.push(HeaderAction {
                         line_num,
-                        line: comment.full_line().to_string(),
+                        line: comment.full_line().to_owned(),
                         action: LineAction::MigrateToUiTest {
-                            compiletest_name: directive.compiletest_name().to_string(),
-                            ui_test_name: directive.ui_test_name().unwrap().to_string(),
+                            compiletest_name: directive.compiletest_name().to_owned(),
+                            ui_test_name: directive.ui_test_name().unwrap().to_owned(),
                         },
                     });
                     break;
                 }
                 DirectiveMatchResult::UseUITestName => {
                     errors.push(HeaderAction {
                         line_num,
-                        line: comment.full_line().to_string(),
+                        line: comment.full_line().to_owned(),
                         action: LineAction::UseUITestName {
-                            compiletest_name: directive.compiletest_name().to_string(),
-                            ui_test_name: directive.ui_test_name().unwrap().to_string(),
+                            compiletest_name: directive.compiletest_name().to_owned(),
+                            ui_test_name: directive.ui_test_name().unwrap().to_owned(),
                         },
                     });
                     break;
@@ -75,24 +75,29 @@ pub(super) fn check_file_headers(file_path: &Path) -> Result<(), HeaderError> {
             Err(ConditionError::ConvertToUiTest { compiletest_name, ui_test_name }) => {
                 errors.push(HeaderAction {
                     line_num,
-                    line: comment.full_line().to_string(),
+                    line: comment.full_line().to_owned(),
                     action: LineAction::MigrateToUiTest { compiletest_name, ui_test_name },
                 })
             }
             Err(ConditionError::UiTestUnknownTarget { target_substr }) => {
                 errors.push(HeaderAction {
                     line_num,
-                    line: comment.full_line().to_string(),
+                    line: comment.full_line().to_owned(),
                     action: LineAction::Error {
                         message: format!("invalid target substring: {}", target_substr),
                     },
                 })
             }
             Err(ConditionError::UseUiTestComment) => errors.push(HeaderAction {
                 line_num,
-                line: comment.full_line().to_string(),
+                line: comment.full_line().to_owned(),
                 action: LineAction::UseUiTestComment,
             }),
+            Err(ConditionError::UnknownCondition { name }) => errors.push(HeaderAction {
+                line_num,
+                line: comment.full_line().to_owned(),
+                action: LineAction::Error { message: format!("unknown condition {}", name) },
+            }),
         }
     });
 
@@ -182,6 +187,8 @@ enum ConditionError {
     ConvertToUiTest { compiletest_name: String, ui_test_name: String },
     /// The target substring in the comment is not known.
     UiTestUnknownTarget { target_substr: String },
+    /// The comment looked like a condition, but was not known.
+    UnknownCondition { name: String },
 }
 
 /// Checks that a test comment uses the ui_test style only or ignore directives, and that
@@ -213,19 +220,16 @@ fn check_condition(comment: &TestComment<'_>) -> Result<(), ConditionError> {
                 // wasm32-bare is an alias for wasm32-unknown-unknown
                 Err(ConditionError::ConvertToUiTest {
                     compiletest_name: String::from("wasm32-bare"),
-                    ui_test_name: String::from("wasm32-unknown-unknown"),
+                    ui_test_name: String::from("target-wasm32-unknown-unknown"),
                 })
-            } else if condition.starts_with("tidy-")
-                || ["stable", "beta", "nightly", "stage1", "cross-compile", "remote"]
-                    .contains(&condition)
-            {
-                // Ignore tidy directives, and a few other unknown comment types
-                // FIXME (ui_test): make ui_test know about all of these
+            } else if condition.starts_with("tidy-") {
+                // Ignore tidy directives
                 Ok(())
-            } else {
-                // Unknown only/ignore directive, or the target is not known, do nothing.
-                eprintln!("unknown comment: {:#?}", comment);
+            } else if UNSUPPORTED_CONDITIONS.contains(&condition) {
                 Ok(())
+            } else {
+                // Unknown only/ignore directive, or the target is not known.
+                Err(ConditionError::UnknownCondition { name: condition.to_owned() })
             }
         }
 
@@ -321,6 +325,30 @@ impl From<io::Error> for HeaderError {
     }
 }
 
+/// Directives that ui_test does not yet support. Used for emitting a warning instead of an error
+/// in tidy.
+// FIXME (ui_test): This should ideally be empty/removed by supporting everything.
+const UNSUPPORTED_CONDITIONS: &[&str] = [
+    "stable",
+    "beta",
+    "nightly",
+    "stage1",
+    "remote",
+    "cross-compile",
+    "debug", // When building with debug assertions enabled. Primarily used for codegen tests.
+    // `ignore-pass` is sorta like ui_test's `check-pass` but not really, compiletest instead
+    // Falls back to a pass-mode which can be set by headers.
+    "pass",
+    "endian-big",
+    "compare-mode-polonius",
+    "compare-mode-next-solver",
+    "compare-mode-next-solver-coherence",
+    "compare-mode-split-dwarf",
+    "compare-mode-split-dwarf-single",
+    "llvm-version",
+]
+.as_slice();
+
 // All components of target triples (including the whole triple) that were known by compiletest
 // at the time of writing (2023-08-13). This list is used to guide migration to ui_test style
 // only/ignore directives, since ui_test only knows how to do substring matches on target triples
@@ -556,6 +584,7 @@ const KNOWN_TARGET_COMPONENTS: &[&str] = [
     "tvos",
     "uefi",
     "unknown",
+    "uwp",
     "vita",
     "vxworks",
     "wasi",