Warn about complex deps

selene-lib/src/ast_util/scopes.rs

@@ -90,7 +90,9 @@ pub struct Reference {
     pub within_function_stmt: Option<WithinFunctionStmt>,
 
     // x.y["z"] produces ["y", "z"]
-    // x.y.z().w produce ["y", "z", "w"]
+    // x.y.z().w produces ["y", "z", "w"]
+    // x.y[z].w produces ["y", None, "w"]. If this changes, ensure `roact_non_exhaustive_deps` is updated
+    // to detect dynamic indexing.
     pub indexing: Option<Vec<IndexEntry>>,
 }
 
@@ -1164,5 +1166,6 @@ mod tests {
 
         test_indexing("x.y.z", &[Some("y"), Some("z")]);
         test_indexing("a.b.c().d", &[Some("b"), Some("c()"), Some("d")]);
+        test_indexing("a[b].c().d", &[None, Some("c()"), Some("d")]);
     }
 }

selene-lib/src/lints/roblox_roact_non_exhaustive_deps.rs

@@ -4,6 +4,7 @@ use std::{
     collections::{HashMap, HashSet},
     convert::Infallible,
     hash::Hash,
+    vec,
 };
 
 use full_moon::{
@@ -47,6 +48,7 @@ impl Lint for RoactNonExhaustiveDepsLint {
             fn_declaration_starts_stack: Vec::new(),
             missing_dependencies: Vec::new(),
             unnecessary_dependencies: Vec::new(),
+            complex_dependencies: Vec::new(),
             non_reactive_upvalue_starts: HashSet::new(),
         };
 
@@ -105,6 +107,22 @@ impl Lint for RoactNonExhaustiveDepsLint {
             }
         }
 
+        for invalid_event in visitor.complex_dependencies {
+            diagnostics.push(Diagnostic::new_complete(
+                "roblox_roact_non_exhaustive_deps",
+                format!(
+                    "react hook {} has a complex expression in the dependency array",
+                    invalid_event.hook_name
+                ),
+                Label::new(invalid_event.range),
+                vec![
+                    "help: extract it to a separate variable so it can be statically checked"
+                        .to_string(),
+                ],
+                Vec::new(),
+            ));
+        }
+
         diagnostics
     }
 }
@@ -148,6 +166,16 @@ fn get_formatted_error_message(
     )
 }
 
+fn is_lua_valid_identifier(string: &str) -> bool {
+    // Valid identifier cannot start with numbers
+    let first_char = string.chars().next().unwrap();
+    if !first_char.is_alphabetic() && first_char != '_' {
+        return false;
+    }
+
+    string.chars().all(|c| c.is_alphanumeric() || c == '_')
+}
+
 fn get_last_function_call_suffix(prefix: &ast::Prefix, suffixes: &[&ast::Suffix]) -> String {
     let last_suffix = match suffixes.last() {
         Some(ast::Suffix::Call(ast::Call::MethodCall(_))) => suffixes.last(),
@@ -195,6 +223,7 @@ struct RoactMissingDependencyVisitor<'a> {
     scope_manager: &'a ScopeManager,
     missing_dependencies: Vec<MissingDependency>,
     unnecessary_dependencies: Vec<UnnecessaryDependency>,
+    complex_dependencies: Vec<ComplexDependency>,
     fn_declaration_starts_stack: Vec<usize>,
 
     // Some variables are safe to omit from the dependency array, such as setState
@@ -206,60 +235,80 @@ struct Upvalue {
     /// `a.b` yields `a`
     prefix: String,
 
-    /// `a.b["c"].d`, `a.b.[c].d`, and `a.b.c().d` yield `["a", "b"]`
+    /// `a.b["c d"].e` yields ["a", "b", "c d", "e"]`
+    ///
+    /// `a.b.[c].d` yields `["a", "b"]`
+    ///
+    /// `a.b.c().d` yields `["a", "b", "c"]`
+    // eslint requires passing in `a.b` for `a.b.c()` since js implicitly passes `a.b`
+    // as `this` to `c`. Lua doesn't do this, so we can allow `a.b.c` as deps
     indexing_identifiers: Vec<String>,
 
+    /// True if there's any dynamic indexing or function calls such as `a[b]` or `a.b()`
+    /// FIXME: This false negatives for function calls without indexing such as `a()`
+    is_complex_expression: bool,
+
     /// Knowing where referenced variable was initialized lets us narrow down whether it's a reactive variable
     resolved_start_range: Option<usize>,
 }
 
 impl Upvalue {
     fn new(reference: &Reference, resolved_start_range: &Option<usize>) -> Self {
-        let indexing_identifiers = reference
-            .indexing
-            .as_ref()
-            .unwrap_or(&vec![])
+        let default_indexing = Vec::new();
+        let indexing = reference.indexing.as_ref().unwrap_or(&default_indexing);
+
+        let indexing_identifiers = indexing
             .iter()
-            .map_while(|index_entry| {
-                if index_entry.is_function_call {
+            .enumerate()
+            .map_while(|(i, index_entry)| {
+                if i > 0 && indexing[i - 1].is_function_call {
                     return None;
                 }
 
                 index_entry.static_name.as_ref().and_then(|static_name| {
-                    if let TokenType::Identifier { identifier } = static_name.token().token_type() {
-                        Some(identifier.to_string())
-                    } else {
-                        None
+                    match static_name.token().token_type() {
+                        TokenType::Identifier { identifier } => Some(identifier.to_string()),
+                        TokenType::StringLiteral { literal, .. } => Some(literal.to_string()),
+                        _ => None,
                     }
                 })
             })
             .collect::<Vec<_>>();
 
+        let is_complex_expression = indexing
+            .iter()
+            .any(|index_entry| index_entry.static_name.is_none() || index_entry.is_function_call);
+
         Upvalue {
             prefix: reference.name.clone(),
             indexing_identifiers,
+            is_complex_expression,
             resolved_start_range: *resolved_start_range,
         }
     }
 
-    /// `a.b["c"].d`, `a.b.[c].d`, and `a.b.c().d` yield `a.b`
+    /// `a.b["c"]["d e"]` yields `a.b.c["d e"]`
+    ///
+    /// `a.b.c().d` yields `a.b.c`
+    ///
     /// `a` just yields `a`
     fn indexing_expression_name(&self) -> String {
-        let mut current_name = self.prefix.clone();
-        for index_name in &self.indexing_identifiers {
-            current_name.push('.');
-            current_name.push_str(index_name);
-        }
-        current_name
+        self.indexing_prefixes()
+            .last()
+            .unwrap_or(&"".to_string())
+            .to_string()
     }
 
     /// `a.b.c` yields `["a", "a.b", "a.b.c"]`
     fn indexing_prefixes(&self) -> Vec<String> {
         let mut prefixes = vec![self.prefix.clone()];
         let mut current_name = self.prefix.clone();
         for index_name in &self.indexing_identifiers {
-            current_name.push('.');
-            current_name.push_str(index_name);
+            if is_lua_valid_identifier(index_name) {
+                current_name.push_str(format!(".{}", index_name).as_str());
+            } else {
+                current_name.push_str(format!("[\"{}\"]", index_name).as_str());
+            }
             prefixes.push(current_name.clone());
         }
         prefixes
@@ -292,6 +341,12 @@ struct UnnecessaryDependency {
     hook_name: String,
 }
 
+#[derive(Debug)]
+struct ComplexDependency {
+    range: (usize, usize),
+    hook_name: String,
+}
+
 impl RoactMissingDependencyVisitor<'_> {
     fn get_upvalues_in_expression(&self, expression: &Expression) -> HashSet<Upvalue> {
         self.scope_manager
@@ -308,9 +363,8 @@ impl RoactMissingDependencyVisitor<'_> {
                         // FIXME: We need the start range where the variable was last set. Otherwise
                         // a variable can be first set outside but set again inside a component, and it
                         // identifies as non-reactive. However, this seems to only capture when user
-                        // does `local` again. Is there an alternative to also capture var = without local?
-                        // This is low priority as this only matters if user does something weird, like
-                        // writing to an outside variable within a component
+                        // does `local` again. This is low priority as this only matters if user does something
+                        // weird like writing to an outside variable within a component, which breaks a different rule
                         variable.identifiers.last().map(|(start, _)| *start)
                     });
 
@@ -381,6 +435,17 @@ impl Visitor for RoactMissingDependencyVisitor<'_> {
             .map(|upvalue| (upvalue.indexing_expression_name(), upvalue))
             .collect::<HashMap<_, _>>();
 
+        if dependencies
+            .iter()
+            .any(|(_, dependency)| dependency.is_complex_expression)
+        {
+            self.complex_dependencies.push(ComplexDependency {
+                range: range(dependency_array_expr),
+                hook_name: last_suffix.to_string(),
+            });
+            return;
+        }
+
         let mut missing_dependencies = referenced_upvalues
             .iter()
             .filter(|upvalue| {
@@ -569,6 +634,15 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_too_complex_deps() {
+        test_lint(
+            RoactNonExhaustiveDepsLint::new(()).unwrap(),
+            "roblox_roact_non_exhaustive_deps",
+            "too_complex_deps",
+        );
+    }
+
     #[test]
     fn test_use_memo() {
         test_lint(

selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.lua

@@ -43,8 +43,29 @@ local function Component()
     local a = {}
     local d = {}
     React.useEffect(function()
-        print(a.b.c()) -- Bad
-        print(a.b.d()) -- Bad, but already reported
+        print(a.b.c()) -- Good
+        print(a.b.d()) -- Bad
         print(d.e.f.g()) -- Good
     end, { a.b.c, d.e.f })
 end
+
+local function Component()
+    local a = {}
+    React.useEffect(function()
+        print(a.b["c d"]["e"])
+    end, {})
+end
+
+local function Component()
+    local a = {}
+    React.useEffect(function()
+        print(a["b c"]["d"])
+    end, { a["b c"]["d"] })
+end
+
+local function Component()
+    local a = {}
+    React.useEffect(function()
+        local _ = a["b c"]()
+    end, {})
+end

selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.stderr

@@ -22,11 +22,27 @@ error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing depend
    │
    = help: either include it or remove the dependency array
 
-error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'a.b'
+error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'a.b.d'
    ┌─ complex_deps.lua:49:10
    │
 49 │     end, { a.b.c, d.e.f })
    │          ^^^^^^^^^^^^^^^^
    │
    = help: either include it or remove the dependency array
 
+error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'a.b["c d"].e'
+   ┌─ complex_deps.lua:56:10
+   │
+56 │     end, {})
+   │          ^^
+   │
+   = help: either include it or remove the dependency array
+
+error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'a["b c"]'
+   ┌─ complex_deps.lua:70:10
+   │
+70 │     end, {})
+   │          ^^
+   │
+   = help: either include it or remove the dependency array
+

selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.lua

@@ -0,0 +1,39 @@
+local React
+
+local function Component()
+    local a = {}
+    React.useEffect(function()
+        print(a[b])
+    end, { a[b] })
+end
+
+local function Component()
+    local a = {}
+    React.useEffect(function()
+        print(a.b["c"]())
+    end, { a.b["c"]() })
+end
+
+local function Component()
+    local a = {}
+    React.useEffect(function()
+        print(a())
+        -- FIXME: false negatives for function calls without indexing
+    end, { a() })
+end
+
+local function Component()
+    local a = {}
+    React.useEffect(function()
+        -- A function call in place of array brackets should NOT warn about complex expr
+        -- due to lua-specific pattern of helper functions to patch holes in arrays
+    end, a.b())
+
+    React.useEffect(function()
+        -- Now this should warn
+    end, a.b(a.b()))
+
+    React.useEffect(function()
+        -- Now this should warn
+    end, { a.b() })
+end

selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.std.yml

@@ -0,0 +1,2 @@
+---
+name: roblox

selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.stderr

@@ -0,0 +1,32 @@
+error[roblox_roact_non_exhaustive_deps]: react hook useEffect has a complex expression in the dependency array
+  ┌─ too_complex_deps.lua:7:10
+  │
+7 │     end, { a[b] })
+  │          ^^^^^^^^
+  │
+  = help: extract it to a separate variable so it can be statically checked
+
+error[roblox_roact_non_exhaustive_deps]: react hook useEffect has a complex expression in the dependency array
+   ┌─ too_complex_deps.lua:14:10
+   │
+14 │     end, { a.b["c"]() })
+   │          ^^^^^^^^^^^^^^
+   │
+   = help: extract it to a separate variable so it can be statically checked
+
+error[roblox_roact_non_exhaustive_deps]: react hook useEffect has a complex expression in the dependency array
+   ┌─ too_complex_deps.lua:34:10
+   │
+34 │     end, a.b(a.b()))
+   │          ^^^^^^^^^^
+   │
+   = help: extract it to a separate variable so it can be statically checked
+
+error[roblox_roact_non_exhaustive_deps]: react hook useEffect has a complex expression in the dependency array
+   ┌─ too_complex_deps.lua:38:10
+   │
+38 │     end, { a.b() })
+   │          ^^^^^^^^^
+   │
+   = help: extract it to a separate variable so it can be statically checked
+