Should not sleep Fixes

crates/dreamchecker/src/lib.rs

@@ -562,7 +562,6 @@ pub struct AnalyzeObjectTree<'o> {
     impure_procs: ViolatingProcs<'o>,
     waitfor_procs: HashSet<ProcRef<'o>>,
 
-    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::<ProcRef<'o>>::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 {

crates/dreamchecker/src/test_helpers.rs

@@ -26,23 +26,36 @@ pub fn check_errors_match<S: Into<Cow<'static, str>>>(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());
     }
 }

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);
+}

crates/dreamchecker/tests/sleep_pure_tests.rs → 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,8 +45,10 @@ 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"),
+    (8, 21, "/mob/living/proc/bar calls /mob/proc/New which has override child proc that sleeps /mob/dead/proc/New"),
 ];
 
 #[test]
@@ -109,10 +111,12 @@ 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"),
     ]);
 }
 
-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 +154,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 +180,30 @@ 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()
-"##.trim();
-    check_errors_match(code, PURE_ERRORS);
-}
+/datum/proc/is_valid(value)
+    set SpacemanDMM_should_not_sleep = 1
 
-// 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"),
-];
+/datum/choiced/is_valid(value)
+    get_choices()
 
-#[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()
+/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, PURE2_ERRORS);
+    check_errors_match(code, SLEEP_ERROR6);
 }

crates/dreammaker/src/objtree.rs

@@ -266,6 +266,17 @@ impl<'a> TypeRef<'a> {
         }
     }
 
+    /// Recursively visit this and all child **types**.
+    pub fn recurse_types<F: FnMut(TypeRef<'a>)>(&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<F: FnMut(TypeRef<'a>)>(&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<F: FnMut(ProcRef<'a>)>(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<Type>,
     types: BTreeMap<String, NodeIndex>,
+    redirected_parent_types: Vec<NodeIndex>,
 }
 
 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);
+            }
         }
     }