From ae1f2f1ae70ffc92be1dba2b1b8ede32f362c64c Mon Sep 17 00:00:00 2001 From: AlexHaxe Date: Mon, 25 Apr 2022 14:54:28 +0200 Subject: [PATCH] fixed for loop shadowing, fixes vshaxe/vshaxe#136 --- CHANGELOG.md | 4 +++ haxelib.json | 4 +-- package-lock.json | 4 +-- package.json | 2 +- src/refactor/PrintHelper.hx | 4 +-- src/refactor/Refactor.hx | 6 +++- src/refactor/discover/IdentifierType.hx | 3 +- src/refactor/discover/UsageCollector.hx | 21 +++++++++-- src/refactor/rename/RenameHelper.hx | 4 +-- src/refactor/rename/RenameScopedLocal.hx | 19 +++++++++- test/refactor/ClassTest.hx | 27 +++++++++++++++ test/refactor/ScopedLocalTest.hx | 44 ++++++++++++++++++++++-- testcases/scopedlocal/Refactor.hx | 10 ++++++ 13 files changed, 136 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d22c29f..c86c558 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/haxelib.json b/haxelib.json index e89d68d..e09e0b8 100644 --- a/haxelib.json +++ b/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" ], diff --git a/package-lock.json b/package-lock.json index 2802fd2..03e4db2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@haxecheckstyle/haxe-rename", - "version": "2.1.1", + "version": "2.1.2", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@haxecheckstyle/haxe-rename", - "version": "2.1.1", + "version": "2.1.2", "license": "MIT", "bin": { "haxe-rename": "bin/rename.js" diff --git a/package.json b/package.json index 3d1c1b8..06bcc5b 100644 --- a/package.json +++ b/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", diff --git a/src/refactor/PrintHelper.hx b/src/refactor/PrintHelper.hx index 6f0d436..166de2d 100644 --- a/src/refactor/PrintHelper.hx +++ b/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'; } diff --git a/src/refactor/Refactor.hx b/src/refactor/Refactor.hx index a4e24c5..2c523bc 100644 --- a/src/refactor/Refactor.hx +++ b/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; diff --git a/src/refactor/discover/IdentifierType.hx b/src/refactor/discover/IdentifierType.hx index 7560935..e9ea9e1 100644 --- a/src/refactor/discover/IdentifierType.hx +++ b/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); Var; CaseCapture; - ForLoop(loopIdentifiers:Array); + ForLoop(scopeStart:Int, loopIdentifiers:Array); } enum TypedefFieldType { diff --git a/src/refactor/discover/UsageCollector.hx b/src/refactor/discover/UsageCollector.hx index acdf431..b92b874 100644 --- a/src/refactor/discover/UsageCollector.hx +++ b/src/refactor/discover/UsageCollector.hx @@ -682,7 +682,12 @@ class UsageCollector { function readForIteration(context:UsageContext, identifier:Identifier, token:TokenTree, scopeEnd:Int) { var loopIdentifiers:Array = []; - var ident:Identifier = makeIdentifier(context, token, ScopedLocal(scopeEnd, ForLoop(loopIdentifiers)), identifier); + var pClose:Null = 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; @@ -690,12 +695,17 @@ class UsageCollector { 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: diff --git a/src/refactor/rename/RenameHelper.hx b/src/refactor/rename/RenameHelper.hx index d855937..79aeb5c 100644 --- a/src/refactor/rename/RenameHelper.hx +++ b/src/refactor/rename/RenameHelper.hx @@ -190,12 +190,12 @@ class RenameHelper { var typeHint:Null = candidate.getTypeHint(); switch (candidate.type) { - case ScopedLocal(_, ForLoop(loopIdent)): + case ScopedLocal(_, ForLoop(_, loopIdent)): var index:Int = loopIdent.indexOf(candidate); var changes:Array> = []; for (child in loopIdent) { switch (child.type) { - case ScopedLocal(_, ForLoop(_)): + case ScopedLocal(_, ForLoop(_, _)): continue; default: changes.push(findTypeOfIdentifier(context, { diff --git a/src/refactor/rename/RenameScopedLocal.hx b/src/refactor/rename/RenameScopedLocal.hx index 74859e6..7ac7cc3 100644 --- a/src/refactor/rename/RenameScopedLocal.hx +++ b/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 diff --git a/test/refactor/ClassTest.hx b/test/refactor/ClassTest.hx index 626c4d3..ffe6c96 100644 --- a/test/refactor/ClassTest.hx +++ b/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 = []; + 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 = []; + 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 = []; + 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 = []; + 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 = []; + 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 = [ makeReplaceTestEdit("testcases/classes/BaseClass.hx", "data", 386, 387), diff --git a/test/refactor/ScopedLocalTest.hx b/test/refactor/ScopedLocalTest.hx index 42b55f9..33388fd 100644 --- a/test/refactor/ScopedLocalTest.hx +++ b/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 = [ + 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 = [ + 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 = [ + 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 = [ + 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 = [ + 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); + } } diff --git a/testcases/scopedlocal/Refactor.hx b/testcases/scopedlocal/Refactor.hx index c9e7119..7cd01a2 100644 --- a/testcases/scopedlocal/Refactor.hx +++ b/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); + } } }