Support range format

[CppCXY]

test deepseek

[github-actions]

Reasoning Details

We are asked to review the code changes in a pull request diff. The diff introduces a new module `range_format` in the formatter, adds public functions `reformat_range` and `reformat_range_in_chunk` to the library crate, modifies the document formatting handler to extract a shared config builder, and rewrites the range formatting handler to use the new range formatter instead of the old approach of formatting the whole document and filtering edits.

We need to analyze for potential issues: code style, logical errors, security vulnerabilities, performance, correctness, etc. Provide a concise list of problems and recommendations.

Let's go through the diff file by file.

1. crates/emmylua_formatter/src/formatter/mod.rs: adds pub(crate) mod range_format; – fine.

2. crates/emmylua_formatter/src/formatter/range_format/mod.rs: This is the bulk. Let's examine.

• reformat_range entry point: takes &SourceText, parses, gets chunk, calls reformat_range_in_chunk. SourceText has text and level. Good.

• reformat_range_in_chunk: clamps selection to chunk end. Then tries to find explicit format target (table expr, call arg list, param list). If found, formats just that fragment wrapped. Otherwise, falls back to select_format_range to expand selection to a block or overlapping syntax nodes.

Potential issues:

• select_explicit_format_target looks for the smallest containing table expr, call arg list, param list. But it uses find_table_expr_target which first checks if selection is inside a table field, then parent table expr; otherwise finds smallest containing table expr. That seems fine. However, the pick_better_target logic picks the smaller range among candidates. But note: it's called for each candidate type. If multiple candidates exist (e.g., selection inside both a table expr and a call arg list), the smaller one wins. That might be correct, but could be surprising if the user wanted the outer one. However, it's likely expected.

• format_explicit_target wraps fragment and formats, then extracts the inner node. The wrapping is done to get proper formatting context (e.g., to handle indentation, commas, etc.). This is a common technique. However, there is a potential issue: The wrapping uses a fixed prefix __emmylua_range_target. This could conflict with user code if that identifier is used. But it's unlikely and acceptable for a tool.

• extract_explicit_fragment re-parses the formatted wrapper and extracts the node. It returns Option<String>. If the wrapper fails to parse (syntax errors), it returns None. Good.

• select_format_range: uses FormatContext and FormatPlan from the existing formatter. It calls layout::analyze_layout to get layout info. Then find_deepest_block_slice and find_overlapping_slice to expand selection to full statement lines. This seems to reuse existing layout analysis.

• find_deepest_block_slice recursively looks for a block that fully contains the selection. If found, it looks inside its children for overlapping slices. If no deeper block, returns None. Then find_overlapping_slice is called if nothing found. That seems okay.

• find_overlapping_slice collects indices of nodes that intersect the selection, then returns range from first to last. This could include nodes that are not contiguous? Actually it's based on index order, which is maybe the order in the layout plan (presumably tree order). That should give a contiguous range of nodes that intersect selection. Good.

• layout_node_text_range: returns text range for comment or syntax nodes. Good.

• Helper functions: clamp_range, contains_range, intersects_range, expand_to_full_lines, line_start_offset, line_end_offset, line_indent_prefix, strip_base_indent, apply_base_indent, map_lines, split_line_ending, format_fragment. All seem straightforward.

• format_fragment parses fragment, formats chunk, uses Printer. It adds a 20% capacity margin. That's fine.

Potential issues:

• In reformat_range_in_chunk, after selecting explicit target, it calls format_explicit_target. But if that returns None (e.g., empty fragment or parse error), it falls through to the general path? No, it returns early if target found. If format_explicit_target returns None, the function returns None. That means if explicit target formatting fails, whole range formatting fails. Could there be cases where explicit target is detected but formatting fails, but a general range formatting would succeed? Possibly, but unlikely. Might be okay.

• format_explicit_target uses format_fragment which re-parses the wrapped fragment. Wrapping might introduce syntax errors if fragment is malformed? Fragment is presumably valid Lua because it came from a node, but could be incomplete? Actually, the fragment is the exact text of a node (e.g., table expr {1,2,3}). Wrapping produces valid Lua (e.g., local __emmylua_range_target = {1,2,3}). So re-parsing should be fine. However, for CallArgList fragment like (a,b,c), wrapping as __emmylua_range_target(a,b,c) is valid Lua (function call with no arguments? Actually that's __emmylua_range_target(a,b,c) which is a function call with arguments a,b,c. That will parse correctly, and the LuaCallArgList node will be the arguments of that call. Extraction works. For ParamList fragment like (a,b,c), wrapping as local function __emmylua_range_target(a,b,c)\nend is valid. So good.

• extract_explicit_fragment expects to find exactly one node of the given kind. If there are multiple (e.g., nested table exprs in wrapper), it takes the first via .next(). That should be the outermost? For table expr wrapper, the only table expr is the one assigned. For call arg list, there might be other call arg lists inside if fragment contains nested calls? But fragment is the exact text of the call arg list, so inside it there could be other call arg lists (e.g., f(g())). The wrapper becomes __emmylua_range_target(f(g())). The call arg list of the wrapper is the outermost (f(g())). The inner g() has its own call arg list. If we take the first descendant, that would be outermost (since root is chunk, then call expr, then call arg list? Actually, we parse, get chunk, then descendants<LuaCallArgList>() returns in order of pre-order traversal. The outermost call arg list should come first. So it's correct. For param list, similarly.

• In format_fragment, it uses format_chunk which formats the entire chunk. That's fine.

• Potential performance: select_format_range calls layout::analyze_layout which may be heavy. But it's only done if no explicit target is found, and range formatting is already a special operation.

• Security: No obvious vulnerabilities. All inputs are strings from the user's document.

3. crates/emmylua_formatter/src/formatter/range_format/test.rs: Tests seem comprehensive. They test various scenarios: selection inside table, top-level, consistency between entrypoints, call args, param list, table field. Also test for the new handler's behavior (range edit expands to full statement lines). Good.

4. crates/emmylua_formatter/src/lib.rs: Adds pub use rowan::TextRange; and pub use formatter::range_format::RangeFormatOutput; and adds two public functions that delegate to the module. This exposes TextRange from rowan. That could be a dependency leak, but it's already used internally. It's okay.

5. crates/emmylua_ls/src/handlers/document_formatting/mod.rs: Refactors format_with_workspace_formatter into two functions, build_workspace_formatter_config and format_with_workspace_formatter (which now calls the new config builder and then formats). This is fine, promotes reuse.

6. crates/emmylua_ls/src/handlers/document_range_formatting/mod.rs: Major rewrite. The handler now uses reformat_range_in_chunk directly instead of formatting the entire document and filtering edits. This is a significant improvement in efficiency and correctness. However, there are some potential issues:

• The handler now expects a syntax tree from the analysis database (analysis.compilation.get_db().get_vfs().get_syntax_tree(&file_id)?). Previously it used LuaDocument's parsed tree? Actually, it used document.get_text() and then called format_with_workspace_formatter which parsed the text again. Now it gets the syntax tree from the database, which is good (no re-parsing). But note: The syntax tree might be from a different version? It should be up-to-date because the document is from the same VFS. However, the syntax_tree might be the one that was parsed with the current project config, not necessarily the same as what would be obtained by parsing the raw text. That's fine.

• The handler now directly returns a TextEdit with the replace range from RangeFormatOutput. Previously it used build_range_edits which filtered full edits to those intersecting requested lines and merged them. The new approach produces a single edit that replaces exactly the range that the formatter determined (either explicit target or expanded block). This is simpler and more correct. However, we must ensure that the replace_range is correct and that the text is properly normalized (newlines). The handler does if client_id.is_intellij() || client_id.is_other() { new_text = new_text.replace("\r\n", "\n"); }. That's okay.

• There is a fallback path: if the external tool range format returns something, it uses that. For the internal path, now it returns early with Some(vec![TextEdit ...]). But note: the variable let mut formatted_text = formatted_result.text; below the if-else block is only used if the external tool path was taken (the else block has a return). Actually, looking at the code: the if block (external tool) returns early? No, it doesn't return; the if block sets formatted_result and then continues to the common code after the if-else. The else block (internal) returns early with return Some(vec![...]);. So after the if-else, only the external tool path reaches the common code. That's fine.

• In the internal else block, syntax_tree.get_chunk_node() returns a LuaChunk which is a reference, not owned. But reformat_range_in_chunk expects &LuaChunk. That's fine. However, note that the syntax_tree is obtained from the VFS, which may be shared. That's okay.

• There is a potential issue: document.to_rowan_range(request_range)? converts LSP range to rowan range. This could fail if the range is invalid. Returns None? Actually, to_rowan_range likely returns Option<TextRange>. The ? operator will propagate None. Good.

• The test section: The old tests for build_range_edits and helper functions have been removed. New tests use build_range_edit which directly uses reformat_range_in_chunk. The tests seem to cover the new behavior. However, note: the test range_edit_expands_only_to_selected_statement_lines expects that selecting the first line (with character to end of line) results in an edit that replaces only the statement (line 0 to line 1). That matches the behavior of expand_to_full_lines and the block selection logic. Good.

• There is a test range_edit_keeps_full_multiline_expansion_when_selecting_block that selects the whole document and expects the edit to replace the entire range with the formatted text. That seems correct.

Potential logical issues:

• In the old handler, build_range_edits would filter full document edits to only those intersecting the requested range. The new handler replaces only the range determined by the formatter, which is intentionally expanded to full statement lines. This could change behavior: previously, if the user selected a range that didn't cover a full statement, the edits would be filtered to only that range (with partial line replacements). Now, the formatter expands the range to the enclosing block/statement. This is actually more aligned with typical IDE range formatting (e.g., VS Code's "Format Selection" expands to the containing statement). This seems like an improvement.

• However, there is a subtlety: the old handler used format_diff which produces minimal text edits for each changed line. The new handler replaces a large contiguous range with the formatted text. That is simpler, but might produce larger edits. It should still be correct as long as the range is correct.

• Also, the old handler had merge_replace_edits that combined consecutive deletions and insertions. The new handler does not need that.

• The new on_range_formatting_handler function's structure: it declares formatted_result outside the if-else, then uses it later. But in the internal else branch, it returns early, so formatted_result is not used after that branch. That's fine, but the variable is still declared and could be moved? Not a problem.

• One potential issue: In the internal else branch, the handler returns Some(vec![TextEdit ...]). But if reformat_range_in_chunk returns None, the ? operator will propagate None, causing the function to return None. That might be interpreted as no changes, which is correct.

• The handler now uses analysis.compilation.get_db().get_vfs().get_syntax_tree(&file_id) which requires analysis to be ServerAnalysis with access to the database. That's fine.

• The build_workspace_formatter_config function is now used in both formatting handlers, avoiding duplication. Good.

• In the tests for the range formatting module, build_range_edit creates a new parser instead of using the analysis's tree. That's okay for tests.

Potential missing edge cases:

• What if the selection is empty (cursor position)? The range formatting request can have zero-length range (cursor). In the old handler, it would filter edits intersecting that line. In the new handler, reformat_range_in_chunk will receive an empty range. clamp_range will keep it. select_explicit_format_target with empty range: find_smallest_containing filters nodes that contain the selection (empty range is contained if start and end are equal and within node). It should find the smallest containing node (e.g., a table expr if cursor is inside it). That's probably correct. select_format_range will find deepest block slice: an empty selection is contained in the current block/syntax node; it will expand to the enclosing statement. That's typical behavior. So empty selection should work.

• What if the selection spans multiple lines but not covering full statements? The formatter expands to full lines and then to statement boundaries. That's fine.

• The format_fragment function adds a capacity margin of 20%. That's heuristic, could be too small if formatted text expands significantly (e.g., inline table becomes multiline). But Printer::print likely handles dynamic growth if capacity is insufficient. It's a minor optimization.

• The code uses unwrap_or but never panics in expected paths. Good.

• Public API: reformat_range and reformat_range_in_chunk are exported. Their signatures seem appropriate.

• The RangeFormatOutput struct is public. Good.

Now, list issues and recommendations in a concise manner.

Issues:

1. The reformat_range_in_chunk function early-returns None if explicit target formatting fails, even if a fallback to select_format_range might succeed. This could reject valid formatting scenarios where explicit target detection is overly eager but formatting fails.
2. In extract_explicit_fragment, using .next() on descendants assumes the first descendant of the target kind is the intended one. For nested structures, this might pick the wrong node (e.g., a nested call arg list inside a call arg list). However, due to wrapping, it's likely the outermost. But it's fragile.
3. The format_fragment function uses a fixed 20% capacity increase, which could cause reallocation if expansion is larger. Not critical but could be more precise.
4. The new range formatting handler in LSP replaces the entire expanded range with formatted text. This could be a large edit, but it's functionally correct. However, it may be less efficient than the previous diff-based approach for small selections within large files. Acceptable trade-off.
5. The test range_edit_expands_only_to_selected_statement_lines selects the first line up to its end, but expects replacement to include newline of that line and nothing else. The formatted text is "local a = 1\n" which includes newline. That's fine.
6. The build_range_edit in tests uses LuaLanguageLevel::Lua55 hardcoded, while the actual handler uses the language level from emmyrc. This might cause inconsistency in tests if the level matters. For these tests, it's probably fine.
7. In select_explicit_format_target, the order of checks is table expr, call arg list, param list. The pick_better_target is used to choose the smallest. However, if the selection is inside a table expr that is also inside a call arg list, the table expr might be smaller and win. That seems correct for typical usage. No issue.

Minor style issues: None that stand out.

Recommendations:

• Consider handling the case where explicit target formatting fails by falling back to select_format_range. Alternatively, ensure explicit target detection is always correct.
• In extract_explicit_fragment, consider using find_ancestors_or_self or more precise selection to avoid ambiguity, though current approach likely works.
• The capacity in format_fragment could be calculated based on worst-case expansion (e.g., 2x) or simply let Printer grow.
• Ensure that when range selection includes the final newline, the formatted output also ends with newline (the code already handles insert_final_newline).

No security vulnerabilities identified.

Recommendation: The changes are well-structured and appear correct. The new range formatting approach is cleaner and more efficient by avoiding formatting the entire document. The main potential improvement is the fallback behavior if explicit target formatting fails.

Code Review Summary

The PR introduces a new range formatting module for the EmmyLua formatter, replacing the previous approach of formatting the entire document and filtering edits. The changes are well-structured and improve efficiency, but a few areas deserve attention.

---

Potential Issues

1. In reformat_range_in_chunk:
   If an explicit format target is found (e.g., table expression, call argument list, parameter list) but format_explicit_target fails (returns None), the function returns None early, discarding any possible fallback to the general block-expansion path. This could cause unexpected failures for legitimate selections that happen to match an explicit target but fail to wrap/parse (e.g., due to edge cases in wrapping).
   Recommendation: If format_explicit_target fails, fall back to select_format_range instead of returning None immediately.

2. Fragile node extraction in extract_explicit_fragment:
   The function uses .next() on descendants() to retrieve the first node of the expected kind. For nested structures (e.g., a call argument list containing another call argument list inside a function call), the first descendant might not be the intended outer node. While the wrapping strategy makes this unlikely, it is still a potential hidden bug if the wrapped code contains multiple nodes of the same kind.
   Recommendation: Use a more precise method to locate the target node, such as finding the child of the wrapper expression at a fixed depth (e.g., the direct argument of the assignment in local __emmylua_range_target = <expr>). Alternatively, document the assumption and add a test for nested cases.

3. Hardcoded capacity heuristic in format_fragment:
   printer.with_capacity(capacity) uses (fragment.len() as f64 * 1.2).ceil(). If the formatted output expands significantly (e.g., an inline table becomes multiline), the buffer may still require reallocation. This is a minor performance nit, but the heuristic could be more robust.
   Recommendation: Either remove the capacity hint and let Printer manage growth dynamically, or use a larger scaling factor (e.g., 1.5x).

4. Test uses hardcoded LuaLanguageLevel::Lua55:
   The test build_range_edit in document_range_formatting/mod.rs hardcodes language level Lua55, while the actual handler retrieves the level from the project configuration (emmyrc). If the formatter behaves differently across language versions, tests may not reflect real-world behavior.
   Recommendation: Use the same level as the actual context (e.g., parse with the same language level as the document).

5. Public export of rowan::TextRange:
   The addition of pub use rowan::TextRange; in lib.rs re-exports a third-party type from the crate's public API. While convenient for consumers, it couples the public API to rowan’s version and could cause conflicts.
   Recommendation: Consider wrapping TextRange in a newtype or re-exporting only the necessary types through an internal module to reduce dependency leakage.

---

Positive Highlights

• Efficiency: The new approach only parses and formats the relevant code fragment instead of the entire document, making range formatting much faster for large files.
• Correctness: Explicit target wrapping (for tables, call arguments, parameter lists) ensures formatting context (e.g., indentation, commas) is preserved.
• Clean API: The refactored build_workspace_formatter_config eliminates duplicate config logic between full and range formatting.
• Test Coverage: New tests cover explicit targets, block expansion, and consistency between entry points.

---

Minor Observations

• The line_height function (not shown) is unused? No, it's not present in the diff.
• Edge case: If the selection is exactly a single newline character, select_format_range may return a range covering an empty line, and expand_to_full_lines will expand to the surrounding lines, which is acceptable.

No security vulnerabilities were identified. The changes are a solid improvement to the formatter’s capabilities.

[gemini-code-assist]

Code Review

This pull request introduces a dedicated range formatting implementation for Lua. It adds the range_format module to emmylua_formatter, which supports formatting specific AST nodes like tables and parameter lists by wrapping them in a temporary context, as well as formatting arbitrary selections by expanding them to full lines. The emmylua_ls range formatting handler is refactored to utilize this new logic, replacing the previous diff-based approach. Review feedback highlights an opportunity to optimize node lookups using covering_element and identifies a potential indentation issue when formatting multi-line sub-expressions that are not at the start of a line.

[gemini-code-assist]

When formatting an explicit target (like a TableExpr or ParamList) that spans multiple lines, the resulting text may have incorrect indentation if the target is not at the start of the line. The base indentation of the line where the target starts should be calculated and applied to all subsequent lines of the formatted output to maintain correct alignment.

Suggested change

	if let Some(target) = select_explicit_format_target(chunk, selection) {
	return format_explicit_target(source_text, target, level, config);
	}
	if let Some(target) = select_explicit_format_target(chunk, selection) {
	let line_start = line_start_offset(source_text, usize::from(target.replace_range.start()));
	let indent_prefix = line_indent_prefix(source_text, TextSize::new(line_start as u32));
	return format_explicit_target(source_text, target, level, config, &indent_prefix);
	}

[gemini-code-assist]

Update format_explicit_target to accept an indent_prefix and apply it to all lines of the formatted output except the first one. This ensures that multi-line sub-expressions are correctly indented relative to their starting line in the original source.

fn format_explicit_target(
    source_text: &str,
    target: ExplicitFormatTarget,
    level: LuaLanguageLevel,
    config: &LuaFormatConfig,
    indent_prefix: &str,
) -> Option<RangeFormatOutput> {
    let fragment = &source_text
        [usize::from(target.replace_range.start())..usize::from(target.replace_range.end())];
    if fragment.trim().is_empty() {
        return None;
    }

    let wrapped_fragment = wrap_explicit_fragment(target.kind, fragment);
    let formatted_wrapper = format_fragment(&wrapped_fragment, level, config)?;
    let extracted = extract_explicit_fragment(&formatted_wrapper, target.kind, level)?;

    let mut text = String::new();
    for (i, line) in extracted.split_inclusive('\n').enumerate() {
        let (content, newline) = split_line_ending(line);
        if i > 0 && !content.is_empty() {
            text.push_str(indent_prefix);
        }
        text.push_str(content);
        text.push_str(newline);
    }

    Some(RangeFormatOutput {
        replace_range: target.replace_range,
        text,
    })
}

[gemini-code-assist]

The current implementation of find_smallest_containing is inefficient for large files as it iterates over all descendants of the root node ($O(N)$). Since you have the selection range, you can use covering_element to jump directly to the smallest element containing the range and then traverse upwards through its ancestors ($O(\text{depth})$).

fn find_smallest_containing<N: LuaAstNode>(
    root: &LuaSyntaxNode,
    selection: TextRange,
) -> Option<N> {
    root.covering_element(selection)
        .ancestors()
        .find_map(N::cast)
}

[CppCXY]

/review

[CppCXY]

/review

[CppCXY]

/review

[CppCXY]

/review

[CppCXY]

/review

[github-actions]

Code Review Summary

This PR introduces range formatting support for the EmmyLua formatter. Here are the key findings:

✅ Positive Aspects

• Well-structured code with clear separation of concerns
• Good test coverage for various edge cases
• Proper handling of different format targets (table expressions, call args, param lists)
• Correct newline normalization for different clients

⚠️ Issues & Recommendations

1. Potential Performance Concern (Medium)

• File: crates/emmylua_formatter/src/formatter/range_format/mod.rs
• Issue: find_smallest_containing traverses all descendants multiple times for different node types
• Suggestion: Consider caching the descendant traversal or combining the searches:

fn select_explicit_format_target(
    chunk: &LuaChunk,
    selection: TextRange,
) -> Option<ExplicitFormatTarget> {
    let root = chunk.syntax();
    let mut best = None;
    
    // Collect all candidates in one pass
    for node in root.descendants() {
        // Check each type and update best
    }
    best
}

2. Unused Import (Minor)

• File: crates/emmylua_ls/src/handlers/document_range_formatting/mod.rs
• Issue: use std::cmp::Reverse; is imported but only used in test code
• Suggestion: Move the import inside the test module or remove if not needed

3. Error Handling Gap (Medium)

• File: crates/emmylua_ls/src/handlers/document_range_formatting/mod.rs
• Issue: The ? operator on document.to_rowan_range(request_range)? could panic if the range is invalid
• Suggestion: Add proper error handling:

let selection = match document.to_rowan_range(request_range) {
    Some(range) => range,
    None => return None,
};

4. Redundant Clone (Minor)

• File: crates/emmylua_formatter/src/formatter/range_format/mod.rs
• Issue: config.clone() is unnecessary if insert_final_newline is the only field being modified
• Suggestion: Consider making insert_final_newline a parameter instead of cloning the entire config

5. Test Coverage Gap (Low)

• File: crates/emmylua_formatter/src/formatter/range_format/test.rs
• Issue: No tests for edge cases like:
  • Empty selections
  • Selections at document boundaries
  • Selections with syntax errors
  • Very large files

🔧 Overall Assessment

The implementation is solid and well-tested. The main concerns are around performance optimization and error handling robustness. The code follows good Rust practices with proper use of Option and error propagation.