From 3cc64b4f7c706ce5e4875b6e78deeb39da603817 Mon Sep 17 00:00:00 2001 From: Junichi Yamamoto Date: Fri, 17 Mar 2023 11:24:11 +0900 Subject: [PATCH] Fix incorrect unused coloring #5551 When there is an anonymous class in a method body, the anonymous class also removes method blocks of the enclosing class that need to be scanned. So, to prevent that, use `Map>` instead of `List` --- .../php/editor/csl/SemanticAnalysis.java | 52 +++++++++------- .../data/testfiles/semantic/gh5551_01.php | 51 ++++++++++++++++ .../testfiles/semantic/gh5551_01.php.semantic | 51 ++++++++++++++++ .../data/testfiles/semantic/gh5551_02.php | 59 +++++++++++++++++++ .../testfiles/semantic/gh5551_02.php.semantic | 59 +++++++++++++++++++ .../php/editor/csl/SemanticAnalyzerTest.java | 8 +++ 6 files changed, 259 insertions(+), 21 deletions(-) create mode 100644 php/php.editor/test/unit/data/testfiles/semantic/gh5551_01.php create mode 100644 php/php.editor/test/unit/data/testfiles/semantic/gh5551_01.php.semantic create mode 100644 php/php.editor/test/unit/data/testfiles/semantic/gh5551_02.php create mode 100644 php/php.editor/test/unit/data/testfiles/semantic/gh5551_02.php.semantic diff --git a/php/php.editor/src/org/netbeans/modules/php/editor/csl/SemanticAnalysis.java b/php/php.editor/src/org/netbeans/modules/php/editor/csl/SemanticAnalysis.java index dbe978070881..a1aa52130c2b 100644 --- a/php/php.editor/src/org/netbeans/modules/php/editor/csl/SemanticAnalysis.java +++ b/php/php.editor/src/org/netbeans/modules/php/editor/csl/SemanticAnalysis.java @@ -227,7 +227,7 @@ public ASTNodeColoring(ASTNode identifier, Set coloring) { // for unsed private method: name, identifier private final Map privateUnusedMethods; // this is holder of blocks, which has to be scanned for usages in the class. - private List needToScan = new ArrayList<>(); + private final Map> needToScan = new HashMap<>(); private final Snapshot snapshot; @@ -409,18 +409,16 @@ public void visit(ClassDeclaration cldec) { scan(cldec.getInterfaes()); Identifier name = cldec.getName(); addColoringForNode(name, createTypeNameColoring(name)); - needToScan = new ArrayList<>(); + needToScan.put(typeInfo, new ArrayList<>()); if (cldec.getBody() != null) { cldec.getBody().accept(this); // find all usages in the method bodies - while (!needToScan.isEmpty()) { - Block block = needToScan.remove(0); - block.accept(this); - } + scanMethodBodies(); addColoringForUnusedPrivateConstants(); addColoringForUnusedPrivateFields(); } + needToScan.remove(typeInfo); removeFromPath(); } @@ -515,8 +513,8 @@ public void visit(MethodDeclaration md) { // don't scan the body now. It should be scanned after all declarations // are known Block body = md.getFunction().getBody(); - if (body != null) { - needToScan.add(body); + if (body != null && needToScan.get(typeInfo) != null) { + needToScan.get(typeInfo).add(body); } } } @@ -595,23 +593,24 @@ public void visit(ClassInstanceCreation node) { // to avoid recognizing $this as an instance of an anonymous class scan(node.ctorParams()); addToPath(node); + // GH-5551 keep original type info to scan parent blocks + TypeInfo originalTypeInfo = typeInfo; typeInfo = new ClassInstanceCreationTypeInfo(node); scan(node.getAttributes()); scan(node.getSuperClass()); scan(node.getInterfaces()); - needToScan = new ArrayList<>(); + needToScan.put(typeInfo, new ArrayList<>()); Block body = node.getBody(); if (body != null) { body.accept(this); // find all usages in the method bodies - while (!needToScan.isEmpty()) { - Block block = needToScan.remove(0); - block.accept(this); - } + scanMethodBodies(); addColoringForUnusedPrivateConstants(); addColoringForUnusedPrivateFields(); } + needToScan.remove(typeInfo); + typeInfo = originalTypeInfo; removeFromPath(); } else { super.visit(node); @@ -639,14 +638,13 @@ public void visit(TraitDeclaration node) { typeInfo = new TypeDeclarationTypeInfo(node); Identifier name = node.getName(); addColoringForNode(name, createTypeNameColoring(name)); - needToScan = new ArrayList<>(); + needToScan.put(typeInfo, new ArrayList<>()); if (node.getBody() != null) { node.getBody().accept(this); - for (Block block : needToScan) { - block.accept(this); - } + scanMethodBodies(); addColoringForUnusedPrivateFields(); } + needToScan.remove(typeInfo); removeFromPath(); } @@ -661,13 +659,12 @@ public void visit(EnumDeclaration node) { typeInfo = new TypeDeclarationTypeInfo(node); Identifier name = node.getName(); addColoringForNode(name, createTypeNameColoring(name)); - needToScan = new ArrayList<>(); + needToScan.put(typeInfo, new ArrayList<>()); if (node.getBody() != null) { node.getBody().accept(this); - for (Block block : needToScan) { - block.accept(this); - } + scanMethodBodies(); } + needToScan.remove(typeInfo); removeFromPath(); } @@ -991,6 +988,19 @@ public void visit(NamedArgument node) { super.visit(node); } + /** + * Find all usages in the method bodies. + */ + private void scanMethodBodies() { + if (needToScan.get(typeInfo) == null) { + return; + } + for (Block block : needToScan.get(typeInfo)) { + block.accept(this); + } + needToScan.get(typeInfo).clear(); + } + private class FieldAccessVisitor extends DefaultVisitor { private final Set coloring; diff --git a/php/php.editor/test/unit/data/testfiles/semantic/gh5551_01.php b/php/php.editor/test/unit/data/testfiles/semantic/gh5551_01.php new file mode 100644 index 000000000000..4c1256e5e37f --- /dev/null +++ b/php/php.editor/test/unit/data/testfiles/semantic/gh5551_01.php @@ -0,0 +1,51 @@ +usedPrivateMethod1()->testMethod("test1"), PHP_EOL; + echo $this->usedPrivateMethod2()->testMethod("test2"), PHP_EOL; + } +} diff --git a/php/php.editor/test/unit/data/testfiles/semantic/gh5551_01.php.semantic b/php/php.editor/test/unit/data/testfiles/semantic/gh5551_01.php.semantic new file mode 100644 index 000000000000..dbcdcefa6a71 --- /dev/null +++ b/php/php.editor/test/unit/data/testfiles/semantic/gh5551_01.php.semantic @@ -0,0 +1,51 @@ +CLASS:TestClass<| { + public function |>METHOD:testMethod<|(string $test): string { + return $test; + } +} + +interface |>CLASS:TestInterface<| { + public function |>METHOD:testMethod<|(string $test): string; +} + +class |>CLASS:GH5551<| { + + private function |>METHOD:usedPrivateMethod1<|(): TestClass { + return new TestClass(); + } + + private function |>METHOD:usedPrivateMethod2<|(): TestInterface { + return new class implements TestInterface { + public function |>METHOD:testMethod<|(string $test): string { + return $test; + } + }; + } + + private function |>METHOD,UNUSED:unusedPrivateMethod<|(): void { + } + + public function |>METHOD:test<|() { + echo $this->|>CUSTOM1:usedPrivateMethod1<|()->|>CUSTOM1:testMethod<|("test1"), PHP_EOL; + echo $this->|>CUSTOM1:usedPrivateMethod2<|()->|>CUSTOM1:testMethod<|("test2"), PHP_EOL; + } +} diff --git a/php/php.editor/test/unit/data/testfiles/semantic/gh5551_02.php b/php/php.editor/test/unit/data/testfiles/semantic/gh5551_02.php new file mode 100644 index 000000000000..2d7f8cb0d0df --- /dev/null +++ b/php/php.editor/test/unit/data/testfiles/semantic/gh5551_02.php @@ -0,0 +1,59 @@ +usedPrivateMethod(); + return $test; + } + }; + } + + private function unusedPrivateMethod(): void { + } + + public function test() { + echo $this->usedPrivateMethod1()->testMethod("test1"), PHP_EOL; + echo $this->usedPrivateMethod2()->testMethod("test2"), PHP_EOL; + } +} diff --git a/php/php.editor/test/unit/data/testfiles/semantic/gh5551_02.php.semantic b/php/php.editor/test/unit/data/testfiles/semantic/gh5551_02.php.semantic new file mode 100644 index 000000000000..870be797d90b --- /dev/null +++ b/php/php.editor/test/unit/data/testfiles/semantic/gh5551_02.php.semantic @@ -0,0 +1,59 @@ +CLASS:TestClass<| { + public function |>METHOD:testMethod<|(string $test): string { + return $test; + } +} + +interface |>CLASS:TestInterface<| { + public function |>METHOD:testMethod<|(string $test): string; +} + +class |>CLASS:GH5551<| { + + private function |>METHOD:usedPrivateMethod1<|(): TestClass { + return new TestClass(); + } + + private function |>METHOD:usedPrivateMethod2<|(): TestInterface { + return new class implements TestInterface { + + private function |>METHOD,UNUSED:unusedPrivateMethod<|(): void { + } + + private function |>METHOD:usedPrivateMethod<|(): void { + } + + public function |>METHOD:testMethod<|(string $test): string { + $this->|>CUSTOM1:usedPrivateMethod<|(); + return $test; + } + }; + } + + private function |>METHOD,UNUSED:unusedPrivateMethod<|(): void { + } + + public function |>METHOD:test<|() { + echo $this->|>CUSTOM1:usedPrivateMethod1<|()->|>CUSTOM1:testMethod<|("test1"), PHP_EOL; + echo $this->|>CUSTOM1:usedPrivateMethod2<|()->|>CUSTOM1:testMethod<|("test2"), PHP_EOL; + } +} diff --git a/php/php.editor/test/unit/src/org/netbeans/modules/php/editor/csl/SemanticAnalyzerTest.java b/php/php.editor/test/unit/src/org/netbeans/modules/php/editor/csl/SemanticAnalyzerTest.java index 633e0221b805..f7c50a0cc901 100644 --- a/php/php.editor/test/unit/src/org/netbeans/modules/php/editor/csl/SemanticAnalyzerTest.java +++ b/php/php.editor/test/unit/src/org/netbeans/modules/php/editor/csl/SemanticAnalyzerTest.java @@ -217,4 +217,12 @@ public void testEnumerations() throws Exception { public void testConstantsInTraits() throws Exception { checkSemantic("testfiles/semantic/constantsInTraits.php"); } + + public void testGH5551_01() throws Exception { + checkSemantic("testfiles/semantic/gh5551_01.php"); + } + + public void testGH5551_02() throws Exception { + checkSemantic("testfiles/semantic/gh5551_02.php"); + } }