From 0471800faab324e24dc7cfde42c3c17c07b158de Mon Sep 17 00:00:00 2001 From: Dominion Date: Sat, 6 May 2023 12:44:06 -0400 Subject: [PATCH 01/20] Don't panic when there are still expected errors but no additional errors --- crates/dreamchecker/src/test_helpers.rs | 40 ++++++++++++++++--------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/crates/dreamchecker/src/test_helpers.rs b/crates/dreamchecker/src/test_helpers.rs index e0ea5e0c0..19cfa4625 100644 --- a/crates/dreamchecker/src/test_helpers.rs +++ b/crates/dreamchecker/src/test_helpers.rs @@ -26,20 +26,32 @@ pub fn check_errors_match>>(buffer: S, errorlist: &[(u let errors = context.errors(); let mut iter = errors.iter(); for (line, column, desc) in errorlist { - let nexterror = iter.next().unwrap(); - if nexterror.location().line != *line - || nexterror.location().column != *column - || nexterror.description() != *desc - { - panic!( - "possible feature regression in dreamchecker, expected {}:{}:{}, found {}:{}:{}", - *line, - *column, - *desc, - nexterror.location().line, - nexterror.location().column, - nexterror.description() - ); + let nexterror_option = iter.next(); + match nexterror_option { + Some(nexterror) => { + if nexterror.location().line != *line + || nexterror.location().column != *column + || nexterror.description() != *desc + { + panic!( + "possible feature regression in dreamchecker, expected {}:{}:{}, found {}:{}:{}", + *line, + *column, + *desc, + nexterror.location().line, + nexterror.location().column, + nexterror.description() + ); + } + }, + None => { + panic!( + "possible feature regression in dreamchecker, expected {}:{}:{}, found no additional errors!", + *line, + *column, + *desc + ); + } } } if iter.next().is_some() { From 109ae64240c4494ea8ae38313d2efdb4d09e9365 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sat, 6 May 2023 12:52:25 -0400 Subject: [PATCH 02/20] Split sleep tests and pure tests --- crates/dreamchecker/tests/pure_tests.rs | 48 +++++++++++++++++++ .../{sleep_pure_tests.rs => sleep_tests.rs} | 45 ----------------- 2 files changed, 48 insertions(+), 45 deletions(-) create mode 100644 crates/dreamchecker/tests/pure_tests.rs rename crates/dreamchecker/tests/{sleep_pure_tests.rs => sleep_tests.rs} (80%) diff --git a/crates/dreamchecker/tests/pure_tests.rs b/crates/dreamchecker/tests/pure_tests.rs new file mode 100644 index 000000000..33df92412 --- /dev/null +++ b/crates/dreamchecker/tests/pure_tests.rs @@ -0,0 +1,48 @@ +extern crate dreamchecker as dc; + +use dc::test_helpers::check_errors_match; + +pub const PURE_ERRORS: &[(u32, u16, &str)] = &[ + (12, 16, "/mob/proc/test2 sets SpacemanDMM_should_be_pure but calls a /proc/impure that does impure operations"), +]; + +#[test] +fn pure() { + let code = r##" +/proc/pure() + return 1 +/proc/impure() + world << "foo" +/proc/foo() + pure() +/proc/bar() + impure() +/mob/proc/test() + set SpacemanDMM_should_be_pure = TRUE + return foo() +/mob/proc/test2() + set SpacemanDMM_should_be_pure = TRUE + bar() +"##.trim(); + check_errors_match(code, PURE_ERRORS); +} + +// these tests are separate because the ordering the errors are reported in isn't determinate and I CBF figuring out why -spookydonut Jan 2020 +// TODO: find out why +pub const PURE2_ERRORS: &[(u32, u16, &str)] = &[ + (5, 5, "call to pure proc test discards return value"), +]; + +#[test] +fn pure2() { + let code = r##" +/mob/proc/test() + set SpacemanDMM_should_be_pure = TRUE + return 1 +/mob/proc/test2() + test() +/mob/proc/test3() + return test() +"##.trim(); + check_errors_match(code, PURE2_ERRORS); +} diff --git a/crates/dreamchecker/tests/sleep_pure_tests.rs b/crates/dreamchecker/tests/sleep_tests.rs similarity index 80% rename from crates/dreamchecker/tests/sleep_pure_tests.rs rename to crates/dreamchecker/tests/sleep_tests.rs index 1f62b56a6..ebf91dd32 100644 --- a/crates/dreamchecker/tests/sleep_pure_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -174,48 +174,3 @@ fn sleep5() { "##.trim(); check_errors_match(code, SLEEP_ERROR5); } - -pub const PURE_ERRORS: &[(u32, u16, &str)] = &[ - (12, 16, "/mob/proc/test2 sets SpacemanDMM_should_be_pure but calls a /proc/impure that does impure operations"), -]; - -#[test] -fn pure() { - let code = r##" -/proc/pure() - return 1 -/proc/impure() - world << "foo" -/proc/foo() - pure() -/proc/bar() - impure() -/mob/proc/test() - set SpacemanDMM_should_be_pure = TRUE - return foo() -/mob/proc/test2() - set SpacemanDMM_should_be_pure = TRUE - bar() -"##.trim(); - check_errors_match(code, PURE_ERRORS); -} - -// these tests are separate because the ordering the errors are reported in isn't determinate and I CBF figuring out why -spookydonut Jan 2020 -// TODO: find out why -pub const PURE2_ERRORS: &[(u32, u16, &str)] = &[ - (5, 5, "call to pure proc test discards return value"), -]; - -#[test] -fn pure2() { - let code = r##" -/mob/proc/test() - set SpacemanDMM_should_be_pure = TRUE - return 1 -/mob/proc/test2() - test() -/mob/proc/test3() - return test() -"##.trim(); - check_errors_match(code, PURE2_ERRORS); -} From 3d15d6aac029432d40e7979544aae9f233bcb88b Mon Sep 17 00:00:00 2001 From: Dominion Date: Sat, 6 May 2023 12:52:59 -0400 Subject: [PATCH 03/20] Remove unnecessary pub's --- crates/dreamchecker/tests/pure_tests.rs | 4 ++-- crates/dreamchecker/tests/sleep_tests.rs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/dreamchecker/tests/pure_tests.rs b/crates/dreamchecker/tests/pure_tests.rs index 33df92412..7ed1b20cd 100644 --- a/crates/dreamchecker/tests/pure_tests.rs +++ b/crates/dreamchecker/tests/pure_tests.rs @@ -2,7 +2,7 @@ extern crate dreamchecker as dc; use dc::test_helpers::check_errors_match; -pub const PURE_ERRORS: &[(u32, u16, &str)] = &[ +const PURE_ERRORS: &[(u32, u16, &str)] = &[ (12, 16, "/mob/proc/test2 sets SpacemanDMM_should_be_pure but calls a /proc/impure that does impure operations"), ]; @@ -29,7 +29,7 @@ fn pure() { // these tests are separate because the ordering the errors are reported in isn't determinate and I CBF figuring out why -spookydonut Jan 2020 // TODO: find out why -pub const PURE2_ERRORS: &[(u32, u16, &str)] = &[ +const PURE2_ERRORS: &[(u32, u16, &str)] = &[ (5, 5, "call to pure proc test discards return value"), ]; diff --git a/crates/dreamchecker/tests/sleep_tests.rs b/crates/dreamchecker/tests/sleep_tests.rs index ebf91dd32..ebd6c8d85 100644 --- a/crates/dreamchecker/tests/sleep_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -3,7 +3,7 @@ extern crate dreamchecker as dc; use dc::test_helpers::check_errors_match; -pub const SLEEP_ERRORS: &[(u32, u16, &str)] = &[ +const SLEEP_ERRORS: &[(u32, u16, &str)] = &[ (16, 16, "/mob/proc/test3 sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/sleepingproc"), ]; @@ -45,7 +45,7 @@ fn sleep() { check_errors_match(code, SLEEP_ERRORS); } -pub const SLEEP_ERRORS2: &[(u32, u16, &str)] = &[ +const SLEEP_ERRORS2: &[(u32, u16, &str)] = &[ (8, 21, "/mob/living/proc/bar calls /mob/living/proc/foo which has override child proc that sleeps /mob/living/carbon/proc/foo"), ]; @@ -112,7 +112,7 @@ fn sleep3() { ]); } -pub const SLEEP_ERROR4: &[(u32, u16, &str)] = &[ +const SLEEP_ERROR4: &[(u32, u16, &str)] = &[ (1, 16, "/mob/proc/test1 sets SpacemanDMM_should_not_sleep but calls blocking built-in(s)"), (1, 16, "/mob/proc/test1 sets SpacemanDMM_should_not_sleep but calls blocking proc /mob/proc/test2"), (1, 16, "/mob/proc/test1 sets SpacemanDMM_should_not_sleep but calls blocking proc /client/proc/checksoundquery"), @@ -150,7 +150,7 @@ fn sleep4() { } // Test overrides and for regression of issue #267 -pub const SLEEP_ERROR5: &[(u32, u16, &str)] = &[ +const SLEEP_ERROR5: &[(u32, u16, &str)] = &[ (7, 19, "/datum/sub/proc/checker sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/sleeper"), ]; From 0415643b59284440f8313b2ce7d7451bf6743a4b Mon Sep 17 00:00:00 2001 From: Dominion Date: Sat, 6 May 2023 13:25:52 -0400 Subject: [PATCH 04/20] Output test errors when finding more than expected --- crates/dreamchecker/src/test_helpers.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/dreamchecker/src/test_helpers.rs b/crates/dreamchecker/src/test_helpers.rs index 19cfa4625..8fe81a0e1 100644 --- a/crates/dreamchecker/src/test_helpers.rs +++ b/crates/dreamchecker/src/test_helpers.rs @@ -54,7 +54,8 @@ pub fn check_errors_match>>(buffer: S, errorlist: &[(u } } } - if iter.next().is_some() { - panic!("found more errors than was expected"); + if let Some(error) = iter.next() { + let error_loc = error.location(); + panic!("found more errors than was expected: {}:{}:{}", error_loc.line, error_loc.column, error.description()); } } From 23693c79fa7807d7c96c7c6c625c3440ec30bb22 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sat, 6 May 2023 15:02:56 -0400 Subject: [PATCH 05/20] Fix ProcRef.recurse_children ignoring children in different typepaths --- crates/dreammaker/src/objtree.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/crates/dreammaker/src/objtree.rs b/crates/dreammaker/src/objtree.rs index 461697769..6b01faa57 100644 --- a/crates/dreammaker/src/objtree.rs +++ b/crates/dreammaker/src/objtree.rs @@ -266,6 +266,17 @@ impl<'a> TypeRef<'a> { } } + /// Recursively visit this and all child **types**. + pub fn recurse_types)>(&self, f: &mut F) { + self.recurse(f); + for parent_typed_idx in &self.tree.redirected_parent_types { + let parent_typed = &self.tree.graph[parent_typed_idx.index()]; + if parent_typed.parent_type_index().unwrap() == self.index() { + f(TypeRef::new(self.tree, *parent_typed_idx)); + } + } + } + /// Recursively visit this and all parent **types**. pub fn visit_parent_types)>(&self, f: &mut F) { let mut next = Some(*self); @@ -569,7 +580,7 @@ impl<'a> ProcRef<'a> { /// Recursively visit this and all public-facing procs which override it. pub fn recurse_children)>(self, f: &mut F) { - self.ty.recurse(&mut move |ty| { + self.ty.recurse_types(&mut move |ty| { if let Some(proc) = ty.get().procs.get(self.name) { f(ProcRef { ty, @@ -628,6 +639,7 @@ impl<'a> std::hash::Hash for ProcRef<'a> { pub struct ObjectTree { graph: Vec, types: BTreeMap, + redirected_parent_types: Vec, } impl ObjectTree { @@ -752,6 +764,7 @@ impl Default for ObjectTreeBuilder { let mut tree = ObjectTree { graph: Vec::with_capacity(0x4000), types: Default::default(), + redirected_parent_types: Vec::new() }; tree.graph.push(Type { path: String::new(), @@ -906,7 +919,11 @@ impl ObjectTreeBuilder { } }; - self.inner.graph[type_idx.index()].parent_type = idx; + let type_ref = &mut self.inner.graph[type_idx.index()]; + type_ref.parent_type = idx; + if type_ref.parent_path != idx { + self.inner.redirected_parent_types.push(type_idx); + } } } From 6a14a25ba8f54c8c056c9e8a71d39a3154aa11ae Mon Sep 17 00:00:00 2001 From: Dominion Date: Sat, 6 May 2023 15:03:33 -0400 Subject: [PATCH 06/20] Walk proc overrides when checking should not sleep. - Updates tests - Adds regression test --- crates/dreamchecker/src/lib.rs | 72 ++++++++++++++---------- crates/dreamchecker/tests/sleep_tests.rs | 35 +++++++++++- 2 files changed, 76 insertions(+), 31 deletions(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index ee275de9c..97ccc7d4e 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -562,7 +562,6 @@ pub struct AnalyzeObjectTree<'o> { impure_procs: ViolatingProcs<'o>, waitfor_procs: HashSet>, - sleeping_overrides: ViolatingOverrides<'o>, impure_overrides: ViolatingOverrides<'o>, } @@ -588,7 +587,6 @@ impl<'o> AnalyzeObjectTree<'o> { sleeping_procs: Default::default(), impure_procs: Default::default(), waitfor_procs: Default::default(), - sleeping_overrides: Default::default(), impure_overrides: Default::default(), } } @@ -651,16 +649,22 @@ impl<'o> AnalyzeObjectTree<'o> { .with_blocking_builtins(sleepvec) .register(self.context) } + let mut visited = HashSet::>::new(); - let mut to_visit = VecDeque::<(ProcRef<'o>, CallStack, bool)>::new(); - if let Some(procscalled) = self.call_tree.get(procref) { - for (proccalled, location, new_context) in procscalled { - let mut callstack = CallStack::default(); - callstack.add_step(*proccalled, *location, *new_context); - to_visit.push_back((*proccalled, callstack, *new_context)); + let mut to_visit = VecDeque::<(ProcRef<'o>, CallStack, bool, ProcRef<'o>)>::new(); + + visited.insert(*procref); + + if let Some(calledvec) = self.call_tree.get(procref) { + for (proccalled, location, new_context) in calledvec.iter() { + let mut newstack = CallStack::default(); + newstack.add_step(*proccalled, *location, *new_context); + to_visit.push_back((*proccalled, newstack, *new_context, *proccalled)); } } - while let Some((nextproc, callstack, new_context)) = to_visit.pop_front() { + + let procref_type_index = procref.ty().index(); + while let Some((nextproc, callstack, new_context, parent_proc)) = to_visit.pop_front() { if !visited.insert(nextproc) { continue } @@ -673,31 +677,46 @@ impl<'o> AnalyzeObjectTree<'o> { if new_context { continue } + + let parent_proc_type_index = parent_proc.ty().index(); + let next_proc_type_index = nextproc.ty().index(); + + let proc_is_on_same_type_as_setting = next_proc_type_index == procref_type_index; + let proc_is_override = next_proc_type_index != parent_proc_type_index; + if let Some(sleepvec) = self.sleeping_procs.get_violators(nextproc) { - error(procref.get().location, format!("{} sets SpacemanDMM_should_not_sleep but calls blocking proc {}", procref, nextproc)) - .with_note(location, "SpacemanDMM_should_not_sleep set here") - .with_errortype("must_not_sleep") - .with_callstack(&callstack) - .with_blocking_builtins(sleepvec) - .register(self.context) - } else if let Some(overridesleep) = self.sleeping_overrides.get_override_violators(nextproc) { - for child_violator in overridesleep { - if procref.ty().is_subtype_of(&nextproc.ty()) && !child_violator.ty().is_subtype_of(&procref.ty()) { - continue - } - error(procref.get().location, format!("{} calls {} which has override child proc that sleeps {}", procref, nextproc, child_violator)) + if proc_is_on_same_type_as_setting && proc_is_override { + error(procref.get().location, format!("{} sets SpacemanDMM_should_not_sleep but has override child proc that sleeps {}", procref, nextproc)) .with_note(location, "SpacemanDMM_should_not_sleep set here") .with_errortype("must_not_sleep") .with_callstack(&callstack) - .with_blocking_builtins(self.sleeping_procs.get_violators(*child_violator).unwrap()) + .with_blocking_builtins(sleepvec) + .register(self.context) + } else if proc_is_override { + error(procref.get().location, format!("{} calls {} which has override child proc that sleeps {}", procref, parent_proc, nextproc)) + .with_note(location, "SpacemanDMM_should_not_sleep set here") + .with_errortype("must_not_sleep") + .with_callstack(&callstack) + .with_blocking_builtins(sleepvec) + .register(self.context) + } else { + error(procref.get().location, format!("{} sets SpacemanDMM_should_not_sleep but calls blocking proc {}", procref, nextproc)) + .with_note(location, "SpacemanDMM_should_not_sleep set here") + .with_errortype("must_not_sleep") + .with_callstack(&callstack) + .with_blocking_builtins(sleepvec) .register(self.context) } } + + nextproc.recurse_children(&mut |child_proc| + to_visit.push_back((child_proc, callstack.clone(), false, nextproc))); + if let Some(calledvec) = self.call_tree.get(&nextproc) { for (proccalled, location, new_context) in calledvec.iter() { let mut newstack = callstack.clone(); newstack.add_step(*proccalled, *location, *new_context); - to_visit.push_back((*proccalled, newstack, *new_context)); + to_visit.push_back((*proccalled, newstack, *new_context, *proccalled)); } } } @@ -829,13 +848,6 @@ impl<'o> AnalyzeObjectTree<'o> { if proc.name() == "New" { // New() propogates via ..() and causes weirdness return; } - if self.sleeping_procs.get_violators(proc).is_some() { - let mut next = proc.parent_proc(); - while let Some(current) = next { - self.sleeping_overrides.insert_override(current, proc); - next = current.parent_proc(); - } - } if self.impure_procs.get_violators(proc).is_some() { let mut next = proc.parent_proc(); while let Some(current) = next { diff --git a/crates/dreamchecker/tests/sleep_tests.rs b/crates/dreamchecker/tests/sleep_tests.rs index ebd6c8d85..343b4a8ed 100644 --- a/crates/dreamchecker/tests/sleep_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -47,6 +47,8 @@ fn sleep() { const SLEEP_ERRORS2: &[(u32, u16, &str)] = &[ (8, 21, "/mob/living/proc/bar calls /mob/living/proc/foo which has override child proc that sleeps /mob/living/carbon/proc/foo"), + (8, 21, "/mob/living/proc/bar calls /mob/proc/thing which has override child proc that sleeps /mob/dead/proc/thing"), + (8, 21, "/mob/living/proc/bar calls /mob/proc/New which has override child proc that sleeps /mob/dead/proc/New"), ]; #[test] @@ -109,6 +111,8 @@ fn sleep3() { "##.trim(); check_errors_match(code, &[ (8, 23, "/atom/movable/proc/bar calls /atom/movable/proc/foo which has override child proc that sleeps /mob/proc/foo"), + (8, 23, "/atom/movable/proc/bar calls /atom/proc/thing which has override child proc that sleeps /atom/dead/proc/thing"), + (8, 23, "/atom/movable/proc/bar calls /atom/proc/New[1/2] which has override child proc that sleeps /atom/dead/proc/New"), ]); } @@ -151,7 +155,8 @@ fn sleep4() { // Test overrides and for regression of issue #267 const SLEEP_ERROR5: &[(u32, u16, &str)] = &[ - (7, 19, "/datum/sub/proc/checker sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/sleeper"), + (7, 19, "/datum/sub/proc/checker calls /datum/proc/proxy which has override child proc that sleeps /datum/hijack/proc/proxy"), + (7, 19, "/datum/sub/proc/checker sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/sleeper"), ]; #[test] @@ -174,3 +179,31 @@ fn sleep5() { "##.trim(); check_errors_match(code, SLEEP_ERROR5); } + +// Test overrides and for regression of issue #355 +const SLEEP_ERROR6: &[(u32, u16, &str)] = &[ + (4, 24, "/datum/choiced/proc/is_valid sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/stoplag"), +]; + +#[test] +fn sleep6() { + let code = r##" +/datum/proc/is_valid(value) + set SpacemanDMM_should_not_sleep = 1 + +/datum/choiced/is_valid(value) + get_choices() + +/datum/choiced/proc/get_choices() + init_possible_values() + +/datum/choiced/proc/init_possible_values() + +/datum/choiced/ai_core_display/init_possible_values() + stoplag() + +/proc/stoplag() + sleep(1) +"##.trim(); + check_errors_match(code, SLEEP_ERROR6); +} From dc54c725b396d30d8369ef2a7f3ddab04f7a3bdb Mon Sep 17 00:00:00 2001 From: Dominion Date: Sat, 6 May 2023 21:06:43 -0400 Subject: [PATCH 07/20] Improvements --- crates/dreamchecker/src/lib.rs | 63 ++++++++++++++++-------- crates/dreamchecker/tests/sleep_tests.rs | 53 +++++++++++++++++++- 2 files changed, 94 insertions(+), 22 deletions(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index 97ccc7d4e..e0ef18dd6 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -327,7 +327,7 @@ fn run_inner(context: &Context, objtree: &ObjectTree, cli: bool) { check_var_defs(objtree, context); - let mut analyzer = AnalyzeObjectTree::new(context, objtree); + let mut analyzer = AnalyzeObjectTree::new(context, objtree.clone()); cli_println!("============================================================"); cli_println!("Gathering proc settings...\n"); @@ -641,6 +641,10 @@ impl<'o> AnalyzeObjectTree<'o> { } pub fn check_proc_call_tree(&mut self) { + let mut sleeping_procs_transitive = ViolatingProcs{ + violators: HashMap::new() + }; + for (procref, &(_, location)) in self.must_not_sleep.directive.iter() { if let Some(sleepvec) = self.sleeping_procs.get_violators(*procref) { error(procref.get().location, format!("{} sets SpacemanDMM_should_not_sleep but calls blocking built-in(s)", procref)) @@ -668,6 +672,7 @@ impl<'o> AnalyzeObjectTree<'o> { if !visited.insert(nextproc) { continue } + if self.waitfor_procs.get(&nextproc).is_some() { continue } @@ -684,31 +689,47 @@ impl<'o> AnalyzeObjectTree<'o> { let proc_is_on_same_type_as_setting = next_proc_type_index == procref_type_index; let proc_is_override = next_proc_type_index != parent_proc_type_index; - if let Some(sleepvec) = self.sleeping_procs.get_violators(nextproc) { - if proc_is_on_same_type_as_setting && proc_is_override { - error(procref.get().location, format!("{} sets SpacemanDMM_should_not_sleep but has override child proc that sleeps {}", procref, nextproc)) - .with_note(location, "SpacemanDMM_should_not_sleep set here") - .with_errortype("must_not_sleep") - .with_callstack(&callstack) - .with_blocking_builtins(sleepvec) - .register(self.context) + let mut sleepvec_option = sleeping_procs_transitive.get_violators(nextproc); + let transitive = sleepvec_option.is_some(); + if !transitive { + sleepvec_option = self.sleeping_procs.get_violators(nextproc); + } + + if let Some(sleepvec) = sleepvec_option { + let desc = if transitive { + format!("{} calls {} which transitively sleeps", procref, nextproc) + } else if proc_is_on_same_type_as_setting && proc_is_override { + format!("{} sets SpacemanDMM_should_not_sleep but has override child proc that sleeps {}", procref, nextproc) } else if proc_is_override { - error(procref.get().location, format!("{} calls {} which has override child proc that sleeps {}", procref, parent_proc, nextproc)) - .with_note(location, "SpacemanDMM_should_not_sleep set here") - .with_errortype("must_not_sleep") - .with_callstack(&callstack) - .with_blocking_builtins(sleepvec) - .register(self.context) + format!("{} calls {} which has override child proc that sleeps {}", procref, parent_proc, nextproc) } else { - error(procref.get().location, format!("{} sets SpacemanDMM_should_not_sleep but calls blocking proc {}", procref, nextproc)) - .with_note(location, "SpacemanDMM_should_not_sleep set here") - .with_errortype("must_not_sleep") - .with_callstack(&callstack) - .with_blocking_builtins(sleepvec) - .register(self.context) + format!("{} sets SpacemanDMM_should_not_sleep but calls blocking proc {}", procref, nextproc) + }; + + error(procref.get().location, desc) + .with_note(location, "SpacemanDMM_should_not_sleep set here") + .with_errortype("must_not_sleep") + .with_callstack(&callstack) + .with_blocking_builtins(sleepvec) + .register(self.context); + + if callstack.call_stack.len() > 1 { + let working_sleepvec = sleepvec.clone(); + for (builtin, location) in working_sleepvec { + let mut working_callstack = callstack.call_stack.clone(); + working_callstack.pop_back(); + for (calling_proc, _calling_location, _new_context) in working_callstack { + sleeping_procs_transitive.insert_violator(calling_proc, &builtin, location); + } + } } } + // no more recursion if we hit a transitive sleeper, we've already been there + if transitive { + continue; + } + nextproc.recurse_children(&mut |child_proc| to_visit.push_back((child_proc, callstack.clone(), false, nextproc))); diff --git a/crates/dreamchecker/tests/sleep_tests.rs b/crates/dreamchecker/tests/sleep_tests.rs index 343b4a8ed..d5166b2e3 100644 --- a/crates/dreamchecker/tests/sleep_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -1,7 +1,7 @@ extern crate dreamchecker as dc; -use dc::test_helpers::check_errors_match; +use dc::test_helpers::{check_errors_match, parse_a_file_for_test}; const SLEEP_ERRORS: &[(u32, u16, &str)] = &[ (16, 16, "/mob/proc/test3 sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/sleepingproc"), @@ -207,3 +207,54 @@ fn sleep6() { "##.trim(); check_errors_match(code, SLEEP_ERROR6); } + +// This one is mainly for debuggable code coverage on the sleeping_procs_transitive optimization +#[test] +fn sleep7() { + let code = r##" +/proc/sleeper() + sleep(1) + +/proc/sleep_caller() + sleeper() + +/proc/nosleep1() + set SpacemanDMM_should_not_sleep = 1 + sleep_caller() + +/proc/nosleep2() + set SpacemanDMM_should_not_sleep = 1 + sleep_caller() +"##.trim(); + let context = parse_a_file_for_test(code); + let errors = context.errors(); + assert_eq!(2, errors.len()); + + // the parser or dreamchecker does shit in a random order and i CBA to find out why + for error in errors.iter() { + assert_eq!("must_not_sleep", error.errortype().unwrap()); + } +} + +// Testing a possible infinite loop? +#[test] +fn sleep8() { + let code = r##" +/proc/sleeper() + sleep(1) + +/proc/sleep_caller() + nosleep() + nosleep() + nosleep() + nosleep() + sleeper() + +/proc/nosleep() + set SpacemanDMM_should_not_sleep = 1 + sleep_caller() +"##.trim(); + check_errors_match(code, &[ + (11, 14, "/proc/nosleep sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/sleeper"), + ]); +} From ca20006f38b4232b11c2f9f8b4b333ad7b7bc2a7 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 11:09:19 -0400 Subject: [PATCH 08/20] Optimization --- crates/dreamchecker/src/lib.rs | 75 ++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index e0ef18dd6..2c43d7c0a 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -10,6 +10,7 @@ use dm::constants::{Constant, ConstFn}; use dm::ast::*; use std::collections::{BTreeMap, HashMap, HashSet, VecDeque}; +use std::time::SystemTime; use ahash::RandomState; @@ -362,7 +363,7 @@ fn run_inner(context: &Context, objtree: &ObjectTree, cli: bool) { cli_println!("============================================================"); cli_println!("Analyzing proc call tree...\n"); - analyzer.check_proc_call_tree(); + analyzer.check_proc_call_tree(cli); } // ---------------------------------------------------------------------------- @@ -640,12 +641,28 @@ impl<'o> AnalyzeObjectTree<'o> { } } - pub fn check_proc_call_tree(&mut self) { - let mut sleeping_procs_transitive = ViolatingProcs{ - violators: HashMap::new() - }; + pub fn check_proc_call_tree(&mut self, cli: bool) { + // prepare for the worst case, avoiding the reallocations _is_ faster and less memory expensive + let total_procs = self.objtree.iter_types().flat_map(|type_ref: TypeRef| type_ref.iter_self_procs()).count(); + let mut sleeping_procs_transitive = Box::new(HashSet::::with_capacity(total_procs)); + + let total_must_not_sleep_procs = self.must_not_sleep.directive.len(); + if cli { + println!("{} procs to anaylze call chains for sleeps", total_must_not_sleep_procs); + } + + let mut last_update = SystemTime::now(); + for (index, (procref, &(_, location))) in self.must_not_sleep.directive.iter().enumerate() { + if cli { + let now = SystemTime::now(); + if let Ok(duration) = now.duration_since(last_update) { + if duration.as_secs() > 10 { + println!("{}/{} procs anaylzed", index, total_must_not_sleep_procs); + last_update = now; + } + } + } - for (procref, &(_, location)) in self.must_not_sleep.directive.iter() { if let Some(sleepvec) = self.sleeping_procs.get_violators(*procref) { error(procref.get().location, format!("{} sets SpacemanDMM_should_not_sleep but calls blocking built-in(s)", procref)) .with_note(location, "SpacemanDMM_should_not_sleep set here") @@ -672,7 +689,6 @@ impl<'o> AnalyzeObjectTree<'o> { if !visited.insert(nextproc) { continue } - if self.waitfor_procs.get(&nextproc).is_some() { continue } @@ -682,23 +698,18 @@ impl<'o> AnalyzeObjectTree<'o> { if new_context { continue } + let mut transitive = false; + if let Some(error) = if let Some(transitive_sleeping_proc_ref) = sleeping_procs_transitive.get(&nextproc) { + transitive = true; + Some(error(procref.get().location, format!("{} calls {} which transitively sleeps.", procref, nextproc))) + } else if let Some(sleepvec) = self.sleeping_procs.get_violators(nextproc) { + let parent_proc_type_index = parent_proc.ty().index(); + let next_proc_type_index = nextproc.ty().index(); - let parent_proc_type_index = parent_proc.ty().index(); - let next_proc_type_index = nextproc.ty().index(); + let proc_is_on_same_type_as_setting = next_proc_type_index == procref_type_index; + let proc_is_override = next_proc_type_index != parent_proc_type_index; - let proc_is_on_same_type_as_setting = next_proc_type_index == procref_type_index; - let proc_is_override = next_proc_type_index != parent_proc_type_index; - - let mut sleepvec_option = sleeping_procs_transitive.get_violators(nextproc); - let transitive = sleepvec_option.is_some(); - if !transitive { - sleepvec_option = self.sleeping_procs.get_violators(nextproc); - } - - if let Some(sleepvec) = sleepvec_option { - let desc = if transitive { - format!("{} calls {} which transitively sleeps", procref, nextproc) - } else if proc_is_on_same_type_as_setting && proc_is_override { + let desc = if proc_is_on_same_type_as_setting && proc_is_override { format!("{} sets SpacemanDMM_should_not_sleep but has override child proc that sleeps {}", procref, nextproc) } else if proc_is_override { format!("{} calls {} which has override child proc that sleeps {}", procref, parent_proc, nextproc) @@ -706,21 +717,21 @@ impl<'o> AnalyzeObjectTree<'o> { format!("{} sets SpacemanDMM_should_not_sleep but calls blocking proc {}", procref, nextproc) }; - error(procref.get().location, desc) + Some(error(procref.get().location, desc) + .with_blocking_builtins(sleepvec)) + } else { + None + } { + error .with_note(location, "SpacemanDMM_should_not_sleep set here") .with_errortype("must_not_sleep") .with_callstack(&callstack) - .with_blocking_builtins(sleepvec) .register(self.context); if callstack.call_stack.len() > 1 { - let working_sleepvec = sleepvec.clone(); - for (builtin, location) in working_sleepvec { - let mut working_callstack = callstack.call_stack.clone(); - working_callstack.pop_back(); - for (calling_proc, _calling_location, _new_context) in working_callstack { - sleeping_procs_transitive.insert_violator(calling_proc, &builtin, location); - } + let mut working_callstack = callstack.call_stack.clone(); + while let Some((calling_proc, _calling_location, _new_context)) = working_callstack.pop_back() { + sleeping_procs_transitive.insert(calling_proc); } } } @@ -743,6 +754,8 @@ impl<'o> AnalyzeObjectTree<'o> { } } + drop(sleeping_procs_transitive); + for (procref, (_, location)) in self.must_be_pure.directive.iter() { if let Some(impurevec) = self.impure_procs.get_violators(*procref) { error(procref.get().location, format!("{} does impure operations", procref)) From ebc661c5edfd2d464c259a80ed6f1eac0beb14cc Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 11:13:22 -0400 Subject: [PATCH 09/20] Asd says this box is unnecessary --- crates/dreamchecker/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index 2c43d7c0a..aed871e70 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -644,7 +644,7 @@ impl<'o> AnalyzeObjectTree<'o> { pub fn check_proc_call_tree(&mut self, cli: bool) { // prepare for the worst case, avoiding the reallocations _is_ faster and less memory expensive let total_procs = self.objtree.iter_types().flat_map(|type_ref: TypeRef| type_ref.iter_self_procs()).count(); - let mut sleeping_procs_transitive = Box::new(HashSet::::with_capacity(total_procs)); + let mut sleeping_procs_transitive = HashSet::::with_capacity(total_procs); let total_must_not_sleep_procs = self.must_not_sleep.directive.len(); if cli { From 3a608441be50bdd1a6a9a5144c1f18d7fa66a0d5 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 11:14:31 -0400 Subject: [PATCH 10/20] Fix a typo --- crates/dreamchecker/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index aed871e70..410752ea0 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -657,7 +657,7 @@ impl<'o> AnalyzeObjectTree<'o> { let now = SystemTime::now(); if let Ok(duration) = now.duration_since(last_update) { if duration.as_secs() > 10 { - println!("{}/{} procs anaylzed", index, total_must_not_sleep_procs); + println!("{}/{} procs analyzed", index, total_must_not_sleep_procs); last_update = now; } } From e5545143f8fd8ea30b42d2db5111f41509c5b339 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 11:15:56 -0400 Subject: [PATCH 11/20] Remove this .clone() --- crates/dreamchecker/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index 410752ea0..976499003 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -328,7 +328,7 @@ fn run_inner(context: &Context, objtree: &ObjectTree, cli: bool) { check_var_defs(objtree, context); - let mut analyzer = AnalyzeObjectTree::new(context, objtree.clone()); + let mut analyzer = AnalyzeObjectTree::new(context, objtree); cli_println!("============================================================"); cli_println!("Gathering proc settings...\n"); From 3429a014dc5c577c151e75815e26760bb50d740a Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 12:49:14 -0400 Subject: [PATCH 12/20] Final optimizations --- crates/dreamchecker/src/lib.rs | 56 ++++++++++++---------------------- 1 file changed, 20 insertions(+), 36 deletions(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index 976499003..7e3dc732c 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -642,17 +642,21 @@ impl<'o> AnalyzeObjectTree<'o> { } pub fn check_proc_call_tree(&mut self, cli: bool) { - // prepare for the worst case, avoiding the reallocations _is_ faster and less memory expensive - let total_procs = self.objtree.iter_types().flat_map(|type_ref: TypeRef| type_ref.iter_self_procs()).count(); - let mut sleeping_procs_transitive = HashSet::::with_capacity(total_procs); - let total_must_not_sleep_procs = self.must_not_sleep.directive.len(); if cli { println!("{} procs to anaylze call chains for sleeps", total_must_not_sleep_procs); } + // prepare for the worst case, avoiding the reallocations _is_ faster and less memory expensive + let total_procs = self.objtree.iter_types().flat_map(|type_ref: TypeRef| type_ref.iter_self_procs()).count(); + let mut visited = HashSet::>::with_capacity(total_procs); + let mut to_visit = VecDeque::<(ProcRef<'o>, CallStack, bool, ProcRef<'o>)>::new(); let mut last_update = SystemTime::now(); for (index, (procref, &(_, location))) in self.must_not_sleep.directive.iter().enumerate() { + if !visited.insert(*procref) { + continue; + } + if cli { let now = SystemTime::now(); if let Ok(duration) = now.duration_since(last_update) { @@ -671,11 +675,6 @@ impl<'o> AnalyzeObjectTree<'o> { .register(self.context) } - let mut visited = HashSet::>::new(); - let mut to_visit = VecDeque::<(ProcRef<'o>, CallStack, bool, ProcRef<'o>)>::new(); - - visited.insert(*procref); - if let Some(calledvec) = self.call_tree.get(procref) { for (proccalled, location, new_context) in calledvec.iter() { let mut newstack = CallStack::default(); @@ -686,23 +685,20 @@ impl<'o> AnalyzeObjectTree<'o> { let procref_type_index = procref.ty().index(); while let Some((nextproc, callstack, new_context, parent_proc)) = to_visit.pop_front() { - if !visited.insert(nextproc) { + if new_context { continue } - if self.waitfor_procs.get(&nextproc).is_some() { + if !visited.insert(nextproc) { continue } - if self.sleep_exempt.get(nextproc).is_some() { + if self.waitfor_procs.contains(&nextproc) { continue } - if new_context { + if self.sleep_exempt.get(nextproc).is_some() { continue } - let mut transitive = false; - if let Some(error) = if let Some(transitive_sleeping_proc_ref) = sleeping_procs_transitive.get(&nextproc) { - transitive = true; - Some(error(procref.get().location, format!("{} calls {} which transitively sleeps.", procref, nextproc))) - } else if let Some(sleepvec) = self.sleeping_procs.get_violators(nextproc) { + + if let Some(sleepvec) = self.sleeping_procs.get_violators(nextproc) { let parent_proc_type_index = parent_proc.ty().index(); let next_proc_type_index = nextproc.ty().index(); @@ -717,28 +713,15 @@ impl<'o> AnalyzeObjectTree<'o> { format!("{} sets SpacemanDMM_should_not_sleep but calls blocking proc {}", procref, nextproc) }; - Some(error(procref.get().location, desc) - .with_blocking_builtins(sleepvec)) - } else { - None - } { - error + error(procref.get().location, desc) + .with_blocking_builtins(sleepvec) .with_note(location, "SpacemanDMM_should_not_sleep set here") .with_errortype("must_not_sleep") .with_callstack(&callstack) .register(self.context); - if callstack.call_stack.len() > 1 { - let mut working_callstack = callstack.call_stack.clone(); - while let Some((calling_proc, _calling_location, _new_context)) = working_callstack.pop_back() { - sleeping_procs_transitive.insert(calling_proc); - } - } - } - - // no more recursion if we hit a transitive sleeper, we've already been there - if transitive { - continue; + // Abort now, we don't want to go unnecessarily deep + continue } nextproc.recurse_children(&mut |child_proc| @@ -754,7 +737,8 @@ impl<'o> AnalyzeObjectTree<'o> { } } - drop(sleeping_procs_transitive); + drop(visited); + drop(to_visit); for (procref, (_, location)) in self.must_be_pure.directive.iter() { if let Some(impurevec) = self.impure_procs.get_violators(*procref) { From c5667bfcf674f40bee6a0c3d3273e360b11c4be9 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 12:51:59 -0400 Subject: [PATCH 13/20] Fix tests --- crates/dreamchecker/tests/sleep_tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/dreamchecker/tests/sleep_tests.rs b/crates/dreamchecker/tests/sleep_tests.rs index d5166b2e3..addf2b302 100644 --- a/crates/dreamchecker/tests/sleep_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -208,7 +208,7 @@ fn sleep6() { check_errors_match(code, SLEEP_ERROR6); } -// This one is mainly for debuggable code coverage on the sleeping_procs_transitive optimization +// When there are transitive errors for a sleeping proc (sleep_caller in this case) only the first will be returned #[test] fn sleep7() { let code = r##" @@ -228,7 +228,7 @@ fn sleep7() { "##.trim(); let context = parse_a_file_for_test(code); let errors = context.errors(); - assert_eq!(2, errors.len()); + assert_eq!(1, errors.len()); // the parser or dreamchecker does shit in a random order and i CBA to find out why for error in errors.iter() { From 505a52c88c2d1c94d67dfeb4c5780705024312e3 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 12:54:48 -0400 Subject: [PATCH 14/20] Remove unnecessary instrumenting --- crates/dreamchecker/src/lib.rs | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index 7e3dc732c..1dfea4d70 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -10,7 +10,6 @@ use dm::constants::{Constant, ConstFn}; use dm::ast::*; use std::collections::{BTreeMap, HashMap, HashSet, VecDeque}; -use std::time::SystemTime; use ahash::RandomState; @@ -363,7 +362,7 @@ fn run_inner(context: &Context, objtree: &ObjectTree, cli: bool) { cli_println!("============================================================"); cli_println!("Analyzing proc call tree...\n"); - analyzer.check_proc_call_tree(cli); + analyzer.check_proc_call_tree(); } // ---------------------------------------------------------------------------- @@ -641,32 +640,16 @@ impl<'o> AnalyzeObjectTree<'o> { } } - pub fn check_proc_call_tree(&mut self, cli: bool) { - let total_must_not_sleep_procs = self.must_not_sleep.directive.len(); - if cli { - println!("{} procs to anaylze call chains for sleeps", total_must_not_sleep_procs); - } - + pub fn check_proc_call_tree(&mut self) { // prepare for the worst case, avoiding the reallocations _is_ faster and less memory expensive let total_procs = self.objtree.iter_types().flat_map(|type_ref: TypeRef| type_ref.iter_self_procs()).count(); let mut visited = HashSet::>::with_capacity(total_procs); let mut to_visit = VecDeque::<(ProcRef<'o>, CallStack, bool, ProcRef<'o>)>::new(); - let mut last_update = SystemTime::now(); for (index, (procref, &(_, location))) in self.must_not_sleep.directive.iter().enumerate() { if !visited.insert(*procref) { continue; } - if cli { - let now = SystemTime::now(); - if let Ok(duration) = now.duration_since(last_update) { - if duration.as_secs() > 10 { - println!("{}/{} procs analyzed", index, total_must_not_sleep_procs); - last_update = now; - } - } - } - if let Some(sleepvec) = self.sleeping_procs.get_violators(*procref) { error(procref.get().location, format!("{} sets SpacemanDMM_should_not_sleep but calls blocking built-in(s)", procref)) .with_note(location, "SpacemanDMM_should_not_sleep set here") From da085a733f5b2fdda13f18a81f51cba70142cf3a Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 13:54:20 -0400 Subject: [PATCH 15/20] Add missing docs for ALLOWED_TO_SLEEP --- crates/dreamchecker/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/dreamchecker/README.md b/crates/dreamchecker/README.md index 0e9451422..19b34fb87 100644 --- a/crates/dreamchecker/README.md +++ b/crates/dreamchecker/README.md @@ -62,6 +62,7 @@ be enabled: #define UNLINT(X) SpacemanDMM_unlint(X) #define SHOULD_NOT_OVERRIDE(X) set SpacemanDMM_should_not_override = X #define SHOULD_NOT_SLEEP(X) set SpacemanDMM_should_not_sleep = X + #define ALLOWED_TO_SLEEP(X) set SpacemanDMM_allowed_to_sleep = X #define SHOULD_BE_PURE(X) set SpacemanDMM_should_be_pure = X #define PRIVATE_PROC(X) set SpacemanDMM_private_proc = X #define PROTECTED_PROC(X) set SpacemanDMM_protected_proc = X @@ -75,6 +76,7 @@ be enabled: #define UNLINT(X) X #define SHOULD_NOT_OVERRIDE(X) #define SHOULD_NOT_SLEEP(X) + #define ALLOWED_TO_SLEEP(X) #define SHOULD_BE_PURE(X) #define PRIVATE_PROC(X) #define PROTECTED_PROC(X) From c6d4ba5ef124a8c070f6561c34b5ce9f25f16e85 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 16:38:52 -0400 Subject: [PATCH 16/20] Do not recurse child procs when checking /New() No measurable change to checking time. --- crates/dreamchecker/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index 1dfea4d70..915ea6d23 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -707,8 +707,10 @@ impl<'o> AnalyzeObjectTree<'o> { continue } - nextproc.recurse_children(&mut |child_proc| - to_visit.push_back((child_proc, callstack.clone(), false, nextproc))); + if nextproc.ty().index() != self.objtree.root().index() && nextproc.name() != "New" { + nextproc.recurse_children(&mut |child_proc| + to_visit.push_back((child_proc, callstack.clone(), false, nextproc))); + } if let Some(calledvec) = self.call_tree.get(&nextproc) { for (proccalled, location, new_context) in calledvec.iter() { From b9a0314642d3ac89576f095c215f9628fca61fb7 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 16:48:17 -0400 Subject: [PATCH 17/20] Fix sleep_tests (previous behavior was incorrect) --- crates/dreamchecker/tests/sleep_tests.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/dreamchecker/tests/sleep_tests.rs b/crates/dreamchecker/tests/sleep_tests.rs index addf2b302..44181f236 100644 --- a/crates/dreamchecker/tests/sleep_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -48,7 +48,6 @@ fn sleep() { const SLEEP_ERRORS2: &[(u32, u16, &str)] = &[ (8, 21, "/mob/living/proc/bar calls /mob/living/proc/foo which has override child proc that sleeps /mob/living/carbon/proc/foo"), (8, 21, "/mob/living/proc/bar calls /mob/proc/thing which has override child proc that sleeps /mob/dead/proc/thing"), - (8, 21, "/mob/living/proc/bar calls /mob/proc/New which has override child proc that sleeps /mob/dead/proc/New"), ]; #[test] @@ -112,7 +111,6 @@ fn sleep3() { check_errors_match(code, &[ (8, 23, "/atom/movable/proc/bar calls /atom/movable/proc/foo which has override child proc that sleeps /mob/proc/foo"), (8, 23, "/atom/movable/proc/bar calls /atom/proc/thing which has override child proc that sleeps /atom/dead/proc/thing"), - (8, 23, "/atom/movable/proc/bar calls /atom/proc/New[1/2] which has override child proc that sleeps /atom/dead/proc/New"), ]); } From f19545a3e002dce05969529ca3f00fefed8134ae Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 7 May 2023 18:39:55 -0400 Subject: [PATCH 18/20] For this error to make sense, the builtin should come _after_ the callstack --- crates/dreamchecker/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index 915ea6d23..c42c4b49e 100644 --- a/crates/dreamchecker/src/lib.rs +++ b/crates/dreamchecker/src/lib.rs @@ -697,10 +697,10 @@ impl<'o> AnalyzeObjectTree<'o> { }; error(procref.get().location, desc) - .with_blocking_builtins(sleepvec) .with_note(location, "SpacemanDMM_should_not_sleep set here") .with_errortype("must_not_sleep") .with_callstack(&callstack) + .with_blocking_builtins(sleepvec) .register(self.context); // Abort now, we don't want to go unnecessarily deep From e934dac115120ad6a8d22c670a11191c05c27371 Mon Sep 17 00:00:00 2001 From: Dominion Date: Fri, 12 May 2023 22:27:36 -0400 Subject: [PATCH 19/20] Failing test: Walking incorrect type tree --- crates/dreamchecker/tests/sleep_tests.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/dreamchecker/tests/sleep_tests.rs b/crates/dreamchecker/tests/sleep_tests.rs index 44181f236..dae3bcacd 100644 --- a/crates/dreamchecker/tests/sleep_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -256,3 +256,24 @@ fn sleep8() { (11, 14, "/proc/nosleep sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/sleeper"), ]); } + +// Testing avoidance of false positive from overrides of different type chains +#[test] +fn sleep9() { + let code = r##" +/proc/main() + set SpacemanDMM_should_not_sleep = 1 + var/datum/override1/O = new + O.StartTest() + +/datum/proc/CommonProc() + return + +/datum/override1/proc/StartTest() + CommonProc() + +/datum/override2/CommonProc() + sleep(1) +"##.trim(); + check_errors_match(code, &[]); +} From 92cc07f0f7c306597a6a823dfe156c84b98daad1 Mon Sep 17 00:00:00 2001 From: Jordan Dominion Date: Sat, 20 May 2023 23:11:13 -0400 Subject: [PATCH 20/20] Fixing this test may not be possible actually --- crates/dreamchecker/tests/sleep_tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/dreamchecker/tests/sleep_tests.rs b/crates/dreamchecker/tests/sleep_tests.rs index dae3bcacd..df820cd39 100644 --- a/crates/dreamchecker/tests/sleep_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -258,6 +258,7 @@ fn sleep8() { } // Testing avoidance of false positive from overrides of different type chains +/* This test _should_ be enabled but it currently fails because I'm not sure if there's a way to know if we're calling from src or another var #[test] fn sleep9() { let code = r##" @@ -277,3 +278,4 @@ fn sleep9() { "##.trim(); check_errors_match(code, &[]); } +*/