From e2d36a1553d959d81f723eb66cb3761d7fb4c9b3 Mon Sep 17 00:00:00 2001 From: Christopher Chang <51393127+chriscerie@users.noreply.github.com> Date: Tue, 19 Sep 2023 21:27:28 -0700 Subject: [PATCH] Handle function calls --- selene-lib/src/ast_util/scopes.rs | 37 +++++++++---- .../lints/roblox_roact_non_exhaustive_deps.rs | 54 ++++++++++--------- selene-lib/src/lints/unused_variable.rs | 1 + .../complex_deps.lua | 14 ++++- .../complex_deps.stderr | 8 +++ 5 files changed, 78 insertions(+), 36 deletions(-) diff --git a/selene-lib/src/ast_util/scopes.rs b/selene-lib/src/ast_util/scopes.rs index 832a9ef1..8f4168b3 100644 --- a/selene-lib/src/ast_util/scopes.rs +++ 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,7 @@ pub struct Reference { pub within_function_stmt: Option, // 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 produce ["y", "z", "w"] pub indexing: Option>, } @@ -131,6 +130,7 @@ pub struct WithinFunctionStmt { pub struct IndexEntry { pub index: Range, pub static_name: Option, + pub is_function_call: bool, } #[derive(Debug, Default)] @@ -349,6 +349,8 @@ impl ScopeVisitor { ast::Value::FunctionCall(call) => { self.read_prefix(call.prefix()); + self.adjust_indexing(call.suffixes(), range(call)); + for suffix in call.suffixes() { self.read_suffix(suffix); } @@ -492,7 +494,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); @@ -701,17 +703,24 @@ impl ScopeVisitor { } } - fn adjust_indexing(&mut self, var_expr: &VarExpression) { - let mut index_entries = Vec::new(); + fn adjust_indexing<'a, I: Iterator>( + &mut self, + suffixes: I, + expr_range: (usize, usize), + ) { + let mut index_entries: Vec = 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, .. }) => { @@ -728,6 +737,7 @@ impl ScopeVisitor { index_entries.push(IndexEntry { index: range(suffix), static_name: static_name.cloned(), + is_function_call: false, }) } @@ -735,7 +745,7 @@ impl ScopeVisitor { 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; }; @@ -1137,7 +1147,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::>>() ); } @@ -1147,5 +1163,6 @@ mod tests { } test_indexing("x.y.z", &[Some("y"), Some("z")]); + test_indexing("a.b.c().d", &[Some("b"), 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 1e19c7d4..dc47a5b8 100644 --- a/selene-lib/src/lints/roblox_roact_non_exhaustive_deps.rs +++ b/selene-lib/src/lints/roblox_roact_non_exhaustive_deps.rs @@ -1,5 +1,5 @@ use super::*; -use crate::ast_util::range; +use crate::ast_util::{range, scopes::Reference}; use std::{ collections::{HashMap, HashSet}, convert::Infallible, @@ -214,6 +214,34 @@ struct Upvalue { } impl Upvalue { + fn new(reference: &Reference, resolved_start_range: &Option) -> Self { + let indexing_identifiers = reference + .indexing + .as_ref() + .unwrap_or(&vec![]) + .iter() + .map_while(|index_entry| { + if index_entry.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 + } + }) + }) + .collect::>(); + + Upvalue { + prefix: reference.name.clone(), + indexing_identifiers, + resolved_start_range: *resolved_start_range, + } + } + /// `a.b["c"].d`, `a.b.[c].d`, and `a.b.c().d` yield `a.b` /// `a` just yields `a` fn indexing_expression_name(&self) -> String { @@ -286,29 +314,7 @@ impl RoactMissingDependencyVisitor<'_> { variable.identifiers.last().map(|(start, _)| *start) }); - let indexing_identifiers = reference - .indexing - .as_ref() - .unwrap_or(&vec![]) - .iter() - .filter_map(|index_entry| { - 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 - } - }) - }) - .collect::>(); - - Some(Upvalue { - prefix: reference.name.clone(), - indexing_identifiers, - resolved_start_range, - }) + Some(Upvalue::new(reference, &resolved_start_range)) } else { None } diff --git a/selene-lib/src/lints/unused_variable.rs b/selene-lib/src/lints/unused_variable.rs index c60c932d..fe781c54 100644 --- a/selene-lib/src/lints/unused_variable.rs +++ b/selene-lib/src/lints/unused_variable.rs @@ -81,6 +81,7 @@ impl Lint for UnusedVariableLint { if is_static_table && indexing.len() == 1 // This restriction can be lifted someday, but only once we can verify that the value has no side effects/is its own static table && indexing.iter().any(|index| index.static_name.is_some()) + && indexing.iter().all(|index| !index.is_function_call) { return AnalyzedReference::ObservedWrite(Label::new_with_message( reference.identifier, 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 868a9d3b..d760b630 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 @@ -19,7 +19,7 @@ local function Component2() end, { a.b }) end -local function Component3() +local function Component() local a = {} React.useEffect(function() print(a) -- Bad @@ -30,7 +30,7 @@ local function Component3() end, { a.b.c }) end -local function Component3() +local function Component() local a = {} React.useEffect(function() print(a.b) -- Bad @@ -38,3 +38,13 @@ local function Component3() print(a.b.c.d) end, { a.b.c.d }) end + +local function Component() + local a = {} + local d = {} + React.useEffect(function() + print(a.b.c()) -- Bad + print(a.b.d()) -- Bad, but already reported + print(d.e.f.g()) -- Good + end, { a.b.c, d.e.f }) +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 667a224a..66be7494 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,3 +22,11 @@ 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' + ┌─ complex_deps.lua:49:10 + │ +49 │ end, { a.b.c, d.e.f }) + │ ^^^^^^^^^^^^^^^^ + │ + = help: either include it or remove the dependency array +