Fix issue #73: Complete variable name validator improvements with systematic refactoring

.quality-baseline.json

@@ -381,12 +381,6 @@
       "singleUseVariable"
     ]
   },
-  "src/core/services/variable-name-validator.ts": {
-    "lastCommitId": "4d7170d18e23cea4dc86b7c500ac66e148a95683",
-    "violations": [
-      "singleUseVariable"
-    ]
-  },
   "src/core/services/variable-reference-request.ts": {
     "lastCommitId": "5add9ade67733f16f53716881aab85e467d3c68f",
     "violations": [
@@ -609,13 +603,6 @@
       "singleUseVariable"
     ]
   },
-  "tests/unit/services/variable-name-validator.test.ts": {
-    "lastCommitId": "0f4e7736995c00491695acae2238fe55da1bd0a0",
-    "violations": [
-      "fileSize",
-      "singleUseVariable"
-    ]
-  },
   "tests/utils/file-operations.ts": {
     "lastCommitId": "89ea54e72e552715acec38f728fc0face807d2ad",
     "violations": [
@@ -631,24 +618,6 @@
       "singleUseVariable"
     ]
   },
-  "tests/unit/cli-location-handling.test.ts": {
-    "lastCommitId": "1690de88db858021fcf493912253e06dab9dd7b4",
-    "violations": [
-      "singleUseVariable"
-    ]
-  },
-  "tests/unit/file-validator.test.ts": {
-    "lastCommitId": "5add9ade67733f16f53716881aab85e467d3c68f",
-    "violations": [
-      "singleUseVariable"
-    ]
-  },
-  "tests/unit/usage-tracker.test.ts": {
-    "lastCommitId": "608b51cb41bc489a35141052b3269596700e71f3",
-    "violations": [
-      "singleUseVariable"
-    ]
-  },
   "tests/utils/fixture-location.ts": {
     "lastCommitId": "c419bc00c74e093452bf8eb2aa0e61e34598225d",
     "violations": [
@@ -709,8 +678,20 @@
       "singleUseVariable"
     ]
   },
-  "tests/integration/fixture.test.ts": {
-    "lastCommitId": "f7d3433d1b8e57e08257e50af05bf009f175e2d0",
+  "tests/unit/cli-location-handling.test.ts": {
+    "lastCommitId": "1690de88db858021fcf493912253e06dab9dd7b4",
+    "violations": [
+      "singleUseVariable"
+    ]
+  },
+  "tests/unit/file-validator.test.ts": {
+    "lastCommitId": "5add9ade67733f16f53716881aab85e467d3c68f",
+    "violations": [
+      "singleUseVariable"
+    ]
+  },
+  "tests/unit/usage-tracker.test.ts": {
+    "lastCommitId": "608b51cb41bc489a35141052b3269596700e71f3",
     "violations": [
       "singleUseVariable"
     ]
@@ -733,6 +714,24 @@
       "singleUseVariable"
     ]
   },
+  "tests/integration/fixture.test.ts": {
+    "lastCommitId": "f7d3433d1b8e57e08257e50af05bf009f175e2d0",
+    "violations": [
+      "singleUseVariable"
+    ]
+  },
+  "tests/utils/parsers/location-parser.ts": {
+    "lastCommitId": "d9034ceebf77dd46366b48298985149e2e7922bb",
+    "violations": [
+      "singleUseVariable"
+    ]
+  },
+  "tests/utils/parsers/meta-parser.ts": {
+    "lastCommitId": "fb39b25c30bad5bbfce825da839abba185c7a1d4",
+    "violations": [
+      "singleUseVariable"
+    ]
+  },
   "tests/unit/core/location-parser.test.ts": {
     "lastCommitId": "d9034ceebf77dd46366b48298985149e2e7922bb",
     "violations": [
@@ -913,18 +912,6 @@
       "singleUseVariable"
     ]
   },
-  "tests/utils/parsers/location-parser.ts": {
-    "lastCommitId": "d9034ceebf77dd46366b48298985149e2e7922bb",
-    "violations": [
-      "singleUseVariable"
-    ]
-  },
-  "tests/utils/parsers/meta-parser.ts": {
-    "lastCommitId": "fb39b25c30bad5bbfce825da839abba185c7a1d4",
-    "violations": [
-      "singleUseVariable"
-    ]
-  },
   "tests/unit/locators/services/node-assignment-analyzer.test.ts": {
     "lastCommitId": "e37f849566f5affaa17254ceaebb6b99b12da3d9",
     "violations": [

guides/REFACTORING_STYLE.md

@@ -130,4 +130,10 @@ const lineCount = this.getLineCount(func);
 ### Naming Conventions
 - **Intention-revealing names** that describe the business purpose
 - **Avoid technical jargon** in favor of domain language
-- **Use verbs for methods, nouns for classes and properties**
+- **Use verbs for methods, nouns for classes and properties**
+
+### Systematic Refactoring
+
+#### Line-Based Operations
+- **Work bottom-up for coordinate-based edits** - Start with highest line numbers to avoid coordinate shifts
+- **Example**: When inlining multiple variables, process line 167 before line 94 to maintain stable targets

src/core/services/variable-name-validator.ts

@@ -1,12 +1,11 @@
-import { Node, ParameterDeclaration, FunctionDeclaration, MethodDeclaration, ArrowFunction, FunctionExpression, BindingElement, OmittedExpression, ConstructorDeclaration, ObjectBindingPattern } from 'ts-morph';
+import { Node, ParameterDeclaration, FunctionDeclaration, MethodDeclaration, ArrowFunction, FunctionExpression, BindingElement, OmittedExpression, ConstructorDeclaration } from 'ts-morph';
+import { ScopeAnalyzer } from './scope-analyzer';
 
 type FunctionLikeNode = FunctionDeclaration | MethodDeclaration | ArrowFunction | FunctionExpression | ConstructorDeclaration;
 
 export class VariableNameValidator {
   generateUniqueName(baseName: string, scope: Node): string {
-    const existingNames = this.getExistingVariableNames(scope);
-
-    if (!existingNames.has(baseName)) {
+    if (!this.getExistingVariableNames(scope).has(baseName)) {
       return baseName;
     }
 
@@ -18,6 +17,7 @@ export class VariableNameValidator {
 
     this.addFunctionParametersIfInBlock(scope, names);
     this.addDescendantVariableNames(scope, names);
+    this.addParentScopeVariableNames(scope, names);
 
     return names;
   }
@@ -44,21 +44,48 @@ export class VariableNameValidator {
     });
   }
 
+  private addParentScopeVariableNames(scope: Node, names: Set<string>): void {
+    let parentScope = ScopeAnalyzer.getParentScope(scope);
+    while (parentScope) {
+      this.addDirectVariableNames(parentScope, names);
+      this.addFunctionParametersIfInBlock(parentScope, names);
+      parentScope = ScopeAnalyzer.getParentScope(parentScope);
+    }
+  }
+
+  private addDirectVariableNames(scope: Node, names: Set<string>): void {
+    scope.forEachChild((child) => {
+      this.addChildVariableNames(child, names);
+    });
+  }
+
+  private addChildVariableNames(child: Node, names: Set<string>): void {
+    if (Node.isVariableStatement(child)) {
+      child.getDeclarations().forEach((declaration) => {
+        this.addVariableNameIfExists(declaration, names);
+      });
+    } else if (Node.isVariableDeclaration(child)) {
+      this.addVariableNameIfExists(child, names);
+    } else if (Node.isParameterDeclaration(child)) {
+      this.addParameterNameIfExists(child, names);
+    }
+  }
+
+  private addNameIfIdentifier(node: Node, names: Set<string>): void {
+    if (Node.isIdentifier(node)) {
+      names.add(node.getText());
+    }
+  }
+
   private addVariableNameIfExists(node: Node, names: Set<string>): void {
     if (Node.isVariableDeclaration(node)) {
-      const nameNode = node.getNameNode();
-      if (Node.isIdentifier(nameNode)) {
-        names.add(nameNode.getText());
-      }
+      this.addNameIfIdentifier(node.getNameNode(), names);
     }
   }
 
   private addParameterNameIfExists(node: Node, names: Set<string>): void {
     if (Node.isParameterDeclaration(node)) {
-      const nameNode = node.getNameNode();
-      if (Node.isIdentifier(nameNode)) {
-        names.add(nameNode.getText());
-      }
+      this.addNameIfIdentifier(node.getNameNode(), names);
     }
   }
 
@@ -69,16 +96,13 @@ export class VariableNameValidator {
   }
 
   private processParameters(functionNode: Node, names: Set<string>): void {
-    const parameters = this.getParametersFromFunction(functionNode);
-    for (const param of parameters) {
+    for (const param of this.getParametersFromFunction(functionNode)) {
       this.processParameter(param, names);
     }
   }
 
   private getParametersFromFunction(functionNode: Node): ParameterDeclaration[] {
-    if (Node.isFunctionDeclaration(functionNode) || Node.isMethodDeclaration(functionNode) || 
-        Node.isArrowFunction(functionNode) || Node.isFunctionExpression(functionNode) || 
-        Node.isConstructorDeclaration(functionNode)) {
+    if (this.isFunctionNode(functionNode)) {
       return (functionNode as FunctionLikeNode).getParameters();
     }
     return [];
@@ -87,54 +111,27 @@ export class VariableNameValidator {
   private processParameter(param: ParameterDeclaration, names: Set<string>): void {
     const nameNode = param.getNameNode();
     if (Node.isIdentifier(nameNode)) {
-      names.add(nameNode.getText());
+      this.addNameIfIdentifier(nameNode, names);
     } else {
-      this.addDestructuredParameterNames(nameNode, names);
-    }
-  }
-
-  private addDestructuredParameterNames(nameNode: Node, names: Set<string>): void {
-    if (Node.isObjectBindingPattern(nameNode)) {
-      this.addObjectBindingNames(nameNode, names);
-    } else if (Node.isArrayBindingPattern(nameNode)) {
-      this.addArrayBindingNames(nameNode, names);
+      this.addBindingPatternNames(nameNode, names);
     }
   }
 
-  private addObjectBindingNames(nameNode: Node, names: Set<string>): void {
-    if (Node.isObjectBindingPattern(nameNode)) {
-      this.processBindingElements(nameNode, names);
-    }
-  }
-
-  private processBindingElements(nameNode: ObjectBindingPattern, names: Set<string>): void {
-    nameNode.getElements().forEach((element: BindingElement | OmittedExpression) => {
-      this.processBindingElement(element, names);
-    });
-  }
-
-  private processBindingElement(element: BindingElement | OmittedExpression, names: Set<string>): void {
-    if (!Node.isBindingElement(element)) return;
-    const elementName = element.getNameNode();
-    if (Node.isIdentifier(elementName)) {
-      names.add(elementName.getText());
-    }
-  }
-
-  private addArrayBindingNames(nameNode: Node, names: Set<string>): void {
-    if (Node.isArrayBindingPattern(nameNode)) {
-      nameNode.getElements().forEach((element: BindingElement | OmittedExpression) => {
-        this.processArrayBindingElement(element, names);
+  private addBindingPatternNames(pattern: Node, names: Set<string>): void {
+    if (Node.isObjectBindingPattern(pattern)) {
+      pattern.getElements().forEach(element => {
+        this.addBindingElementName(element, names);
+      });
+    } else if (Node.isArrayBindingPattern(pattern)) {
+      pattern.getElements().forEach(element => {
+        this.addBindingElementName(element, names);
       });
     }
   }
 
-  private processArrayBindingElement(element: BindingElement | OmittedExpression, names: Set<string>): void {
+  private addBindingElementName(element: BindingElement | OmittedExpression, names: Set<string>): void {
     if (element && Node.isBindingElement(element)) {
-      const elementName = element.getNameNode();
-      if (Node.isIdentifier(elementName)) {
-        names.add(elementName.getText());
-      }
+      this.addNameIfIdentifier(element.getNameNode(), names);
     }
   }
 }

tests/fixtures/commands/rename/child-scope-conflict.expected.err

@@ -0,0 +1 @@
+Variable name 'x' already exists in this scope. Please choose a different name.

tests/fixtures/commands/rename/child-scope-conflict.expected.ts

@@ -0,0 +1,14 @@
+/**
+ * @description Test rename with child scope conflict - renaming y to x should fail
+ * @command refakts rename "[child-scope-conflict.input.ts 9:15-9:16]" --to "x"
+ * @expect-error true
+ */
+function f() {
+    const x = 1;
+    {
+        const y = 2;
+        {
+            return x;
+        }
+    }
+}

tests/fixtures/commands/rename/child-scope-conflict.input.ts

@@ -0,0 +1,14 @@
+/**
+ * @description Test rename with child scope conflict - renaming y to x should fail
+ * @command refakts rename "[child-scope-conflict.input.ts 9:15-9:16]" --to "x"
+ * @expect-error true
+ */
+function f() {
+    const x = 1;
+    {
+        const y = 2;
+        {
+            return x;
+        }
+    }
+}