Allow suspected manual table clone when variable and loop are defined…

… at different depths (#546)

Closes #502.

CHANGELOG.md

@@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Moved `math.log` second argument addition from Lua 5.3 std lib to 5.2 std lib
 - `undefined_variable` now correctly errors when defining multiple methods in undefined tables
 - Corrected `os.exit` definition in Lua 5.2 standard library
+- Fixed `manual_table_clone` incorrectly warning when loop and table are defined at different depths
 
 ## [0.25.0](https://github.com/Kampfkarren/selene/releases/tag/0.25.0) - 2023-03-12
 ### Added

docs/src/lints/manual_table_clone.md

@@ -27,6 +27,7 @@ Very little outside this exact pattern is matched. This is the list of circumsta
 - Any usage of the output variable in between the definition and the loop (as determined by position in code).
 - If the input variable is not a plain locally initialized variable. For example, `self.state[key] = value` will not lint.
 - If the input variable is not defined as a completely empty table.
+- If the loop and input variable are defined at different depths.
 
 ---
 

selene-lib/src/ast_util/loop_tracker.rs

@@ -1,3 +1,6 @@
+// Remove this once loop_tracker is used again
+#![allow(dead_code)]
+
 use full_moon::{ast, node::Node, visitors::Visitor};
 
 #[derive(Debug, Clone, Copy)]

selene-lib/src/lints/manual_table_clone.rs

@@ -3,12 +3,10 @@ use full_moon::{
     visitors::Visitor,
 };
 
-use crate::ast_util::{
-    expression_to_ident, range, scopes::AssignedValue, strip_parentheses, LoopTracker,
-};
+use crate::ast_util::{expression_to_ident, range, scopes::AssignedValue, strip_parentheses};
 
 use super::*;
-use std::convert::Infallible;
+use std::{collections::HashSet, convert::Infallible};
 
 pub struct ManualTableCloneLint;
 
@@ -34,9 +32,9 @@ impl Lint for ManualTableCloneLint {
 
         let mut visitor = ManualTableCloneVisitor {
             matches: Vec::new(),
-            loop_tracker: LoopTracker::new(ast),
             scope_manager: &ast_context.scope_manager,
-            stmt_begins: Vec::new(),
+            completed_stmt_begins: Vec::new(),
+            inside_stmt_begins: HashSet::new(),
         };
 
         visitor.visit_ast(ast);
@@ -92,9 +90,9 @@ impl ManualTableCloneMatch {
 
 struct ManualTableCloneVisitor<'ast> {
     matches: Vec<ManualTableCloneMatch>,
-    loop_tracker: LoopTracker,
     scope_manager: &'ast ScopeManager,
-    stmt_begins: Vec<usize>,
+    completed_stmt_begins: Vec<usize>,
+    inside_stmt_begins: HashSet<usize>,
 }
 
 #[derive(Debug)]
@@ -214,7 +212,7 @@ impl ManualTableCloneVisitor<'_> {
     ) -> bool {
         debug_assert!(assigment_start > definition_end);
 
-        for &stmt_begin in self.stmt_begins.iter() {
+        for &stmt_begin in self.completed_stmt_begins.iter() {
             if stmt_begin > definition_end {
                 return true;
             } else if stmt_begin > assigment_start {
@@ -224,6 +222,13 @@ impl ManualTableCloneVisitor<'_> {
 
         false
     }
+
+    fn get_depth_at_byte(&self, byte: usize) -> usize {
+        self.inside_stmt_begins
+            .iter()
+            .filter(|&&start| start < byte)
+            .count()
+    }
 }
 
 fn has_filter_comment(for_loop: &ast::GenericFor) -> bool {
@@ -374,9 +379,7 @@ impl Visitor for ManualTableCloneVisitor<'_> {
 
         let (position_start, position_end) = range(node);
 
-        if self.loop_tracker.depth_at_byte(position_start)
-            != self.loop_tracker.depth_at_byte(*definition_start)
-        {
+        if self.get_depth_at_byte(*definition_start) != self.get_depth_at_byte(position_start) {
             return;
         }
 
@@ -401,8 +404,13 @@ impl Visitor for ManualTableCloneVisitor<'_> {
         });
     }
 
+    fn visit_stmt(&mut self, stmt: &ast::Stmt) {
+        self.inside_stmt_begins.insert(range(stmt).0);
+    }
+
     fn visit_stmt_end(&mut self, stmt: &ast::Stmt) {
-        self.stmt_begins.push(range(stmt).0);
+        self.completed_stmt_begins.push(range(stmt).0);
+        self.inside_stmt_begins.remove(&range(stmt).0);
     }
 }
 

selene-lib/tests/lints/manual_table_clone/false_positive.lua

@@ -39,6 +39,34 @@ local function falsePositive3(t)
 	return result
 end
 
+local result4 = {}
+local function falsePositive4(t)
+    for key, value in t do
+        result3[key] = value
+    end
+end
+
+local result5 = {}
+local function falsePositive5(t)
+	local function f() end
+
+    for key, value in t do
+        result4[key] = value
+    end
+end
+
+local function falsePositive6(t)
+    local result = {}
+
+	if b then return end
+
+    if a then
+        for key, value in t do
+            result4[key] = value
+        end
+    end
+end
+
 local function notFalsePositive1(t)
 	local result = {}
 

selene-lib/tests/lints/manual_table_clone/false_positive.stderr

@@ -1,24 +1,24 @@
 error[manual_table_clone]: manual implementation of table.clone
-   ┌─ false_positive.lua:49:2
+   ┌─ false_positive.lua:77:2
    │  
-43 │       local result = {}
+71 │       local result = {}
    │       ----------------- remove this definition
    ·  
-49 │ ╭     for key, value in pairs(t) do
-50 │ │         result[key] = value
-51 │ │     end
+77 │ ╭     for key, value in pairs(t) do
+78 │ │         result[key] = value
+79 │ │     end
    │ ╰───────^
    │  
    = try `local result = table.clone(t)`
 
 error[manual_table_clone]: manual implementation of table.clone
-   ┌─ false_positive.lua:58:3
+   ┌─ false_positive.lua:86:3
    │  
-58 │ ╭         local result = {}
-59 │ │ 
-60 │ │         for key, value in pairs(t) do
-61 │ │             result[key] = value
-62 │ │         end
+86 │ ╭         local result = {}
+87 │ │ 
+88 │ │         for key, value in pairs(t) do
+89 │ │             result[key] = value
+90 │ │         end
    │ ╰───────────^
    │  
    = try `local result = table.clone(t)`