Obey precedence rules in which; Fix #2875 (#2885)

* Obay precedence rules in which; Fix #2875

Before which did not obay the precedence of alias before def commands.
Furthermore, `which -a echo` would only report either an alias or a def command or an
internal command with the provided name. Not all.

With this commit applied its fixed :)

Example:
```shell
/home/leo/repos/nushell(fix/which_reports_wrong_usage)> def echo [] {^echo hi}
/home/leo/repos/nushell(fix/which_reports_wrong_usage)> echo
hi
/home/leo/repos/nushell(fix/which_reports_wrong_usage)> which -a echo
───┬──────┬──────────────────────────┬─────────
 # │ arg  │           path           │ builtin
───┼──────┼──────────────────────────┼─────────
 0 │ echo │ Nushell custom command   │ No
 1 │ echo │ Nushell built-in command │ Yes
 2 │ echo │ /usr/bin/echo            │ No
───┴──────┴──────────────────────────┴─────────
/home/leo/repos/nushell(fix/which_reports_wrong_usage)> alias echo = ^echo hi there
/home/leo/repos/nushell(fix/which_reports_wrong_usage)> echo
hi there
/home/leo/repos/nushell(fix/which_reports_wrong_usage)> which -a echo
───┬──────┬──────────────────────────┬─────────
 # │ arg  │           path           │ builtin
───┼──────┼──────────────────────────┼─────────
 0 │ echo │ Nushell alias            │ No
 1 │ echo │ Nushell custom command   │ No
 2 │ echo │ Nushell built-in command │ Yes
 3 │ echo │ /usr/bin/echo            │ No
───┴──────┴──────────────────────────┴─────────
```

* Fix clippy lint

* Fix vec always Some even if empty

crates/nu-cli/src/commands/which_.rs

@@ -1,6 +1,7 @@
 use crate::commands::WholeStreamCommand;
 use crate::prelude::*;
 use indexmap::map::IndexMap;
+use log::trace;
 use nu_errors::ShellError;
 use nu_protocol::{Primitive, ReturnSuccess, Signature, SyntaxShape, UntaggedValue, Value};
 use nu_source::Tagged;
@@ -55,6 +56,59 @@ macro_rules! create_entry {
     };
 }
 
+fn get_entries_in_aliases(scope: &Scope, name: &str, tag: Tag) -> Vec<Value> {
+    let aliases = scope
+        .get_aliases_with_name(name)
+        .unwrap_or_default()
+        .into_iter()
+        .map(|_| create_entry!(name, "Nushell alias", tag.clone(), false))
+        .collect::<Vec<_>>();
+    trace!("Found {} aliases", aliases.len());
+    aliases
+}
+
+fn get_entries_in_custom_command(scope: &Scope, name: &str, tag: Tag) -> Vec<Value> {
+    scope
+        .get_custom_commands_with_name(name)
+        .unwrap_or_default()
+        .into_iter()
+        .map(|_| create_entry!(name, "Nushell custom command", tag.clone(), false))
+        .collect()
+}
+
+fn get_entry_in_commands(scope: &Scope, name: &str, tag: Tag) -> Option<Value> {
+    if scope.has_command(name) {
+        Some(create_entry!(name, "Nushell built-in command", tag, true))
+    } else {
+        None
+    }
+}
+
+fn get_entries_in_nu(
+    scope: &Scope,
+    name: &str,
+    tag: Tag,
+    skip_after_first_found: bool,
+) -> Vec<Value> {
+    let mut all_entries = vec![];
+
+    all_entries.extend(get_entries_in_aliases(scope, name, tag.clone()));
+    if !all_entries.is_empty() && skip_after_first_found {
+        return all_entries;
+    }
+
+    all_entries.extend(get_entries_in_custom_command(scope, name, tag.clone()));
+    if !all_entries.is_empty() && skip_after_first_found {
+        return all_entries;
+    }
+
+    if let Some(entry) = get_entry_in_commands(scope, name, tag) {
+        all_entries.push(entry);
+    }
+
+    all_entries
+}
+
 #[allow(unused)]
 macro_rules! entry_path {
     ($arg:expr, $path:expr, $tag:expr) => {
@@ -67,6 +121,34 @@ macro_rules! entry_path {
     };
 }
 
+#[cfg(feature = "ichwh")]
+async fn get_first_entry_in_path(item: &str, tag: Tag) -> Option<Value> {
+    ichwh::which(item)
+        .await
+        .unwrap_or(None)
+        .map(|path| entry_path!(item, path.into(), tag))
+}
+
+#[cfg(not(feature = "ichwh"))]
+async fn get_first_entry_in_path(_: &str, _: Tag) -> Option<Value> {
+    None
+}
+
+#[cfg(feature = "ichwh")]
+async fn get_all_entries_in_path(item: &str, tag: Tag) -> Vec<Value> {
+    ichwh::which_all(&item)
+        .await
+        .unwrap_or_default()
+        .into_iter()
+        .map(|path| entry_path!(item, path.into(), tag.clone()))
+        .collect()
+}
+
+#[cfg(not(feature = "ichwh"))]
+async fn get_all_entries_in_path(_: &str, _: Tag) -> Vec<Value> {
+    vec![]
+}
+
 #[derive(Deserialize, Debug)]
 struct WhichArgs {
     application: Tagged<String>,
@@ -74,52 +156,53 @@ struct WhichArgs {
 }
 
 async fn which(args: CommandArgs) -> Result<OutputStream, ShellError> {
-    let mut output = vec![];
     let scope = args.scope.clone();
 
     let (WhichArgs { application, all }, _) = args.process().await?;
-    let external = application.starts_with('^');
-    let item = if external {
-        application.item[1..].to_string()
+
+    let (external, prog_name) = if application.starts_with('^') {
+        (true, application.item[1..].to_string())
     } else {
-        application.item.clone()
+        (false, application.item.clone())
     };
-    if !external {
-        if let Some(entry) = entry_for(&scope, &item, application.tag.clone()) {
-            output.push(ReturnSuccess::value(entry));
-        }
-    }
 
-    #[cfg(feature = "ichwh")]
+    let mut output = vec![];
+
+    //If prog_name is an external command, don't search for nu-specific programs
+    //If all is false, we can save some time by only searching for the first matching
+    //program
+    //This match handles all different cases
+    match (all, external) {
+        (true, true) => {
+            output.extend(get_all_entries_in_path(&prog_name, application.tag.clone()).await);
+        }
+        (true, false) => {
+            output.extend(get_entries_in_nu(
+                &scope,
+                &prog_name,
+                application.tag.clone(),
+                false,
+            ));
+            output.extend(get_all_entries_in_path(&prog_name, application.tag.clone()).await);
+        }
+        (false, true) => {
+            if let Some(entry) = get_first_entry_in_path(&prog_name, application.tag.clone()).await
             {
-        if let Ok(paths) = ichwh::which_all(&item).await {
-            for path in paths {
-                output.push(ReturnSuccess::value(entry_path!(
-                    item,
-                    path.into(),
-                    application.tag.clone()
-                )));
+                output.push(entry);
+            }
+        }
+        (false, false) => {
+            let nu_entries = get_entries_in_nu(&scope, &prog_name, application.tag.clone(), true);
+            if !nu_entries.is_empty() {
+                output.push(nu_entries[0].clone());
+            } else if let Some(entry) =
+                get_first_entry_in_path(&prog_name, application.tag.clone()).await
+            {
+                output.push(entry);
             }
         }
     }
-
-    if all {
-        Ok(futures::stream::iter(output.into_iter()).to_output_stream())
-    } else {
-        Ok(futures::stream::iter(output.into_iter().take(1)).to_output_stream())
-    }
-}
-
-fn entry_for(scope: &Scope, name: &str, tag: Tag) -> Option<Value> {
-    if scope.has_custom_command(name) {
-        Some(create_entry!(name, "Nushell custom command", tag, false))
-    } else if scope.has_command(name) {
-        Some(create_entry!(name, "Nushell built-in command", tag, true))
-    } else if scope.has_alias(name) {
-        Some(create_entry!(name, "Nushell alias", tag, false))
-    } else {
-        None
-    }
+    Ok(futures::stream::iter(output.into_iter().map(ReturnSuccess::value)).to_output_stream())
 }
 
 #[cfg(test)]

crates/nu-cli/src/evaluate/scope.rs

@@ -31,6 +31,37 @@ impl Scope {
         None
     }
 
+    pub fn get_aliases_with_name(&self, name: &str) -> Option<Vec<Vec<Spanned<String>>>> {
+        let aliases: Vec<_> = self
+            .frames
+            .lock()
+            .iter()
+            .rev()
+            .filter_map(|frame| frame.aliases.get(name).cloned())
+            .collect();
+        if aliases.is_empty() {
+            None
+        } else {
+            Some(aliases)
+        }
+    }
+
+    pub fn get_custom_commands_with_name(&self, name: &str) -> Option<Vec<Block>> {
+        let custom_commands: Vec<_> = self
+            .frames
+            .lock()
+            .iter()
+            .rev()
+            .filter_map(|frame| frame.custom_commands.get(name).cloned())
+            .collect();
+
+        if custom_commands.is_empty() {
+            None
+        } else {
+            Some(custom_commands)
+        }
+    }
+
     pub fn add_command(&self, name: String, command: Command) {
         // Note: this is assumed to always be true, as there is always a global top frame
         if let Some(frame) = self.frames.lock().last_mut() {

crates/nu-cli/tests/commands/mod.rs

@@ -56,5 +56,6 @@ mod touch;
 mod uniq;
 mod update;
 mod where_;
+mod which;
 mod with_env;
 mod wrap;

crates/nu-cli/tests/commands/which.rs

@@ -0,0 +1,96 @@
+use nu_test_support::nu;
+
+#[test]
+fn which_ls() {
+    let actual = nu!(
+        cwd: ".",
+        "which ls | get path | str trim"
+    );
+
+    assert_eq!(actual.out, "Nushell built-in command");
+}
+
+#[test]
+fn which_alias_ls() {
+    let actual = nu!(
+        cwd: ".",
+        "alias ls = ls -a; which ls | get path | str trim"
+    );
+
+    assert_eq!(actual.out, "Nushell alias");
+}
+
+#[test]
+fn which_def_ls() {
+    let actual = nu!(
+        cwd: ".",
+        "def ls [] {echo def}; which ls | get path | str trim"
+    );
+
+    assert_eq!(actual.out, "Nushell custom command");
+}
+
+#[test]
+fn correct_precedence_alias_def_custom() {
+    let actual = nu!(
+        cwd: ".",
+        "def ls [] {echo def}; alias ls = echo alias; which ls | get path | str trim"
+    );
+
+    assert_eq!(actual.out, "Nushell alias");
+}
+
+#[test]
+fn multiple_reports_for_alias_def_custom() {
+    let actual = nu!(
+        cwd: ".",
+        "def ls [] {echo def}; alias ls = echo alias; which -a ls | count"
+    );
+
+    let count: i32 = actual.out.parse().unwrap();
+    assert!(count >= 3);
+}
+
+// `get_aliases_with_name` and `get_custom_commands_with_name` don't return the correct count of
+// values
+// I suspect this is due to the ScopeFrame getting discarded at '}' and the command is then
+// executed in the parent scope
+// See: parse_definition, line 2187 for reference.
+#[ignore]
+#[test]
+fn multiple_reports_of_multiple_alias() {
+    let actual = nu!(
+        cwd: ".",
+        "alias xaz = echo alias1; def helper [] {alias xaz = echo alias2; which -a xaz}; helper | count"
+    );
+
+    let count: i32 = actual.out.parse().unwrap();
+    assert!(count == 2);
+}
+
+#[ignore]
+#[test]
+fn multiple_reports_of_multiple_defs() {
+    let actual = nu!(
+        cwd: ".",
+        "def xaz [] {echo def1}; def helper [] { def xaz [] { echo def2 }; which -a xaz }; helper | count"
+    );
+
+    let count: i32 = actual.out.parse().unwrap();
+    assert!(count == 2);
+}
+
+//Fails due to ParserScope::add_definition
+// frame.custom_commands.insert(name.clone(), block.clone());
+// frame.commands.insert(name, whole_stream_command(block));
+#[ignore]
+#[test]
+fn def_only_seen_once() {
+    let actual = nu!(
+        cwd: ".",
+        "def xaz [] {echo def1}; which -a xaz | count"
+    );
+    //count is 2. One custom_command (def) one built in ("wrongly" added)
+    let count: i32 = actual.out.parse().unwrap();
+    assert!(count == 1);
+}