Support for trailing commas & skip Comments with concrete syntax (#596)

Co-authored-by: Ameya Ketkar <94497232+ketkarameya@users.noreply.github.com>

Author: danieltrt

experimental/requirements.txt

@@ -1,4 +1,4 @@
-tree-sitter
+tree-sitter==0.20.1
 tree-sitter-languages
 attrs
 openai
@@ -8,4 +8,4 @@ pytest
 flask
 flask-socketio
 comby
-eventlet
+eventlet

src/models/concrete_syntax.rs

@@ -17,6 +17,7 @@ use regex::Regex;
 
 use std::collections::HashMap;
 use tree_sitter::{Node, TreeCursor};
+use tree_sitter_traversal::Cursor;
 
 use crate::models::capture_group_patterns::ConcreteSyntax;
 use crate::models::matches::Match;
@@ -128,7 +129,13 @@ pub(crate) fn get_matches_for_node(
     );
   }
 
-  let node = cursor.node();
+  let mut node = cursor.node();
+
+  // Skip comment nodes always
+  while node.kind().contains("comment") && cursor.goto_next_sibling() {
+    node = cursor.node();
+  }
+
   // In case the template starts with :[var_name], we try match
   if let Some(caps) = RE_VAR.captures(match_template) {
     let var_name = &caps["var_name"];
@@ -148,6 +155,15 @@ pub(crate) fn get_matches_for_node(
       let current_node_code = current_node.utf8_text(source_code).unwrap();
       find_next_sibling(&mut tmp_cursor);
 
+      // Support for trailing commas
+      // This skips trailing commas as we are parsing through the match template
+      // Skips the comma node if the template doesn't contain it.
+      let next_node = tmp_cursor.node();
+      let next_node_text = next_node.utf8_text(source_code).unwrap();
+      if next_node_text == "," && !meta_advanced.0.starts_with(',') {
+        find_next_sibling(&mut tmp_cursor); // Skip comma
+      }
+
       if let (mut recursive_matches, true) =
         get_matches_for_node(&mut tmp_cursor, source_code, &meta_advanced)
       {

src/models/unit_tests/concrete_syntax_test.rs

@@ -13,12 +13,14 @@
 
 use crate::models::capture_group_patterns::ConcreteSyntax;
 use crate::models::concrete_syntax::get_all_matches_for_concrete_syntax;
+use crate::models::default_configs::GO;
 use crate::models::{default_configs::JAVA, language::PiranhaLanguage};
 
 fn run_test(
   code: &str, pattern: &str, expected_matches: usize, expected_vars: Vec<Vec<(&str, &str)>>,
+  language: &str,
 ) {
-  let java = PiranhaLanguage::from(JAVA);
+  let java = PiranhaLanguage::from(language);
   let mut parser = java.parser();
   let tree = parser.parse(code.as_bytes(), None).unwrap();
   let meta = ConcreteSyntax(String::from(pattern));
@@ -49,6 +51,7 @@ fn test_single_match() {
     "public int :[name] = :[value];",
     1,
     vec![vec![("name", "a"), ("value", "10")]],
+    JAVA,
   );
 }
 
@@ -62,6 +65,7 @@ fn test_multiple_match() {
       vec![("name", "a"), ("value", "10")],
       vec![("name", "b"), ("value", "20")],
     ],
+    JAVA,
   );
 }
 
@@ -72,5 +76,19 @@ fn test_no_match() {
     "public String :[name] = :[value];",
     0,
     vec![],
+    JAVA,
+  );
+}
+
+#[test]
+fn test_trailing_comma() {
+  run_test(
+    "a.foo(x, // something about the first argument
+           y, // something about the second argumet
+           );",
+    ":[var].foo(:[arg1], :[arg2])",
+    2,
+    vec![vec![("var", "a"), ("arg1", "x"), ("arg2", "y")]],
+    GO,
   );
 }

test-resources/java/feature_flag_system_2/control/input/ExperimentInterface.java

@@ -13,8 +13,7 @@
 
 interface SomeParameter {
 
-  @BoolParam(key = "STALE_FLAG", namespace = "some_long_name")
-  BoolParameter isStaleFeature();
+  
 
   @BoolParam(key = "other_flag", namespace = "some_long_name")
   BoolParameter isOtherFlag();

test-resources/java/feature_flag_system_2/control/input/ExperimentInterfaceAnnotatedInterface.java

@@ -14,8 +14,7 @@
 @ParameterDefinition(namespace = "some_long_name")
 interface SomeParameter {
 
-  @BoolParam(key = "STALE_FLAG")
-  BoolParameter isStaleFeature();
+  
 
   @BoolParam(key = "other_flag", namespace = "some_long_name")
   BoolParameter isOtherFlag();

test-resources/java/feature_flag_system_2/control/input/XPFlagCleanerInterfaceMethod.java

@@ -17,57 +17,39 @@ class XPFlagCleanerPositiveCases {
 
   private ExperimentInterface experimentation;
 
-  private boolean ftBool = experimentation.isStaleFeature().getCachedValue();
+  
 
   public void conditional_contains_stale_flag() {
 
-    if (experimentation.isStaleFeature().getCachedValue()) {
-      System.out.println("Hello World");
-    }
+    System.out.println("Hello World");
   }
 
   public void conditional_with_else_contains_stale_flag() {
 
-    if (experimentation.isStaleFeature().getCachedValue()) {
-      System.out.println("Hello World");
-    } else {
-      System.out.println("Hi world");
-    }
+    System.out.println("Hello World");
   }
 
   public void conditional_with_else_contains_stale_flag_tbool() {
 
-    bool tBool = exp.isStaleFeature().getCachedValue();
-    if (tBool && true) {
-      System.out.println("Hello World");
-    } else {
-      System.out.println("Hi world");
-    }
+    
+    System.out.println("Hello World");
   }
 
   public void conditional_with_else_contains_stale_flag_tbool(int a) {
 
-    bool tBool = exp.isStaleFeature().getCachedValue();
-    if (tBool && true) {
-      System.out.println("Hello World");
-    } else {
-      System.out.println("Hi world");
-    }
+    
+    System.out.println("Hello World");
   }
 
   public void conditional_with_else_contains_stale_flag_tbool(int a, bool abc) {
 
-    bool tBool = exp.isStaleFeature().getCachedValue();
-    if (!tBool && true) {
-      System.out.println("Hello World");
-    } else {
-      System.out.println("Hi world");
-    }
+    
+    System.out.println("Hi world");
   }
 
   public void conditional_with_else_contains_stale_flag_tbool_reassigned(int a, bool abc, int z) {
     // Currently if there is another assignment, variable will not be inlined.
-    bool tBool = exp.isStaleFeature().getCachedValue();
+    bool tBool = true;
     tBool = abc() && tBool;
     if (!tBool && true) {
       System.out.println("Hello World");
@@ -79,40 +61,28 @@ public void conditional_with_else_contains_stale_flag_tbool_reassigned(int a, bo
   public void conditional_with_else_contains_stale_flag_tbool_reassigned_to_same_val(
       int a, bool abc, int z) {
 
-    bool tBool = exp.isStaleFeature().getCachedValue();
-    tBool = true;
-    if (!tBool && true) {
-      System.out.println("Hello World");
-    } else {
-      System.out.println("Hi world");
-    }
+    
+    
+    System.out.println("Hi world");
   }
 
   public void conditional_with_else_contains_stale_flag_ftbool(int a) {
 
-    if (ftBool && true) {
-      System.out.println("Hello World");
-    } else {
-      System.out.println("Hi world");
-    }
+    System.out.println("Hello World");
   }
 
   public void conditional_with_else_contains_stale_flag_tbool_reassigned_ftbool(
       int a, bool abc, int z) {
     // Currently if there is another assignment, variable will not be inlined.
-    ftBool = exp.isStaleFeature().getCachedValue();
-    if (!ftBool && true) {
-      System.out.println("Hello World");
-    } else {
-      System.out.println("Hi world");
-    }
+    
+    System.out.println("Hi world");
   }
 
   public void conditional_with_else_contains_stale_flag_tbool_reassigned_ftbool_1(
       int a, bool abc, int z) {
     // Currently if there is another assignment, variable will not be inlined.
     bool ftBool = abc();
-    ftBool = exp.isStaleFeature().getCachedValue();
+    ftBool = true;
     if (!ftBool && true) {
       System.out.println("Hello World");
     } else {

test-resources/java/feature_flag_system_2/control/input/XPMethodChainCases.java

@@ -26,29 +26,23 @@ public void testDontMatchNonInstanceNested() {
   public static void foobar(Parameter cp) {
     SomeParam sp = SomeParam.create(cp);
     // Matches API
-    if (sp.isStaleFeature().getCachedValue()) {
-      System.out.println("!");
-    }
+    System.out.println("!");
     // Matches API
-    if (!sp.isStaleFeature().getCachedValue()) {
-      System.out.println("!!!");
-    }
+    
     // Does not match API
     if (sp.otherFlag().getCachedValue()) {
       System.out.println("!!!");
     }
-    if (sp.otherFlag().getCachedValue() && sp.isStaleFeature().getCachedValue()) {
-      System.out.println("!!!");
-    }
-    if (sp.otherFlag().getCachedValue() || sp.isStaleFeature().getCachedValue()) {
+    if (sp.otherFlag().getCachedValue()) {
       System.out.println("!!!");
     }
+    System.out.println("!!!");
     // test for identifier && false
-    if (a && sp.isStaleFeature().getCachedValue()){
+    if (a){
       System.out.println("!!! Testing identifier conjunction false");
     }
     // test for identifier || true
-    if (a || !sp.isStaleFeature().getCachedValue()){
+    if (a){
       System.out.println("!!!! Testing identifier disjunction true");
     }
     SomeParamRev spr = SomeParamRev.create(cp);
@@ -72,8 +66,8 @@ public static void foobar(Parameter cp) {
 
     System.out.println("done!");
     // Matches API
-    cp.put(sp.isStaleFeature(), true);
-    cp.put(sp.isStaleFeature(), false);
+    
+    
 
     // Do not match API
     cp.put(sp.otherFlag(), true);
@@ -82,16 +76,13 @@ public static void foobar(Parameter cp) {
 
   class TestMethodChainTest {
     // Matches annotation
-    @ParameterValue(ns = "some_long_name", key = "STALE_FLAG", val = "true")
+    
     public void testSomethingTreated() {
       System.out.println();
     }
 
     // Matches annotation
-    @ParameterValue(ns = "some_long_name", key = "STALE_FLAG", val = "false")
-    public void testSomethingControl() {
-      System.out.println();
-    }
+    
 
     // Does not match annotation
     @ParameterValue(ns = "some_long_name", key = "other_flag", val = "false")