diff --git a/src/analyzer.rs b/src/analyzer.rs index c0ee1b913c..4d97aed153 100644 --- a/src/analyzer.rs +++ b/src/analyzer.rs @@ -189,11 +189,17 @@ impl<'run, 'src> Analyzer<'run, 'src> { functions.insert(function.clone()); } - AssignmentResolver::resolve_assignments(&assignments, &functions)?; + let mut variable_resolver = VariableResolver::new(&assignments, &functions)?; + + let mut variable_references = HashSet::new(); for set in self.sets.values() { for expression in set.value.expressions() { - Analyzer::resolve_references(&assignments, &functions, expression)?; + variable_resolver.resolve_expression( + expression, + &ExpressionContext::new(), + &mut variable_references, + )?; } } @@ -213,52 +219,36 @@ impl<'run, 'src> Analyzer<'run, 'src> { .values() .any(|set| matches!(set.value, Setting::Lists(true))); - let variable_references = self - .sets - .values() - .flat_map(|set| set.value.expressions()) - .chain( - self - .recipes - .iter() - .flat_map(|recipe| &recipe.attributes) - .flat_map(|attribute| { - let mut expressions = Vec::new(); - match attribute { - Attribute::Arg { - help_property, - pattern_property, - .. - } => { - if let Some((_, expression)) = help_property { - expressions.push(expression); - } - if let Some((_, expression)) = pattern_property { - expressions.push(expression); - } - } - Attribute::Doc(Some(expression)) => expressions.push(expression), - _ => {} - } - expressions - }), - ) - .chain(module_docs.iter().map(|(_, expression)| *expression)) - .flat_map(|expression| expression.references()) - .filter_map(|reference| { - if let Reference::Variable(variable) = reference { - Some(variable.lexeme()) - } else { - None + for attribute in self.recipes.iter().flat_map(|recipe| &recipe.attributes) { + match attribute { + Attribute::Arg { + help_property, + pattern_property, + .. + } => { + if let Some((_, expression)) = help_property { + variable_resolver.collect_references(expression, &mut variable_references); + } + if let Some((_, expression)) = pattern_property { + variable_resolver.collect_references(expression, &mut variable_references); + } } - }) - .collect::>(); + Attribute::Doc(Some(expression)) => { + variable_resolver.collect_references(expression, &mut variable_references); + } + _ => {} + } + } + + for (_name, expression) in &module_docs { + variable_resolver.collect_references(expression, &mut variable_references); + } Evaluator::evaluate_const_assignments( &assignments, overrides, &scope, - variable_references, + &variable_references, lists, )? }; @@ -276,7 +266,11 @@ impl<'run, 'src> Analyzer<'run, 'src> { } for (name, expression) in module_docs { - Analyzer::resolve_references(&assignments, &functions, expression)?; + variable_resolver.resolve_expression( + expression, + &ExpressionContext::new(), + &mut variable_references, + )?; let value = evaluator.evaluate_value_const(expression)?; self.modules.get_mut(name).unwrap().doc = if value.is_empty() { None @@ -341,13 +335,12 @@ impl<'run, 'src> Analyzer<'run, 'src> { let (recipes, disabled_recipes) = RecipeResolver::resolve_recipes( &absent_modules, - &assignments, &mut evaluator, - &functions, &ast.module_path, &self.modules, &settings, deduplicated_recipes, + &mut variable_resolver, )?; let mut recipe_aliases = Table::new(); @@ -535,54 +528,6 @@ impl<'run, 'src> Analyzer<'run, 'src> { Ok(()) } - - fn resolve_references( - assignments: &Table<'src, Assignment<'src>>, - functions: &'run Table<'src, FunctionDefinition<'src>>, - expression: &Expression<'src>, - ) -> CompileResult<'src> { - for reference in expression.references() { - match reference { - Reference::Call { name, arguments } => { - Self::resolve_call(functions, name, arguments)?; - } - Reference::Variable(variable) => { - let name = variable.lexeme(); - if !assignments.contains_key(name) && !constants().contains_key(name) { - return Err(variable.error(UndefinedVariable { variable: name })); - } - } - } - } - - Ok(()) - } - - pub(crate) fn resolve_call( - functions: &'run Table<'src, FunctionDefinition<'src>>, - name: Name<'src>, - arguments: usize, - ) -> CompileResult<'src> { - let function = name.lexeme(); - - let expected = if let Some(function) = functions.get(function) { - function.parameters.len()..=function.parameters.len() - } else if let Some(function) = function::get(function) { - function.expected_arguments() - } else { - return Err(name.error(CompileErrorKind::UndefinedFunction { function })); - }; - - if !expected.contains(&arguments) { - return Err(name.error(CompileErrorKind::FunctionArgumentCountMismatch { - arguments, - expected, - function, - })); - } - - Ok(()) - } } #[cfg(test)] diff --git a/src/evaluator.rs b/src/evaluator.rs index 8e0c621042..0ec3f2e809 100644 --- a/src/evaluator.rs +++ b/src/evaluator.rs @@ -25,7 +25,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { assignments: &'run Table<'src, Assignment<'src>>, overrides: &'run HashMap, scope: &'run Scope<'src, 'run>, - variable_references: BTreeSet<&str>, + variable_references: &HashSet, lists: bool, ) -> CompileResult<'src, Self> { let mut evaluator = Self { @@ -42,7 +42,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { }; for assignment in assignments.values() { - if variable_references.contains(assignment.name.lexeme()) { + if variable_references.contains(&assignment.number) { match evaluator .evaluate_assignment(assignment) .map_err(Error::unwrap_const) diff --git a/src/expression_context.rs b/src/expression_context.rs new file mode 100644 index 0000000000..38c6833155 --- /dev/null +++ b/src/expression_context.rs @@ -0,0 +1,38 @@ +use super::*; + +#[derive(Default)] +pub(crate) struct ExpressionContext<'src> { + bindings: HashMap<&'src str, Number>, +} + +impl ExpressionContext<'_> { + pub(crate) fn new() -> Self { + Self::default() + } + + pub(crate) fn shadows(&self, name: &str) -> bool { + self.bindings.contains_key(name) + } +} + +impl<'src> From<&[(Name<'src>, Number)]> for ExpressionContext<'src> { + fn from(parameters: &[(Name<'src>, Number)]) -> Self { + Self { + bindings: parameters + .iter() + .map(|(name, number)| (name.lexeme(), *number)) + .collect(), + } + } +} + +impl<'src> From<&[Parameter<'src>]> for ExpressionContext<'src> { + fn from(parameters: &[Parameter<'src>]) -> Self { + Self { + bindings: parameters + .iter() + .map(|parameter| (parameter.name.lexeme(), parameter.number)) + .collect(), + } + } +} diff --git a/src/lib.rs b/src/lib.rs index cfc3fd6873..c259bd8043 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,7 +11,6 @@ pub(crate) use { analyzer::Analyzer, arg_attribute::ArgAttribute, assignment::Assignment, - assignment_resolver::AssignmentResolver, ast::Ast, attribute::{Attribute, AttributeKind}, attribute_set::AttributeSet, @@ -54,6 +53,7 @@ pub(crate) use { execution_context::ExecutionContext, executor::Executor, expression::Expression, + expression_context::ExpressionContext, format_string_part::FormatStringPart, fragment::Fragment, function::Function, @@ -134,6 +134,7 @@ pub(crate) use { usage::Usage, use_color::UseColor, value::Value, + variable_resolver::VariableResolver, verbosity::Verbosity, version::Version, warning::Warning, @@ -232,7 +233,6 @@ mod analyzer; mod arg_attribute; mod arguments; mod assignment; -mod assignment_resolver; mod ast; mod attribute; mod attribute_set; @@ -275,6 +275,7 @@ mod evaluator; mod execution_context; mod executor; mod expression; +mod expression_context; mod filesystem; mod format_string_part; mod fragment; @@ -359,6 +360,7 @@ mod unstable_feature; mod usage; mod use_color; mod value; +mod variable_resolver; mod verbosity; mod version; mod warning; diff --git a/src/recipe_resolver.rs b/src/recipe_resolver.rs index ebf90e159b..7d6d832f40 100644 --- a/src/recipe_resolver.rs +++ b/src/recipe_resolver.rs @@ -2,39 +2,36 @@ use {super::*, CompileErrorKind::*}; pub(crate) struct RecipeResolver<'src: 'run, 'run> { absent_modules: &'run BTreeSet, - assignments: &'run Table<'src, Assignment<'src>>, disabled_recipes: Table<'src, Disabled<'src>>, evaluator: &'run mut Evaluator<'src, 'run>, - functions: &'run Table<'src, FunctionDefinition<'src>>, modulepath: &'run Modulepath, modules: &'run Table<'src, Justfile<'src>>, resolved_recipes: Table<'src, Arc>>, settings: &'run Settings, unresolved_recipes: Table<'src, UnresolvedRecipe<'src>>, + variable_resolver: &'run mut VariableResolver<'src, 'run>, } impl<'src: 'run, 'run> RecipeResolver<'src, 'run> { pub(crate) fn resolve_recipes( absent_modules: &'run BTreeSet, - assignments: &'run Table<'src, Assignment<'src>>, evaluator: &'run mut Evaluator<'src, 'run>, - functions: &'run Table<'src, FunctionDefinition<'src>>, modulepath: &'run Modulepath, modules: &'run Table<'src, Justfile<'src>>, settings: &'run Settings, unresolved_recipes: Table<'src, UnresolvedRecipe<'src>>, + variable_resolver: &'run mut VariableResolver<'src, 'run>, ) -> CompileResult<'src, (Table<'src, Arc>>, Table<'src, Disabled<'src>>)> { let mut resolver = Self { absent_modules, - assignments, disabled_recipes: Table::new(), evaluator, - functions, modulepath, modules, resolved_recipes: Table::new(), settings, unresolved_recipes, + variable_resolver, }; while let Some(unresolved) = resolver.unresolved_recipes.pop() { @@ -80,12 +77,11 @@ impl<'src: 'run, 'run> RecipeResolver<'src, 'run> { if disabled_by.is_empty() { let resolved = Arc::new(recipe.resolve( - self.assignments, self.evaluator, - self.functions, self.modulepath, dependencies, self.settings, + self.variable_resolver, )?); self.resolved_recipes.insert(Arc::clone(&resolved)); Ok(Resolution::Resolved(resolved)) diff --git a/src/unresolved_recipe.rs b/src/unresolved_recipe.rs index a9b5af6a67..2c6f01f798 100644 --- a/src/unresolved_recipe.rs +++ b/src/unresolved_recipe.rs @@ -5,12 +5,11 @@ pub(crate) type UnresolvedRecipe<'src> = Recipe<'src, UnresolvedDependency<'src> impl<'src> UnresolvedRecipe<'src> { pub(crate) fn resolve( mut self, - assignments: &Table<'src, Assignment<'src>>, evaluator: &mut Evaluator<'src, '_>, - functions: &Table<'src, FunctionDefinition<'src>>, modulepath: &Modulepath, resolved: Vec>>, settings: &Settings, + variable_resolver: &mut VariableResolver<'src, '_>, ) -> CompileResult<'src, Recipe<'src>> { assert_eq!( self.dependencies.len(), @@ -23,26 +22,18 @@ impl<'src> UnresolvedRecipe<'src> { let mut variable_references = HashSet::new(); for (i, parameter) in self.parameters.iter().enumerate() { + let parameters = ExpressionContext::from(&self.parameters[..i]); if let Some(expression) = ¶meter.default { - Self::resolve_expression( - assignments, - expression, - functions, - &self.parameters[..i], - &mut variable_references, - )?; + variable_resolver.resolve_expression(expression, ¶meters, &mut variable_references)?; } if let Some(expression) = ¶meter.value { - Self::resolve_expression( - assignments, - expression, - functions, - &self.parameters[..i], - &mut variable_references, - )?; + variable_resolver.resolve_expression(expression, ¶meters, &mut variable_references)?; } } + let parameters = ExpressionContext::from(self.parameters.as_slice()); + let empty = ExpressionContext::new(); + for dependency in &self.dependencies { if dependency.starred() && !settings.lists { return Err( @@ -54,25 +45,17 @@ impl<'src> UnresolvedRecipe<'src> { } for argument in &dependency.arguments { - Self::resolve_expression( - assignments, + variable_resolver.resolve_expression( &argument.expression, - functions, - &self.parameters, + ¶meters, &mut variable_references, )?; } } for attribute in &self.attributes { - let mut resolve_expression = |expression, parameters| { - Self::resolve_expression( - assignments, - expression, - functions, - parameters, - &mut variable_references, - ) + let mut resolve_expression = |expression, context| { + variable_resolver.resolve_expression(expression, context, &mut variable_references) }; match attribute { @@ -82,10 +65,10 @@ impl<'src> UnresolvedRecipe<'src> { .. } => { if let Some((_key, expression)) = help_property { - resolve_expression(expression, &[])?; + resolve_expression(expression, &empty)?; } if let Some((_key, expression)) = pattern_property { - resolve_expression(expression, &[])?; + resolve_expression(expression, &empty)?; } } Attribute::Cache { @@ -94,24 +77,24 @@ impl<'src> UnresolvedRecipe<'src> { outputs, } => { if let Some(extra) = extra { - resolve_expression(extra, &self.parameters)?; + resolve_expression(extra, ¶meters)?; } if let Some(inputs) = inputs { - resolve_expression(inputs, &self.parameters)?; + resolve_expression(inputs, ¶meters)?; } if let Some(outputs) = outputs { - resolve_expression(outputs, &self.parameters)?; + resolve_expression(outputs, ¶meters)?; } } Attribute::Confirm(Some(expression)) | Attribute::WorkingDirectory(expression) => { - resolve_expression(expression, &self.parameters)?; + resolve_expression(expression, ¶meters)?; } Attribute::Doc(Some(expression)) => { - resolve_expression(expression, &[])?; + resolve_expression(expression, &empty)?; } Attribute::Env(key, value) => { - resolve_expression(key, &[])?; - resolve_expression(value, &[])?; + resolve_expression(key, &empty)?; + resolve_expression(value, &empty)?; } Attribute::Android | Attribute::Confirm(None) @@ -208,11 +191,9 @@ impl<'src> UnresolvedRecipe<'src> { for fragment in &line.fragments { if let Fragment::Interpolation { expression, .. } = fragment { - Self::resolve_expression( - assignments, + variable_resolver.resolve_expression( expression, - functions, - &self.parameters, + ¶meters, &mut variable_references, )?; } @@ -272,44 +253,4 @@ impl<'src> UnresolvedRecipe<'src> { variable_references, }) } - - fn resolve_expression( - assignments: &Table<'src, Assignment<'src>>, - expression: &Expression<'src>, - functions: &Table<'src, FunctionDefinition<'src>>, - parameters: &[Parameter], - variable_references: &mut HashSet, - ) -> CompileResult<'src> { - for reference in expression.references() { - match reference { - Reference::Variable(variable) => { - Self::resolve_variable(assignments, parameters, variable, variable_references)?; - } - Reference::Call { name, arguments } => { - Analyzer::resolve_call(functions, name, arguments)?; - } - } - } - Ok(()) - } - - fn resolve_variable( - assignments: &Table<'src, Assignment<'src>>, - parameters: &[Parameter], - variable: Name<'src>, - variable_references: &mut HashSet, - ) -> CompileResult<'src> { - let name = variable.lexeme(); - - if parameters.iter().any(|p| p.name.lexeme() == name) { - Ok(()) - } else if let Some(assignment) = assignments.get(name) { - variable_references.insert(assignment.number); - Ok(()) - } else if constants().contains_key(name) { - Ok(()) - } else { - Err(variable.error(CompileErrorKind::UndefinedVariable { variable: name })) - } - } } diff --git a/src/assignment_resolver.rs b/src/variable_resolver.rs similarity index 58% rename from src/assignment_resolver.rs rename to src/variable_resolver.rs index ca1acb402f..eb1a824ca3 100644 --- a/src/assignment_resolver.rs +++ b/src/variable_resolver.rs @@ -1,17 +1,17 @@ use {super::*, CompileErrorKind::*}; -pub(crate) struct AssignmentResolver<'src: 'run, 'run> { +pub(crate) struct VariableResolver<'src: 'run, 'run> { assignments: &'run Table<'src, Assignment<'src>>, evaluated: BTreeSet<&'src str>, functions: &'run Table<'src, FunctionDefinition<'src>>, stack: Vec<&'src str>, } -impl<'src: 'run, 'run> AssignmentResolver<'src, 'run> { - pub(crate) fn resolve_assignments( +impl<'src: 'run, 'run> VariableResolver<'src, 'run> { + pub(crate) fn new( assignments: &'run Table<'src, Assignment<'src>>, functions: &'run Table<'src, FunctionDefinition<'src>>, - ) -> CompileResult<'src> { + ) -> CompileResult<'src, Self> { let mut resolver = Self { assignments, evaluated: BTreeSet::new(), @@ -24,10 +24,65 @@ impl<'src: 'run, 'run> AssignmentResolver<'src, 'run> { } for function in functions.values() { + let context = ExpressionContext::from(function.parameters.as_slice()); for reference in function.body.references() { - resolver.resolve_reference(Some(&function.parameters), reference)?; + resolver.resolve_reference(&context, reference)?; + } + } + + Ok(resolver) + } + + pub(crate) fn resolve_expression( + &mut self, + expression: &Expression<'src>, + context: &ExpressionContext<'src>, + references: &mut HashSet, + ) -> CompileResult<'src> { + for reference in expression.references() { + match reference { + Reference::Call { name, arguments } => self.resolve_call(name, arguments)?, + Reference::Variable(variable) => { + self.resolve_variable(context, variable, Some(&mut *references))?; + } + } + } + + Ok(()) + } + + pub(crate) fn collect_references( + &self, + expression: &Expression<'src>, + references: &mut HashSet, + ) { + for reference in expression.references() { + if let Reference::Variable(variable) = reference + && let Some(assignment) = self.assignments.get(variable.lexeme()) + { + references.insert(assignment.number); } } + } + + pub(crate) fn resolve_call(&self, name: Name<'src>, arguments: usize) -> CompileResult<'src> { + let function = name.lexeme(); + + let expected = if let Some(function) = self.functions.get(function) { + function.parameters.len()..=function.parameters.len() + } else if let Some(function) = function::get(function) { + function.expected_arguments() + } else { + return Err(name.error(UndefinedFunction { function })); + }; + + if !expected.contains(&arguments) { + return Err(name.error(FunctionArgumentCountMismatch { + arguments, + expected, + function, + })); + } Ok(()) } @@ -41,8 +96,10 @@ impl<'src: 'run, 'run> AssignmentResolver<'src, 'run> { self.stack.push(name); + let context = ExpressionContext::new(); + for reference in assignment.value.references() { - self.resolve_reference(None, reference)?; + self.resolve_reference(&context, reference)?; } self.evaluated.insert(name); @@ -54,15 +111,15 @@ impl<'src: 'run, 'run> AssignmentResolver<'src, 'run> { fn resolve_reference( &mut self, - parameters: Option<&[(Name<'src>, Number)]>, + context: &ExpressionContext<'src>, reference: Reference<'src>, ) -> CompileResult<'src> { match reference { Reference::Call { name, arguments } => { - Analyzer::resolve_call(self.functions, name, arguments)?; + self.resolve_call(name, arguments)?; self.resolve_function_variables(name.lexeme()) } - Reference::Variable(name) => self.resolve_variable(parameters, name), + Reference::Variable(name) => self.resolve_variable(context, name, None), } } @@ -81,11 +138,13 @@ impl<'src: 'run, 'run> AssignmentResolver<'src, 'run> { continue; }; + let context = ExpressionContext::from(function.parameters.as_slice()); + for reference in function.body.references() { match reference { Reference::Call { name, .. } => queue.push(name.lexeme()), Reference::Variable(variable) => { - self.resolve_variable(Some(&function.parameters), variable)?; + self.resolve_variable(&context, variable, None)?; } } } @@ -96,20 +155,20 @@ impl<'src: 'run, 'run> AssignmentResolver<'src, 'run> { fn resolve_variable( &mut self, - parameters: Option<&[(Name<'src>, Number)]>, + context: &ExpressionContext<'src>, variable: Name<'src>, + references: Option<&mut HashSet>, ) -> CompileResult<'src> { let name = variable.lexeme(); - if let Some(parameters) = parameters - && parameters - .iter() - .any(|(parameter, _number)| parameter.lexeme() == name) - { + if context.shadows(name) { return Ok(()); } - if self.evaluated.contains(name) || constants().contains_key(name) { + if self.evaluated.contains(name) { + if let Some(references) = references { + references.insert(self.assignments[name].number); + } return Ok(()); } @@ -123,13 +182,17 @@ impl<'src: 'run, 'run> AssignmentResolver<'src, 'run> { circle: self.stack.clone(), }), ); - } else if let Some(assignment) = self.assignments.get(name) { - self.resolve_assignment(assignment)?; - } else { - return Err(variable.error(UndefinedVariable { variable: name })); } - Ok(()) + if let Some(assignment) = self.assignments.get(name) { + return self.resolve_assignment(assignment); + } + + if constants().contains_key(name) { + return Ok(()); + } + + Err(variable.error(UndefinedVariable { variable: name })) } } @@ -177,6 +240,26 @@ mod tests { kind: CircularVariableDependency { variable: "a", circle: vec!["a", "a"] }, } + analysis_error! { + name: constant_shadowing_self_variable_dependency, + input: "HEX := HEX", + offset: 0, + line: 0, + column: 0, + width: 3, + kind: CircularVariableDependency { variable: "HEX", circle: vec!["HEX", "HEX"] }, + } + + analysis_error! { + name: constant_shadowing_circular_variable_dependency, + input: "x := HEX\nHEX := x", + offset: 9, + line: 1, + column: 0, + width: 3, + kind: CircularVariableDependency { variable: "HEX", circle: vec!["HEX", "x", "HEX"] }, + } + #[test] fn function_parameters_shadow_variables() { testing::compile("a := f('x')\nf(a) := a");