Fix reporting of which and $nu.scope (#4836)

* Refactor & fix which

Instead of fetching all definitions / aliases, only show the one that is
visible.

* Fix $nu.scope to show only visible definitions

* Add missing tests file; Rename one which test

crates/nu-command/src/system/which_.rs

@@ -51,8 +51,8 @@ impl Command for Which {
     }
 }
 
-/// Shortcuts for creating an entry to the output table
+// Shortcut for creating an entry to the output table
-fn entry(arg: impl Into<String>, path: Value, builtin: bool, span: Span) -> Value {
+fn entry(arg: impl Into<String>, path: impl Into<String>, builtin: bool, span: Span) -> Value {
     let mut cols = vec![];
     let mut vals = vec![];
 
@@ -60,56 +60,46 @@ fn entry(arg: impl Into<String>, path: Value, builtin: bool, span: Span) -> Valu
     vals.push(Value::string(arg.into(), span));
 
     cols.push("path".to_string());
-    vals.push(path);
+    vals.push(Value::string(path.into(), span));
 
-    cols.push("builtin".to_string());
+    cols.push("built-in".to_string());
     vals.push(Value::Bool { val: builtin, span });
 
     Value::Record { cols, vals, span }
 }
 
-macro_rules! create_entry {
+fn get_entry_in_aliases(engine_state: &EngineState, name: &str, span: Span) -> Option<Value> {
-    ($arg:expr, $path:expr, $span:expr, $is_builtin:expr) => {
+    if let Some(alias_id) = engine_state.find_alias(name.as_bytes()) {
-        entry(
+        let alias = engine_state.get_alias(alias_id);
-            $arg.clone(),
+        let alias_str = alias
-            Value::string($path.to_string(), $span),
+            .iter()
-            $is_builtin,
+            .map(|alias_span| String::from_utf8_lossy(engine_state.get_span_contents(alias_span)))
-            $span,
+            .join(" ");
-        )
-    };
-}
 
-fn get_entries_in_aliases(engine_state: &EngineState, name: &str, span: Span) -> Vec<Value> {
+        trace!("Found alias: {}", name);
-    let aliases = engine_state.find_aliases(name);
 
-    let aliases = aliases
+        Some(entry(
-        .into_iter()
+            name,
-        .map(|spans| {
+            format!("Nushell alias: {}", alias_str),
-            spans
+            false,
-                .iter()
+            span,
-                .map(|span| {
+        ))
-                    String::from_utf8_lossy(engine_state.get_span_contents(span)).to_string()
+    } else {
-                })
+        None
-                .join(" ")
+    }
-        })
-        .map(|alias| create_entry!(name, format!("Nushell alias: {}", alias), span, false))
-        .collect::<Vec<_>>();
-    trace!("Found {} aliases", aliases.len());
-    aliases
-}
-
-fn get_entries_in_custom_command(engine_state: &EngineState, name: &str, span: Span) -> Vec<Value> {
-    let custom_commands = engine_state.find_custom_commands(name);
-
-    custom_commands
-        .into_iter()
-        .map(|_| create_entry!(name, "Nushell custom command", span, false))
-        .collect::<Vec<_>>()
 }
 
 fn get_entry_in_commands(engine_state: &EngineState, name: &str, span: Span) -> Option<Value> {
-    if engine_state.find_decl(name.as_bytes()).is_some() {
+    if let Some(decl_id) = engine_state.find_decl(name.as_bytes()) {
-        Some(create_entry!(name, "Nushell built-in command", span, true))
+        let (msg, is_builtin) = if engine_state.get_decl(decl_id).get_block_id().is_some() {
+            ("Nushell custom command", false)
+        } else {
+            ("Nushell built-in command", true)
+        };
+
+        trace!("Found command: {}", name);
+
+        Some(entry(name, msg, is_builtin, span))
     } else {
         None
     }
@@ -123,30 +113,21 @@ fn get_entries_in_nu(
 ) -> Vec<Value> {
     let mut all_entries = vec![];
 
-    all_entries.extend(get_entries_in_aliases(engine_state, name, span));
+    if let Some(ent) = get_entry_in_aliases(engine_state, name, span) {
+        all_entries.push(ent);
+    }
+
     if !all_entries.is_empty() && skip_after_first_found {
         return all_entries;
     }
 
-    all_entries.extend(get_entries_in_custom_command(engine_state, name, span));
+    if let Some(ent) = get_entry_in_commands(engine_state, name, span) {
-    if !all_entries.is_empty() && skip_after_first_found {
+        all_entries.push(ent);
-        return all_entries;
-    }
-
-    if let Some(entry) = get_entry_in_commands(engine_state, name, span) {
-        all_entries.push(entry);
     }
 
     all_entries
 }
 
-#[allow(unused)]
-macro_rules! entry_path {
-    ($arg:expr, $path:expr, $span:expr) => {
-        entry($arg.clone(), Value::string($path, $span), false, $span)
-    };
-}
-
 #[cfg(feature = "which")]
 fn get_first_entry_in_path(
     item: &str,
@@ -155,7 +136,7 @@ fn get_first_entry_in_path(
     paths: impl AsRef<OsStr>,
 ) -> Option<Value> {
     which::which_in(item, Some(paths), cwd)
-        .map(|path| entry_path!(item, path.to_string_lossy().to_string(), span))
+        .map(|path| entry(item, path.to_string_lossy().to_string(), false, span))
         .ok()
 }
 
@@ -178,11 +159,12 @@ fn get_all_entries_in_path(
 ) -> Vec<Value> {
     which::which_in_all(&item, Some(paths), cwd)
         .map(|iter| {
-            iter.map(|path| entry_path!(item, path.to_string_lossy().to_string(), span))
+            iter.map(|path| entry(item, path.to_string_lossy().to_string(), false, span))
                 .collect()
         })
         .unwrap_or_default()
 }
+
 #[cfg(not(feature = "which"))]
 fn get_all_entries_in_path(
     _item: &str,

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

@@ -58,38 +58,69 @@ fn multiple_reports_for_alias_def_custom() {
 // See: parse_definition, line 2187 for reference.
 #[ignore]
 #[test]
-fn multiple_reports_of_multiple_alias() {
+fn correctly_report_of_shadowed_alias() {
     let actual = nu!(
         cwd: ".",
-        "alias xaz = echo alias1; def helper [] {alias xaz = echo alias2; which -a xaz}; helper | length"
+        r#"alias xaz = echo alias1
+        def helper [] {
+            alias xaz = echo alias2
+            which -a xaz
+        }
+        helper | get path | str contains alias2"#
     );
 
-    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(actual.out, "true");
-    assert_eq!(length, 2);
 }
 
 #[test]
-fn multiple_reports_of_multiple_defs() {
+fn one_report_of_multiple_defs() {
     let actual = nu!(
         cwd: ".",
-        "def xaz [] {echo def1}; def helper [] { def xaz [] { echo def2 }; which -a xaz }; helper | length"
+        r#"def xaz [] { echo def1 }
+        def helper [] {
+            def xaz [] { echo def2 }
+            which -a xaz
+        }
+        helper | length"#
     );
 
     let length: i32 = actual.out.parse().unwrap();
-    assert_eq!(length, 2);
+    assert_eq!(length, 1);
 }
 
-//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 | length"
     );
-    //length is 2. One custom_command (def) one built in ("wrongly" added)
+
     let length: i32 = actual.out.parse().unwrap();
     assert_eq!(length, 1);
 }
+
+#[test]
+fn do_not_show_hidden_aliases() {
+    let actual = nu!(
+        cwd: ".",
+        r#"alias foo = echo foo
+        hide foo
+        which foo | length"#
+    );
+
+    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(length, 0);
+}
+
+#[test]
+fn do_not_show_hidden_commands() {
+    let actual = nu!(
+        cwd: ".",
+        r#"def foo [] { echo foo }
+        hide foo
+        which foo | length"#
+    );
+
+    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(length, 0);
+}

crates/nu-engine/src/eval.rs

@@ -4,7 +4,7 @@ use std::io::Write;
 
 use nu_path::expand_path_with;
 use nu_protocol::ast::{Block, Call, Expr, Expression, Operator};
-use nu_protocol::engine::{EngineState, Stack};
+use nu_protocol::engine::{EngineState, Stack, Visibility};
 use nu_protocol::{
     IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, Range, ShellError, Span,
     Spanned, Unit, Value, VarId, ENV_VARIABLE_ID,
@@ -752,41 +752,55 @@ pub fn create_scope(
     let mut output_vals = vec![];
 
     let mut vars = vec![];
-
     let mut commands = vec![];
     let mut aliases = vec![];
     let mut overlays = vec![];
 
+    let mut vars_map = HashMap::new();
+    let mut commands_map = HashMap::new();
+    let mut aliases_map = HashMap::new();
+    let mut overlays_map = HashMap::new();
+    let mut visibility = Visibility::new();
+
     for frame in &engine_state.scope {
-        for var in &frame.vars {
+        vars_map.extend(&frame.vars);
-            let var_name = Value::string(String::from_utf8_lossy(var.0).to_string(), span);
+        commands_map.extend(&frame.decls);
+        aliases_map.extend(&frame.aliases);
+        overlays_map.extend(&frame.overlays);
 
-            let var_type = Value::string(engine_state.get_var(*var.1).ty.to_string(), span);
+        visibility.merge_with(frame.visibility.clone());
+    }
 
-            let var_value = if let Ok(val) = stack.get_var(*var.1, span) {
+    for var in vars_map {
-                val
+        let var_name = Value::string(String::from_utf8_lossy(var.0).to_string(), span);
-            } else {
-                Value::nothing(span)
-            };
 
-            vars.push(Value::Record {
+        let var_type = Value::string(engine_state.get_var(*var.1).ty.to_string(), span);
-                cols: vec!["name".to_string(), "type".to_string(), "value".to_string()],
-                vals: vec![var_name, var_type, var_value],
-                span,
-            })
-        }
 
-        for command in &frame.decls {
+        let var_value = if let Ok(val) = stack.get_var(*var.1, span) {
+            val
+        } else {
+            Value::nothing(span)
+        };
+
+        vars.push(Value::Record {
+            cols: vec!["name".to_string(), "type".to_string(), "value".to_string()],
+            vals: vec![var_name, var_type, var_value],
+            span,
+        })
+    }
+
+    for (command_name, decl_id) in commands_map {
+        if visibility.is_decl_id_visible(decl_id) {
             let mut cols = vec![];
             let mut vals = vec![];
 
             cols.push("command".into());
             vals.push(Value::String {
-                val: String::from_utf8_lossy(command.0).to_string(),
+                val: String::from_utf8_lossy(command_name).to_string(),
                 span,
             });
 
-            let decl = engine_state.get_decl(*command.1);
+            let decl = engine_state.get_decl(*decl_id);
             let signature = decl.signature();
             cols.push("category".to_string());
             vals.push(Value::String {
@@ -998,8 +1012,10 @@ pub fn create_scope(
 
             commands.push(Value::Record { cols, vals, span })
         }
+    }
 
-        for (alias_name, alias_id) in &frame.aliases {
+    for (alias_name, alias_id) in aliases_map {
+        if visibility.is_alias_id_visible(alias_id) {
             let alias = engine_state.get_alias(*alias_id);
             let mut alias_text = String::new();
             for span in alias {
@@ -1017,13 +1033,13 @@ pub fn create_scope(
                 Value::string(alias_text, span),
             ));
         }
+    }
 
-        for overlay in &frame.overlays {
+    for overlay in overlays_map {
-            overlays.push(Value::String {
+        overlays.push(Value::String {
-                val: String::from_utf8_lossy(overlay.0).to_string(),
+            val: String::from_utf8_lossy(overlay.0).to_string(),
-                span,
+            span,
-            });
+        });
-        }
     }
 
     output_cols.push("vars".to_string());

crates/nu-protocol/src/engine/engine_state.rs

@@ -20,24 +20,24 @@ static PWD_ENV: &str = "PWD";
 
 // Tells whether a decl etc. is visible or not
 #[derive(Debug, Clone)]
-struct Visibility {
+pub struct Visibility {
     decl_ids: HashMap<DeclId, bool>,
     alias_ids: HashMap<AliasId, bool>,
 }
 
 impl Visibility {
-    fn new() -> Self {
+    pub fn new() -> Self {
         Visibility {
             decl_ids: HashMap::new(),
             alias_ids: HashMap::new(),
         }
     }
 
-    fn is_decl_id_visible(&self, decl_id: &DeclId) -> bool {
+    pub fn is_decl_id_visible(&self, decl_id: &DeclId) -> bool {
         *self.decl_ids.get(decl_id).unwrap_or(&true) // by default it's visible
     }
 
-    fn is_alias_id_visible(&self, alias_id: &AliasId) -> bool {
+    pub fn is_alias_id_visible(&self, alias_id: &AliasId) -> bool {
         *self.alias_ids.get(alias_id).unwrap_or(&true) // by default it's visible
     }
 
@@ -57,7 +57,7 @@ impl Visibility {
         self.alias_ids.insert(*alias_id, true);
     }
 
-    fn merge_with(&mut self, other: Visibility) {
+    pub fn merge_with(&mut self, other: Visibility) {
         // overwrite own values with the other
         self.decl_ids.extend(other.decl_ids);
         self.alias_ids.extend(other.alias_ids);
@@ -80,6 +80,12 @@ impl Visibility {
     }
 }
 
+impl Default for Visibility {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 #[derive(Debug, Clone)]
 pub struct ScopeFrame {
     pub vars: HashMap<Vec<u8>, VarId>,
@@ -88,7 +94,7 @@ pub struct ScopeFrame {
     pub aliases: HashMap<Vec<u8>, AliasId>,
     pub env_vars: HashMap<Vec<u8>, BlockId>,
     pub overlays: HashMap<Vec<u8>, OverlayId>,
-    visibility: Visibility,
+    pub visibility: Visibility,
 }
 
 impl ScopeFrame {
@@ -377,35 +383,6 @@ impl EngineState {
         }
     }
 
-    pub fn find_aliases(&self, name: &str) -> Vec<&[Span]> {
-        let mut output = vec![];
-
-        for frame in &self.scope {
-            if let Some(alias_id) = frame.aliases.get(name.as_bytes()) {
-                let alias = self.get_alias(*alias_id);
-                output.push(alias.as_ref());
-            }
-        }
-
-        output
-    }
-
-    pub fn find_custom_commands(&self, name: &str) -> Vec<Block> {
-        let mut output = vec![];
-
-        for frame in &self.scope {
-            if let Some(decl_id) = frame.decls.get(name.as_bytes()) {
-                let decl = self.get_decl(*decl_id);
-
-                if let Some(block_id) = decl.get_block_id() {
-                    output.push(self.get_block(block_id).clone());
-                }
-            }
-        }
-
-        output
-    }
-
     pub fn find_decl(&self, name: &[u8]) -> Option<DeclId> {
         let mut visibility: Visibility = Visibility::new();
 
@@ -422,6 +399,22 @@ impl EngineState {
         None
     }
 
+    pub fn find_alias(&self, name: &[u8]) -> Option<AliasId> {
+        let mut visibility: Visibility = Visibility::new();
+
+        for scope in self.scope.iter().rev() {
+            visibility.append(&scope.visibility);
+
+            if let Some(alias_id) = scope.aliases.get(name) {
+                if visibility.is_alias_id_visible(alias_id) {
+                    return Some(*alias_id);
+                }
+            }
+        }
+
+        None
+    }
+
     #[cfg(feature = "plugin")]
     pub fn plugin_decls(&self) -> impl Iterator<Item = &Box<dyn Command + 'static>> {
         let mut unique_plugin_decls = HashMap::new();

tests/main.rs

@@ -3,4 +3,5 @@ extern crate nu_test_support;
 mod parsing;
 mod path;
 mod plugins;
+mod scope;
 mod shell;

tests/scope/mod.rs

@@ -0,0 +1,104 @@
+use nu_test_support::nu;
+
+#[test]
+fn scope_shows_alias() {
+    let actual = nu!(
+        cwd: ".",
+        r#"alias xaz = echo alias1
+        $nu.scope.aliases | find xaz | length
+        "#
+    );
+
+    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(length, 1);
+}
+
+#[test]
+fn scope_shows_command() {
+    let actual = nu!(
+        cwd: ".",
+        r#"def xaz [] { echo xaz }
+        $nu.scope.commands | find xaz | length
+        "#
+    );
+
+    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(length, 1);
+}
+
+#[test]
+fn scope_doesnt_show_scoped_hidden_alias() {
+    let actual = nu!(
+        cwd: ".",
+        r#"alias xaz = echo alias1
+        do {
+            hide xaz
+            $nu.scope.aliases | find xaz | length
+        }
+        "#
+    );
+
+    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(length, 0);
+}
+
+#[test]
+fn scope_doesnt_show_hidden_alias() {
+    let actual = nu!(
+        cwd: ".",
+        r#"alias xaz = echo alias1
+        hide xaz
+        $nu.scope.aliases | find xaz | length
+        "#
+    );
+
+    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(length, 0);
+}
+
+#[test]
+fn scope_doesnt_show_scoped_hidden_command() {
+    let actual = nu!(
+        cwd: ".",
+        r#"def xaz [] { echo xaz }
+        do {
+            hide xaz
+            $nu.scope.commands | find xaz | length
+        }
+        "#
+    );
+
+    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(length, 0);
+}
+
+#[test]
+fn scope_doesnt_show_hidden_command() {
+    let actual = nu!(
+        cwd: ".",
+        r#"def xaz [] { echo xaz }
+        hide xaz
+        $nu.scope.commands | find xaz | length
+        "#
+    );
+
+    let length: i32 = actual.out.parse().unwrap();
+    assert_eq!(length, 0);
+}
+
+// same problem as 'which' command
+#[ignore]
+#[test]
+fn correctly_report_of_shadowed_alias() {
+    let actual = nu!(
+        cwd: ".",
+        r#"alias xaz = echo alias1
+        def helper [] {
+            alias xaz = echo alias2
+            $nu.scope.aliases
+        }
+        helper | where alias == xaz | get expansion.0"#
+    );
+
+    assert_eq!(actual.out, "echo alias2");
+}