diff --git a/task-maker-format/src/ioi/dag/checker.rs b/task-maker-format/src/ioi/dag/checker.rs index c88c60022..2bcc54e35 100644 --- 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::()); + + 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 index 9007fc632..84a3061ea 100644 --- 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 index 0c7cf02a9..ccb1b006e 100644 --- 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 index 81288c67c..761c7ca9d 100644 --- 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>( 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 index bf891cdc0..854fc348b 100644 --- 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 index fe7420081..164428598 100644 --- 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::>(); - - 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::>(); - 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 index 09d303849..106041d3e 100644 --- a/task-maker-format/tests/ioi_sanity_checks.rs +++ b/task-maker-format/tests/ioi_sanity_checks.rs @@ -64,7 +64,7 @@ fn test_sanity_checks_att_graders() { std::fs::create_dir(tmpdir.path().join("sol")).unwrap(); std::fs::create_dir(tmpdir.path().join("att")).unwrap(); std::fs::write(tmpdir.path().join("sol/grader.cpp"), "x").unwrap(); - std::fs::write(tmpdir.path().join("att/template.cpp"), "x").unwrap(); + std::fs::write(tmpdir.path().join("att/task.cpp"), "x").unwrap(); task.grader_map = Arc::new(GraderMap::new(vec![tmpdir.path().join("sol/grader.cpp")])); let warnings = get_warnings(&task); @@ -185,8 +185,8 @@ fn test_sanity_checks_sol_graders() { let mut task = utils::new_task_with_context(tmpdir.path()); std::fs::create_dir(tmpdir.path().join("sol")).unwrap(); std::fs::write(tmpdir.path().join("sol/grader.cpp"), "x").unwrap(); - std::fs::write(tmpdir.path().join("sol/template.cpp"), "x").unwrap(); - std::fs::write(tmpdir.path().join("sol/template.c"), "x").unwrap(); + std::fs::write(tmpdir.path().join("sol/task.cpp"), "x").unwrap(); + std::fs::write(tmpdir.path().join("sol/task.c"), "x").unwrap(); task.grader_map = Arc::new(GraderMap::new(vec![tmpdir.path().join("sol/grader.cpp")])); let warnings = get_warnings(&task);