Add Roact exhaustive dependencies lint

CHANGELOG.md

@@ -2,6 +2,8 @@
 This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased](https://github.com/Kampfkarren/selene/compare/0.26.1...HEAD)
+### Added
+- Added new [`roblox_roact_non_exhaustive_deps` lint](https://kampfkarren.github.io/selene/lints/roblox_roact_non_exhaustive_deps.html), which will check for valid dependency arrays.
 
 ## [0.26.1](https://github.com/Kampfkarren/selene/releases/tag/0.26.1) - 2023-11-11
 ### Fixed

docs/src/SUMMARY.md

@@ -33,6 +33,7 @@
   - [parenthese_conditions](./lints/parenthese_conditions.md)
   - [roblox_incorrect_color3_new_bounds](./lints/roblox_incorrect_color3_new_bounds.md)
   - [roblox_incorrect_roact_usage](./lints/roblox_incorrect_roact_usage.md)
+  - [roblox_roact_non_exhaustive_deps](./lints/roblox_roact_non_exhaustive_deps.md) 
   - [roblox_suspicious_udim2_new](./lints/roblox_suspicious_udim2_new.md)
   - [shadowing](./lints/shadowing.md)
   - [suspicious_reverse_loop](./lints/suspicious_reverse_loop.md)

docs/src/lints/roblox_roact_non_exhaustive_deps.md

@@ -0,0 +1,63 @@
+# roblox_roact_non_exhaustive_deps
+## What it does
+Checks for valid dependency arrays. Verifies that dependencies are [reactive values](https://react.dev/learn/lifecycle-of-reactive-effects#effects-react-to-reactive-values) and that upvalues referenced in hooks with dependency arrays are included in the array.
+
+## Why this is bad
+Hooks that are missing dependencies will read stale values and cause bugs.
+
+## Example
+```lua
+local function Component(props)
+    React.useEffect(function()
+        print(props)
+    end) -- ok
+
+    React.useEffect(function()
+        print(props)
+    end, {}) -- Missing `props`
+
+    React.useEffect(function()
+        print(props.a)
+    end, { props.a }) -- ok
+
+    React.useEffect(function()
+        print(props.a)
+    end, { props.a.b }) -- Missing `props.a`
+
+    React.useEffect(function()
+        print(props.a())
+    end, { props.a() }) -- Too complex, extract `props.a()` to variable
+
+    local a1 = props.a()
+    React.useEffect(function()
+        print(a1)
+    end, { a1 }) -- now ok
+
+    React.useEffect(function()
+        print(props[a])
+    end, { props[a] }) -- Too complex, extract `props[a]` to variable
+
+    local a2 = props[a]
+    React.useEffect(function()
+        print(a2)
+    end, { a2 }) -- now ok
+
+    React.useEffect(function()
+        print(props)
+    end, helperFunction(props)) -- ok
+end
+```
+
+## Remarks
+1. This lint assumes either Roact or React is defined. [`undefined_variable`](./undefined_variable.md) will still lint, however.
+2. This lint assumes the hook is prefixed with either `React`, `Roact`, `hooks` for legacy Roact, or a variable assigned to them.
+3. This lint only takes effect for `useEffect`, `useMemo`, `useCallback`, and `useLayoutEffect`. Custom hooks that take dependency arrays will not lint.
+4. This lint does not take effect if nothing is passed to the second argument of the hook.
+5. This lint is only active if you are using the Roblox standard library.
+6. This lint warns against complex dependency expressions like function calls and dynamic indexing. This currently false negatives with function calls without any indexing, such as `{ a() }`. 
+7. This lint will ignore upvalues that are not reactive. Some examples of this are variables defined outside the component and variables that are known to be stable such as setter functions to `useState`.
+8. There is a false positive where complex function calls without variable assignments will require the base table to be passed as dependencies. For example, `a.b` as dependency should satisfy `a.b()`, but that's not the case. However, it does satisfy `local _ = a.b()`.
+
+## Deviations from [eslint-plugin-react-hooks/exhaustive-deps](https://www.npmjs.com/package/eslint-plugin-react-hooks)
+1. ESLint requires passing in `a` for `a.b()` since js can implicitly pass `a` as `this` to `a.b`. Lua doesn't do this, so Selene allows `a.b` as dependencies.
+2. ESLint complains about brackets in dependencies like `a["b"]` as being too complex. If string literals are inside the brackets and not a variable like `a[b]`, Selene recognizes that and treat it the same as `a.b`.

selene-lib/src/ast_util/scopes.rs

@@ -1,7 +1,7 @@
 use std::{borrow::Cow, collections::HashSet};
 
 use full_moon::{
-    ast::{self, VarExpression},
+    ast::{self},
     node::Node,
     tokenizer::{Symbol, TokenKind, TokenReference, TokenType},
     visitors::Visitor,
@@ -90,8 +90,9 @@ pub struct Reference {
     pub within_function_stmt: Option<WithinFunctionStmt>,
 
     // x.y["z"] produces ["y", "z"]
-    // x.y.z().w is None currently, but could change if necessary.
-    // If that change is made, ensure unused_variable is adjusted for write_only.
+    // 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>>,
 }
 
@@ -131,6 +132,7 @@ pub struct WithinFunctionStmt {
 pub struct IndexEntry {
     pub index: Range,
     pub static_name: Option<TokenReference>,
+    pub is_function_call: bool,
 }
 
 #[derive(Debug, Default)]
@@ -347,6 +349,7 @@ impl ScopeVisitor {
                 self.read_prefix(call.prefix());
                 for suffix in call.suffixes() {
                     self.read_suffix(suffix);
+                    self.adjust_indexing(call.suffixes(), range(call));
                 }
             }
 
@@ -488,7 +491,7 @@ impl ScopeVisitor {
         match var {
             ast::Var::Expression(var_expr) => {
                 self.read_prefix(var_expr.prefix());
-                self.adjust_indexing(var_expr);
+                self.adjust_indexing(var_expr.suffixes(), range(var_expr));
 
                 for suffix in var_expr.suffixes() {
                     self.read_suffix(suffix);
@@ -697,17 +700,24 @@ impl ScopeVisitor {
         }
     }
 
-    fn adjust_indexing(&mut self, var_expr: &VarExpression) {
-        let mut index_entries = Vec::new();
+    fn adjust_indexing<'a, I: Iterator<Item = &'a ast::Suffix>>(
+        &mut self,
+        suffixes: I,
+        expr_range: (usize, usize),
+    ) {
+        let mut index_entries: Vec<IndexEntry> = Vec::new();
 
-        for suffix in var_expr.suffixes() {
+        for suffix in suffixes {
             #[cfg_attr(
                 feature = "force_exhaustive_checks",
                 deny(non_exhaustive_omitted_patterns)
             )]
             let static_name = match suffix {
                 ast::Suffix::Call(_) => {
-                    return;
+                    if let Some(last_entry) = index_entries.last_mut() {
+                        last_entry.is_function_call = true;
+                    }
+                    continue;
                 }
 
                 ast::Suffix::Index(ast::Index::Brackets { expression, .. }) => {
@@ -724,14 +734,15 @@ impl ScopeVisitor {
             index_entries.push(IndexEntry {
                 index: range(suffix),
                 static_name: static_name.cloned(),
+                is_function_call: false,
             })
         }
 
         if index_entries.is_empty() {
             return;
         }
 
-        let Some(reference) = self.scope_manager.reference_at_byte_mut(range(var_expr).0) else {
+        let Some(reference) = self.scope_manager.reference_at_byte_mut(expr_range.0) else {
             return;
         };
 
@@ -1133,7 +1144,13 @@ mod tests {
                         .as_ref()
                         .expect("indexing was None")
                         .iter()
-                        .map(|index| index.static_name.as_ref().map(|token| token.to_string()))
+                        .map(|index| index.static_name.as_ref().map(|token| {
+                            if index.is_function_call {
+                                format!("{}()", token)
+                            } else {
+                                token.to_string()
+                            }
+                        }))
                         .collect::<Vec<Option<String>>>()
                 );
             }
@@ -1143,5 +1160,7 @@ 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/lib.rs

@@ -328,6 +328,7 @@ use_lints! {
     {
         roblox_incorrect_color3_new_bounds: lints::roblox_incorrect_color3_new_bounds::Color3BoundsLint,
         roblox_incorrect_roact_usage: lints::roblox_incorrect_roact_usage::IncorrectRoactUsageLint,
+        roblox_roact_non_exhaustive_deps: lints::roblox_roact_non_exhaustive_deps::RoactNonExhaustiveDepsLint,
         roblox_suspicious_udim2_new: lints::roblox_suspicious_udim2_new::SuspiciousUDim2NewLint,
     },
 }

selene-lib/src/lints.rs

@@ -42,6 +42,9 @@ pub mod roblox_incorrect_color3_new_bounds;
 #[cfg(feature = "roblox")]
 pub mod roblox_incorrect_roact_usage;
 
+#[cfg(feature = "roblox")]
+pub mod roblox_roact_non_exhaustive_deps;
+
 #[cfg(feature = "roblox")]
 pub mod roblox_suspicious_udim2_new;