From db9b0f4bb10968295b9878cb96e13697dde215c2 Mon Sep 17 00:00:00 2001 From: Christopher Chang <51393127+chriscerie@users.noreply.github.com> Date: Wed, 20 Sep 2023 11:51:11 -0700 Subject: [PATCH] Warn about complex deps --- selene-lib/src/ast_util/scopes.rs | 5 +- .../lints/roblox_roact_non_exhaustive_deps.rs | 120 ++++++++++++++---- .../complex_deps.lua | 25 +++- .../complex_deps.stderr | 18 ++- .../too_complex_deps.lua | 39 ++++++ .../too_complex_deps.std.yml | 2 + .../too_complex_deps.stderr | 32 +++++ 7 files changed, 214 insertions(+), 27 deletions(-) create mode 100644 selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.lua create mode 100644 selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.std.yml create mode 100644 selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.stderr diff --git a/selene-lib/src/ast_util/scopes.rs b/selene-lib/src/ast_util/scopes.rs index 8f4168b3..4d272964 100644 --- a/selene-lib/src/ast_util/scopes.rs +++ b/selene-lib/src/ast_util/scopes.rs @@ -90,7 +90,9 @@ pub struct Reference { pub within_function_stmt: Option, // 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>, } @@ -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")]); } } 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 dc47a5b8..fa0f5124 100644 --- a/selene-lib/src/lints/roblox_roact_non_exhaustive_deps.rs +++ b/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, unnecessary_dependencies: Vec, + complex_dependencies: Vec, fn_declaration_starts_stack: Vec, // Some variables are safe to omit from the dependency array, such as setState @@ -206,51 +235,68 @@ 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, + /// 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, } impl Upvalue { fn new(reference: &Reference, resolved_start_range: &Option) -> 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::>(); + 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"]` @@ -258,8 +304,11 @@ impl Upvalue { 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 { 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::>(); + 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( diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.lua b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.lua index d760b630..d20b1328 100644 --- a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.lua +++ b/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 diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.stderr b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.stderr index 66be7494..a19a8812 100644 --- a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.stderr +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/complex_deps.stderr @@ -22,7 +22,7 @@ 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 }) @@ -30,3 +30,19 @@ 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["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 + diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.lua b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.lua new file mode 100644 index 00000000..126e7e82 --- /dev/null +++ b/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 diff --git a/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.std.yml b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.std.yml new file mode 100644 index 00000000..f434ea3e --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.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/too_complex_deps.stderr b/selene-lib/tests/lints/roblox_roact_non_exhaustive_deps/too_complex_deps.stderr new file mode 100644 index 00000000..0455021d --- /dev/null +++ b/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 +