From 72daf8c64ec16991124ae001e143f2c1e42ec23f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Sun, 13 Mar 2022 21:32:46 +0200 Subject: [PATCH] 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 | 98 +++++++---------- crates/nu-command/tests/commands/which.rs | 55 +++++++-- crates/nu-engine/src/eval.rs | 68 +++++++----- crates/nu-protocol/src/engine/engine_state.rs | 63 +++++------ tests/main.rs | 1 + tests/scope/mod.rs | 104 ++++++++++++++++++ 6 files changed, 258 insertions(+), 131 deletions(-) create mode 100644 tests/scope/mod.rs diff --git a/crates/nu-command/src/system/which_.rs b/crates/nu-command/src/system/which_.rs index f8847d7a7f..146823e3c4 100644 --- a/crates/nu-command/src/system/which_.rs +++ b/crates/nu-command/src/system/which_.rs @@ -51,8 +51,8 @@ impl Command for Which { } } -/// Shortcuts for creating an entry to the output table -fn entry(arg: impl Into, path: Value, builtin: bool, span: Span) -> Value { +// Shortcut for creating an entry to the output table +fn entry(arg: impl Into, path: impl Into, builtin: bool, span: Span) -> Value { let mut cols = vec![]; let mut vals = vec![]; @@ -60,56 +60,46 @@ fn entry(arg: impl Into, 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 { - ($arg:expr, $path:expr, $span:expr, $is_builtin:expr) => { - entry( - $arg.clone(), - Value::string($path.to_string(), $span), - $is_builtin, - $span, - ) - }; -} +fn get_entry_in_aliases(engine_state: &EngineState, name: &str, span: Span) -> Option { + if let Some(alias_id) = engine_state.find_alias(name.as_bytes()) { + let alias = engine_state.get_alias(alias_id); + let alias_str = alias + .iter() + .map(|alias_span| String::from_utf8_lossy(engine_state.get_span_contents(alias_span))) + .join(" "); -fn get_entries_in_aliases(engine_state: &EngineState, name: &str, span: Span) -> Vec { - let aliases = engine_state.find_aliases(name); + trace!("Found alias: {}", name); - let aliases = aliases - .into_iter() - .map(|spans| { - spans - .iter() - .map(|span| { - String::from_utf8_lossy(engine_state.get_span_contents(span)).to_string() - }) - .join(" ") - }) - .map(|alias| create_entry!(name, format!("Nushell alias: {}", alias), span, false)) - .collect::>(); - trace!("Found {} aliases", aliases.len()); - aliases -} - -fn get_entries_in_custom_command(engine_state: &EngineState, name: &str, span: Span) -> Vec { - let custom_commands = engine_state.find_custom_commands(name); - - custom_commands - .into_iter() - .map(|_| create_entry!(name, "Nushell custom command", span, false)) - .collect::>() + Some(entry( + name, + format!("Nushell alias: {}", alias_str), + false, + span, + )) + } else { + None + } } fn get_entry_in_commands(engine_state: &EngineState, name: &str, span: Span) -> Option { - if engine_state.find_decl(name.as_bytes()).is_some() { - Some(create_entry!(name, "Nushell built-in command", span, true)) + if let Some(decl_id) = engine_state.find_decl(name.as_bytes()) { + 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 { 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 !all_entries.is_empty() && skip_after_first_found { - return all_entries; - } - - if let Some(entry) = get_entry_in_commands(engine_state, name, span) { - all_entries.push(entry); + if let Some(ent) = get_entry_in_commands(engine_state, name, span) { + all_entries.push(ent); } 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, ) -> Option { 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 { 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, diff --git a/crates/nu-command/tests/commands/which.rs b/crates/nu-command/tests/commands/which.rs index 67bbd93a30..a871f7917a 100644 --- a/crates/nu-command/tests/commands/which.rs +++ b/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!(length, 2); + assert_eq!(actual.out, "true"); } #[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); +} diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 9f9ffe78c3..27d8aaa0c1 100644 --- a/crates/nu-engine/src/eval.rs +++ b/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 { - let var_name = Value::string(String::from_utf8_lossy(var.0).to_string(), span); + vars_map.extend(&frame.vars); + 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) { - val - } else { - Value::nothing(span) - }; + for var in vars_map { + let var_name = Value::string(String::from_utf8_lossy(var.0).to_string(), 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, - }) - } + let var_type = Value::string(engine_state.get_var(*var.1).ty.to_string(), 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 { - overlays.push(Value::String { - val: String::from_utf8_lossy(overlay.0).to_string(), - span, - }); - } + for overlay in overlays_map { + overlays.push(Value::String { + val: String::from_utf8_lossy(overlay.0).to_string(), + span, + }); } output_cols.push("vars".to_string()); diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index f370e22a58..aff6a93ac5 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/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, alias_ids: HashMap, } 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, VarId>, @@ -88,7 +94,7 @@ pub struct ScopeFrame { pub aliases: HashMap, AliasId>, pub env_vars: HashMap, BlockId>, pub overlays: HashMap, 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 { - 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 { let mut visibility: Visibility = Visibility::new(); @@ -422,6 +399,22 @@ impl EngineState { None } + pub fn find_alias(&self, name: &[u8]) -> Option { + 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> { let mut unique_plugin_decls = HashMap::new(); diff --git a/tests/main.rs b/tests/main.rs index 600cb50742..d508e265b4 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -3,4 +3,5 @@ extern crate nu_test_support; mod parsing; mod path; mod plugins; +mod scope; mod shell; diff --git a/tests/scope/mod.rs b/tests/scope/mod.rs new file mode 100644 index 0000000000..69b8e85676 --- /dev/null +++ b/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"); +}