Repository: uber/piranha (language: python)

Issue:
[Piranha Generic] Incomplete Enum cleanup compared to the behavior with PiranhaJava
PiranhaJava basically removes not only the treated part, but it also removes the enum values which are now going to be obsolete and have no usages.

But I don't see the same behaviour when working with `Piranha Generic`.

Here's an example

**Original Java class**

```java
class SampleJava {

  public void sampleMethod(ExperimentInterface exp) {
    if (exp.isToggleEnabled(MyEnum.SAMPLE_STALE_FLAG)) {
      System.out.println("SAMPLE_STALE_FLAG is enabled");
    } else {
      System.out.println("SAMPLE_STALE_FLAG is disabled");
    }
  }

  public void sampleMethod1(ExperimentInterface exp) {
    if (exp.isToggleDisabled(ExpEnum.SAMPLE_STALE_FLAG)) {
      System.out.println("SAMPLE_STALE_FLAG is disabled");
    } else {
      System.out.println("SAMPLE_STALE_FLAG is enabled");
    }
  }

  public enum MyEnum{
    SAMPLE_STALE_FLAG,
    DRAGON_WARRIOR
  }
}
```

Now with the below arguments and configuration file, when I run the piranha cli, I notice the output class looks like below

**Altered Java class by Piranha**

```java
class SampleJava {

  public void sampleMethod(ExperimentInterface exp) {
    System.out.println("SAMPLE_STALE_FLAG is enabled");
  }

  public void sampleMethod1(ExperimentInterface exp) {
    if (exp.isToggleDisabled(ExpEnum.SAMPLE_STALE_FLAG)) {
      System.out.println("SAMPLE_STALE_FLAG is disabled");
    } else {
      System.out.println("SAMPLE_STALE_FLAG is enabled");
    }
  }

  public enum MyEnum{
    SAMPLE_STALE_FLAG,
    DRAGON_WARRIOR
  }
}
```

The java based piranha was removing the stale entry in my enum `MyEnum`, but piranha generic doesn't do that. It only removes the enum reference from the method, but not from the actual enum as well.

`piranha_arguments.toml`

```
language = ["java"]
substitutions = [
    ["stale_flag_name", "MyEnum.SAMPLE_STALE_FLAG"],
    ["treated", "true"],
    ["treated_complement", "false"],
]
```

`rules.toml`

```
[[rules]]
name = "replace_isToggleEnabled_with_boolean_literal"
query = """((
    (method_invocation 
        name : (_) @name
        arguments: ((argument_list 
                        ([
                          (field_access field: (_)@argument)
                          (_) @argument
                         ])) )
            
    ) @method_invocation
)
(#eq? @name "isToggleEnabled")
(#eq? @argument "@stale_flag_name")
)"""
replace_node = "method_invocation"
replace = "@treated"
groups = [ "replace_expression_with_boolean_literal"]
holes = ["treated", "stale_flag_name"]

#
# For @stale_flag_name = STALE_FLAG and @treated = true
# Before :
#  exp.isToggleDisabled(STALE_FLAG)
# After :
#  false
#
[[rules]]
name = "replace_isToggleDisabled_with_boolean_literal"
query = """((
    (method_invocation 
        name : (_) @name
        arguments: ((argument_list 
                        ([
                          (field_access field: (_)@argument)
                          (_) @argument
                         ])) )
            
    ) @method_invocation
)
(#eq? @name "isToggleDisabled")
(#eq? @argument "@stale_flag_name")
)"""
replace_node = "method_invocation"
replace = "@treated_complement"
groups = [ "replace_expression_with_boolean_literal"]
holes = ["treated_complement", "stale_flag_name"]
```

The other difference I noticed was that I now had to give the qualified name of the enum 

```
    ["stale_flag_name", "MyEnum.SAMPLE_STALE_FLAG"],
```

in my configuration, which wasn't the case when dealing with `PiranhaJava`


Discussion / hints:
ping @ketkarameya 
@krmahadevan Do you have similarly named flag name (`SAMPLE_STALE_FLAG`) under two separate enum declarations (`MyEnum` and `ExpEnum`)? 
> Do you have similarly named flag names under two separate enum declarations ?

Yes It does. That was why I guessed that the fully qualified name was needed. I can live with that, but the enum itself is not getting cleaned up. Any thing that I need to be adding to get it to do that as well, else over a period of time, the enum is going to have a lot of stale values which would need a manual clean-up
I see. 
1. All you are missing is a  rule that cleans up the enum constant like this one :) https://github.com/uber/piranha/blob/9993d613b011e06fe0bd9824d1854333d28df975/generic/piranha/test-resources/java/feature_flag_system_1/treated/configurations/rules.toml#L109
2. But the above rule will delete the enum constant under `ExpEnum` too. So we could do something more - by adding the qualifier name too in the query. 
In this [branch](https://github.com/ketkarameya/piranha/tree/ketkara-krmahadevan-piranha-generic/generic/piranha/demo/java) I have basically replicated your scenario in the `generic/piranha/demo/java/`- And added rules to delete the API and Enum , such that the qualifier is considered. 


I know Piranha doesn't delete the `,`.  I'll add a fix in a couple of days :) 
I usually play around with queries here - https://tree-sitter.github.io/tree-sitter/playground
Adding a rule for deleting empty Enum declarations could be done easily by adding a cleanup rule for Java. I will do that. 
> I usually play around with queries here - https://tree-sitter.github.io/tree-sitter/playground

@ketkarameya 

This is nice. Any pointers with how I can try out actually writing some queries ? 

For e.g., here's what I did

* Opened up the playground portal
* Copy pasted the same class used in this example.
* Pasted the below query 

```
((
    (method_invocation
        name : (_) @name
        arguments: ((argument_list
                        ([
                          (field_access field: (_)@argument)
                          (_) @argument
                         ])) )

    ) @method_invocation
)
(#eq? @name "isActive")
(#eq? @argument "MyEnum.SAMPLE_STALE_FLAG")
)
```

Nothing happened 😢 
Aaah! What "happens" is syntax highlighting :) Observe the code in the top box gets highlighted appropriately. 

>  
> Now with the below arguments and configuration file, when I run the piranha cli, I notice the output class looks like below
> ```
>   public void sampleMethod1(ExperimentInterface exp) {
>     if (exp.isToggleDisabled(ExpEnum.SAMPLE_STALE_FLAG)) {
>       System.out.println("SAMPLE_STALE_FLAG is disabled");
>     } else {
>       System.out.println("SAMPLE_STALE_FLAG is enabled");
>     }
>   }
>
> }
> ```
> 

> 
> ```
> [[rules]]
> #
> # For @stale_flag_name = STALE_FLAG and @treated = true
> # Before :
> #  exp.isToggleDisabled(STALE_FLAG)
> # After :
> #  false
> #
> [[rules]]
> name = "replace_isToggleDisabled_with_boolean_literal"
> query = """((
>     (method_invocation 
>         name : (_) @name
>         arguments: ((argument_list 
>                         ([
>                           (field_access field: (_)@argument)
>                           (_) @argument
>                          ])) )
>             
>     ) @method_invocation
> )
> (#eq? @name "isToggleDisabled")
> (#eq? @argument "@stale_flag_name")
> )"""
> replace_node = "method_invocation"
> replace = "@treated_complement"
> groups = [ "replace_expression_with_boolean_literal"]
> holes = ["treated_complement", "stale_flag_name"]
> ```

In your example, was the invocation of `isToggleDisabled` deleted? The updated `sampleMethod1` still shows the invocation. 

Fix patch:
diff --git a/generic/piranha/Cargo.toml b/generic/piranha/Cargo.toml
index 507b7a933..efbebfb40 100644
--- a/generic/piranha/Cargo.toml
+++ b/generic/piranha/Cargo.toml
@@ -13,6 +13,7 @@ cc = "1.0.73"
 
 [dependencies]
 tree-sitter = "0.20.6"
+tree-sitter-traversal = "0.1.2"
 json = "0.12.4"
 toml = "0.5.9"
 serde = "1.0.136"
diff --git a/generic/piranha/src/models/source_code_unit.rs b/generic/piranha/src/models/source_code_unit.rs
index 01bae8904..958813169 100644
--- a/generic/piranha/src/models/source_code_unit.rs
+++ b/generic/piranha/src/models/source_code_unit.rs
@@ -4,9 +4,10 @@ use std::{
   path::{Path, PathBuf},
 };
 
-use tree_sitter::{InputEdit, Node, Parser, Tree};
+use tree_sitter::{InputEdit, Node, Parser, Range, Tree};
+use tree_sitter_traversal::{traverse, Order};
 
-use crate::utilities::tree_sitter_utilities::get_tree_sitter_edit;
+use crate::utilities::{eq_without_whitespace, tree_sitter_utilities::get_tree_sitter_edit};
 
 use super::edit::Edit;
 
@@ -46,6 +47,12 @@ impl SourceCodeUnit {
     fs::write(&self.path, self.code.as_str()).expect("Unable to Write file");
   }
 
+ 
+  pub(crate) fn apply_edit(&mut self, edit: &Edit, parser: &mut Parser) -> InputEdit {
+    // Get the tree_sitter's input edit representation
+    self._apply_edit(edit.replacement_range(), edit.replacement_string(), parser)
+  }
+
   /// Applies an edit to the source code unit
   /// # Arguments
   /// * `replace_range` - the range of code to be replaced
@@ -56,13 +63,12 @@ impl SourceCodeUnit {
   /// The `edit:InputEdit` performed.
   ///
   /// Note - Causes side effect. - Updates `self.ast` and `self.code`
-  pub(crate) fn apply_edit(&mut self, edit: &Edit, parser: &mut Parser) -> InputEdit {
+  pub(crate) fn _apply_edit(
+    &mut self, range: Range, replacement_string: &str, parser: &mut Parser,
+  ) -> InputEdit {
     // Get the tree_sitter's input edit representation
-    let (new_source_code, ts_edit) = get_tree_sitter_edit(
-      self.code.clone(),
-      edit.replacement_range(),
-      edit.replacement_string(),
-    );
+    let (new_source_code, ts_edit) =
+      get_tree_sitter_edit(self.code.clone(), range, replacement_string);
     // Apply edit to the tree
     self.ast.edit(&ts_edit);
     // Create a new updated tree from the previous tree
@@ -71,9 +77,31 @@ impl SourceCodeUnit {
       .expect("Could not generate new tree!");
     self.ast = new_tree;
     self.code = new_source_code;
+    // Handle errors, like removing extra comma.
+    self.remove_additional_comma_from_sequence_list(parser);
+    if self.ast.root_node().has_error() {
+      panic!("Produced syntactically incorrect source code {}", self.code());
+  }
     ts_edit
   }
 
+  /// Sometimes our rewrite rules may produce errors (recoverable errors), like failing to remove an extra comma.
+  /// This function, applies the recovery strategies.
+  /// Currently, we only support recovering from extra comma. 
+  fn remove_additional_comma_from_sequence_list(&mut self, parser: &mut Parser) {
+    if self.ast.root_node().has_error() {
+      let c_ast = self.ast.clone();
+      for n in traverse(c_ast.walk(), Order::Post) {
+        // Remove the extra comma
+        if n.is_extra() && eq_without_whitespace(n.utf8_text(self.code().as_bytes()).unwrap(), ",")
+        {
+          self._apply_edit(n.range(), "", parser);
+          break;
+        }
+      }
+    }
+  }
+
   // #[cfg(test)] // Rust analyzer FP
   pub fn code(&self) -> String {
     String::from(&self.code)


Test patch:
diff --git a/generic/piranha/src/models/unit_tests/source_code_unit_test.rs b/generic/piranha/src/models/unit_tests/source_code_unit_test.rs
index f7455c3b9..7617d6112 100644
--- a/generic/piranha/src/models/unit_tests/source_code_unit_test.rs
+++ b/generic/piranha/src/models/unit_tests/source_code_unit_test.rs
@@ -12,7 +12,7 @@ use {
 #[test]
 fn test_apply_edit_positive() {
   let source_code = "class Test {
-      pub void foobar(){
+      public void foobar(){
         boolean isFlagTreated = true;
         isFlagTreated = true;
         if (isFlagTreated) {
@@ -33,8 +33,8 @@ fn test_apply_edit_positive() {
   let _ = source_code_unit.apply_edit(
     &Edit::new(
       Range {
-        start_byte: 46,
-        end_byte: 75,
+        start_byte: 49,
+        end_byte: 78,
         start_point: tree_sitter::Point { row: 3, column: 9 },
         end_point: tree_sitter::Point { row: 3, column: 38 },
       },
@@ -46,7 +46,7 @@ fn test_apply_edit_positive() {
   );
   assert!(eq_without_whitespace(
     r#"class Test {
-      pub void foobar(){
+      public void foobar(){
         
         isFlagTreated = true;
         if (isFlagTreated) {
@@ -63,7 +63,7 @@ fn test_apply_edit_positive() {
 #[should_panic(expected = "byte index 1000 is out of bounds")]
 fn test_apply_edit_negative() {
   let source_code = "class Test {
-      pub void foobar(){
+      public void foobar(){
         boolean isFlagTreated = true;
         isFlagTreated = false;
         if (isFlagTreated) {
diff --git a/generic/piranha/test-resources/java/feature_flag_system_1/control/expected/XPFlagCleanerPositiveCases.java b/generic/piranha/test-resources/java/feature_flag_system_1/control/expected/XPFlagCleanerPositiveCases.java
index a33e717b5..9164e496b 100644
--- a/generic/piranha/test-resources/java/feature_flag_system_1/control/expected/XPFlagCleanerPositiveCases.java
+++ b/generic/piranha/test-resources/java/feature_flag_system_1/control/expected/XPFlagCleanerPositiveCases.java
@@ -22,9 +22,15 @@
 
 class XPFlagCleanerPositiveCases {
 
-  enum TestExperimentName {}
+  enum TestExperimentName {
+    SOME_FLAG,
 
-  enum AnotherTestExperimentName {}
+  }
+
+  enum AnotherTestExperimentName {
+    
+    SOME_OTHER_FLAG
+  }
 
   @Retention(RetentionPolicy.RUNTIME)
   public @interface Autorollout {
diff --git a/generic/piranha/test-resources/java/feature_flag_system_1/control/input/XPFlagCleanerPositiveCases.java b/generic/piranha/test-resources/java/feature_flag_system_1/control/input/XPFlagCleanerPositiveCases.java
index e89065da8..674b7dd23 100644
--- a/generic/piranha/test-resources/java/feature_flag_system_1/control/input/XPFlagCleanerPositiveCases.java
+++ b/generic/piranha/test-resources/java/feature_flag_system_1/control/input/XPFlagCleanerPositiveCases.java
@@ -23,12 +23,14 @@
 class XPFlagCleanerPositiveCases {
 
   enum TestExperimentName {
+    SOME_FLAG,
     STALE_FLAG
   }
 
   enum AnotherTestExperimentName {
     @Autorollout
-    STALE_FLAG
+    STALE_FLAG,
+    SOME_OTHER_FLAG
   }
 
   @Retention(RetentionPolicy.RUNTIME)
diff --git a/generic/piranha/test-resources/java/feature_flag_system_1/treated/expected/XPFlagCleanerPositiveCases.java b/generic/piranha/test-resources/java/feature_flag_system_1/treated/expected/XPFlagCleanerPositiveCases.java
index 45a052af5..0716b04cc 100644
--- a/generic/piranha/test-resources/java/feature_flag_system_1/treated/expected/XPFlagCleanerPositiveCases.java
+++ b/generic/piranha/test-resources/java/feature_flag_system_1/treated/expected/XPFlagCleanerPositiveCases.java
@@ -22,9 +22,13 @@
 
 class XPFlagCleanerPositiveCases {
 
-  enum TestExperimentName {}
+  enum TestExperimentName {
+    SOME_FLAG,
+  }
 
-  enum AnotherTestExperimentName {}
+  enum AnotherTestExperimentName {
+    SOME_OTHER_FLAG
+  }
 
   @Retention(RetentionPolicy.RUNTIME)
   public @interface Autorollout {
diff --git a/generic/piranha/test-resources/java/feature_flag_system_1/treated/input/XPFlagCleanerPositiveCases.java b/generic/piranha/test-resources/java/feature_flag_system_1/treated/input/XPFlagCleanerPositiveCases.java
index e89065da8..674b7dd23 100644
--- a/generic/piranha/test-resources/java/feature_flag_system_1/treated/input/XPFlagCleanerPositiveCases.java
+++ b/generic/piranha/test-resources/java/feature_flag_system_1/treated/input/XPFlagCleanerPositiveCases.java
@@ -23,12 +23,14 @@
 class XPFlagCleanerPositiveCases {
 
   enum TestExperimentName {
+    SOME_FLAG,
     STALE_FLAG
   }
 
   enum AnotherTestExperimentName {
     @Autorollout
-    STALE_FLAG
+    STALE_FLAG,
+    SOME_OTHER_FLAG
   }
 
   @Retention(RetentionPolicy.RUNTIME)
diff --git a/generic/piranha/test-resources/kt/feature_flag_system_1/control/expected/XPFlagCleanerCases.kt b/generic/piranha/test-resources/kt/feature_flag_system_1/control/expected/XPFlagCleanerCases.kt
index 83458552b..c983c43cf 100644
--- a/generic/piranha/test-resources/kt/feature_flag_system_1/control/expected/XPFlagCleanerCases.kt
+++ b/generic/piranha/test-resources/kt/feature_flag_system_1/control/expected/XPFlagCleanerCases.kt
@@ -18,9 +18,11 @@ import java.lang.annotation.RetentionPolicy
 
 internal class XPFlagCleanerPositiveCases {
     enum class TestExperimentName {
+        SOME_FLAG,
     }
 
     enum class AnotherTestExperimentName {
+        SOME_OTHER_FLAG
     }
 
     @Retention(RetentionPolicy.RUNTIME)
diff --git a/generic/piranha/test-resources/kt/feature_flag_system_1/control/input/XPFlagCleanerCases.kt b/generic/piranha/test-resources/kt/feature_flag_system_1/control/input/XPFlagCleanerCases.kt
index f48f5cf40..6d261e578 100644
--- a/generic/piranha/test-resources/kt/feature_flag_system_1/control/input/XPFlagCleanerCases.kt
+++ b/generic/piranha/test-resources/kt/feature_flag_system_1/control/input/XPFlagCleanerCases.kt
@@ -18,12 +18,14 @@ import java.lang.annotation.RetentionPolicy
 
 internal class XPFlagCleanerPositiveCases {
     enum class TestExperimentName {
+        SOME_FLAG,
         STALE_FLAG
     }
 
     enum class AnotherTestExperimentName {
         @Autorollout
-        STALE_FLAG
+        STALE_FLAG,
+        SOME_OTHER_FLAG
     }
 
     @Retention(RetentionPolicy.RUNTIME)
diff --git a/generic/piranha/test-resources/kt/feature_flag_system_1/treated/expected/XPFlagCleanerCases.kt b/generic/piranha/test-resources/kt/feature_flag_system_1/treated/expected/XPFlagCleanerCases.kt
index 228c4625a..92f6c8b7c 100644
--- a/generic/piranha/test-resources/kt/feature_flag_system_1/treated/expected/XPFlagCleanerCases.kt
+++ b/generic/piranha/test-resources/kt/feature_flag_system_1/treated/expected/XPFlagCleanerCases.kt
@@ -17,9 +17,11 @@ import java.lang.annotation.RetentionPolicy
 
 internal class XPFlagCleanerPositiveCases {
     enum class TestExperimentName {
+        SOME_FLAG,
     }
 
     enum class AnotherTestExperimentName {
+        SOME_OTHER_FLAG
     }
 
     @Retention(RetentionPolicy.RUNTIME)
diff --git a/generic/piranha/test-resources/kt/feature_flag_system_1/treated/input/XPFlagCleanerCases.kt b/generic/piranha/test-resources/kt/feature_flag_system_1/treated/input/XPFlagCleanerCases.kt
index 5fac3a692..8cdcb9d10 100644
--- a/generic/piranha/test-resources/kt/feature_flag_system_1/treated/input/XPFlagCleanerCases.kt
+++ b/generic/piranha/test-resources/kt/feature_flag_system_1/treated/input/XPFlagCleanerCases.kt
@@ -18,12 +18,14 @@ import java.lang.annotation.RetentionPolicy
 
 internal class XPFlagCleanerPositiveCases {
     enum class TestExperimentName {
+        SOME_FLAG,
         STALE_FLAG
     }
 
     enum class AnotherTestExperimentName {
         @Autorollout
-        STALE_FLAG
+        STALE_FLAG,
+        SOME_OTHER_FLAG
     }
 
     @Retention(RetentionPolicy.RUNTIME)