Add column completion support

[nibon7]

Description

column completion for get or select to complete column name from previous input.

Closes #6697

• record

• echo

• mutiple expressions

• variable

• string interpolation

• external command

• command has side effect

Tests

Make sure you've done the following:

• [x] Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
• [ ] Try to think about corner cases and various ways how your changes could break. Cover them with tests.
• [ ] If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

• [x] cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
• [x] cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• [x] cargo test --workspace --features=extra to check that all the tests pass

Documentation

• [x] If your PR touches a user-facing nushell feature then make sure that there is an entry in the documentation (https://github.com/nushell/nushell.github.io) for the feature, and update it if necessary.

[merelymyself]

I'm seeing a problem with this - it seems to run the external command when completion is attempted. Try something like htop | get <tab> to see what I mean. With commands like top, the screen turns into a confused mess.

[nibon7]

Thanks, I'll fix it.

[sophiajt]

For completions, I don't think we want to have any side effects in the expressions. Something like this:

do { rm foo.txt; ls } | get <tab>

Shouldn't rm anything. We could probably do something like what you're doing but remove all the commands, like cp, rm, etc that would change the system in some way if they run. That's relatively easy to do, if you create a new version of create_default_context that removes all the commands that cause a side effect.

[nibon7]

don't think we want to have any side effects in the expressions

Thanks for pointing it out, I didn't realized it :joy:

if you create a new version of create_default_context that removes all the commands that cause a side effect.

Or could we tweak Command trait to add a function like has_side_effect, and then we can filter out the command? :) @jntrnr

diff --git a/crates/nu-cli/src/completions/column_completions.rs b/crates/nu-cli/src/completions/column_completions.rs
index df2c9fe4f..6696d5418 100644
--- a/crates/nu-cli/src/completions/column_completions.rs
+++ b/crates/nu-cli/src/completions/column_completions.rs
@@ -30,7 +30,7 @@ impl ColumnCompletion {
 impl Completer for ColumnCompletion {
     fn fetch(
         &mut self,
-        _working_set: &StateWorkingSet,
+        working_set: &StateWorkingSet,
         prefix: Vec<u8>,
         span: Span,
         offset: usize,
@@ -48,9 +48,19 @@ impl Completer for ColumnCompletion {
                 break;
             }
 
-            // Don't evaluate external call
-            if matches!(expr.expr, Expr::ExternalCall(..)) {
-                return vec![];
+            match &expr.expr {
+                // Don't evaluate external call
+                Expr::ExternalCall(..) => return vec![],
+                // Don't evaluate call that has side effect
+                Expr::Call(call) => {
+                    let decl_id = call.decl_id;
+                    let decl = working_set.get_decl(decl_id);
+                    if decl.has_side_effect() {
+                        return vec![];
+                    }
+                }
+
+                _ => (),
             }
 
             // Evaluate first expression without input
diff --git a/crates/nu-command/src/filesystem/rm.rs b/crates/nu-command/src/filesystem/rm.rs
index c3533e799..820c6e45c 100644
--- a/crates/nu-command/src/filesystem/rm.rs
+++ b/crates/nu-command/src/filesystem/rm.rs
@@ -77,6 +77,10 @@ impl Command for Rm {
             .category(Category::FileSystem)
     }
 
+    fn has_side_effect(&self) -> bool {
+        true
+    }
+
     fn run(
         &self,
         engine_state: &EngineState,
diff --git a/crates/nu-protocol/src/engine/command.rs b/crates/nu-protocol/src/engine/command.rs
index 0fa1e2279..11fff1952 100644
--- a/crates/nu-protocol/src/engine/command.rs
+++ b/crates/nu-protocol/src/engine/command.rs
@@ -71,6 +71,11 @@ pub trait Command: Send + Sync + CommandClone {
     fn search_terms(&self) -> Vec<&str> {
         vec![]
     }
+
+    // Whether the command has side effect, e.g. changing the system
+    fn has_side_effect(&self) -> bool {
+        false
+    }
 }
 
 pub trait CommandClone {

[nibon7]

~~Seems there are at least two problems:~~ ~~1.get/select completion may have side effects~~ ~~2. expressions which contain Block like do {} or each {} will panic due to an unknown block id~~

[kubouch]

Honestly, I don't feel comfortable landing this because you're evaluating the previous pipeline which, apart from side effects like rm, could be also something that takes a long time to run. You included some filtering but it could be bypassed using, for example, each { ... } and putting random code in that block. IMO, the benefits of having column completions is not worth the cost of maintaining bullet-proof expression filtering that allows only "safe" and "short-duration" commands.

[sholderbach]

Good point about unpredictable runtime by @kubouch. Definitely props to @nibon7 for giving this a shot. I think to have acceptable safety and speed we probably have to do this bottom-up instead of top-down to bring in piece by piece completions that are benign.

[sophiajt]

Thanks for the effort. Sounds like the next step is to try this again as a bottom-up approach, where commands are selected one-by-one to participate in completions.