From 34b2c92a583e4408d3f00ea43761c4b740515709 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Thu, 21 Nov 2019 06:48:47 -0600 Subject: [PATCH] Resolve recipe dependencies The analyzer checks that all recipe dependencies exist, but before this diff, recipe dependencies remained names that would have to be looked up when run. This diff changes analysis to resolve recipe dependencies to actual recipes (Rc), to give type-level certainty that resolution was performed correctly. --- 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 | 123 ++++++++++++++++++++++------------------- src/table.rs | 12 ++++ 14 files changed, 137 insertions(+), 96 deletions(-) create mode 100644 src/dependency.rs diff --git a/src/alias_resolver.rs b/src/alias_resolver.rs index 724dfe5ffa..dd1343b816 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 32ab2cc82b..77056cf3b4 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 e739cbd87f..c36aff0d26 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 0000000000..113db968a9 --- /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 0555877c5e..ac07fe143d 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 cb078a7cfe..ab1ab792be 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 6a78a86804..a3366807c5 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 dc679a92a9..4a914fd888 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 594e2a40bc..af7ca228ea 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 609c6cc86d..ded71d26fe 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 f3292697c6..aaf149ec77 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 5c8d425926..a23561ac3a 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 e0995c4906..8a3fd99033 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)?; - } + + 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(), + dependency: name, + })); } - None => { - return Err(dependency_token.error(UnknownDependency { + + 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(), - unknown: dependency_token.lexeme(), + 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 0cd702fe97..567937ecd4 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> {