Fix go comments bug #392

ketkarameya · 2023-02-23T06:28:42Z

Bug Diagnosis (#376): This problem is only inGo (because of its grammar, in my observation). The problem is caused when the comment is associated to the last statement of a method body in Go. Diagnosis:
Our comment fetching algorithm looks for comments on lines on the same line as deleted element and some lines above it (determined by the cleanup_comment_buffer flag). There is a caveat in this algorithm - (i) Get all the nodes that either start and end at [row] (ii) If all nodes are comments.
Now lets take the Go case where the comment is associated to the last statement of a method body in Go.
When we delete the last statement of the method body, now the last "statement" of the method body is a comment.
For some reason, tree sitter reports that this last row is where the comment and the block end. :|

Fix: Add a field ignore_node_for_comment property to PiranhaLanguage, now our algorithm will ignore the node kinds when reasoning about all nodes that start/end at last row.

Test Plan Added the test cases as reported in the issue. I added similar test cases for Java and Kotlin too, but I did not find this inconsistency for them (so did not have set the ignore_node_for_comment property for Java and Kotlin)

ketkarameya

Self review and annotations

ketkarameya · 2023-02-23T23:14:53Z

src/models/language.rs

  #[get = "pub"]
  comment_nodes: Vec<String>,
+  /// The node kinds to be ignored when reasoning about comments
+  #[get = "pub"]
+  ignore_nodes_for_comments: Vec<String>,


The property i mentioned in the PR description.

ketkarameya · 2023-02-23T23:15:03Z

src/models/piranha_arguments.rs

@@ -386,6 +386,13 @@ impl SourceCodeUnit {
      .unwrap_or_else(|| self.root_node());

    for node in traverse(node.walk(), Order::Post) {
+      if self


The bug fix.

lazaroclapp

Change looks great (assuming I understood it correctly). Some minor requests regarding documentation and what seems to be a few stale comments 🙂

lazaroclapp · 2023-02-24T21:04:05Z

test-resources/go/feature_flag/builtin_rules/statement_cleanup/input/sample.go

+    } else {
+        fmt.Println("remain")
+    }
+    // this comment doesnt get removed but it should


No longer true, no? Shouldn't the comment be updated to saying it will get removed (same with all the cases below)

lazaroclapp · 2023-02-24T21:07:48Z

src/models/language.rs

  #[get = "pub"]
  comment_nodes: Vec<String>,
+  /// The node kinds to be ignored when reasoning about comments


Suggested change

/// The node kinds to be ignored when reasoning about comments

/// The node kinds to be ignored when searching for comments

Just to be clear, the algorithm will find the nearest node to the deleted position that is not one of these. Then, if that node is a comment, it will delete it. Correct? Am I understanding the logic?

Yes. You have got it correct. I have made it more explicit now.

lazaroclapp · 2023-02-24T21:09:42Z

src/models/language.rs

@@ -66,6 +76,13 @@ impl PiranhaLanguage {
    self.comment_nodes().contains(&kind)
  }

+  pub fn should_ignore_node_for_comment(&self, node: &Node) -> bool {


Documentation on this one, explaining what it does and how it's used.

Like, is the following correct?

[Piranha] will find the nearest node to the deleted position that is not one of these. Then, if that node is a comment, it will delete it.

(Even if so, please rewrite the doc comment a bit with some more info about how this function is used to implement the algorithm above)

ketkarameya

Addressed comments.
c1086c9 - Adds more comments, fixes comments, and slight cleanup

76bce7d - move _get_comment_at_line from piranha_argument to language

ketkarameya

Aaah! I missed a corner case in this bug fix. :|

ketkarameya · 2023-02-25T19:28:11Z

test-resources/go/feature_flag/builtin_rules/statement_cleanup/expected/sample.go

@@ -72,7 +72,6 @@ func simplify_if_statement_false_comment_demo_single_comment() {

 func simplify_if_statement_false_comment_demo_double_comment() {
    fmt.Println("remain")
-    // this comment doesnt get removed


The bug was not fully solved :| . Now its solved.

ketkarameya · 2023-02-25T19:32:28Z

src/models/piranha_arguments.rs

-        debug!("Deleting an associated comment");
-        *applied_edit = self._apply_edit(&Edit::delete_range(self.code(), comment_range), parser);
-      }
+    &mut self, edit: &Edit, parser: &mut tree_sitter::Parser,


from apply_edit we call _delete_associated_comment and from _delete_associated_comment we recursively call apply_edit. This ensures that can delete a series of comments.

Let's say apply_edit deletes int foo in the below example.

// some // comment int foo;

becomes

// some // comment

now _delete_associated_comment is triggered, and now we produce

// some

Since _delete_associated_comment is effectively called recursively

<all comments are deleted>

Can we have this documented in a comment in the code in addition to in the PR review comments? Possibly in the documentation for this function.

ketkarameya · 2023-02-25T19:36:22Z

src/models/source_code_unit.rs

@@ -313,13 +313,6 @@ impl SourceCodeUnit {
    self.perform_delete_consecutive_new_lines();
  }

-  pub(crate) fn apply_edit(&mut self, edit: &Edit, parser: &mut Parser) -> InputEdit {


inlined apply_edit, this simplifies the code and also kinda solves this bug further.

Earlier we had a function called apply_edit which called _apply_edit and then called _delete_associated_comment . Now _delete_associated_comment called _apply_edit (not apply_edit ), therefore _delete_associated_comment was not getting invoked recursively, and therefore we were not deleting a series of comments.

I just inlined apply_edit into _apply_edit and renamed _apply_edit to apply_edit. Now when apply_edit is called from _delete_associated_comment, it ensures that _delete_associated_comment is called recursively. (I adjusted all the signatures and simplified the code)

lazaroclapp

LGTM. One small request documentation wise, though:

lazaroclapp · 2023-03-15T17:33:56Z

src/models/piranha_arguments.rs

-        debug!("Deleting an associated comment");
-        *applied_edit = self._apply_edit(&Edit::delete_range(self.code(), comment_range), parser);
-      }
+    &mut self, edit: &Edit, parser: &mut tree_sitter::Parser,


Can we have this documented in a comment in the code in addition to in the PR review comments? Possibly in the documentation for this function.

lazaroclapp

LGTM!

Fix go comments bug

8f2231f

ketkarameya mentioned this pull request Feb 23, 2023

Go comments not being cleaned up correctly #376

Closed

ketkarameya added the bug Something isn't working label Feb 23, 2023

ketkarameya linked an issue Feb 23, 2023 that may be closed by this pull request

Go comments not being cleaned up correctly #376

Closed

Fix go comments bug

1db7fa3

ketkarameya force-pushed the go_comment_bug branch from 815a0f5 to 1db7fa3 Compare February 23, 2023 23:09

ketkarameya marked this pull request as ready for review February 23, 2023 23:10

ketkarameya commented Feb 23, 2023

View reviewed changes

ketkarameya requested a review from lazaroclapp February 23, 2023 23:15

lazaroclapp reviewed Feb 24, 2023

View reviewed changes

ketkarameya force-pushed the go_comment_bug branch from 796083d to 084fab3 Compare February 25, 2023 17:34

ketkarameya added 2 commits February 25, 2023 09:38

Address comments [part-1]

c1086c9

Address comments [part-2]

76bce7d

ketkarameya force-pushed the go_comment_bug branch from 084fab3 to 76bce7d Compare February 25, 2023 17:38

ketkarameya commented Feb 25, 2023

View reviewed changes

Solve the bug even further

14d77ae

ketkarameya commented Feb 25, 2023

View reviewed changes

lazaroclapp approved these changes Mar 15, 2023

View reviewed changes

ketkarameya added 2 commits March 16, 2023 09:53

Add the PR comment explanation into documentation (code review)

d0893e9

.

f069921

lazaroclapp approved these changes Mar 16, 2023

View reviewed changes

ketkarameya added 2 commits March 16, 2023 13:12

.

3bb053c

Merge branch 'master' into go_comment_bug

d0037b6

ketkarameya merged commit ec82222 into master Mar 16, 2023

ketkarameya deleted the go_comment_bug branch July 13, 2023 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix go comments bug #392

Fix go comments bug #392

ketkarameya commented Feb 23, 2023 •

edited

Loading

ketkarameya left a comment

ketkarameya Feb 23, 2023

ketkarameya Feb 23, 2023

lazaroclapp left a comment

lazaroclapp Feb 24, 2023

lazaroclapp Feb 24, 2023

ketkarameya Feb 25, 2023

lazaroclapp Feb 24, 2023

ketkarameya left a comment

ketkarameya left a comment

ketkarameya Feb 25, 2023

ketkarameya Feb 25, 2023

lazaroclapp Mar 15, 2023

ketkarameya Feb 25, 2023

lazaroclapp left a comment

lazaroclapp Mar 15, 2023

lazaroclapp left a comment

	/// The node kinds to be ignored when reasoning about comments
	/// The node kinds to be ignored when searching for comments

Fix go comments bug #392

Fix go comments bug #392

Conversation

ketkarameya commented Feb 23, 2023 • edited Loading

ketkarameya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketkarameya left a comment

Choose a reason for hiding this comment

ketkarameya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

ketkarameya commented Feb 23, 2023 •

edited

Loading