This pr is ready. Is it possible to get it merged? Is there anything holding it back?

Hi @tmillr , I really want to see those bug fixed in the next release.

Just current whole implementation of completion_helper bugs me a lot, I'd like to make an overhaul of it.
Maybe we can work together for this.

several problems that I noticed while checking the existing code, I think you can help to confirm:

1. all pipeline elements before the position is flattened and all its span items got iterated over. Why not just skip irrelevant ones?
2. mixed logic of internal/external completion, which is actually quite separable by the Call/ExternalCall enum type of Expr.
3. weird previous_expr stuff, which should be Call.head.
4. and the nested block issue addressed in this PR. Ideally this should be addressed with issue No 1. together in a single AST scan with sublinear complexity.
5. other minor issues, like custom completion should be checked first so that can override any default behavior.

Let's involve some core members, @fdncred @132ikl @ysthakur

Hi there! I’m currently testing Deepseek for PR reviews. Here’s the review result for your reference—let me know if it’s helpful!

Code Review

Issues Identified:

1. Debug Logging:

   • The debug! macro is used to log the prefix, new_span, offset, and pos. However, the log message is not very descriptive and could be improved for better debugging clarity.
   • Recommendation: Consider adding more context to the debug log, such as the function name or a brief description of what is being logged.

2. Redundant Code:

   • The code block that filters and reduces the blocks to find the target block is somewhat complex and could be simplified.
   • Recommendation: Consider refactoring the block filtering logic to make it more readable and maintainable. For example, you could extract this logic into a separate function.

3. Unnecessary Conversion:

   • The conversion of current_span to a Vec<u8> and then back to a string slice is unnecessary.
   • Recommendation: Directly use current_span as a slice without converting it to a Vec<u8>.

4. Potential Performance Issue:

   • The reduce operation on the blocks could be expensive if there are many blocks. This could potentially impact performance.
   • Recommendation: Consider optimizing the block filtering logic or adding a limit to the number of blocks processed.

5. Code Duplication in Tests:

   • The test cases nested_block, incomplete_nested_block, and deeply_nested_block have similar structures and expected results.
   • Recommendation: Consider creating a helper function to reduce code duplication in the test cases.

Recommendations:

1. Improve Debug Logging:

   debug!(
       "process_completion: prefix: '{}', new_span: {:?}, offset: {}, pos: {}",
       String::from_utf8_lossy(prefix),
       new_span,
       offset,
       pos
   );

2. Refactor Block Filtering Logic:

   fn find_target_block(working_set: &mut WorkingSet, pos: usize, outermost_block: &Block) -> &Block {
       working_set
           .delta
           .blocks
           .iter()
           .filter_map(|block| block.span.map(|span| (block, span)))
           .filter(|(_, span)| span.contains(pos))
           .reduce(|prev, cur| {
               match cur.1.start.cmp(&prev.1.start) {
                   core::cmp::Ordering::Greater => cur,
                   core::cmp::Ordering::Equal if cur.1.end < prev.1.end => cur,
                   _ => prev,
               }
           })
           .map(|(block, _)| block)
           .unwrap_or(outermost_block)
   }

3. Avoid Unnecessary Conversion:

   let current_span_str = String::from_utf8_lossy(current_span);

4. Optimize Block Filtering:

   • Consider adding a limit to the number of blocks processed or optimizing the filtering logic.

5. Reduce Test Code Duplication:

   fn test_nested_block(completer: &mut NuCompleter, input: &str, pos: usize) {
       let expected: Vec<String> = vec!["--help".into(), "--mod".into(), "-h".into(), "-s".into()];
       let suggestions = completer.complete(input, pos);
       match_suggestions(&expected, &suggestions);
   }
   
   #[rstest]
   fn nested_block(mut completer: NuCompleter) {
       test_nested_block(&mut completer, "somecmd | lines | each { tst - }", 30);
       test_nested_block(&mut completer, "somecmd | lines | each { tst -}", 30);
   }
   
   #[rstest]
   fn incomplete_nested_block(mut completer: NuCompleter) {
       test_nested_block(&mut completer, "somecmd | lines | each { tst -", 30);
   }
   
   #[rstest]
   fn deeply_nested_block(mut completer: NuCompleter) {
       test_nested_block(&mut completer, "somecmd | lines | each { print ([each (print) (tst -)]) }", 52);
   }

These changes should improve the readability, maintainability, and performance of the code.

Looks good to me, let's do it. Thanks for the fix!

Also, thanks for the ping @blindFS, glad to get this in before the release. Feel free to overhaul this section of code in a future PR if you think you could improve it.