LSP: ignore completion in comments

[nicklimmm]

Closes #2161

Simply check if the current cursor is in a comment context by comparing comment spans (from Module's extras).

[lpil]

Thank you

[lpil]

How come this is done here and not within is_within_comment?

[nicklimmm]

is_within_comment simply checks the byte index is in the range [span.start, span.end) in any comment.

For example:

// comment
          ^cursor is at byte 10 in insert mode (after `t`)

The comment above has the span of [2, 10), then is_within_comment(10) here returns false. So to ensure it knows it is in the comment context, we -1 it.

For completeness, only using byte_index - 1 doesn't cover the case when the following happens:

//
  ^cursor is at byte 2 (after the last `/`)

The above has the span of [2, 2), then is_within_comment(2 - 1) here returns false. So we don't -1 in this case.

[lpil]

Right, so why isn't this done inside the method? Seems like without that it doesn't do what it claims to do. Is that right?

[nicklimmm]

Right, so why isn't this done inside the method? Seems like without that it doesn't do what it claims to do. Is that right?

is_within_comment is working as intended, which is to check if the byte index is in a comment.

If we try to incorporate this -1 adjustment into the method, we will break things.

Suppose we have an empty comment, then followed by a newline character (//\n). If we have a byte index that points to the newline (2 in this case, in vim's normal mode), we shouldn't include the newline in the comment. This means that we shouldn't -1 the byte index in the method.

For reference, changing from normal to insert mode (with a) in the last character of the line bumps the offset number as shown below,

[nicklimmm]

Ignore this section for now, I've found another uncovered edge case on empty comments.

[lpil]

I don't understand why the logic for whether a byte index is inside a comment is different here than in other situations. Could you please explain with examples for both situations. Thank you

[nicklimmm]

@lpil Moving the discussion here after I made some changes to cover all edge cases. Currently thinking of a way to explain further.

[nicklimmm]

Instead of "is this cursor in a comment?", we should view this as "if I insert a char in this cursor, would it be included in the comment?". If true, then do not provide completions.

To demonstrate the new POV, suppose that we have a comment with 2 characters in it

//ab
  ^^^ don't provide completions if our cursor is in any of these positions

Expanded version:

//|ab
  ^ cursor is at byte index 2 (no completion)
//a|b
   ^ cursor is at byte index 3 (no completion)
//ab|
    ^ cursor is at byte index 4 (no completion)

So, with the new POV, we should check if the byte index is in the range [start, end] (note the inclusive end)

[nicklimmm]

To compare:

is_within_comment:

//abc
  ^^^ considers this span

new logic to avoid completions in comments:

     v this position contains no character in the source code, the user may type/insert a new char here
//abc
  ^^^^ considers this span (extended by 1)

[nicklimmm]

If the is_within_comment has the same logic as the new one, then this wouldn't make much sense:

Suppose that the source code is only //abc:

//abc
     ^ is_within_comment(5) -> true (?)

But the source code only has 5 characters, so if we index the source code with 5 (e.g. code[5]), it would panic

[nicklimmm]

Once we have a common understanding, I'll put the updated explanation in the implementation

[nicklimmm]

The modifications made outside of the function are in green (added, which is a special case used for ignoring the completion), and the line in red is the one from SrcSpan::cmp_byte_index

  if byte_index < span.start {
      Ordering::Less
- } else if span.end <= byte_index {
+ } else if span.end < byte_index {
+    // inclusive end
      Ordering::Greater
  } else {
      Ordering::Equal
  }

[lpil]

Again, please give the examples that explain why a special case is needed. Thank you.

[nicklimmm]

So, a special case is needed here to cover a case whenever we ask for completions right after //.

Using only is_within_comment here triggers completions right after //:

Screen.Recording.2024-05-30.at.8.58.59.PM.mov

But using the special case doesn't trigger completions right after //:

Screen.Recording.2024-05-30.at.8.56.20.PM.mov

[lpil]

Sorry, I'm not asking for examples of when this is needed for comments, I'm asking for how it is not needed for the other uses of this function.

[nicklimmm]

I don't think there are any other uses for this "modified" version for now.

Unless, in the future, we need to detect whether we are in a comment during insert mode (vim) for some LSP-related features.