diff --git a/selene-lib/src/ast_util/depth_tracker.rs b/selene-lib/src/ast_util/depth_tracker.rs deleted file mode 100644 index 8d01a775..00000000 --- a/selene-lib/src/ast_util/depth_tracker.rs +++ /dev/null @@ -1,230 +0,0 @@ -use full_moon::{ast, node::Node, visitors::Visitor}; - -#[derive(Debug, Clone, Copy)] -struct Depth { - byte: usize, - depth: u32, -} - -#[derive(Debug)] -pub struct DepthTracker { - depths: Vec, -} - -// TODO: Can we merge depth_tracker here? -impl DepthTracker { - pub fn new(ast: &ast::Ast) -> Self { - let mut visitor = DepthTrackerVisitor { - depths: Vec::new(), - depth: 0, - }; - - visitor.visit_ast(ast); - - assert_eq!( - visitor.depth, 0, - "Depth at the end of a depth tracker should be 0" - ); - - let mut depths = visitor.depths; - - depths.sort_by_cached_key(|depth| depth.byte); - - Self { depths } - } - - pub fn depth_at_byte(&self, byte: usize) -> u32 { - match self.depths.binary_search_by_key(&byte, |depth| depth.byte) { - Ok(index) => self.depths[index].depth, - Err(index) => { - if index == 0 { - 0 - } else { - self.depths[index - 1].depth - } - } - } - } -} - -struct DepthTrackerVisitor { - depths: Vec, - depth: u32, -} - -impl DepthTrackerVisitor { - fn add_depth(&mut self, node: impl Node) { - self.depth += 1; - - let Some((start, _)) = node.range() else { - return; - }; - - self.depths.push(Depth { - byte: start.bytes(), - depth: self.depth, - }); - } - - fn remove_depth(&mut self, node: impl Node) { - self.depth -= 1; - - let Some((_, end)) = node.range() else { - return; - }; - - self.depths.push(Depth { - byte: end.bytes(), - depth: self.depth, - }); - } -} - -impl Visitor for DepthTrackerVisitor { - fn visit_generic_for(&mut self, node: &ast::GenericFor) { - self.add_depth(node.block()); - } - - fn visit_generic_for_end(&mut self, node: &ast::GenericFor) { - self.remove_depth(node); - } - - fn visit_numeric_for(&mut self, node: &ast::NumericFor) { - self.add_depth(node.block()); - } - - fn visit_numeric_for_end(&mut self, node: &ast::NumericFor) { - self.remove_depth(node); - } - - fn visit_while(&mut self, node: &ast::While) { - self.add_depth(node.block()); - } - - fn visit_while_end(&mut self, node: &ast::While) { - self.remove_depth(node); - } - - fn visit_repeat(&mut self, node: &ast::Repeat) { - self.add_depth(node.block()); - } - - fn visit_repeat_end(&mut self, node: &ast::Repeat) { - self.remove_depth(node); - } - fn visit_function_body(&mut self, node: &ast::FunctionBody) { - self.add_depth(node.block()); - } - - fn visit_function_body_end(&mut self, node: &ast::FunctionBody) { - self.remove_depth(node); - } - - fn visit_do(&mut self, node: &ast::Do) { - self.add_depth(node.block()); - } - - fn visit_do_end(&mut self, node: &ast::Do) { - self.remove_depth(node); - } - - fn visit_if(&mut self, node: &ast::If) { - self.add_depth(node.block()); - } - - fn visit_if_end(&mut self, node: &ast::If) { - self.remove_depth(node); - } -} - -#[cfg(test)] -mod tests { - use super::*; - - use regex::Regex; - - fn expected_depths(code: &str) -> Vec<(usize, u32)> { - let mut depths = Vec::new(); - - let regex = Regex::new(r#"expect\((\d+)\)"#).unwrap(); - - for regex_match in regex.captures_iter(code) { - depths.push(( - regex_match.get(0).unwrap().start(), - regex_match.get(1).unwrap().as_str().parse().unwrap(), - )); - } - - depths - } - - fn test_depths(code: &str) { - let expected_depths = expected_depths(code); - assert!(!expected_depths.is_empty()); - - let ast = full_moon::parse(code).unwrap(); - - let depth_tracker = DepthTracker::new(&ast); - - for (byte, expected_depth) in expected_depths { - let actual_depth = depth_tracker.depth_at_byte(byte); - - assert_eq!(actual_depth, expected_depth); - } - } - - #[test] - fn depth_tracker() { - test_depths( - r#" - expect(0) - - while true do - expect(1) - - for i, v in pairs({}) do - expect(2) - - repeat - expect(3) - until true - end - - expect(1) - end - - expect(0) - "#, - ); - - test_depths( - r#" - expect(0) - - while true do - expect(1) - - local function a() - expect(2) - - b(function() - expect(3) - end, function() - expect(3) - - if true then - expect(4) - else - expect(4) - end - end) - end - - expect(1) - end - - expect(0) - "#, - ); - } -} diff --git a/selene-lib/src/ast_util/mod.rs b/selene-lib/src/ast_util/mod.rs index 8ba0f2ad..7947d18e 100644 --- a/selene-lib/src/ast_util/mod.rs +++ b/selene-lib/src/ast_util/mod.rs @@ -6,7 +6,6 @@ use full_moon::{ tokenizer::{self, Position, TokenReference}, }; -mod depth_tracker; mod extract_static_token; mod loop_tracker; pub mod name_paths; @@ -16,7 +15,6 @@ mod side_effects; mod strip_parentheses; pub mod visit_nodes; -pub use depth_tracker::DepthTracker; pub use extract_static_token::extract_static_token; pub use loop_tracker::LoopTracker; pub use purge_trivia::purge_trivia; diff --git a/selene-lib/src/lints/roblox_roact_non_exhaustive_deps.rs b/selene-lib/src/lints/roblox_roact_non_exhaustive_deps.rs index 9faf7bd5..1dac1c67 100644 --- a/selene-lib/src/lints/roblox_roact_non_exhaustive_deps.rs +++ b/selene-lib/src/lints/roblox_roact_non_exhaustive_deps.rs @@ -12,8 +12,6 @@ use full_moon::{ }; use if_chain::if_chain; -use crate::ast_util::DepthTracker; - pub struct RoactNonExhaustiveDepsLint; impl Lint for RoactNonExhaustiveDepsLint { @@ -45,7 +43,7 @@ impl Lint for RoactNonExhaustiveDepsLint { let mut visitor = RoactMissingDependencyVisitor { scope_manager, - depth_tracker: DepthTracker::new(ast), + fn_declaration_starts_stack: Vec::new(), missing_dependencies: Vec::new(), unnecessary_dependencies: Vec::new(), non_reactive_upvalue_starts: HashSet::new(), @@ -59,7 +57,11 @@ impl Lint for RoactNonExhaustiveDepsLint { if !invalid_event.missing_dependencies.is_empty() { diagnostics.push(Diagnostic::new_complete( "roblox_roact_non_exhaustive_deps", - get_formatted_error_message(&invalid_event.missing_dependencies, "missing"), + get_formatted_error_message( + &invalid_event.hook_name, + &invalid_event.missing_dependencies, + "missing", + ), Label::new(invalid_event.range), vec![format!( "help: either include {} or remove the dependency array", @@ -81,6 +83,7 @@ impl Lint for RoactNonExhaustiveDepsLint { diagnostics.push(Diagnostic::new_complete( "roblox_roact_non_exhaustive_deps", get_formatted_error_message( + &invalid_event.hook_name, &invalid_event.unnecessary_dependencies, "unnecessary", ), @@ -106,11 +109,13 @@ impl Lint for RoactNonExhaustiveDepsLint { } fn get_formatted_error_message( + hook_name: &String, missing_dependencies: &Vec, missing_or_unnecessary: &str, ) -> String { format!( - "hook useEffect has {}: {}", + "react hook {} has {}: {}", + hook_name, if missing_dependencies.len() == 1 { format!("{} dependency", missing_or_unnecessary) } else { @@ -185,7 +190,7 @@ struct RoactMissingDependencyVisitor<'a> { scope_manager: &'a ScopeManager, missing_dependencies: Vec, unnecessary_dependencies: Vec, - depth_tracker: DepthTracker, + fn_declaration_starts_stack: Vec, // Some variables are safe to omit from the dependency array, such as setState non_reactive_upvalue_starts: HashSet, @@ -216,12 +221,14 @@ impl PartialEq for Upvalue { struct MissingDependency { missing_dependencies: Vec, range: (usize, usize), + hook_name: String, } #[derive(Debug)] struct UnnecessaryDependency { unnecessary_dependencies: Vec, range: (usize, usize), + hook_name: String, } impl RoactMissingDependencyVisitor<'_> { @@ -256,6 +263,25 @@ impl RoactMissingDependencyVisitor<'_> { }) .collect() } + + /// Useful for determining whether a byte is outside the component when called from the context of a hook + /// ```lua + /// local var1 + /// local function component() + /// local var2 + /// -- Called with var1 - TRUE + /// -- Called with var2 - FALSE + /// useEffect(function() + /// -- Called with var1 - TRUE + /// -- Called with var2 - FALSE + /// end) + /// end + /// ``` + fn is_byte_outside_enclosing_named_fn(&self, byte: usize) -> bool { + self.fn_declaration_starts_stack + .last() + .map_or(false, |&last_fn_start| byte < last_fn_start) + } } impl Visitor for RoactMissingDependencyVisitor<'_> { @@ -268,7 +294,7 @@ impl Visitor for RoactMissingDependencyVisitor<'_> { _ => return, }; - if last_suffix.as_str() == "useEffect" && is_roact_function(call) { + if ["useEffect", "useMemo"].contains(&last_suffix.as_str()) && is_roact_function(call) { if let ast::FunctionArgs::Parentheses { arguments, .. } = function_args { if let Some(dependency_array_expr) = arguments.iter().nth(1) { let referenced_upvalues = @@ -286,25 +312,28 @@ impl Visitor for RoactMissingDependencyVisitor<'_> { .map(|upvalue| (upvalue.name.clone(), upvalue)) .collect::>(); - let use_effect_depth = self.depth_tracker.depth_at_byte(range(call).0); - let mut missing_dependencies = referenced_upvalues .iter() .filter(|upvalue| { - // Assume referenced variables but not initialized are globals and therefore not reactive - let is_non_reactive = - upvalue.resolved_start_range.map_or(true, |start_range| { - // Variables declared outside the component are not reactive - if use_effect_depth - != self.depth_tracker.depth_at_byte(start_range) - { - return true; + if dependencies.contains_key(&upvalue.name) { + return false; + } + + upvalue + .resolved_start_range + // Treat unresolved variables as globals, which are not reactive + .map_or(false, |resolved_start| { + // Ignore variables declared inside the hook callback + if upvalue.resolved_start_range >= range(call).0 { + return false; } - self.non_reactive_upvalue_starts.contains(&start_range) - }); + if self.is_byte_outside_enclosing_named_fn(resolved_start) { + return false; + } - !dependencies.contains_key(&upvalue.name) && !is_non_reactive + !self.non_reactive_upvalue_starts.contains(&resolved_start) + }) }) .cloned() .collect::>(); @@ -315,6 +344,7 @@ impl Visitor for RoactMissingDependencyVisitor<'_> { self.missing_dependencies.push(MissingDependency { missing_dependencies, range: range(dependency_array_expr), + hook_name: last_suffix.to_string(), }); } @@ -322,19 +352,14 @@ impl Visitor for RoactMissingDependencyVisitor<'_> { let mut unnecessary_dependencies: Vec = dependencies .iter() .filter_map(|(_, dependency)| { - if let Some(start_range) = dependency.resolved_start_range { - let depth_at_byte = self.depth_tracker.depth_at_byte(start_range); - - // Variables declared outside the component are not reactive - if use_effect_depth != depth_at_byte - || self.non_reactive_upvalue_starts.contains(&start_range) - { + if let Some(resolved_start) = dependency.resolved_start_range { + if self.is_byte_outside_enclosing_named_fn(resolved_start) { Some(dependency.clone()) } else { None } } else { - // Assume referenced variables but not initialized are globals and therefore not reactive + // Assume unresolved variables are globals and should not be included in deps Some(dependency.clone()) } }) @@ -346,6 +371,7 @@ impl Visitor for RoactMissingDependencyVisitor<'_> { self.unnecessary_dependencies.push(UnnecessaryDependency { unnecessary_dependencies, range: range(dependency_array_expr), + hook_name: last_suffix.to_string(), }); } } @@ -365,8 +391,28 @@ impl Visitor for RoactMissingDependencyVisitor<'_> { &call.suffixes().collect::>(), ); - // Setter functions are stable and can be omitted from dependency array - if function_suffix == "useState" || function_suffix == "useBinding" { + // State setter functions are stable and can be omitted from dependency array + if function_suffix == "useState" { + if let Some(second_var) = assignment.names().iter().nth(1) { + self.non_reactive_upvalue_starts + .insert(range(second_var).0); + } + } + + if function_suffix == "useRef" { + if let Some(first_var) = assignment.names().first() { + self.non_reactive_upvalue_starts + .insert(range(first_var).0); + } + } + + // Bindings and setters are both stable + if function_suffix == "useBinding" { + if let Some(first_var) = assignment.names().first() { + self.non_reactive_upvalue_starts + .insert(range(first_var).0); + } + if let Some(second_var) = assignment.names().iter().nth(1) { self.non_reactive_upvalue_starts .insert(range(second_var).0); @@ -375,12 +421,39 @@ impl Visitor for RoactMissingDependencyVisitor<'_> { } } } + + fn visit_function_declaration(&mut self, function_declaration: &ast::FunctionDeclaration) { + self.fn_declaration_starts_stack + .push(range(function_declaration).0); + } + + fn visit_function_declaration_end(&mut self, _: &ast::FunctionDeclaration) { + self.fn_declaration_starts_stack.pop(); + } + + fn visit_local_function(&mut self, local_function: &ast::LocalFunction) { + self.fn_declaration_starts_stack + .push(range(local_function).0); + } + + fn visit_local_function_end(&mut self, _: &ast::LocalFunction) { + self.fn_declaration_starts_stack.pop(); + } } #[cfg(test)] mod tests { use super::{super::test_util::test_lint, *}; + #[test] + fn test_known_stable_vars() { + test_lint( + RoactNonExhaustiveDepsLint::new(()).unwrap(), + "roblox_roact_non_exhaustive_deps", + "known_stable_vars", + ); + } + #[test] fn test_no_roact() { test_lint( @@ -391,11 +464,20 @@ mod tests { } #[test] - fn test_roblox_roact_dangling_connection() { + fn test_roblox_roact_non_exhaustive_deps() { test_lint( RoactNonExhaustiveDepsLint::new(()).unwrap(), "roblox_roact_non_exhaustive_deps", "roblox_roact_non_exhaustive_deps", ); } + + #[test] + fn test_use_memo() { + test_lint( + RoactNonExhaustiveDepsLint::new(()).unwrap(), + "roblox_roact_non_exhaustive_deps", + "use_memo", + ); + } } diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.lua b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.lua new file mode 100644 index 00000000..9041c72e --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.lua @@ -0,0 +1,18 @@ +local React + +local function Component() + local shouldNotIgnoreState, shouldIgnoreSetState = React.useState() + local shouldIgnoreBinding, shouldIgnoreSetBinding = React.useBinding() + local shouldIgnoreRef = React.useRef() + + React.useEffect(function() + print(shouldNotIgnoreState, shouldIgnoreSetState) + print(shouldIgnoreBinding, shouldIgnoreSetBinding) + print(shouldIgnoreRef) + end, {}) + + React.useEffect(function() + print(shouldIgnoreSetState) + -- Even though passing known stable variables isn't needed, it shouldn't warn about it + end, { shouldIgnoreSetState }) +end diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.std.yml b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.std.yml new file mode 100644 index 00000000..f434ea3e --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.std.yml @@ -0,0 +1,2 @@ +--- +name: roblox \ No newline at end of file diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.stderr b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.stderr new file mode 100644 index 00000000..2f3e148a --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/known_stable_vars.stderr @@ -0,0 +1,8 @@ +error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'shouldNotIgnoreState' + ┌─ known_stable_vars.lua:12:10 + │ +12 │ end, {}) + │ ^^ + │ + = help: either include it or remove the dependency array + diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/roblox_roact_non_exhaustive_deps.lua b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/roblox_roact_non_exhaustive_deps.lua index 81c63d61..fbfd6d5c 100644 --- a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/roblox_roact_non_exhaustive_deps.lua +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/roblox_roact_non_exhaustive_deps.lua @@ -30,14 +30,6 @@ local function Component3() end, depArray(reactive1)) end -local function Component4() - local _, setState = React.useState() - - React.useEffect(function() - setState() - end, {}) -end - local function Component5() local reactive1 = {} diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/roblox_roact_non_exhaustive_deps.stderr b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/roblox_roact_non_exhaustive_deps.stderr index 1c454b3e..993a5d21 100644 --- a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/roblox_roact_non_exhaustive_deps.stderr +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/roblox_roact_non_exhaustive_deps.stderr @@ -1,4 +1,4 @@ -error[roblox_roact_non_exhaustive_deps]: hook useEffect has missing dependencies: 'reactive1' and 'reactive2' +error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependencies: 'reactive1' and 'reactive2' ┌─ roblox_roact_non_exhaustive_deps.lua:13:10 │ 13 │ end, {}) @@ -6,7 +6,7 @@ error[roblox_roact_non_exhaustive_deps]: hook useEffect has missing dependencies │ = help: either include them or remove the dependency array -error[roblox_roact_non_exhaustive_deps]: hook useEffect has missing dependency: 'reactive2' +error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'reactive2' ┌─ roblox_roact_non_exhaustive_deps.lua:30:10 │ 30 │ end, depArray(reactive1)) @@ -14,35 +14,35 @@ error[roblox_roact_non_exhaustive_deps]: hook useEffect has missing dependency: │ = help: either include it or remove the dependency array -error[roblox_roact_non_exhaustive_deps]: hook useEffect has missing dependency: 'reactive1' - ┌─ roblox_roact_non_exhaustive_deps.lua:51:10 +error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'reactive1' + ┌─ roblox_roact_non_exhaustive_deps.lua:43:10 │ -51 │ end, {}) +43 │ end, {}) │ ^^ │ = help: either include it or remove the dependency array -error[roblox_roact_non_exhaustive_deps]: hook useEffect has unnecessary dependencies: 'error' and 'notreactive1' - ┌─ roblox_roact_non_exhaustive_deps.lua:55:37 +error[roblox_roact_non_exhaustive_deps]: react hook useEffect has unnecessary dependencies: 'error' and 'notreactive1' + ┌─ roblox_roact_non_exhaustive_deps.lua:47:37 │ -55 │ React.useEffect(function() end, { error, notreactive1 }) +47 │ React.useEffect(function() end, { error, notreactive1 }) │ ^^^^^^^^^^^^^^^^^^^^^^^ │ = help: either exclude them or remove the dependency array = outer scope variables like 'error' aren't valid dependencies because mutating them doesn't re-render the component -error[roblox_roact_non_exhaustive_deps]: hook useEffect has missing dependency: 'reactive1' - ┌─ roblox_roact_non_exhaustive_deps.lua:63:10 +error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'reactive1' + ┌─ roblox_roact_non_exhaustive_deps.lua:55:10 │ -63 │ end, { notreactive1 }) +55 │ end, { notreactive1 }) │ ^^^^^^^^^^^^^^^^ │ = help: either include it or remove the dependency array -error[roblox_roact_non_exhaustive_deps]: hook useEffect has unnecessary dependency: 'notreactive1' - ┌─ roblox_roact_non_exhaustive_deps.lua:63:10 +error[roblox_roact_non_exhaustive_deps]: react hook useEffect has unnecessary dependency: 'notreactive1' + ┌─ roblox_roact_non_exhaustive_deps.lua:55:10 │ -63 │ end, { notreactive1 }) +55 │ end, { notreactive1 }) │ ^^^^^^^^^^^^^^^^ │ = help: either exclude it or remove the dependency array diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.lua b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.lua new file mode 100644 index 00000000..2648e51e --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.lua @@ -0,0 +1,7 @@ +local React + +local function Component(props) + React.useEffect(function() + print(props) + end, {}) +end diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.std.yml b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.std.yml new file mode 100644 index 00000000..f434ea3e --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.std.yml @@ -0,0 +1,2 @@ +--- +name: roblox \ No newline at end of file diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.stderr b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.stderr new file mode 100644 index 00000000..f52f91db --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/use_memo.stderr @@ -0,0 +1,8 @@ +error[roblox_roact_non_exhaustive_deps]: react hook useEffect has missing dependency: 'props' + ┌─ use_memo.lua:6:10 + │ +6 │ end, {}) + │ ^^ + │ + = help: either include it or remove the dependency array +