Warn about invalid filter() kwargs

src/dreamchecker/lib.rs

@@ -1247,54 +1247,65 @@ impl<'o, 's> AnalyzeProc<'o, 's> {
         let mut param_name_map = HashMap::new();
         let mut param_idx_map = HashMap::new();
         let mut param_idx = 0;
+        let mut arglist_used = false;
 
         for arg in args {
             let mut argument_value = arg;
             let mut this_kwarg = None;
-            if let Expression::AssignOp { op: AssignOp::Assign, lhs, rhs } = arg {
-                match lhs.as_term() {
-                    Some(Term::Ident(name)) |
-                    Some(Term::String(name)) => {
-                        // Don't visit_expression the kwarg key.
-                        any_kwargs_yet = true;
-                        this_kwarg = Some(name);
-                        argument_value = rhs;
-
-                        // Check that that kwarg actually exists.
-                        if !proc.parameters.iter().any(|p| p.name == *name) {
-                            // Search for a child proc that does have this keyword argument.
-                            let mut error = error(location,
-                                format!("bad keyword argument {:?} to {}", name, proc));
-                            proc.recurse_children(&mut |child_proc| {
-                                if child_proc.ty() == proc.ty() { return }
-                                if child_proc.parameters.iter().any(|p| p.name == *name) {
-                                    error.add_note(child_proc.location, format!("an override has this parameter: {}", child_proc));
-                                }
-                            });
-                            error.register(self.context);
-                        } else if !is_exact {
-                            // If it does, mark it as "used".
-                            // Format with src/proc/foo here, rather than the
-                            // type the proc actually appears on, so that
-                            // calling /datum/foo() on a /datum/A won't
-                            // complain about /datum/B/foo().
-                            self.env.used_kwargs.entry(format!("{}/proc/{}", src, proc.name()))
-                                .or_insert_with(|| KwargInfo {
-                                    location: proc.location,
-                                    .. Default::default()
-                                })
-                                .called_at
-                                // TODO: use a more accurate location
-                                .entry(name.clone())
-                                .and_modify(|ca| ca.others += 1)
-                                .or_insert(CalledAt {
-                                    location: location,
-                                    others: 0,
+            match arg {
+                Expression::AssignOp { op: AssignOp::Assign, lhs, rhs } => {
+                    match lhs.as_term() {
+                        Some(Term::Ident(name)) |
+                        Some(Term::String(name)) => {
+                            // Don't visit_expression the kwarg key.
+                            any_kwargs_yet = true;
+                            this_kwarg = Some(name);
+                            argument_value = rhs;
+
+                            // Check that that kwarg actually exists.
+                            if !proc.parameters.iter().any(|p| p.name == *name) {
+                                // Search for a child proc that does have this keyword argument.
+                                let mut error = error(location,
+                                    format!("bad keyword argument {:?} to {}", name, proc));
+                                proc.recurse_children(&mut |child_proc| {
+                                    if child_proc.ty() == proc.ty() { return }
+                                    if child_proc.parameters.iter().any(|p| p.name == *name) {
+                                        error.add_note(child_proc.location, format!("an override has this parameter: {}", child_proc));
+                                    }
                                 });
+                                error.register(self.context);
+                            } else if !is_exact {
+                                // If it does, mark it as "used".
+                                // Format with src/proc/foo here, rather than the
+                                // type the proc actually appears on, so that
+                                // calling /datum/foo() on a /datum/A won't
+                                // complain about /datum/B/foo().
+                                self.env.used_kwargs.entry(format!("{}/proc/{}", src, proc.name()))
+                                    .or_insert_with(|| KwargInfo {
+                                        location: proc.location,
+                                        .. Default::default()
+                                    })
+                                    .called_at
+                                    // TODO: use a more accurate location
+                                    .entry(name.clone())
+                                    .and_modify(|ca| ca.others += 1)
+                                    .or_insert(CalledAt {
+                                        location: location,
+                                        others: 0,
+                                    });
+                            }
+                        }
+                        _ => {}
+                    }
+                },
+                expr => {
+                    if let Some(Term::Call(callname, _)) = expr.as_term() {
+                        // only interested in the first expression being arglist
+                        if callname.as_str() == "arglist" && param_name_map.len() == 0 && param_idx == 0 {
+                            arglist_used = true;
                         }
                     }
-                    _ => {}
-                }
+                },
             }
 
             if any_kwargs_yet && this_kwarg.is_none() && !(proc.ty().is_root() && proc.name() == "animate") {
@@ -1312,6 +1323,39 @@ impl<'o, 's> AnalyzeProc<'o, 's> {
             }
         }
 
+        // filter call checking
+        // TODO: check flags for valid values
+        //  eg "wave" type "flags" param only works with WAVE_SIDEWAYS, WAVE_BOUND
+        // also some filters have limits for their numerical params
+        //  eg "rays" type "threshold" param defaults to 0.5, can be 0 to 1
+        if proc.ty().is_root() && proc.name() == "filter" {
+            guard!(let Some(typename) = param_name_map.get("type") else {
+                if !arglist_used {
+                    error(location, "filter() called without mandatory keyword parameter 'type'")
+                        .register(self.context);
+                } // regardless, we're done here
+                return Analysis::empty()
+            });
+            guard!(let Some(Constant::String(typevalue)) = &typename.value else {
+                error(location, format!("filter() called with non-string type keyword parameter value '{:?}'", typename.value))
+                    .register(self.context);
+                return Analysis::empty()
+            });
+            guard!(let Some(arglist) = VALID_FILTER_TYPES.get(typevalue.as_str()) else {
+                error(location, format!("filter() called with invalid type keyword parameter value '{}'", typevalue))
+                    .register(self.context);
+                return Analysis::empty()
+            });
+            for arg in param_name_map.keys() {
+                if *arg != "type" && arglist.iter().position(|&x| x == *arg).is_none() {
+                    error(location, format!("filter(type=\"{}\") called with invalid keyword parameter '{}'", typevalue, arg))
+                        // luckily lummox has made the anchor url match the type= value for each filter
+                        .with_note(location, format!("See: http://www.byond.com/docs/ref/#/{{notes}}/filters/{} for the permitted arguments", typevalue))
+                        .register(self.context);
+                }
+            }
+        }
+
         if let Some(return_type) = self.env.return_type.get(&proc) {
             let ec = type_expr::TypeExprContext {
                 objtree: self.objtree,

src/dreamchecker/tests/kwargs_tests.rs

@@ -17,4 +17,38 @@ fn after_kwarg() {
     check_errors_match(code, AFTER_KWARG_ERRORS);
 }
 
-//TODO: test filter() stuff when that PR is merged
+pub const FILTER_KWARGS_ERRORS: &[(u32, u16, &str)] = &[
+    (15, 5, "filter(type=\"alpha\") called with invalid keyword parameter 'color'"),
+    (16, 5, "filter(type=\"blur\") called with invalid keyword parameter 'x'"),
+    (17, 5, "filter() called with invalid type keyword parameter value 'fakename'"),
+    (18, 5, "filter() called without mandatory keyword parameter 'type'"),
+    (19, 5, "filter() called without mandatory keyword parameter 'type'"),
+    (20, 5, "filter(type=\"wave\") called with invalid keyword parameter 'color'"),
+];
+
+#[test]
+fn filter_kwarg() {
+    let code = r##"
+/proc/test()
+    filter(type="alpha", x=1, y=2, icon=null, render_source=null, flags=0)
+    filter(type="angular_blur", x=1, y=2, size=null)
+    filter(type="color", color=null, space=null)
+    filter(type="displace", x=1, y=2, size=3, icon=null, render_source=null)
+    filter(type="drop_shadow", x=1, y=2, size=3, offset=4, color=5)
+    filter(type="blur", size=1)
+    filter(type="layer", x=1, y=2, icon=null, render_source=null, flags=0, color=0, transform=null, blend_mode=null)
+    filter(type="motion_blur", x=1, y=2)
+    filter(type="outline", size=1, color=2)
+    filter(type="radial_blur", x=1, y=2, size=3)
+    filter(type="rays", x=1, y=2, size=3, color=null, offset=4, density=5, threshold=6, factor=7, flags=0)
+    filter(type="ripple", x=1, y=2, size=3, repeat=1, radius=4, falloff=5, flags=0)
+    filter(type="wave", x=1, y=2, size=4, offset=5, flags=0)
+    filter(type="alpha", color=null)
+    filter(type="blur", x=1)
+    filter(type="fakename", x=3)
+    filter(x=4)
+    filter("alpha", x=1)
+    filter(type="wave", color=null)
+"##.trim();
+    check_errors_match(code, FILTER_KWARGS_ERRORS);
+}

src/dreammaker/Cargo.toml

@@ -18,6 +18,7 @@ serde = { version = "1.0.103", features = ["derive"] }
 serde_derive = "1.0.103"
 toml = "0.5.5"
 guard = "0.5.0"
+phf = { version = "0.8.0", features = ["macros"] }
 
 [dependencies.linked-hash-map]
 git = "https://github.com/SpaceManiac/linked-hash-map"