From 1c36b1aa77c2376051bb397b83f079e5320ac3ee Mon Sep 17 00:00:00 2001 From: Ameya Ketkar <94497232+ketkarameya@users.noreply.github.com> Date: Thu, 17 Aug 2023 14:56:09 -0700 Subject: [PATCH 1/3] Fix over-deletion when looking for associated comma --- src/models/matches.rs | 39 +++++++++++++++++-- .../const_same_file/expected/sample.go | 12 ++++++ .../system_1/const_same_file/input/sample.go | 13 +++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/models/matches.rs b/src/models/matches.rs index 2fe8b1929..f05855b6d 100644 --- a/src/models/matches.rs +++ b/src/models/matches.rs @@ -126,9 +126,11 @@ impl Match { } else { current_node.prev_sibling() } { - let content = sibling.utf8_text(code.as_bytes()).unwrap(); - // Check if the sibling is a comment - if !found_comma && content.trim().eq(",") { + // Check if the sibling is a comma + if !found_comma + && self.is_comma(code, &sibling) + && self.is_comma_safe_to_delete(&sibling, trailing) + { // Add the comma to the associated matches self.associated_comma = Some(sibling.range().into()); current_node = sibling; @@ -160,6 +162,37 @@ impl Match { *piranha_arguments.cleanup_comments() && piranha_arguments.language().comment_nodes().contains(&kind) } + // Checks if the given node is a comma + fn is_comma(&self, code: &str, node: &Node) -> bool { + let content = node.utf8_text(code.as_bytes()).unwrap(); + return content.trim().eq(","); + } + + /// Checks whether it is safe to delete the provided comma node, by checking if + /// no neighbor node lies between the comma and the of nodes intended to be deleted. + /// + /// When `trailing` is true, it considers the previous node sibling (wrt the comma + /// Otherwise, it considers the next sibling (wrt the comma) + fn is_comma_safe_to_delete(&self, comma: &Node, trailing: bool) -> bool { + let (start_range, end_range) = self.get_first_and_last_associated_ranges(); + + if trailing { + if let Some(prev_node) = comma.prev_sibling() { + // Ensure that the previous node's end byte is not beyond the deleted node's end byte + if prev_node.end_byte() > end_range.end_byte { + return false; + } + } + } else if let Some(next_node) = comma.next_sibling() { + // Ensure that the next node's start byte is not before the deleted node's start byte + if next_node.start_byte() < start_range.start_byte { + return false; + } + } + + // It is safe to delete the comma node + true + } /// Checks if the given comment is safe to delete. fn _is_comment_safe_to_delete( diff --git a/test-resources/go/feature_flag/system_1/const_same_file/expected/sample.go b/test-resources/go/feature_flag/system_1/const_same_file/expected/sample.go index 8604b5b50..dd07eba6f 100644 --- a/test-resources/go/feature_flag/system_1/const_same_file/expected/sample.go +++ b/test-resources/go/feature_flag/system_1/const_same_file/expected/sample.go @@ -68,6 +68,18 @@ func callerFunc() { // should not replace the function name func isFlagEnabledFunc() bool { + + // Should not delete this comment and the func below + t.Run("message", func(t *Foobar.T) { + fmt.Println("some logging statement") + }) + + // Should not delete this comment and the func below + t.Run1(func(t *Foobar.T) { + fmt.Println("some other logging statement") + }, "other_message" ) + + fmt.Println("not enabled") return false } diff --git a/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go b/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go index ee1c53a03..2534b7949 100644 --- a/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go +++ b/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go @@ -87,6 +87,19 @@ func callerFunc() { // should not replace the function name func isFlagEnabledFunc() bool { + // Should not delete this comment and the func below + t.Run("message", func(t *Foobar.T) { + isFlgEnabled := exp.BoolValue(staleFlagConst) + fmt.Println("some logging statement") + }) + + // Should not delete this comment and the func below + t.Run1(func(t *Foobar.T) { + isFlgEnabled1 := exp.BoolValue(staleFlagConst) + fmt.Println("some other logging statement") + }, "other_message" ) + + isFlagEnabledFunc := exp.BoolValue(staleFlagConst) if !isFlagEnabledFunc { From 6a383e7fbceb295e8d00cd3700e0908a35c28460 Mon Sep 17 00:00:00 2001 From: Ameya Ketkar <94497232+ketkarameya@users.noreply.github.com> Date: Tue, 22 Aug 2023 07:20:06 -0700 Subject: [PATCH 2/3] Improve test case --- .../go/feature_flag/system_1/const_same_file/input/sample.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go b/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go index 2534b7949..12138291c 100644 --- a/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go +++ b/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go @@ -92,11 +92,11 @@ func isFlagEnabledFunc() bool { isFlgEnabled := exp.BoolValue(staleFlagConst) fmt.Println("some logging statement") }) - + // Should not delete this comment and the func below t.Run1(func(t *Foobar.T) { - isFlgEnabled1 := exp.BoolValue(staleFlagConst) fmt.Println("some other logging statement") + isFlgEnabled1 := exp.BoolValue(staleFlagConst) }, "other_message" ) From 0c68a1fc848ac2210830370f38f48e316a9774fb Mon Sep 17 00:00:00 2001 From: Ameya Ketkar <94497232+ketkarameya@users.noreply.github.com> Date: Tue, 22 Aug 2023 10:11:33 -0700 Subject: [PATCH 3/3] Address review comments --- .../feature_flag/system_1/const_same_file/expected/sample.go | 4 ++-- .../go/feature_flag/system_1/const_same_file/input/sample.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test-resources/go/feature_flag/system_1/const_same_file/expected/sample.go b/test-resources/go/feature_flag/system_1/const_same_file/expected/sample.go index dd07eba6f..beb0eba59 100644 --- a/test-resources/go/feature_flag/system_1/const_same_file/expected/sample.go +++ b/test-resources/go/feature_flag/system_1/const_same_file/expected/sample.go @@ -69,12 +69,12 @@ func callerFunc() { // should not replace the function name func isFlagEnabledFunc() bool { - // Should not delete this comment and the func below + // After deleting the assignment, this comment and the leading `func {...` is not deleted t.Run("message", func(t *Foobar.T) { fmt.Println("some logging statement") }) - // Should not delete this comment and the func below + // After deleting the assignment, this comment and the trailing `},` is not deleted t.Run1(func(t *Foobar.T) { fmt.Println("some other logging statement") }, "other_message" ) diff --git a/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go b/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go index 12138291c..8b621d649 100644 --- a/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go +++ b/test-resources/go/feature_flag/system_1/const_same_file/input/sample.go @@ -87,13 +87,13 @@ func callerFunc() { // should not replace the function name func isFlagEnabledFunc() bool { - // Should not delete this comment and the func below + // After deleting the assignment, this comment and the leading `func {...` is not deleted t.Run("message", func(t *Foobar.T) { isFlgEnabled := exp.BoolValue(staleFlagConst) fmt.Println("some logging statement") }) - // Should not delete this comment and the func below + // After deleting the assignment, this comment and the trailing `},` is not deleted t.Run1(func(t *Foobar.T) { fmt.Println("some other logging statement") isFlgEnabled1 := exp.BoolValue(staleFlagConst)