Improving sanity checks (#420)

franv314 · web-flow · commit e954367b3d4a · 2026-05-23T14:13:46.000+02:00
* The checker must now output printable UTF-8

* fixed grader/stub difference in attachments

* Added warning for st checks on samples

* Removed st ordering check

* clippy and format

* fixed tests
diff --git a/task-maker-format/src/ioi/dag/checker.rs b/task-maker-format/src/ioi/dag/checker.rs
@@ -96,7 +96,7 @@ impl Checker {
                 group.priority = EVALUATION_PRIORITY - testcase_id.unwrap_or_default() as Priority;
                 let sender = eval.sender.clone();
                 eval.dag.on_execution_done(&group.uuid, move |results| {
-            let res = &results[0];
+                    let res = &results[0];
                     let stdout = res
                         .stdout
                         .as_ref()
@@ -105,8 +105,6 @@ impl Checker {
                         .stderr
                         .as_ref()
                         .ok_or_else(|| anyhow!("Checker stderr not captured"))?;
-                    let message = String::from_utf8_lossy(stderr).trim().to_string();
-                    let message = Self::translate_checker_message(message);
                     if !res.status.is_success() {
                         let message = if let Some(testcase_id) = testcase_id {
                             format!(
@@ -122,9 +120,30 @@ impl Checker {
                         sender.add_diagnostic(diagnostic)?;
                         return Ok(());
                     }
-                    let score = String::from_utf8_lossy(stdout);
-                    let score: f64 = match score.trim().parse() {
-                        Ok(score) => score,
+                    let message = str::from_utf8(stderr)
+                        .map(|s| Self::translate_checker_message(s.trim().to_string()));
+                    let score = str::from_utf8(stdout)
+                        .map(|s| s.trim().parse::<f64>());
+
+                    let Ok(score) = score else {
+                        let message = "The checked return a non UTF-8 score".to_string();
+                        let diagnostic = Diagnostic::error(message)
+                            .with_note(description)
+                            .with_help_attachment(stdout.clone());
+                        sender.add_diagnostic(diagnostic)?;
+                        return Ok(());
+                    };
+                    let Ok(message) = message else {
+                        let message = "The checked return a non UTF-8 message".to_string();
+                        let diagnostic = Diagnostic::error(message)
+                            .with_note(description)
+                            .with_help_attachment(stdout.clone());
+                        sender.add_diagnostic(diagnostic)?;
+                        return Ok(());
+                    };
+
+                    let score: f64 = match &score {
+                        Ok(score) => *score,
                         Err(e) => {
                             let message = if let Some(testcase_id) = testcase_id {
                                 format!(
@@ -141,6 +160,16 @@ impl Checker {
                             return Ok(());
                         }
                     };
+
+                    if !Self::is_unicode_printable(&message) {
+                        let message = format!("The checked return a non printable message ({message})");
+                        let diagnostic = Diagnostic::error(message)
+                            .with_note(description)
+                            .with_help_attachment(stdout.clone());
+                        sender.add_diagnostic(diagnostic)?;
+                        return Ok(());
+                    }
+
                     callback(score, message)
                 });
                 Ok(group)
@@ -195,6 +224,11 @@ impl Checker {
         Ok(())
     }
 
+    /// Checks that a string is a printable message without control characters.
+    fn is_unicode_printable(s: &str) -> bool {
+        s.chars().all(|c| !c.is_control() || c.is_whitespace())
+    }
+
     /// The checker may return a message to be translated. This function maps the message
     /// placeholders to actual messages.
     pub fn translate_checker_message(message: String) -> String {
diff --git a/task-maker-format/src/ioi/sanity_checks/att.rs b/task-maker-format/src/ioi/sanity_checks/att.rs
@@ -1,4 +1,5 @@
 use std::collections::{BTreeSet, HashMap};
+use std::ffi::OsStr;
 use std::io::{Read, Seek, SeekFrom};
 use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
@@ -552,11 +553,8 @@ impl SanityCheck for AttTemplatesShouldCompile {
             let att_name = format!("att/{}.{}", task.name, ext);
             let template = task.path.join(&att_name);
 
-            let grader_name = grader
-                .file_name()
-                .ok_or_else(|| anyhow!("Grader has no file name"))?
-                .to_string_lossy();
-            let att_grader_name = format!("att/{grader_name}");
+            let grader_ext = grader.extension().and_then(OsStr::to_str).unwrap_or("");
+            let att_grader_name = format!("att/grader.{grader_ext}",);
             let att_grader = task.path.join(&att_grader_name);
 
             // Only run the check if the grader is not a symlink, as otherwise we are already
diff --git a/task-maker-format/src/ioi/sanity_checks/checker.rs b/task-maker-format/src/ioi/sanity_checks/checker.rs
@@ -81,9 +81,13 @@ impl SanityCheck for FuzzCheckerWithJunkOutput {
                     test_output_uuid,
                     move |score, outcome| {
                         if score != 0.0 {
-                            sender.add_diagnostic(Diagnostic::error(format!(
-                                "Junk file '{description}' scored {score} (with message '{outcome}')"
-                            )))?;
+                            sender.add_diagnostic(
+                                Diagnostic::warning(format!(
+                                    "Junk file '{description}' scored {score} (with message '{outcome}')"
+                                )).with_note(
+                                    "This almost certainly indicates a bug in the checker, unless it's intended that such a file scores points."
+                                )
+                            )?;
                         }
                         Ok(())
                     },
diff --git a/task-maker-format/src/ioi/sanity_checks/mod.rs b/task-maker-format/src/ioi/sanity_checks/mod.rs
@@ -45,27 +45,20 @@ fn check_missing_graders<P: AsRef<Path>>(
     if !has_grader(task) {
         return Ok(());
     }
-    // some task formats use stub.* others use grader.*
-    // To avoid confusion emit warnings only either for stubs or graders.
-    let is_stub = task
-        .grader_map
-        .all_paths()
-        .filter_map(|p| p.file_stem())
-        .any(|p| p == "stub");
     let mut by_ext = HashMap::new();
     for file in list_files(task.path.join(folder.as_ref()), vec!["*.*"]) {
         let file = task.path_of(&file);
-        let stem = match file.file_stem() {
-            Some(stem) => stem,
-            None => continue,
+        let Some(stem) = file.file_stem() else {
+            continue;
         };
-        // do not check the graders
-        if stem == "grader" || stem == "stub" {
+        // Only check if the file name is that of a template
+        if *stem != *task.name {
             continue;
         }
         if let Some(lang) = LanguageManager::detect_language(file) {
             let ext = lang.extensions()[0];
-            let name = format!("{}.{}", if is_stub { "stub" } else { "grader" }, ext);
+            // Even if some tasks want `stubs`, we always check for `grader`s.
+            let name = format!("grader.{ext}");
             let grader_name = file.with_file_name(name);
             let grader_path = task.path.join(&grader_name);
             by_ext.insert(ext, (grader_path, grader_name, file.to_owned()));
diff --git a/task-maker-format/src/ioi/sanity_checks/sol.rs b/task-maker-format/src/ioi/sanity_checks/sol.rs
@@ -81,12 +81,10 @@ impl SanityCheck for SolutionsWithNoChecks {
     }
 
     fn pre_hook(&self, task: &IOITask, eval: &mut EvaluationData) -> Result<(), Error> {
-        for subtask in task.subtasks.values() {
-            if subtask.name.is_none() {
-                // If not all the subtasks have a name, do not bother with the solutions, it's much
-                // more important to give everything a name before.
-                return Ok(());
-            }
+        // If not all the subtasks have a name, do not bother with the solutions, it's much
+        // more important to give everything a name before.
+        if task.subtasks.values().any(|st| st.name.is_none()) {
+            return Ok(());
         }
 
         let mut solutions = vec![];
@@ -105,13 +103,58 @@ impl SanityCheck for SolutionsWithNoChecks {
                 solution.source_file.relative_path().display()
             ))
         }
+        if !solutions.is_empty() {
+            eval.add_diagnostic(Diagnostic::warning(format!(
+                "The following solutions are missing the subtask checks: {}",
+                solutions.join(", ")
+            )))?;
+        }
+        Ok(())
+    }
+}
+
+/// Check that all the solutions (that are not symlinks) contain at least one check.
+#[derive(Debug, Default)]
+pub struct SolutionsWithSamplesCheck;
+make_sanity_check!(SolutionsWithSamplesCheck);
+
+impl SanityCheck for SolutionsWithSamplesCheck {
+    type Task = IOITask;
+
+    fn name(&self) -> &'static str {
+        "SolutionsWithSamplesChecks"
+    }
+
+    fn category(&self) -> SanityCheckCategory {
+        SanityCheckCategory::Solutions
+    }
+
+    fn pre_hook(&self, task: &IOITask, eval: &mut EvaluationData) -> Result<(), Error> {
+        // If no first subtask exists, or it has no name, skip this check.
+        let Some(Some(samples)) = task.subtasks.get(&0).map(|st| st.name.clone()) else {
+            return Ok(());
+        };
+
+        let mut solutions = vec![];
+        for solution in eval.solutions.iter() {
+            let samples_checked = solution
+                .checks
+                .iter()
+                .any(|check| check.subtask_name_pattern == samples);
+            if samples_checked {
+                solutions.push(format!(
+                    "{}",
+                    solution.source_file.relative_path().display()
+                ))
+            }
+        }
         if !solutions.is_empty() {
             eval.add_diagnostic(
                 Diagnostic::warning(format!(
-                    "The following solutions are missing the subtask checks: {}",
+                    "The following solutions have solution checks for the samples: {}",
                     solutions.join(", ")
                 ))
-                .with_help("Try running task-maker-tools add-solution-checks"),
+                .with_note("Subtask checks on samples are rarely useful."),
             )?;
         }
         Ok(())
diff --git a/task-maker-format/src/ioi/sanity_checks/task.rs b/task-maker-format/src/ioi/sanity_checks/task.rs
@@ -1,4 +1,3 @@
-use std::collections::{HashMap, HashSet};
 use std::fs;
 
 use anyhow::Error;
@@ -7,7 +6,7 @@ use task_maker_diagnostics::{CodeSpan, Diagnostic};
 
 use crate::ioi::IOITask;
 use crate::sanity_checks::{make_sanity_check, SanityCheck, SanityCheckCategory};
-use crate::{list_files, EvaluationData, SolutionCheckResult};
+use crate::{list_files, EvaluationData};
 
 /// The default maximum score of a task.
 const DEFAULT_TASK_MAX_SCORE: f64 = 100.0;
@@ -129,143 +128,6 @@ impl SanityCheck for NoBitsStdCpp {
     }
 }
 
-/// Check that "implied" subtasks dependecies do not form cycles.
-#[derive(Debug, Default)]
-pub struct SubtaskDependencies;
-make_sanity_check!(SubtaskDependencies);
-
-impl SanityCheck for SubtaskDependencies {
-    type Task = IOITask;
-
-    fn name(&self) -> &'static str {
-        "SubtaskDependencies"
-    }
-
-    fn category(&self) -> SanityCheckCategory {
-        SanityCheckCategory::Task
-    }
-
-    fn pre_hook(&self, task: &Self::Task, eval: &mut EvaluationData) -> Result<(), Error> {
-        let non_zero_sts = task
-            .subtasks
-            .keys()
-            .copied()
-            .filter(|st| task.subtasks[st].max_score > 0.0)
-            .collect::<Vec<_>>();
-
-        let mut st_dependencies = HashSet::new();
-        for &st1 in &non_zero_sts {
-            for &st2 in &non_zero_sts {
-                if st1 != st2 {
-                    st_dependencies.insert((st1, st2));
-                }
-            }
-        }
-
-        let mut any_st_check = false;
-        for sol in &eval.solutions {
-            let mut score_range = HashMap::new();
-            for check in &sol.checks {
-                any_st_check = true;
-
-                let val = match check.result {
-                    SolutionCheckResult::Accepted => (1.0, 1.0),
-                    SolutionCheckResult::PartialScore => (0.0, 1.0),
-                    _ => (0.0, 0.0),
-                };
-
-                for subtask in task.find_subtasks_by_pattern_name(&check.subtask_name_pattern) {
-                    score_range.insert(subtask.id, val);
-                }
-            }
-
-            for &st1 in &non_zero_sts {
-                for &st2 in &non_zero_sts {
-                    if let (Some(v1), Some(v2)) = (score_range.get(&st1), score_range.get(&st2)) {
-                        if v1.1 > v2.0 {
-                            st_dependencies.remove(&(st1, st2));
-                        }
-                    }
-                }
-            }
-        }
-
-        if !any_st_check {
-            return Ok(());
-        }
-
-        'outer: for &st1 in &non_zero_sts {
-            let mut sts = vec![st1];
-            for &st2 in &non_zero_sts {
-                if st_dependencies.contains(&(st1, st2)) && st_dependencies.contains(&(st2, st1)) {
-                    if st1 < st2 {
-                        sts.push(st2);
-                    } else {
-                        continue 'outer;
-                    }
-                }
-            }
-            if sts.len() > 1 {
-                sts.sort();
-                let st_names: Vec<_> = sts
-                    .iter()
-                    .map(|st| {
-                        task.subtasks[st]
-                            .name
-                            .as_ref()
-                            .map(|s| s as &dyn std::fmt::Debug)
-                            .unwrap_or(st)
-                    })
-                    .collect();
-                eval.add_diagnostic(
-                    Diagnostic::warning(format!(
-                        "Subtasks {st_names:?} are solved by the same set of solutions",
-                    ))
-                    .with_note("Add a solution that solves only one of them"),
-                )?;
-            }
-        }
-
-        let mut to_swap = Vec::new();
-        for &st1 in &non_zero_sts {
-            for &st2 in &non_zero_sts {
-                if st1 < st2
-                    && st_dependencies.contains(&(st1, st2))
-                    && !st_dependencies.contains(&(st2, st1))
-                {
-                    to_swap.push((st1, st2));
-                }
-            }
-        }
-
-        if !to_swap.is_empty() {
-            let to_swap_names = to_swap
-                .iter()
-                .map(|(st1, st2)| {
-                    let st1_name = task.subtasks[st1]
-                        .name
-                        .as_ref()
-                        .map(|s| s as &dyn std::fmt::Debug)
-                        .unwrap_or(st1);
-                    let st2_name = task.subtasks[st2]
-                        .name
-                        .as_ref()
-                        .map(|s| s as &dyn std::fmt::Debug)
-                        .unwrap_or(st2);
-                    (st1_name, st2_name)
-                })
-                .collect::<Vec<_>>();
-            eval.add_diagnostic(
-                Diagnostic::warning("Subtasks are not in order of difficulty").with_note(format!(
-                    "Based on the current solutions the following pairs of subtasks seems to be ordered incorrectly {to_swap_names:?}"
-                )),
-            )?;
-        }
-
-        Ok(())
-    }
-}
-
 /// Check that the task title is not emtpy.
 #[derive(Debug, Default)]
 pub struct EmptyTitle;
diff --git a/task-maker-format/tests/ioi_sanity_checks.rs b/task-maker-format/tests/ioi_sanity_checks.rs
