feat: support quantified captures

[ribru17]

This commit also removes all uses of #make-range! as it is no longer necessary.

[kiyoon]

Good to see this change as I've been waiting for this for quite a while! I guess this requires nightly nvim (0.10)?

[ribru17]

It does sadly, hopefully we can merge the main branch soon after 0.10 releases!

[kiyoon]

Maybe some people use custom captures or captures from other plugins. Should we deprecate the use of make-range! but remove the code a little later so it won't break any?

[ribru17]

Good point: up to @clason I think. This branch is already pretty breaking so removing the directive might not matter (or maybe we should just add the fake function() end placeholder directive to prevent errors?) I don't mind either way

[kiyoon]

Since we are going to keep the queries in sync with the master, the main branch will likely have #make-range! directives for some time. And merging this will break this syncronisation.

[clason]

It does sadly, hopefully we can merge the main branch soon after 0.10 releases!

No, we will definitely not do that! The main branch will become the default, but master will remain (possibly after renaming) as a legacy branch. This will happen together with the corresponding switch for nvim-treesitter (and nvim-treesitter-refactor) so people can upgrade to main + main or stay on master + master (but not get updates).

In general, though, I am very happy to see #make-range! gone, which always was a headache. But now that @theleop converted this into a proper directive, this is much less urgent.

[ribru17]

No, we will definitely not do that!

Right, sorry :sweat_smile: I remembered you saying that after I left that comment...

But now that @TheLeoP converted this into a proper directive, this is much less urgent.

I see: so should that directive code be kept around on main for a bit, like @kiyoon says?

[clason]

I think so, yes. I do like the change, but keeping queries in sync (for now) makes developing both branches in parallel much easier.

[clason]

In fact, I would make this change on master (in general, PRs should never target main unless they're related to the upstream refactor) as soon as we can require Nvim 0.10 (probably sometime after the first bugfix release, to give people time to update).

The "support matrix" for Nvim in the nvim-treesitter org is

• latest release
• latest commit on HEAD

(That doesn't mean that we'll aggressively drop support for older versions, but we don't make any long-term support guarantees. Delaying updates is just common courtesy.)

[clason]

Although I'm not 100% sure quantified captures are fully equivalent to make-range? I think the latter could create a range that spans other captures as well, which might be useful?

[ribru17]

I think it would be difficult to target master: or at least, the changes would have to be applied to the regular nvim-treesitter iterator utils which this repo uses, then we could apply just the query changes to this repo.

Although I'm not 100% sure quantified captures are fully equivalent to make-range?

Everything I have tested (which, granted, wasn't every single query change but was most unique types of queries that query files use) has shown the behavior to be identical. Hopefully it just always works, but not 100% positive about some wacky edge cases

[clason]

I think it would be difficult to target master: or at least, the changes would have to be applied to the regular nvim-treesitter iterator utils which this repo uses, then we could apply just the query changes to this repo.

If this would help the transition, I'd be willing to merge a PR iff 1. it only changes stuff that is dropped on main anyway and 2. is unlikely to be breaking for other users.

Everything I have tested (which, granted, wasn't every single query change but was most unique types of queries that query files use) has shown the behavior to be identical. Hopefully it just always works, but not 100% positive about some wacky edge cases

I was thinking more that make-range is (much?) more general, even if current queries don't use that (yet). I'm also not unsure if other plugins use it (but that's not necessarily a problem for changes in nvim-treesitter-textobjects).

[ribru17]

Sounds good, I'll mark as draft and sit on it until 0.10 :+1:

[ribru17]

I'm wondering if it is still better to target this on master. Doing some rebasing now and I'm realizing that the capture iteration logic resides in the nvim-treesitter utils that this repo inherits. I was able to change it easily on main since it was all standalone logic. For master, I think there would have to be some API changes on nvim-treesitter's end to allow for { all = true }. Does that sound possible? Or should this be left to target main?

[kiyoon]

From what I understand we would target that on master and rebase it on main when nvim-0.10.1 is out.

[clason]

I think there are a number of possible simplifications by relying on Nvim core APIs rather than nvim-treesitter utils left on the table; these would be worthwhile in any case. The main branch should not target anything below 0.10 (or even HEAD, if that simplifies things; due to the activities around wasm parsers, the timeframe for the full switcheroo now looks like 0.11, with some stabilization/early access in the runup to that).

I see two options:

1. Merge this now to main and essentially freeze the code on master to prevent rebase conflicts (any possible conflicts to queries are fairly easy to resolve manually); this is how I handle nvim-treesittter.
2. Change the logic in nvim-treesitter master and merge this to master (and make followup changes to main).

I am not averse to 2. in principle, but the decision would depend on the exact size of changes (conflicts when rebasing, dropping 0.9.x support or extensive comaptibility shims etc.)

[kiyoon]

I also don't mind freezing master and focusing on updating main which won't break much for the existing users and even encourage them to migrate.

[clason]

I wouldn't encourage people to migrate ... yet.

[ribru17]

I'll opt for solution 1. then?

[clason]

That certainly works; if you want my opinion on option 2, I would need more details ;)

[ribru17]

I think this is good to go now. I have removed the make-range predicate definition; I know we had a discussion that ended with deciding to keep it but I think if plugins use it that means they are also using the nvim-treesitter iteration utils which account for make-range. So I think the only backwards-compatibility this would break would be for user textobjects queries that have make-range. Should I still keep the make-range definition for this case?

[clason]

My concern was that make-range seems strictly more powerful; whether that power is used currently I don't know.

(And whether the implementation actually works correctly...)

[ribru17]

I'm not sure if it is more powerful to be honest. Or perhaps it should be and I have given quantified captures too much power- right now the range is calculated as $\textbf{first-captured-node} \cup \textbf{last-captured-node}$. Same logic as make-range, just that we have effectively replaced the second parameter of the directive with the final capture in the query. This means that we basically have make-range everywhere without even needing quantified captures; e.g. for something like:

(node
  "," @parameter.outer
  (param) @parameter.inner @parameter.outer)

the @parameter.outer range will be the merged range of the comma and param node. So I don't think there is a case that make-range could handle that this can't handle.

[clason]

right now the range is calculated as . Same logic as make-range

Ok, then there's really no reason. I was thinking -- as has been requested a few times -- that it's $\overline{\mathrm{co}} (\textbf{first-captured-node} \cup \textbf{last-captured-node})$ (so includes everything between the two captured nodes).

[ribru17]

Oops, sorry. That is how it works, everything in between the first and last. My (lack of) mathematical notation knowledge fails me :sweat_smile:

[clason]

Ok, so there is worth in the directive; the question is whether we need it here. Since I am not familiar with the queries here, I will leave this to you.

[ribru17]

make-range also included everything between ranges; the logic here is what I referenced for this PR. So I believe the behavior is identical and thus the directive is not needed here. But it may be worthwhile to add a make-range stub here that warns users that it is deprecated and they should use newer query methods.

[clason]

The idea is that make-range could capture the range between different nodes (and not just for text objects -- which is out of scope for this repo, of course).

If all queries in this repo (now and conceivably in the future) don't need this, we can yeet make-range.

[clason]

@ribru17 do we want to go through with this? if so, it needs a (big) rebase.

(I worry a bit about reusing make-range in an incompatible fashion, but maybe we can keep the "make range between" implementation compatible in some way. That would be a follow-up, though, and probably better discussed in core.)

[clason]

Actually, now that Nvim 0.10 has been out a while and had its first bugfix release, I feel much more comfortable bumping the minimum version on master. So how about this:

1. on master, require Nvim 0.10 and update the queries but leave the directive untouched;
2. rebase main to pull in the new queries and then remove make-range (and other simplifications).

It's a bit more work but strikes me as the least painful approach? @ribru17 @kiyoon