fixed for loop shadowing, fixes vshaxe/vshaxe#136

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## dev branch / next version (2.x.x)
 
+## 2.1.2 (2022-04-25)
+
+- fixed handling of loop iterator shadowing, fixes [vshaxe/vshaxe#136](https://github.com/vshaxe/vshaxe/issues/136)
+
 ## 2.1.1 (2022-04-23)
 
 - fixed identifier collection

haxelib.json

@@ -8,8 +8,8 @@
 		"refactor"
 	],
 	"description": "A code renaming tool for Haxe",
-	"version": "2.1.1",
-	"releasenote": "fixed identifier collection",
+	"version": "2.1.2",
+	"releasenote": "fixed handling of loop iterator shadowing",
 	"contributors": [
 		"AlexHaxe"
 	],

package.json

@@ -1,6 +1,6 @@
 {
     "name": "@haxecheckstyle/haxe-rename",
-    "version": "2.1.1",
+    "version": "2.1.2",
     "description": "Renaming tool for Haxe",
     "repository": {
         "type": "git",

src/refactor/PrintHelper.hx

@@ -29,8 +29,8 @@ class PrintHelper {
 		return switch (scopeType) {
 			case Parameter(params):
 				'Parameter(${params.map((i) -> '"${i.name}"')})';
-			case ForLoop(loopIdentifiers):
-				'ForLoop(${loopIdentifiers.map((i) -> '"${i.name}"')})';
+			case ForLoop(scopeStart, loopIdentifiers):
+				'ForLoop(${loopIdentifiers.map((i) -> '"${i.name}"')}) - scopeStart: $scopeStart';
 			default:
 				'$scopeType';
 		}

src/refactor/Refactor.hx

@@ -66,7 +66,7 @@ class Refactor {
 				RenameEnumField.refactorEnumField(context, file, identifier);
 			case Call(true):
 				Promise.reject(RefactorResult.Unsupported(identifier.toString()).printRefactorResult());
-			case Call(false) | Access | ArrayAccess(_):
+			case Call(false) | Access | ArrayAccess(_) | ForIterator:
 				context.verboseLog('rename "${identifier.name}" at call/access location - trying to find definition');
 				findActualWhat(context, file, identifier);
 			case CaseLabel(_):
@@ -105,6 +105,10 @@ class Refactor {
 					if (candidate == null) {
 						candidate = use;
 					}
+				case ScopedLocal(scopeEnd, ForLoop(scopeStart, _)) if (!onlyFields):
+					if ((scopeStart < identifier.pos.start) && (identifier.pos.start < scopeEnd)) {
+						candidate = use;
+					}
 				case ScopedLocal(scopeEnd, _) if (!onlyFields):
 					if ((use.pos.start < identifier.pos.start) && (identifier.pos.start < scopeEnd)) {
 						candidate = use;

src/refactor/discover/IdentifierType.hx

@@ -35,6 +35,7 @@ enum IdentifierType {
 	Call(isNew:Bool);
 	ArrayAccess(posClosing:Int);
 	Access;
+	ForIterator;
 	ScopedLocal(scopeEnd:Int, scopeType:ScopedLocalType);
 	StringConst;
 }
@@ -43,7 +44,7 @@ enum ScopedLocalType {
 	Parameter(params:Array<Identifier>);
 	Var;
 	CaseCapture;
-	ForLoop(loopIdentifiers:Array<Identifier>);
+	ForLoop(scopeStart:Int, loopIdentifiers:Array<Identifier>);
 }
 
 enum TypedefFieldType {

src/refactor/discover/UsageCollector.hx

@@ -682,20 +682,30 @@ class UsageCollector {
 	function readForIteration(context:UsageContext, identifier:Identifier, token:TokenTree, scopeEnd:Int) {
 		var loopIdentifiers:Array<Identifier> = [];
 
-		var ident:Identifier = makeIdentifier(context, token, ScopedLocal(scopeEnd, ForLoop(loopIdentifiers)), identifier);
+		var pClose:Null<TokenTree> = token.access().parent().firstOf(PClose).token;
+		var scopeStart:Int = token.pos.min;
+		if (pClose != null) {
+			scopeStart = pClose.pos.max;
+		}
+		var ident:Identifier = makeIdentifier(context, token, ScopedLocal(scopeEnd, ForLoop(scopeStart, loopIdentifiers)), identifier);
 		loopIdentifiers.push(ident);
 		if (!token.hasChildren()) {
 			return;
 		}
 		for (child in token.children) {
 			switch (child.tok) {
 				case Binop(OpArrow):
-					ident = makeIdentifier(context, child.getFirstChild(), ScopedLocal(scopeEnd, ForLoop(loopIdentifiers)), identifier);
+					ident = makeIdentifier(context, child.getFirstChild(), ScopedLocal(scopeEnd, ForLoop(scopeStart, loopIdentifiers)), identifier);
 					loopIdentifiers.push(ident);
 				default:
 					readExpression(context, ident, child);
 					if (ident.uses != null) {
 						for (use in ident.uses) {
+							switch (use.type) {
+								case Access:
+									use.type = ForIterator;
+								default:
+							}
 							loopIdentifiers.push(use);
 						}
 					}
@@ -880,6 +890,13 @@ class UsageCollector {
 						if (!parent.matches(Kwd(KwdTypedef))) {
 							pOpenToken = child;
 						}
+					case Binop(OpIn):
+					case Binop(OpArrow):
+						switch (type) {
+							case ScopedLocal(_, ForLoop(_)):
+							default:
+								pOpenToken = child;
+						}
 					case BkOpen | Binop(_):
 						pOpenToken = child;
 					case DblDot:

src/refactor/rename/RenameHelper.hx

@@ -190,12 +190,12 @@ class RenameHelper {
 
 			var typeHint:Null<Identifier> = candidate.getTypeHint();
 			switch (candidate.type) {
-				case ScopedLocal(_, ForLoop(loopIdent)):
+				case ScopedLocal(_, ForLoop(_, loopIdent)):
 					var index:Int = loopIdent.indexOf(candidate);
 					var changes:Array<Promise<TypeHintType>> = [];
 					for (child in loopIdent) {
 						switch (child.type) {
-							case ScopedLocal(_, ForLoop(_)):
+							case ScopedLocal(_, ForLoop(_, _)):
 								continue;
 							default:
 								changes.push(findTypeOfIdentifier(context, {

src/refactor/rename/RenameScopedLocal.hx

@@ -53,14 +53,23 @@ class RenameScopedLocal {
 						end: use.pos.start
 					};
 					changelist.addChange(use.pos.fileName, InsertText("this.", pos), use);
-				case ScopedLocal(scopeEnd, scopeType):
+				case ScopedLocal(_, _):
 					return Promise.reject('local var "${context.what.toName}" exists');
 				default:
 			}
 		}
 
+		var skipForIterator:Bool = false;
 		for (use in allUses) {
 			switch (use.type) {
+				case ScopedLocal(scopeEnd, ForLoop(start, _)):
+					if (use.pos.start == identifier.pos.start) {
+						scopeStart = start;
+						skipForIterator = true;
+					} else {
+						scopeStart = scopeEnd;
+						continue;
+					}
 				case ScopedLocal(scopeEnd, _):
 					if (use.pos.start != identifier.pos.start) {
 						// new parameter with identical name, so we skip its scope
@@ -69,7 +78,15 @@ class RenameScopedLocal {
 					}
 				case StructureField(_):
 					continue;
+				case ForIterator:
+					if (skipForIterator) {
+						skipForIterator = false;
+						continue;
+					}
 				default:
+					if (use.pos.start < scopeStart) {
+						continue;
+					}
 			}
 			if (use.name == identifier.name) {
 				// exact match

test/refactor/ClassTest.hx

@@ -192,6 +192,33 @@ class ClassTest extends TestBase {
 		failRefactor({fileName: "testcases/classes/BaseClass.hx", toName: "data", pos: 298}, 'local var "data" exists', async);
 	}
 
+	public function testRenameBaseClassCaseLabel(async:Async) {
+		var edits:Array<TestEdit> = [];
+		failRefactor({fileName: "testcases/classes/BaseClass.hx", toName: "data", pos: 516},
+			"renaming not supported for Case1 testcases/classes/BaseClass.hx@514-519 (CaseLabel(val))", async);
+	}
+
+	public function testRenameUseChildClassParentSubPart(async:Async) {
+		var edits:Array<TestEdit> = [];
+		failRefactor({fileName: "testcases/classes/UseChild.hx", toName: "data", pos: 233}, "could not find identifier to rename", async);
+	}
+
+	public function testRenameBaseClassDataToData(async:Async) {
+		var edits:Array<TestEdit> = [];
+		failRefactor({fileName: "testcases/classes/BaseClass.hx", toName: "data", pos: 43}, "could not find identifier to rename", async);
+	}
+
+	public function testRenameBaseClassNoIdentifier(async:Async) {
+		var edits:Array<TestEdit> = [];
+		failRefactor({fileName: "testcases/classes/BaseClass.hx", toName: "data", pos: 103}, "could not find identifier to rename", async);
+	}
+
+	public function testRenameStaticUsingConstructorCall(async:Async) {
+		var edits:Array<TestEdit> = [];
+		failRefactor({fileName: "testcases/classes/StaticUsing.hx", toName: "NewChildClass", pos: 359},
+			"renaming not supported for ChildClass testcases/classes/StaticUsing.hx@355-365 (Call(true))", async);
+	}
+
 	public function testRenameBaseClassParamterWithShadow2(async:Async) {
 		var edits:Array<TestEdit> = [
 			makeReplaceTestEdit("testcases/classes/BaseClass.hx", "data", 386, 387),

test/refactor/ScopedLocalTest.hx

@@ -20,7 +20,6 @@ class ScopedLocalTest extends TestBase {
 			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "refactorContext", 1332, 1339),
 			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "refactorContext", 1806, 1813),
 			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "refactorContext", 2017, 2024),
-
 		];
 		refactorAndCheck({fileName: "testcases/scopedlocal/Refactor.hx", toName: "refactorContext", pos: 463}, edits, async);
 	}
@@ -64,7 +63,6 @@ class ScopedLocalTest extends TestBase {
 			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "refactorContext", 1332, 1339),
 			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "refactorContext", 1806, 1813),
 			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "refactorContext", 2017, 2024),
-
 		];
 		refactorAndCheck({fileName: "testcases/scopedlocal/Refactor.hx", toName: "refactorContext", pos: 2020}, edits, async);
 	}
@@ -77,4 +75,46 @@ class ScopedLocalTest extends TestBase {
 		];
 		refactorAndCheck({fileName: "testcases/scopedlocal/Structure.hx", toName: "file", pos: 102}, edits, async);
 	}
+
+	public function testRenameFooVar(async:Async) {
+		var edits:Array<TestEdit> = [
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2104, 2107),
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2139, 2142),
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2187, 2190),
+		];
+		refactorAndCheck({fileName: "testcases/scopedlocal/Refactor.hx", toName: "file", pos: 2105}, edits, async);
+	}
+
+	public function testRenameFooArray(async:Async) {
+		var edits:Array<TestEdit> = [
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2104, 2107),
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2139, 2142),
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2187, 2190),
+		];
+		refactorAndCheck({fileName: "testcases/scopedlocal/Refactor.hx", toName: "file", pos: 2140}, edits, async);
+	}
+
+	public function testRenameFooItem(async:Async) {
+		var edits:Array<TestEdit> = [
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2132, 2135),
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2155, 2158),
+		];
+		refactorAndCheck({fileName: "testcases/scopedlocal/Refactor.hx", toName: "file", pos: 2133}, edits, async);
+	}
+
+	public function testRenameFooItem2(async:Async) {
+		var edits:Array<TestEdit> = [
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2173, 2176),
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2203, 2206),
+		];
+		refactorAndCheck({fileName: "testcases/scopedlocal/Refactor.hx", toName: "file", pos: 2174}, edits, async);
+	}
+
+	public function testRenameValItem(async:Async) {
+		var edits:Array<TestEdit> = [
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2180, 2183),
+			makeReplaceTestEdit("testcases/scopedlocal/Refactor.hx", "file", 2218, 2221),
+		];
+		refactorAndCheck({fileName: "testcases/scopedlocal/Refactor.hx", toName: "file", pos: 2181}, edits, async);
+	}
 }

testcases/scopedlocal/Refactor.hx

@@ -67,5 +67,15 @@ class Refactor {
 			case StringConst:
 				Unsupported;
 		}
+
+		var foo = [1, 23, 4, 5];
+		for (foo in foo) {
+			trace(foo);
+		}
+
+		for (foo => val in foo) {
+			trace(foo);
+			trace(val);
+		}
 	}
 }