From 30c77f8d033c7b5dea9ce193bffcf773f66e548a Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Thu, 21 Nov 2019 08:23:32 -0600 Subject: [PATCH] Resolve recipe dependencies (#547) Make analysis resolve recipe dependencies from names (`Name`) to recipes (`Rc`), to give type-level certainty that resolution was performed correctly and remove the need to look up dependencies on run. --- src/alias_resolver.rs | 4 +- src/analyzer.rs | 16 +----- src/common.rs | 13 +++-- src/dependency.rs | 4 ++ src/expression.rs | 4 +- src/item.rs | 2 +- src/justfile.rs | 13 ++--- src/keyed.rs | 8 +++ src/lib.rs | 1 + src/node.rs | 2 +- src/parser.rs | 2 +- src/recipe.rs | 29 ++++++++-- src/recipe_resolver.rs | 125 ++++++++++++++++++++++------------------- src/table.rs | 12 ++++ 14 files changed, 138 insertions(+), 97 deletions(-) create mode 100644 src/dependency.rs diff --git a/src/alias_resolver.rs b/src/alias_resolver.rs index 724dfe5..dd1343b 100644 --- a/src/alias_resolver.rs +++ b/src/alias_resolver.rs @@ -6,13 +6,13 @@ where 'a: 'b, { aliases: &'b Table<'a, Alias<'a>>, - recipes: &'b Table<'a, Recipe<'a>>, + recipes: &'b Table<'a, Rc>>, } impl<'a: 'b, 'b> AliasResolver<'a, 'b> { pub(crate) fn resolve_aliases( aliases: &Table<'a, Alias<'a>>, - recipes: &Table<'a, Recipe<'a>>, + recipes: &Table<'a, Rc>>, ) -> CompilationResult<'a, ()> { let resolver = AliasResolver { aliases, recipes }; diff --git a/src/analyzer.rs b/src/analyzer.rs index 32ab2cc..77056cf 100644 --- a/src/analyzer.rs +++ b/src/analyzer.rs @@ -3,7 +3,7 @@ use crate::common::*; use CompilationErrorKind::*; pub(crate) struct Analyzer<'src> { - recipes: Table<'src, Recipe<'src>>, + recipes: Table<'src, Recipe<'src, Name<'src>>>, assignments: Table<'src, Assignment<'src>>, aliases: Table<'src, Alias<'src>>, sets: Table<'src, Set<'src>>, @@ -50,13 +50,12 @@ impl<'src> Analyzer<'src> { } } - let recipes = self.recipes; let assignments = self.assignments; let aliases = self.aliases; AssignmentResolver::resolve_assignments(&assignments)?; - RecipeResolver::resolve_recipes(&recipes, &assignments)?; + let recipes = RecipeResolver::resolve_recipes(self.recipes, &assignments)?; for recipe in recipes.values() { for parameter in &recipe.parameters { @@ -66,15 +65,6 @@ impl<'src> Analyzer<'src> { })); } } - - for dependency in &recipe.dependencies { - if !recipes[dependency.lexeme()].parameters.is_empty() { - return Err(dependency.error(DependencyHasParameters { - recipe: recipe.name(), - dependency: dependency.lexeme(), - })); - } - } } AliasResolver::resolve_aliases(&aliases, &recipes)?; @@ -99,7 +89,7 @@ impl<'src> Analyzer<'src> { }) } - fn analyze_recipe(&self, recipe: &Recipe<'src>) -> CompilationResult<'src, ()> { + fn analyze_recipe(&self, recipe: &Recipe<'src, Name<'src>>) -> CompilationResult<'src, ()> { if let Some(original) = self.recipes.get(recipe.name.lexeme()) { return Err(recipe.name.token().error(DuplicateRecipe { recipe: original.name(), diff --git a/src/common.rs b/src/common.rs index e739cbd..c36aff0 100644 --- a/src/common.rs +++ b/src/common.rs @@ -11,6 +11,7 @@ pub(crate) use std::{ ops::{Index, Range, RangeInclusive}, path::{Path, PathBuf}, process::{self, Command}, + rc::Rc, str::{self, Chars}, sync::{Mutex, MutexGuard}, usize, vec, @@ -50,12 +51,12 @@ pub(crate) use crate::{ assignment_evaluator::AssignmentEvaluator, assignment_resolver::AssignmentResolver, color::Color, compilation_error::CompilationError, compilation_error_kind::CompilationErrorKind, compiler::Compiler, config::Config, config_error::ConfigError, count::Count, - enclosure::Enclosure, expression::Expression, fragment::Fragment, function::Function, - function_context::FunctionContext, functions::Functions, interrupt_guard::InterruptGuard, - interrupt_handler::InterruptHandler, item::Item, justfile::Justfile, lexer::Lexer, line::Line, - list::List, load_error::LoadError, module::Module, name::Name, output_error::OutputError, - parameter::Parameter, parser::Parser, platform::Platform, position::Position, - positional::Positional, recipe::Recipe, recipe_context::RecipeContext, + dependency::Dependency, enclosure::Enclosure, expression::Expression, fragment::Fragment, + function::Function, function_context::FunctionContext, functions::Functions, + interrupt_guard::InterruptGuard, interrupt_handler::InterruptHandler, item::Item, + justfile::Justfile, lexer::Lexer, line::Line, list::List, load_error::LoadError, module::Module, + name::Name, output_error::OutputError, parameter::Parameter, parser::Parser, platform::Platform, + position::Position, positional::Positional, recipe::Recipe, recipe_context::RecipeContext, recipe_resolver::RecipeResolver, runtime_error::RuntimeError, search::Search, search_config::SearchConfig, search_error::SearchError, set::Set, setting::Setting, settings::Settings, shebang::Shebang, show_whitespace::ShowWhitespace, state::State, diff --git a/src/dependency.rs b/src/dependency.rs new file mode 100644 index 0000000..113db96 --- /dev/null +++ b/src/dependency.rs @@ -0,0 +1,4 @@ +use crate::common::*; + +#[derive(PartialEq, Debug)] +pub(crate) struct Dependency<'a>(pub(crate) Rc>); diff --git a/src/expression.rs b/src/expression.rs index 0555877..ac07fe1 100644 --- a/src/expression.rs +++ b/src/expression.rs @@ -26,9 +26,7 @@ pub(crate) enum Expression<'src> { /// `(contents)` Group { contents: Box> }, /// `"string_literal"` or `'string_literal'` - StringLiteral { - string_literal: StringLiteral<'src>, - }, + StringLiteral { string_literal: StringLiteral<'src> }, /// `variable` Variable { name: Name<'src> }, } diff --git a/src/item.rs b/src/item.rs index cb078a7..ab1ab79 100644 --- a/src/item.rs +++ b/src/item.rs @@ -5,6 +5,6 @@ use crate::common::*; pub(crate) enum Item<'src> { Alias(Alias<'src>), Assignment(Assignment<'src>), - Recipe(Recipe<'src>), + Recipe(Recipe<'src, Name<'src>>), Set(Set<'src>), } diff --git a/src/justfile.rs b/src/justfile.rs index 6a78a86..a336680 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -2,7 +2,7 @@ use crate::common::*; #[derive(Debug, PartialEq)] pub(crate) struct Justfile<'src> { - pub(crate) recipes: Table<'src, Recipe<'src>>, + pub(crate) recipes: Table<'src, Rc>>, pub(crate) assignments: Table<'src, Assignment<'src>>, pub(crate) aliases: Table<'src, Alias<'src>>, pub(crate) settings: Settings<'src>, @@ -11,7 +11,7 @@ pub(crate) struct Justfile<'src> { impl<'src> Justfile<'src> { pub(crate) fn first(&self) -> Option<&Recipe> { - let mut first: Option<&Recipe> = None; + let mut first: Option<&Recipe> = None; for recipe in self.recipes.values() { if let Some(first_recipe) = first { if recipe.line_number() < first_recipe.line_number() { @@ -166,7 +166,7 @@ impl<'src> Justfile<'src> { if let Some(recipe) = self.recipes.get(name) { Some(recipe) } else if let Some(alias) = self.aliases.get(name) { - self.recipes.get(alias.target.lexeme()) + self.recipes.get(alias.target.lexeme()).map(Rc::as_ref) } else { None } @@ -181,10 +181,9 @@ impl<'src> Justfile<'src> { ran: &mut BTreeSet<&'src str>, overrides: &BTreeMap, ) -> RunResult<()> { - for dependency_name in &recipe.dependencies { - let lexeme = dependency_name.lexeme(); - if !ran.contains(lexeme) { - self.run_recipe(context, &self.recipes[lexeme], &[], dotenv, ran, overrides)?; + for Dependency(dependency) in &recipe.dependencies { + if !ran.contains(dependency.name()) { + self.run_recipe(context, dependency, &[], dotenv, ran, overrides)?; } } recipe.run(context, arguments, dotenv, overrides)?; diff --git a/src/keyed.rs b/src/keyed.rs index dc679a9..4a914fd 100644 --- a/src/keyed.rs +++ b/src/keyed.rs @@ -1,3 +1,11 @@ +use crate::common::*; + pub(crate) trait Keyed<'key> { fn key(&self) -> &'key str; } + +impl<'key, T: Keyed<'key>> Keyed<'key> for Rc { + fn key(&self) -> &'key str { + self.as_ref().key() + } +} diff --git a/src/lib.rs b/src/lib.rs index 594e2a4..af7ca22 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -32,6 +32,7 @@ mod config; mod config_error; mod count; mod default; +mod dependency; mod empty; mod enclosure; mod error; diff --git a/src/node.rs b/src/node.rs index 609c6cc..ded71d2 100644 --- a/src/node.rs +++ b/src/node.rs @@ -66,7 +66,7 @@ impl<'src> Node<'src> for Expression<'src> { } } -impl<'src> Node<'src> for Recipe<'src> { +impl<'src> Node<'src> for Recipe<'src, Name<'src>> { fn tree(&self) -> Tree<'src> { let mut t = Tree::atom("recipe"); diff --git a/src/parser.rs b/src/parser.rs index f329269..aaf149e 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -471,7 +471,7 @@ impl<'tokens, 'src> Parser<'tokens, 'src> { &mut self, doc: Option<&'src str>, quiet: bool, - ) -> CompilationResult<'src, Recipe<'src>> { + ) -> CompilationResult<'src, Recipe<'src, Name<'src>>> { let name = self.parse_name()?; let mut positional = Vec::new(); diff --git a/src/recipe.rs b/src/recipe.rs index 5c8d425..a23561a 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -24,8 +24,8 @@ fn error_from_signal( /// A recipe, e.g. `foo: bar baz` #[derive(PartialEq, Debug)] -pub(crate) struct Recipe<'a> { - pub(crate) dependencies: Vec>, +pub(crate) struct Recipe<'a, D = Dependency<'a>> { + pub(crate) dependencies: Vec, pub(crate) doc: Option<&'a str>, pub(crate) body: Vec>, pub(crate) name: Name<'a>, @@ -35,7 +35,7 @@ pub(crate) struct Recipe<'a> { pub(crate) shebang: bool, } -impl<'a> Recipe<'a> { +impl<'a, D> Recipe<'a, D> { pub(crate) fn argument_range(&self) -> RangeInclusive { self.min_arguments()..=self.max_arguments() } @@ -319,7 +319,26 @@ impl<'a> Recipe<'a> { } } -impl<'src> Keyed<'src> for Recipe<'src> { +impl<'src> Recipe<'src, Name<'src>> { + pub(crate) fn resolve(self, resolved: Vec>) -> Recipe<'src> { + assert_eq!(self.dependencies.len(), resolved.len()); + for (name, resolved) in self.dependencies.iter().zip(&resolved) { + assert_eq!(name.lexeme(), resolved.0.name.lexeme()); + } + Recipe { + dependencies: resolved, + doc: self.doc, + body: self.body, + name: self.name, + parameters: self.parameters, + private: self.private, + quiet: self.quiet, + shebang: self.shebang, + } + } +} + +impl<'src, D> Keyed<'src> for Recipe<'src, D> { fn key(&self) -> &'src str { self.name.lexeme() } @@ -342,7 +361,7 @@ impl<'a> Display for Recipe<'a> { } write!(f, ":")?; for dependency in &self.dependencies { - write!(f, " {}", dependency)?; + write!(f, " {}", dependency.0.name())?; } for (i, line) in self.body.iter().enumerate() { diff --git a/src/recipe_resolver.rs b/src/recipe_resolver.rs index e0995c4..8a3fd99 100644 --- a/src/recipe_resolver.rs +++ b/src/recipe_resolver.rs @@ -3,36 +3,31 @@ use crate::common::*; use CompilationErrorKind::*; pub(crate) struct RecipeResolver<'a: 'b, 'b> { - stack: Vec<&'a str>, - seen: BTreeSet<&'a str>, - resolved: BTreeSet<&'a str>, - recipes: &'b Table<'a, Recipe<'a>>, + unresolved_recipes: Table<'a, Recipe<'a, Name<'a>>>, + resolved_recipes: Table<'a, Rc>>, assignments: &'b Table<'a, Assignment<'a>>, } impl<'a, 'b> RecipeResolver<'a, 'b> { pub(crate) fn resolve_recipes( - recipes: &Table<'a, Recipe<'a>>, + unresolved_recipes: Table<'a, Recipe<'a, Name<'a>>>, assignments: &Table<'a, Assignment<'a>>, - ) -> CompilationResult<'a, ()> { + ) -> CompilationResult<'a, Table<'a, Rc>>> { let mut resolver = RecipeResolver { - seen: empty(), - stack: empty(), - resolved: empty(), + resolved_recipes: empty(), + unresolved_recipes, assignments, - recipes, }; - for recipe in recipes.values() { - resolver.resolve_recipe(recipe)?; - resolver.seen = empty(); + while let Some(unresolved) = resolver.unresolved_recipes.pop() { + resolver.resolve_recipe(&mut Vec::new(), unresolved)?; } - for recipe in recipes.values() { + for recipe in resolver.resolved_recipes.values() { for parameter in &recipe.parameters { if let Some(expression) = ¶meter.default { for (function, argc) in expression.functions() { - resolver.resolve_function(function, argc)?; + Function::resolve(&function, argc)?; } for variable in expression.variables() { resolver.resolve_variable(&variable, &[])?; @@ -44,7 +39,7 @@ impl<'a, 'b> RecipeResolver<'a, 'b> { for fragment in &line.fragments { if let Fragment::Interpolation { expression, .. } = fragment { for (function, argc) in expression.functions() { - resolver.resolve_function(function, argc)?; + Function::resolve(&function, argc)?; } for variable in expression.variables() { resolver.resolve_variable(&variable, &recipe.parameters)?; @@ -54,11 +49,7 @@ impl<'a, 'b> RecipeResolver<'a, 'b> { } } - Ok(()) - } - - fn resolve_function(&self, function: Token<'a>, argc: usize) -> CompilationResult<'a, ()> { - Function::resolve(&function, argc) + Ok(resolver.resolved_recipes) } fn resolve_variable( @@ -77,49 +68,67 @@ impl<'a, 'b> RecipeResolver<'a, 'b> { Ok(()) } - fn resolve_recipe(&mut self, recipe: &Recipe<'a>) -> CompilationResult<'a, ()> { - if self.resolved.contains(recipe.name()) { - return Ok(()); + fn resolve_recipe( + &mut self, + stack: &mut Vec<&'a str>, + recipe: Recipe<'a, Name<'a>>, + ) -> CompilationResult<'a, Rc>> { + if let Some(resolved) = self.resolved_recipes.get(recipe.name()) { + return Ok(resolved.clone()); } - self.stack.push(recipe.name()); - self.seen.insert(recipe.name()); - for dependency_token in recipe - .dependencies - .iter() - .map(|dependency| dependency.token()) - { - match self.recipes.get(dependency_token.lexeme()) { - Some(dependency) => { - if !self.resolved.contains(dependency.name()) { - if self.seen.contains(dependency.name()) { - let first = self.stack[0]; - self.stack.push(first); - return Err( - dependency_token.error(CircularRecipeDependency { - recipe: recipe.name(), - circle: self - .stack - .iter() - .skip_while(|name| **name != dependency.name()) - .cloned() - .collect(), - }), - ); - } - self.resolve_recipe(dependency)?; - } - } - None => { - return Err(dependency_token.error(UnknownDependency { + + stack.push(recipe.name()); + + let mut dependencies: Vec = Vec::new(); + for dependency in &recipe.dependencies { + let name = dependency.lexeme(); + + if let Some(resolved) = self.resolved_recipes.get(name) { + // dependency already resolved + if !resolved.parameters.is_empty() { + return Err(dependency.error(DependencyHasParameters { recipe: recipe.name(), - unknown: dependency_token.lexeme(), + dependency: name, })); } + + dependencies.push(Dependency(resolved.clone())); + } else if stack.contains(&name) { + let first = stack[0]; + stack.push(first); + return Err( + dependency.error(CircularRecipeDependency { + recipe: recipe.name(), + circle: stack + .iter() + .skip_while(|name| **name != dependency.lexeme()) + .cloned() + .collect(), + }), + ); + } else if let Some(unresolved) = self.unresolved_recipes.remove(name) { + // resolve unresolved dependency + if !unresolved.parameters.is_empty() { + return Err(dependency.error(DependencyHasParameters { + recipe: recipe.name(), + dependency: name, + })); + } + + dependencies.push(Dependency(self.resolve_recipe(stack, unresolved)?)); + } else { + // dependency is unknown + return Err(dependency.error(UnknownDependency { + recipe: recipe.name(), + unknown: name, + })); } } - self.resolved.insert(recipe.name()); - self.stack.pop(); - Ok(()) + + let resolved = Rc::new(recipe.resolve(dependencies)); + self.resolved_recipes.insert(resolved.clone()); + stack.pop(); + Ok(resolved) } } diff --git a/src/table.rs b/src/table.rs index 0cd702f..567937e 100644 --- a/src/table.rs +++ b/src/table.rs @@ -35,6 +35,18 @@ impl<'key, V: Keyed<'key>> Table<'key, V> { pub(crate) fn iter(&self) -> btree_map::Iter<&'key str, V> { self.map.iter() } + + pub(crate) fn pop(&mut self) -> Option { + if let Some(key) = self.map.keys().next().map(|key| *key) { + self.map.remove(key) + } else { + None + } + } + + pub(crate) fn remove(&mut self, key: &str) -> Option { + self.map.remove(key) + } } impl<'key, V: Keyed<'key>> FromIterator for Table<'key, V> {