fix: range containment check (e.g. weird @parameter.inner behavior)

[Dook97]

Fixes #832

[kiyoon]

@clason In the master branch, we used to use vim.treesitter.is_in_node_range() and node_contains() etc. Why did we decide to make our own version? I think it's a little unintuitive that we have to maintain this in our repo.

[clason]

If git blame serves me, this was a "refactor" to replace our custom range.lua with more minimal utility functions #672

The older upstream functions don't use a proper Range class. (Whether that is an advantage or disadvantage depends on your stance on OOP.)

Upstream is having a similar refactor, so after moving the MSNV to 0.12 (or 0.13, depending on when this gets finished) will hopefully allow us to remove these again.

[clason]

This is not a "fix" but just adding personal investigations as comments... Pick one of the two approaches and (after testing) commit to it.

[Dook97]

This is not a "fix" but just adding personal investigations as comments... Pick one of the two approaches and (after testing) commit to it.

I was hoping people better versed in the codebase would share some insight, but if you don't immediately see an issue with is_in_range() in itself, I think it would be better to replace the contains() implementation since that is clearly broken.

[clason]

neovim/neovim#36397, for reference

[Dook97]

Hm, in that case why not use the neovim API? Like: b512cc9

[clason]

Yes, as I said, that is the goal -- the refactor PR I linked was a partial step towards that. But make sure you're only using API available on Nvim 0.11, not HEAD.

[Dook97]

Hm, you're right - vim.Range doesn't seem to be available in v0.11 at all. At least judging from the docs.

If you're happy with my earlier solution (replacing contains() implementation) it seems to work from my testing. I also ran make tests and it passed without errors.

[clason]

I'd want @kiyoon to have the last look, but that's fine with me. Add a todo comment about replacing it with vim.Range when dropping support for Nvim 0.11.

[clason]

(And consider adding a test for #832 so we don't regress again.)

[Dook97]

Ok, test done.

Before:

Fail    ||      command equality Python: selection_mode.py[10,22]
            ./tests/select/common.lua:30: Commands dia and df) produces different results
            Expected objects to be the same.
            Passed in:
            (table: 0x7f2d02bb9e00) {
              [1] = 'class Test:'
              [2] = '    def __init__(self, *arg):'
              [3] = '        my_list = []'
              [4] = ''
              [5] = '        for arg_ in arg:'
              [6] = '            my_list.append(arg_)'
              [7] = ''
              [8] = '        self.my_list = my_list'
              [9] = ''
             *[10] = '        str.join(' ', )' }
            Expected:
            (table: 0x7f2d02bb45d0) {
              [1] = 'class Test:'
              [2] = '    def __init__(self, *arg):'
              [3] = '        my_list = []'
              [4] = ''
              [5] = '        for arg_ in arg:'
              [6] = '            my_list.append(arg_)'
              [7] = ''
              [8] = '        self.my_list = my_list'
              [9] = ''
             *[10] = '        str.join(' ', map(, ['foo', 'bar', 'baz']))' }

            stack traceback:
                ./tests/select/common.lua:30: in function 'run_compare_cmds_test'
                ./tests/select/common.lua:58: in function <./tests/select/common.lua:56>

After:

Success ||      command equality Python: selection_mode.py[10,22]

[kiyoon]

Thanks for investigating on this, and I like how simple the solution is!

If I understand correctly, it's not really about @parameter.inner being weird, but it's just one of the examples that can go wrong when outer and inner share the same end position? If so, I'd change the PR title a little bit before commiting (commit message follows the PR title)

Can I change the title like this?

fix: range containment check (e.g. weird @parameter.inner behavior)

[Dook97]

Correct, though I haven't encountered other instances of this bug. I've nothing against the name change.