Main

[Starslayerx]

docs: specify nvim-treesitter main branch requirement

Update installation instructions to require nvim-treesitter's main branch,
which is recommended after migrating to Neovim's native treesitter APIs.

[Copilot]

Pull request overview

This PR migrates the wildfire.nvim plugin from nvim-treesitter's utility APIs to Neovim's native treesitter APIs, following nvim-treesitter's recommendation after they migrated to native APIs on their main branch. The installation instructions are updated to explicitly require nvim-treesitter's main branch.

Key Changes:

• Removed dependencies on nvim-treesitter.ts_utils and nvim-treesitter.parsers modules
• Implemented manual coordinate conversion from treesitter's 0-based system to Vim's 1-based system
• Added comprehensive bounds validation and error handling for buffer operations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File	Description
README.md	Updated installation instructions to specify nvim-treesitter main branch dependency and minimum Neovim version requirement (>= 0.9.0)
lua/wildfire/utils.lua	Replaced ts_utils functions with native vim.treesitter APIs, implemented manual coordinate conversion, and added buffer bounds validation to prevent cursor positioning errors
lua/wildfire/init.lua	Migrated from ts_utils.get_node_at_cursor() and parsers.get_parser() to native vim.treesitter equivalents, added error handling with pcall wrappers, and implemented bounds validation for text extraction

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment states that "end_col is already exclusive in treesitter (0-based), converting to 1-based inclusive means no change needed". However, this logic appears to be incorrect.

When treesitter returns an exclusive end column in 0-based indexing, and we need to convert it to 1-based inclusive indexing:

• A 0-based exclusive position N means "just before position N"
• Converting to 1-based: position N becomes N+1
• But we need inclusive, so the last character is at position N (0-based) = N+1 (1-based) - 1 = N

The comment suggests no adjustment is needed, which happens to arrive at the correct result (not adding 1), but the reasoning is misleading. The actual transformation is: (N + 1) - 1 = N, where +1 converts from 0-based to 1-based, and -1 converts from exclusive to inclusive.

Suggested change

	-- Note: treesitter end_col is exclusive, but vim coordinates are inclusive
	start_row = start_row + 1
	start_col = start_col + 1
	end_row = end_row + 1
	-- end_col is already exclusive in treesitter (0-based), converting to 1-based inclusive means no change needed
	-- because: 0-based exclusive position == 1-based inclusive position
	-- Note: treesitter end_col is 0-based and exclusive, while vim uses 1-based inclusive positions.
	start_row = start_row + 1
	start_col = start_col + 1
	end_row = end_row + 1
	-- For the end column, conceptually: (0-based exclusive N) -> (1-based exclusive N+1) -> (1-based inclusive (N+1)-1 = N).
	-- These steps cancel out numerically, so no adjustment to end_col is needed here.

[Copilot]

After checking if a parser exists and is valid, the code issues a warning notification but then continues to return on line 131. This creates a situation where a warning is shown even when get_node() returns nil for legitimate reasons (like being at the start of a file with no node at cursor).

The second return statement on line 131 is redundant and unreachable after the warning notification on line 128 already returns. Either the warning block should not return (to allow the outer return to execute), or the second return should be removed. Additionally, consider whether showing a warning is appropriate when get_node() returns nil, as this may be a normal occurrence.

[Copilot]

The comment explanation about coordinate conversion appears to be misleading. It states "necol: 0-based exclusive == 1-based inclusive, so no +1 needed", but the actual reason is more nuanced.

When converting from 0-based exclusive to 1-based inclusive:

• The transformation is: (N + 1) - 1 = N
• Where +1 converts from 0-based to 1-based
• And -1 converts from exclusive to inclusive

So while the result (no adjustment) is correct, the reasoning "0-based exclusive == 1-based inclusive" oversimplifies the actual mathematical transformation.

Suggested change

	-- Convert 0-based to 1-based indexing to match vim coordinates
	-- Note: treesitter end_col is exclusive, but vim coordinates are inclusive
	nsrow, nscol, nerow, necol = nsrow + 1, nscol + 1, nerow + 1, necol
	-- necol: 0-based exclusive == 1-based inclusive, so no +1 needed
	-- Convert 0-based to 1-based indexing to match vim coordinates.
	-- Treesitter rows/cols are 0-based; Vim's are 1-based. Treesitter end_col is
	-- 0-based *exclusive*, while Vim uses 1-based *inclusive* end columns.
	-- For the end column, the +1 (0-based → 1-based) and -1 (exclusive → inclusive)
	-- cancel out: (necol + 1) - 1 == necol, so we leave `necol` unchanged.
	nsrow, nscol, nerow, necol = nsrow + 1, nscol + 1, nerow + 1, necol

[Copilot]

The validation on line 107 checks if start_col is less than 1, but after the coordinate conversion on line 93, start_col should always be at least 1 (since it's start_col_0based + 1). The only way start_col could be less than 1 after line 93 is if the original 0-based start_col from treesitter was less than 0, which should not happen with valid treesitter data.

Similarly, the check for end_col less than 0 on line 107 would only trigger if the original 0-based exclusive end_col from treesitter was less than 0. While these checks provide defensive programming, they're checking for conditions that indicate corrupted data from treesitter rather than normal edge cases. Consider whether these represent truly recoverable errors or if they should be assertions/errors instead of silent returns.

Suggested change

	-- Validate column bounds
	if start_col < 1 or end_col < 0 then
	-- Validate column bounds only for manually supplied ranges.
	-- Tree-sitter ranges are expected to be valid after conversion above.
	if type(node_or_range) == "table" and (start_col < 1 or end_col < 0) then

[Copilot]

The pcall on line 44 is used to safely handle potential errors from nvim_buf_get_text, but after the comprehensive bounds checking on lines 32-41, it's unclear what additional errors pcall is meant to catch.

The bounds checking validates:

1. Row bounds (lines 34-36)
2. Column lower bounds (lines 39-41)

However, it doesn't validate that ecol is within the length of the end line, which could cause nvim_buf_get_text to fail. If the intention is to handle cases where ecol exceeds the line length, it would be clearer to explicitly validate this condition rather than relying on pcall to catch the error. This would make the error handling more predictable and easier to debug.

Suggested change

	-- Use pcall to safely get text, handle any edge cases
	local ok, lines = pcall(vim.api.nvim_buf_get_text, buf, srow - 1, scol - 1, erow - 1, ecol, {})
	if not ok or not lines or #lines == 0 then
	return false, { srow, scol, erow, ecol }
	end
	-- Validate that the end column does not exceed the length of the end line
	local end_line = vim.api.nvim_buf_get_lines(buf, erow - 1, erow, false)[1] or ""
	if ecol > #end_line then
	return false, { srow, scol, erow, ecol }
	end
	-- Bounds are validated; safely get text without relying on pcall
	local lines = vim.api.nvim_buf_get_text(buf, srow - 1, scol - 1, erow - 1, ecol, {})
	if not lines or #lines == 0 then
	return false, { srow, scol, erow, ecol }
	end

[Copilot]

The column clamping logic may allow the cursor to be positioned one past the end of a line. For a line with length N characters, valid 0-indexed column positions are 0 to N-1. However, the clamping uses math.min(col - 1, line_len), which allows column values up to N (the line length), not N-1 (the last valid character position).

While Vim/Neovim may accept a column value equal to the line length (positioning the cursor after the last character in some modes), this should be verified as intentional behavior. If the intention is to clamp to the last character position, the code should use math.min(col - 1, math.max(0, line_len - 1)) instead.

Suggested change

	start_col = math.max(0, math.min(start_col - 1, start_line_len))
	end_col = math.max(0, math.min(end_col - 1, end_line_len))
	start_col = math.max(0, math.min(start_col - 1, math.max(0, start_line_len - 1)))
	end_col = math.max(0, math.min(end_col - 1, math.max(0, end_line_len - 1)))

[Copilot]

The validation logic calls nvim_buf_get_lines twice to fetch line text for length calculation (lines 113-114), even though this information is only used for clamping. For large files or frequent calls to update_selection, this could have a performance impact.

Consider whether this validation is necessary in all cases. If the coordinates come from treesitter (which should always return valid ranges), the validation may be redundant. Alternatively, the validation could be made conditional or the line fetching could be optimized to fetch both lines in a single call when they're on adjacent or same lines.

Suggested change

	-- Get the actual line lengths to validate column positions
	local start_line_text = api.nvim_buf_get_lines(buf, start_row - 1, start_row, false)[1] or ""
	local end_line_text = api.nvim_buf_get_lines(buf, end_row - 1, end_row, false)[1] or ""
	-- Get the actual line lengths to validate column positions.
	-- Optimize for the common case where start and end are on the same line by
	-- fetching that line only once.
	local start_line_text, end_line_text
	if start_row == end_row then
	local line = api.nvim_buf_get_lines(buf, start_row - 1, start_row, false)[1] or ""
	start_line_text = line
	end_line_text = line
	else
	start_line_text = api.nvim_buf_get_lines(buf, start_row - 1, start_row, false)[1] or ""
	end_line_text = api.nvim_buf_get_lines(buf, end_row - 1, end_row, false)[1] or ""
	end

[Copilot]

The comment states that "end_col is already exclusive in treesitter (0-based), converting to 1-based inclusive means no change needed". However, this logic appears to be incorrect.

When treesitter returns an exclusive end column in 0-based indexing, and we need to convert it to 1-based inclusive indexing:

• A 0-based exclusive position N means "just before position N"
• Converting to 1-based: position N becomes N+1
• But we need inclusive, so the last character is at position N (0-based) = N+1 (1-based) - 1 = N

The comment suggests no adjustment is needed, which happens to arrive at the correct result (not adding 1), but the reasoning is misleading. The actual transformation is: (N + 1) - 1 = N, where +1 converts from 0-based to 1-based, and -1 converts from exclusive to inclusive.

Suggested change

	-- Note: treesitter end_col is exclusive, but vim coordinates are inclusive
	start_row = start_row + 1
	start_col = start_col + 1
	end_row = end_row + 1
	-- end_col is already exclusive in treesitter (0-based), converting to 1-based inclusive means no change needed
	-- because: 0-based exclusive position == 1-based inclusive position
	-- Note: treesitter end_col is 0-based exclusive, while vim coordinates are 1-based inclusive.
	start_row = start_row + 1
	start_col = start_col + 1
	end_row = end_row + 1
	-- Conceptually, converting end_col from 0-based exclusive to 1-based inclusive is: (end_col + 1) - 1.
	-- The +1 (0→1-based) and -1 (exclusive→inclusive) cancel each other, so the numeric value of end_col stays the same.