diff --git a/CHANGELOG.md b/CHANGELOG.md index 6583717b..ab44f9c4 100644 --- a/CHANGELOG.md +++ b/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_dangling_connection`](https://kampfkarren.github.io/selene/lints/roblox_roact_dangling_connection.html), which will check for connections made in components without getting cleaned up. ## [0.26.1](https://github.com/Kampfkarren/selene/releases/tag/0.26.1) - 2023-11-11 ### Fixed diff --git a/docs/src/SUMMARY.md b/docs/src/SUMMARY.md index 51fcca18..046301f8 100644 --- a/docs/src/SUMMARY.md +++ b/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_dangling_connection](./lints/roblox_roact_dangling_connection.md) - [roblox_suspicious_udim2_new](./lints/roblox_suspicious_udim2_new.md) - [shadowing](./lints/shadowing.md) - [suspicious_reverse_loop](./lints/suspicious_reverse_loop.md) diff --git a/docs/src/lints/roblox_roact_dangling_connection.md b/docs/src/lints/roblox_roact_dangling_connection.md new file mode 100644 index 00000000..96074487 --- /dev/null +++ b/docs/src/lints/roblox_roact_dangling_connection.md @@ -0,0 +1,24 @@ +# roblox_roact_dangling_connection +## What it does +Checks for connections in Roact components that are either not assigned to a variable or passed as arguments. + +## Why this is bad +This indicates a memory leak and can cause unexpected behaviors. + +## Example +```lua +local function MyComponent() + useEffect(function() + a:Connect() + end, {}) +end +``` + +## Remarks +This lint is active if the file has a variable named `Roact` or `React` and that the connection is made within a function. + +This checks for connections by identifying the following keywords: +* Connect +* connect +* ConnectParallel +* Once diff --git a/selene-lib/src/ast_util/scopes.rs b/selene-lib/src/ast_util/scopes.rs index 3e3b6ff2..38d6b706 100644 --- a/selene-lib/src/ast_util/scopes.rs +++ b/selene-lib/src/ast_util/scopes.rs @@ -242,7 +242,11 @@ fn get_name_path_from_call(call: &ast::FunctionCall) -> Option> { return None; } - ast::Suffix::Call(_) => { + ast::Suffix::Call(call) => { + if let ast::Call::MethodCall(method_call) = call { + path.push(method_call.name().token().to_string()); + } + if index + 1 == length { return Some(path); } else { diff --git a/selene-lib/src/lib.rs b/selene-lib/src/lib.rs index 77bb09c3..2c2e7983 100644 --- a/selene-lib/src/lib.rs +++ b/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_dangling_connection: lints::roblox_roact_dangling_connection::RoactDanglingConnectionLint, roblox_suspicious_udim2_new: lints::roblox_suspicious_udim2_new::SuspiciousUDim2NewLint, }, } diff --git a/selene-lib/src/lints.rs b/selene-lib/src/lints.rs index fa42553c..a3e3030c 100644 --- a/selene-lib/src/lints.rs +++ b/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_dangling_connection; + #[cfg(feature = "roblox")] pub mod roblox_suspicious_udim2_new; diff --git a/selene-lib/src/lints/roblox_roact_dangling_connection.rs b/selene-lib/src/lints/roblox_roact_dangling_connection.rs new file mode 100644 index 00000000..c9991e51 --- /dev/null +++ b/selene-lib/src/lints/roblox_roact_dangling_connection.rs @@ -0,0 +1,274 @@ +use super::{AstContext, Context, Diagnostic, Label, Lint, LintType, Node, Severity}; +use crate::ast_util::range; +use std::{collections::HashSet, convert::Infallible}; + +use full_moon::{ + ast::{self, Ast}, + visitors::Visitor, +}; + +pub struct RoactDanglingConnectionLint; + +impl Lint for RoactDanglingConnectionLint { + type Config = (); + type Error = Infallible; + + const SEVERITY: Severity = Severity::Error; + const LINT_TYPE: LintType = LintType::Correctness; + + fn new((): Self::Config) -> Result { + Ok(Self) + } + + fn pass( + &self, + ast: &Ast, + context: &Context, + AstContext { scope_manager, .. }: &AstContext, + ) -> Vec { + if !context.is_roblox() { + return Vec::new(); + } + + if scope_manager.variables.iter().all(|(_, variable)| { + !["Roact", "React"].contains(&variable.name.trim_end_matches(char::is_whitespace)) + }) { + return Vec::new(); + } + + let mut visitor = RoactDanglingConnectionVisitor { + dangling_connections: Vec::new(), + dangling_connection_start_ranges: scope_manager + .function_calls + .iter() + .filter_map(|(_, function_call_stmt)| { + function_call_stmt + .call_name_path + .last() + .and_then(|last_name| { + ["Connect", "connect", "ConnectParallel", "Once"] + .contains(&last_name.as_str()) + .then_some(function_call_stmt.call_prefix_range.0) + }) + }) + .collect(), + function_contexts: Vec::new(), + definitions_of_roact_functions: HashSet::new(), + }; + + visitor.visit_ast(ast); + + let mut diagnostics = Vec::new(); + + for invalid_event in visitor.dangling_connections { + if let ConnectionContext::UseEffect = invalid_event.function_context { + diagnostics.push(Diagnostic::new( + "roblox_roact_dangling_connection", + "disconnect the connection in the useEffect cleanup function".to_owned(), + Label::new(invalid_event.range), + )); + } else { + diagnostics.push(Diagnostic::new( + "roblox_roact_dangling_connection", + "disconnect the connection where appropriate".to_owned(), + Label::new(invalid_event.range), + )); + } + } + + diagnostics + } +} + +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(), + Some(ast::Suffix::Call(ast::Call::AnonymousCall(_))) => { + if suffixes.len() == 1 { + // a() + return if let ast::Prefix::Name(name) = prefix { + name.token().to_string() + } else { + String::new() + }; + } else { + // In a.b(), b is the suffix before the last one + Some(&suffixes[suffixes.len() - 2]) + } + } + _ => return String::new(), + }; + + last_suffix + .map(|suffix| match suffix { + ast::Suffix::Index(ast::Index::Dot { name, .. }) => name.token().to_string(), + ast::Suffix::Call(ast::Call::MethodCall(method_call)) => { + method_call.name().token().to_string() + } + ast::Suffix::Call(ast::Call::AnonymousCall(anonymous_call)) => { + anonymous_call.to_string() + } + _ => String::new(), + }) + .unwrap_or_default() +} + +#[derive(Debug, PartialEq, Clone, Copy)] +enum ConnectionContext { + Unknown, + UseEffect, +} + +#[derive(Debug, PartialEq, Clone, Copy)] +enum ConnectionContextType { + FunctionBody, + FunctionCall, +} + +#[derive(Debug)] +struct RoactDanglingConnectionVisitor { + dangling_connections: Vec, + dangling_connection_start_ranges: HashSet, + function_contexts: Vec<(ConnectionContextType, ConnectionContext)>, + definitions_of_roact_functions: HashSet, +} + +#[derive(Debug)] +struct DanglingConnection { + range: (usize, usize), + function_context: ConnectionContext, +} + +fn get_last_known_context( + function_contexts: &[(ConnectionContextType, ConnectionContext)], +) -> ConnectionContext { + match function_contexts + .iter() + .rev() + .find(|(_, connection_type)| *connection_type != ConnectionContext::Unknown) + { + Some(context) => context.1, + None => ConnectionContext::Unknown, + } +} + +fn is_roact_function(prefix: &ast::Prefix) -> bool { + if let ast::Prefix::Name(prefix_token) = prefix { + ["roact", "react", "hooks"] + .contains(&prefix_token.token().to_string().to_lowercase().as_str()) + } else { + false + } +} + +impl Visitor for RoactDanglingConnectionVisitor { + fn visit_function_call(&mut self, call: &ast::FunctionCall) { + let last_suffix = + get_last_function_call_suffix(call.prefix(), &call.suffixes().collect::>()); + + // Only warn in useEffect to prevent false negatives with unrelated functions. Ideally this should also work anywhere + // in the component body. Can we detect React components better? + if self + .function_contexts + .iter() + .any(|(_, context)| *context == ConnectionContext::UseEffect) + { + if let Some(call_range) = call.range() { + if self + .dangling_connection_start_ranges + .contains(&call_range.0.bytes()) + { + self.dangling_connections.push(DanglingConnection { + range: range(call), + function_context: get_last_known_context(&self.function_contexts), + }); + } + } + } + + // Check if caller is Roact. or a variable defined to it + let mut suffixes = call.suffixes().collect::>(); + suffixes.pop(); + + let mut is_this_roact_function = false; + + if suffixes.is_empty() { + // Call is foo(), not foo.bar() + if let ast::Prefix::Name(name) = call.prefix() { + is_this_roact_function = self + .definitions_of_roact_functions + .get(&name.token().to_string()) + .is_some(); + } + } else if suffixes.len() == 1 { + // Call is foo.bar() + is_this_roact_function = is_roact_function(call.prefix()); + } + + self.function_contexts.push(( + ConnectionContextType::FunctionCall, + match last_suffix.as_str() { + "useEffect" if is_this_roact_function => ConnectionContext::UseEffect, + _ => ConnectionContext::Unknown, + }, + )); + } + + fn visit_function_call_end(&mut self, _: &ast::FunctionCall) { + self.function_contexts.pop(); + } + + fn visit_function_body(&mut self, _: &ast::FunctionBody) { + self.function_contexts.push(( + ConnectionContextType::FunctionBody, + ConnectionContext::Unknown, + )); + } + + fn visit_function_body_end(&mut self, _: &ast::FunctionBody) { + self.function_contexts.pop(); + } + + fn visit_local_assignment(&mut self, node: &ast::LocalAssignment) { + for (name, expr) in node.names().iter().zip(node.expressions().iter()) { + if let ast::Expression::Var(ast::Var::Expression(var_expr)) = expr { + if is_roact_function(var_expr.prefix()) { + self.definitions_of_roact_functions + .insert(name.token().to_string()); + } + } + } + } +} + +#[cfg(test)] +mod tests { + use super::{super::test_util::test_lint, *}; + + #[test] + fn test_no_roact() { + test_lint( + RoactDanglingConnectionLint::new(()).unwrap(), + "roblox_roact_dangling_connection", + "no_roact", + ); + } + + #[test] + fn test_roblox_roact_dangling_connection() { + test_lint( + RoactDanglingConnectionLint::new(()).unwrap(), + "roblox_roact_dangling_connection", + "roblox_roact_dangling_connection", + ); + } + + #[test] + fn test_with_roact() { + test_lint( + RoactDanglingConnectionLint::new(()).unwrap(), + "roblox_roact_dangling_connection", + "with_roact", + ); + } +} diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/no_roact.lua b/selene-lib/tests/lints/roblox_roact_dangling_connection/no_roact.lua new file mode 100644 index 00000000..d7fce0c6 --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_dangling_connection/no_roact.lua @@ -0,0 +1,74 @@ +a:Connect() +a.Connect() +a:connect() +a.connect() + +a.b:Connect() +a.b.Connect() +a.b:connect() +a.b.connect() + +a.b.c:Connect() +a.b.c.Connect() +a.b.c:connect() +a.b.c.connect() + +local foo = a:Connect() +local foo = a.Connect() +local foo = a:connect() +local foo = a.connect() + +foo = a:Connect() +foo = a.Connect() +foo = a:connect() +foo = a.connect() + +foo = a.b:Connect() +foo = a.b.Connect() +foo = a.b:connect() +foo = a.b.connect() + +foo = a.b.c:Connect() +foo = a.b.c.Connect() +foo = a.b.c:connect() +foo = a.b.c.connect() + +local function c() + a:Connect() + a:connect() + + a.b:Connect() + a.b:connect() + + a.b.c:Connect() + a.b.c:connect() + + foo = a:Connect() + foo = a:connect() + + foo = a.b:Connect() + foo = a.b:connect() + + foo = a.b.c:Connect() + foo = a.b.c:connect() +end + +function d:e() + a:Connect() + a:connect() + + a.b:Connect() + a.b:connect() + + a.b.c:Connect() + a.b.c:connect() + + foo = a:Connect() + foo = a:connect() + + foo = a.b:Connect() + foo = a.b:connect() + + foo = a.b.c:Connect() + foo = a.b.c:connect() +end diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/no_roact.std.toml b/selene-lib/tests/lints/roblox_roact_dangling_connection/no_roact.std.toml new file mode 100644 index 00000000..98886350 --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_dangling_connection/no_roact.std.toml @@ -0,0 +1,2 @@ +[selene] +name = "roblox" diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/no_roact.stderr b/selene-lib/tests/lints/roblox_roact_dangling_connection/no_roact.stderr new file mode 100644 index 00000000..e69de29b diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.lua b/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.lua new file mode 100644 index 00000000..98550952 --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.lua @@ -0,0 +1,39 @@ +local React +local useEffect = React.useEffect + +a:connect() + +local function Component() + a:Connect() + + useEffect(function() + -- Ignore since `a` might take ownership of connections + a(b:Connect()) + a(function() end, b:Connect()) + + a:Connect() + a:connect() + + a.b:Connect() + a.b:connect() + + a.b.c:Connect() + a.b.c:connect() + + good = a:Connect() + good = a:connect() + + good = a.b:Connect() + good = a.b:connect() + + good = a.b.c:Connect() + good = a.b.c:connect() + end) + + React.useEffect(function() + a(b:Connect()) + local b = a:connect() + + a:connect() + end) +end diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.std.toml b/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.std.toml new file mode 100644 index 00000000..98886350 --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.std.toml @@ -0,0 +1,2 @@ +[selene] +name = "roblox" diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.stderr b/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.stderr new file mode 100644 index 00000000..2db55d95 --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_dangling_connection/roblox_roact_dangling_connection.stderr @@ -0,0 +1,42 @@ +error[roblox_roact_dangling_connection]: disconnect the connection in the useEffect cleanup function + ┌─ roblox_roact_dangling_connection.lua:14:9 + │ +14 │ a:Connect() + │ ^^^^^^^^^^^ + +error[roblox_roact_dangling_connection]: disconnect the connection in the useEffect cleanup function + ┌─ roblox_roact_dangling_connection.lua:15:9 + │ +15 │ a:connect() + │ ^^^^^^^^^^^ + +error[roblox_roact_dangling_connection]: disconnect the connection in the useEffect cleanup function + ┌─ roblox_roact_dangling_connection.lua:17:9 + │ +17 │ a.b:Connect() + │ ^^^^^^^^^^^^^ + +error[roblox_roact_dangling_connection]: disconnect the connection in the useEffect cleanup function + ┌─ roblox_roact_dangling_connection.lua:18:9 + │ +18 │ a.b:connect() + │ ^^^^^^^^^^^^^ + +error[roblox_roact_dangling_connection]: disconnect the connection in the useEffect cleanup function + ┌─ roblox_roact_dangling_connection.lua:20:9 + │ +20 │ a.b.c:Connect() + │ ^^^^^^^^^^^^^^^ + +error[roblox_roact_dangling_connection]: disconnect the connection in the useEffect cleanup function + ┌─ roblox_roact_dangling_connection.lua:21:9 + │ +21 │ a.b.c:connect() + │ ^^^^^^^^^^^^^^^ + +error[roblox_roact_dangling_connection]: disconnect the connection in the useEffect cleanup function + ┌─ roblox_roact_dangling_connection.lua:37:9 + │ +37 │ a:connect() + │ ^^^^^^^^^^^ + diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.lua b/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.lua new file mode 100644 index 00000000..350870e7 --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.lua @@ -0,0 +1,7 @@ +local Roact + +local function Component() + Roact.useEffect(function() + a:Connect() + end) +end diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.std.toml b/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.std.toml new file mode 100644 index 00000000..98886350 --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.std.toml @@ -0,0 +1,2 @@ +[selene] +name = "roblox" diff --git a/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.stderr b/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.stderr new file mode 100644 index 00000000..de82cddb --- /dev/null +++ b/selene-lib/tests/lints/roblox_roact_dangling_connection/with_roact.stderr @@ -0,0 +1,6 @@ +error[roblox_roact_dangling_connection]: disconnect the connection in the useEffect cleanup function + ┌─ with_roact.lua:5:9 + │ +5 │ a:Connect() + │ ^^^^^^^^^^^ +