From f3c8d35eb7a3cfc00a209391c63044f15c2d5ab9 Mon Sep 17 00:00:00 2001 From: Fernando Herrera Date: Fri, 3 Dec 2021 14:29:55 +0000 Subject: [PATCH] Plugin repeated (#417) * not repeated decl in file and help * implemented heashmap for repeated * sorted scope commands --- Cargo.lock | 2 +- crates/nu-engine/Cargo.toml | 1 - crates/nu-engine/src/eval.rs | 21 +++++ crates/nu-parser/Cargo.toml | 3 +- crates/nu-parser/src/lib.rs | 2 +- crates/nu-parser/src/parse_keywords.rs | 51 ++++++++--- crates/nu-parser/src/parser.rs | 4 +- crates/nu-plugin/src/plugin.rs | 36 +------- crates/nu-protocol/src/engine/command.rs | 2 +- crates/nu-protocol/src/engine/engine_state.rs | 91 +++++++++++++------ src/main.rs | 8 -- 11 files changed, 127 insertions(+), 94 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 785a1e56d0..114f1348d4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1479,7 +1479,6 @@ version = "0.1.0" dependencies = [ "chrono", "itertools", - "nu-parser", "nu-path", "nu-protocol", ] @@ -1503,6 +1502,7 @@ version = "0.1.0" dependencies = [ "miette", "nu-path", + "nu-plugin", "nu-protocol", "serde_json", "thiserror", diff --git a/crates/nu-engine/Cargo.toml b/crates/nu-engine/Cargo.toml index 50e2136c3c..8369d10121 100644 --- a/crates/nu-engine/Cargo.toml +++ b/crates/nu-engine/Cargo.toml @@ -4,7 +4,6 @@ version = "0.1.0" edition = "2018" [dependencies] -nu-parser = { path = "../nu-parser" } nu-protocol = { path = "../nu-protocol", features = ["plugin"] } nu-path = { path = "../nu-path" } itertools = "0.10.1" diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 359e344049..4333a986af 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -1,3 +1,5 @@ +use std::cmp::Ordering; + use nu_protocol::ast::{Block, Call, Expr, Expression, Operator, Statement}; use nu_protocol::engine::{EngineState, Stack}; use nu_protocol::{ @@ -637,18 +639,37 @@ pub fn eval_variable( span, }); + commands.sort_by(|a, b| match (a, b) { + (Value::Record { vals: rec_a, .. }, Value::Record { vals: rec_b, .. }) => { + // Comparing the first value from the record + // It is expected that the first value is the name of the column + // The names of the commands should be a value string + match (rec_a.get(0), rec_b.get(0)) { + (Some(val_a), Some(val_b)) => match (val_a, val_b) { + (Value::String { val: str_a, .. }, Value::String { val: str_b, .. }) => { + str_a.cmp(str_b) + } + _ => Ordering::Equal, + }, + _ => Ordering::Equal, + } + } + _ => Ordering::Equal, + }); output_cols.push("commands".to_string()); output_vals.push(Value::List { vals: commands, span, }); + aliases.sort_by(|a, b| a.partial_cmp(b).unwrap_or(Ordering::Equal)); output_cols.push("aliases".to_string()); output_vals.push(Value::List { vals: aliases, span, }); + overlays.sort_by(|a, b| a.partial_cmp(b).unwrap_or(Ordering::Equal)); output_cols.push("overlays".to_string()); output_vals.push(Value::List { vals: overlays, diff --git a/crates/nu-parser/Cargo.toml b/crates/nu-parser/Cargo.toml index 1e75ea18b2..d981d87f03 100644 --- a/crates/nu-parser/Cargo.toml +++ b/crates/nu-parser/Cargo.toml @@ -9,6 +9,7 @@ thiserror = "1.0.29" serde_json = "1.0" nu-path = {path = "../nu-path"} nu-protocol = { path = "../nu-protocol"} +nu-plugin = { path = "../nu-plugin", optional = true } [features] -plugin = [] +plugin = ["nu-plugin"] diff --git a/crates/nu-parser/src/lib.rs b/crates/nu-parser/src/lib.rs index 4001104353..65da4325fd 100644 --- a/crates/nu-parser/src/lib.rs +++ b/crates/nu-parser/src/lib.rs @@ -16,4 +16,4 @@ pub use parse_keywords::{ pub use parser::{find_captures_in_expr, parse, Import}; #[cfg(feature = "plugin")] -pub use parse_keywords::parse_plugin; +pub use parse_keywords::parse_register; diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 4e5062436e..eb35a560db 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -1084,13 +1084,14 @@ pub fn parse_source( } #[cfg(feature = "plugin")] -pub fn parse_plugin( +pub fn parse_register( working_set: &mut StateWorkingSet, spans: &[Span], ) -> (Statement, Option) { use std::{path::PathBuf, str::FromStr}; use nu_path::canonicalize; + use nu_plugin::plugin::{get_signature, PluginDeclaration}; use nu_protocol::Signature; let name = working_set.get_span_contents(spans[0]); @@ -1124,18 +1125,35 @@ pub fn parse_plugin( }) .and_then(|path| { if path.exists() & path.is_file() { - working_set.add_plugin_signature(path, None); - Ok(()) + get_signature(path.as_path()) + .map_err(|err| { + ParseError::LabeledError( + "Error getting signatures".into(), + err.to_string(), + spans[0], + ) + }) + .map(|signatures| (path, signatures)) } else { Err(ParseError::FileNotFound(format!("{:?}", path))) } }) + .map(|(path, signatures)| { + for signature in signatures { + // create plugin command declaration (need struct impl Command) + // store declaration in working set + let plugin_decl = PluginDeclaration::new(path.clone(), signature); + + working_set.add_decl(Box::new(plugin_decl)); + } + + working_set.mark_plugins_file_dirty(); + }) .err() } 3 => { let filename_slice = working_set.get_span_contents(spans[1]); let signature = working_set.get_span_contents(spans[2]); - let mut path = PathBuf::new(); String::from_utf8(filename_slice.to_vec()) .map_err(|_| ParseError::NonUtf8(spans[1])) @@ -1148,18 +1166,23 @@ pub fn parse_plugin( }) }) .and_then(|path_inner| { - path = path_inner; - serde_json::from_slice::(signature).map_err(|_| { - ParseError::LabeledError( - "Signature deserialization error".into(), - "unable to deserialize signature".into(), - spans[0], - ) - }) + serde_json::from_slice::(signature) + .map_err(|_| { + ParseError::LabeledError( + "Signature deserialization error".into(), + "unable to deserialize signature".into(), + spans[0], + ) + }) + .map(|signature| (path_inner, signature)) }) - .and_then(|signature| { + .and_then(|(path, signature)| { if path.exists() & path.is_file() { - working_set.add_plugin_signature(path, Some(signature)); + let plugin_decl = PluginDeclaration::new(path, signature); + + working_set.add_decl(Box::new(plugin_decl)); + + working_set.mark_plugins_file_dirty(); Ok(()) } else { Err(ParseError::FileNotFound(format!("{:?}", path))) diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 1631cee1ae..0e30e6187c 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -23,7 +23,7 @@ use crate::parse_keywords::{ use std::collections::HashSet; #[cfg(feature = "plugin")] -use crate::parse_keywords::parse_plugin; +use crate::parse_keywords::parse_register; #[derive(Debug, Clone)] pub enum Import {} @@ -3226,7 +3226,7 @@ pub fn parse_statement( ), b"hide" => parse_hide(working_set, spans), #[cfg(feature = "plugin")] - b"register" => parse_plugin(working_set, spans), + b"register" => parse_register(working_set, spans), _ => { let (expr, err) = parse_expression(working_set, spans, true); (Statement::Pipeline(Pipeline::from_vec(vec![expr])), err) diff --git a/crates/nu-plugin/src/plugin.rs b/crates/nu-plugin/src/plugin.rs index 9a2a7494f0..842d3961f4 100644 --- a/crates/nu-plugin/src/plugin.rs +++ b/crates/nu-plugin/src/plugin.rs @@ -3,7 +3,7 @@ use std::io::BufReader; use std::path::{Path, PathBuf}; use std::process::{Command as CommandSys, Stdio}; -use nu_protocol::engine::{Command, EngineState, Stack, StateWorkingSet}; +use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::{ast::Call, Signature, Value}; use nu_protocol::{PipelineData, ShellError}; @@ -268,37 +268,3 @@ pub fn serve_plugin(plugin: &mut impl Plugin) { } } } - -pub fn eval_plugin_signatures(working_set: &mut StateWorkingSet) -> Result<(), ShellError> { - let decls = working_set - .get_signatures() - .map(|(path, signature)| match signature { - Some(signature) => { - let plugin_decl = PluginDeclaration::new(path.clone(), signature.clone()); - let plugin_decl: Box = Box::new(plugin_decl); - Ok(vec![plugin_decl]) - } - None => match get_signature(path.as_path()) { - Ok(signatures) => Ok(signatures - .into_iter() - .map(|signature| { - let plugin_decl = PluginDeclaration::new(path.clone(), signature); - let plugin_decl: Box = Box::new(plugin_decl); - plugin_decl - }) - .collect::>>()), - Err(err) => Err(ShellError::PluginFailedToLoad(format!("{}", err))), - }, - }) - // Need to collect the vector in order to check the error from getting the signature - .collect::>>, ShellError>>()?; - - let decls = decls - .into_iter() - .flatten() - .collect::>>(); - - working_set.add_plugin_decls(decls); - - Ok(()) -} diff --git a/crates/nu-protocol/src/engine/command.rs b/crates/nu-protocol/src/engine/command.rs index 7b379db77c..65766cd770 100644 --- a/crates/nu-protocol/src/engine/command.rs +++ b/crates/nu-protocol/src/engine/command.rs @@ -48,7 +48,7 @@ pub trait Command: Send + Sync + CommandClone { self.name().contains(' ') } - // Is a plugin command + // Is a plugin command (returns plugin's name if yes) fn is_plugin(&self) -> Option<&PathBuf> { None } diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index c1ee1439ff..5c89986cd4 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -11,6 +11,7 @@ use std::{ #[cfg(feature = "plugin")] use std::path::PathBuf; + // Tells whether a decl etc. is visible or not #[derive(Debug, Clone)] struct Visibility { @@ -197,17 +198,14 @@ impl EngineState { last.visibility.merge_with(first.visibility); #[cfg(feature = "plugin")] - if !delta.plugin_decls.is_empty() { - for decl in delta.plugin_decls { - let name = decl.name().as_bytes().to_vec(); - self.decls.push_back(decl); - let decl_id = self.decls.len() - 1; + if delta.plugins_changed { + let result = self.update_plugin_file(); - last.decls.insert(name, decl_id); - last.visibility.use_decl_id(&decl_id); + if result.is_ok() { + delta.plugins_changed = false; } - return self.update_plugin_file(); + return result; } } @@ -230,7 +228,7 @@ impl EngineState { let file_name = path.to_str().expect("path should be a str"); let line = serde_json::to_string_pretty(&decl.signature()) - .map(|signature| format!("register {} {}\n", file_name, signature)) + .map(|signature| format!("register {} {}\n\n", file_name, signature)) .map_err(|err| ShellError::PluginFailedToLoad(err.to_string()))?; plugin_file @@ -250,6 +248,40 @@ impl EngineState { } } + #[cfg(feature = "plugin")] + pub fn update_plugin_file_1(&self) -> Result<(), ShellError> { + use std::io::Write; + + // Updating the signatures plugin file with the added signatures + if let Some(plugin_path) = &self.plugin_signatures { + // Always creating the file which will erase previous signatures + let mut plugin_file = std::fs::File::create(plugin_path.as_path()) + .map_err(|err| ShellError::PluginFailedToLoad(err.to_string()))?; + + // Plugin definitions with parsed signature + for decl in self.plugin_decls() { + // A successful plugin registration already includes the plugin filename + // No need to check the None option + let path = decl.is_plugin().expect("plugin should have file name"); + let file_name = path.to_str().expect("path should be a str"); + + let line = serde_json::to_string_pretty(&decl.signature()) + .map(|signature| format!("register {} {}\n\n", file_name, signature)) + .map_err(|err| ShellError::PluginFailedToLoad(err.to_string()))?; + + plugin_file + .write_all(line.as_bytes()) + .map_err(|err| ShellError::PluginFailedToLoad(err.to_string()))?; + } + + Ok(()) + } else { + Err(ShellError::PluginFailedToLoad( + "Plugin file not found".into(), + )) + } + } + pub fn num_files(&self) -> usize { self.files.len() } @@ -311,8 +343,21 @@ impl EngineState { None } + #[cfg(feature = "plugin")] pub fn plugin_decls(&self) -> impl Iterator> { - self.decls.iter().filter(|decl| decl.is_plugin().is_some()) + let mut unique_plugin_decls = HashMap::new(); + + // Make sure there are no duplicate decls: Newer one overwrites the older one + for decl in self.decls.iter().filter(|d| d.is_plugin().is_some()) { + unique_plugin_decls.insert(decl.name(), decl); + } + + let mut plugin_decls: Vec<(&str, &Box)> = + unique_plugin_decls.into_iter().collect(); + + // Sort the plugins by name so we don't end up with a random plugin file each time + plugin_decls.sort_by(|a, b| a.0.cmp(b.0)); + plugin_decls.into_iter().map(|(_, decl)| decl) } pub fn find_overlay(&self, name: &[u8]) -> Option { @@ -423,6 +468,8 @@ impl EngineState { } output.sort_by(|a, b| a.0.name.cmp(&b.0.name)); + output.dedup_by(|a, b| a.0.name.cmp(&b.0.name).is_eq()); + output } @@ -519,9 +566,7 @@ pub struct StateDelta { overlays: Vec, // indexed by OverlayId pub scope: Vec, #[cfg(feature = "plugin")] - pub plugin_signatures: Vec<(PathBuf, Option)>, - #[cfg(feature = "plugin")] - plugin_decls: Vec>, + plugins_changed: bool, // marks whether plugin file should be updated } impl StateDelta { @@ -562,9 +607,7 @@ impl<'a> StateWorkingSet<'a> { overlays: vec![], scope: vec![ScopeFrame::new()], #[cfg(feature = "plugin")] - plugin_signatures: vec![], - #[cfg(feature = "plugin")] - plugin_decls: vec![], + plugins_changed: false, }, permanent_state, } @@ -633,20 +676,8 @@ impl<'a> StateWorkingSet<'a> { } #[cfg(feature = "plugin")] - pub fn add_plugin_decls(&mut self, decls: Vec>) { - for decl in decls { - self.delta.plugin_decls.push(decl); - } - } - - #[cfg(feature = "plugin")] - pub fn add_plugin_signature(&mut self, path: PathBuf, signature: Option) { - self.delta.plugin_signatures.push((path, signature)); - } - - #[cfg(feature = "plugin")] - pub fn get_signatures(&self) -> impl Iterator)> { - self.delta.plugin_signatures.iter() + pub fn mark_plugins_file_dirty(&mut self) { + self.delta.plugins_changed = true; } pub fn merge_predecl(&mut self, name: &[u8]) -> Option { diff --git a/src/main.rs b/src/main.rs index 653423a63f..84e8addc5e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,9 +23,6 @@ use nu_protocol::{ }; use reedline::{Completer, CompletionActionHandler, DefaultPrompt, LineBuffer, Prompt}; -#[cfg(feature = "plugin")] -use nu_plugin::plugin::eval_plugin_signatures; - #[cfg(test)] mod tests; @@ -439,11 +436,6 @@ fn eval_source( return false; } - #[cfg(feature = "plugin")] - if let Err(err) = eval_plugin_signatures(&mut working_set) { - report_error(&working_set, &err); - } - (output, working_set.render()) };