diff --git a/crates/dreamchecker/README.md b/crates/dreamchecker/README.md index 0e945142..19b34fb8 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) diff --git a/crates/dreamchecker/src/lib.rs b/crates/dreamchecker/src/lib.rs index ee275de9..c42c4b49 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(), } } @@ -643,7 +641,15 @@ impl<'o> AnalyzeObjectTree<'o> { } pub fn check_proc_call_tree(&mut self) { - for (procref, &(_, location)) in self.must_not_sleep.directive.iter() { + // 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(); + for (index, (procref, &(_, location))) in self.must_not_sleep.directive.iter().enumerate() { + if !visited.insert(*procref) { + continue; + } + 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") @@ -651,58 +657,74 @@ 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)); + + 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() { - if !visited.insert(nextproc) { + + let procref_type_index = procref.ty().index(); + while let Some((nextproc, callstack, new_context, parent_proc)) = to_visit.pop_front() { + 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 } + 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)) + 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 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) + } else { + 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) - } 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)) - .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()) - .register(self.context) - } + .register(self.context); + + // Abort now, we don't want to go unnecessarily deep + continue + } + + 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() { 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)); } } } } + 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) { error(procref.get().location, format!("{} does impure operations", procref)) @@ -829,13 +851,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/src/test_helpers.rs b/crates/dreamchecker/src/test_helpers.rs index e0ea5e0c..8fe81a0e 100644 --- a/crates/dreamchecker/src/test_helpers.rs +++ b/crates/dreamchecker/src/test_helpers.rs @@ -26,23 +26,36 @@ 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() { - 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()); } } diff --git a/crates/dreamchecker/tests/pure_tests.rs b/crates/dreamchecker/tests/pure_tests.rs new file mode 100644 index 00000000..7ed1b20c --- /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; + +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 +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 57% rename from crates/dreamchecker/tests/sleep_pure_tests.rs rename to crates/dreamchecker/tests/sleep_tests.rs index 1f62b56a..df820cd3 100644 --- a/crates/dreamchecker/tests/sleep_pure_tests.rs +++ b/crates/dreamchecker/tests/sleep_tests.rs @@ -1,9 +1,9 @@ extern crate dreamchecker as dc; -use dc::test_helpers::check_errors_match; +use dc::test_helpers::{check_errors_match, parse_a_file_for_test}; -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,8 +45,9 @@ 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"), + (8, 21, "/mob/living/proc/bar calls /mob/proc/thing which has override child proc that sleeps /mob/dead/proc/thing"), ]; #[test] @@ -109,10 +110,11 @@ 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"), ]); } -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,8 +152,9 @@ fn sleep4() { } // Test overrides and for regression of issue #267 -pub const SLEEP_ERROR5: &[(u32, u16, &str)] = &[ - (7, 19, "/datum/sub/proc/checker sets SpacemanDMM_should_not_sleep but calls blocking proc /proc/sleeper"), +const SLEEP_ERROR5: &[(u32, u16, &str)] = &[ + (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] @@ -175,47 +178,104 @@ fn sleep5() { 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 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 pure() { +fn sleep6() { 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() +/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, PURE_ERRORS); + check_errors_match(code, SLEEP_ERROR6); } -// 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"), -]; +// 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##" +/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!(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() { + assert_eq!("must_not_sleep", error.errortype().unwrap()); + } +} + +// Testing a possible infinite loop? #[test] -fn pure2() { +fn sleep8() { let code = r##" -/mob/proc/test() - set SpacemanDMM_should_be_pure = TRUE - return 1 -/mob/proc/test2() - test() -/mob/proc/test3() - return test() +/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"), + ]); +} + +// 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##" +/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, PURE2_ERRORS); + check_errors_match(code, &[]); } +*/ diff --git a/crates/dreammaker/src/objtree.rs b/crates/dreammaker/src/objtree.rs index 46169776..6b01faa5 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); + } } }