From 38873dcb74dd69851e8ea5318d913260c0dc353c Mon Sep 17 00:00:00 2001 From: Greg Shuflin Date: Wed, 5 Jun 2024 13:16:47 -0700 Subject: [PATCH] Allow unexporting environment variables (#2098) --- README.md | 17 +++++++ src/analyzer.rs | 26 +++++++--- src/command_ext.rs | 28 +++++++--- src/compile_error.rs | 6 +++ src/compile_error_kind.rs | 6 +++ src/evaluator.rs | 9 +++- src/item.rs | 4 ++ src/justfile.rs | 7 ++- src/keyword.rs | 1 + src/lib.rs | 2 +- src/node.rs | 5 ++ src/parser.rs | 5 ++ src/recipe.rs | 5 +- src/recipe_context.rs | 1 + tests/completions.rs | 6 +-- tests/json.rs | 19 +++++++ tests/lib.rs | 1 + tests/unexport.rs | 104 ++++++++++++++++++++++++++++++++++++++ 18 files changed, 231 insertions(+), 21 deletions(-) create mode 100644 tests/unexport.rs diff --git a/README.md b/README.md index 6b05577..9efa80a 100644 --- a/README.md +++ b/README.md @@ -2053,6 +2053,23 @@ a $A $B=`echo $A`: When [export](#export) is set, all `just` variables are exported as environment variables. +#### Unexporting Environment Variablesmaster + +Environment variables can be unexported with the `unexport keyword`: + +```just +unexport FOO + +@foo: + echo $FOO +``` + +``` +$ export FOO=bar +$ just foo +sh: FOO: unbound variable +``` + #### Getting Environment Variables from the environment Environment variables from the environment are passed automatically to the diff --git a/src/analyzer.rs b/src/analyzer.rs index ed9fb37..f2e0b65 100644 --- a/src/analyzer.rs +++ b/src/analyzer.rs @@ -37,6 +37,8 @@ impl<'src> Analyzer<'src> { let mut modules: Table = Table::new(); + let mut unexports: HashSet = HashSet::new(); + let mut definitions: HashMap<&str, (&'static str, Name)> = HashMap::new(); let mut define = |name: Name<'src>, @@ -98,6 +100,13 @@ impl<'src> Analyzer<'src> { self.analyze_set(set)?; self.sets.insert(set.clone()); } + Item::Unexport { name } => { + if !unexports.insert(name.lexeme().to_string()) { + return Err(name.token.error(DuplicateUnexport { + variable: name.lexeme(), + })); + } + } } } @@ -109,21 +118,23 @@ impl<'src> Analyzer<'src> { let mut recipe_table: Table<'src, UnresolvedRecipe<'src>> = Table::default(); for assignment in assignments { - if !settings.allow_duplicate_variables - && self.assignments.contains_key(assignment.name.lexeme()) - { - return Err(assignment.name.token.error(DuplicateVariable { - variable: assignment.name.lexeme(), - })); + let variable = assignment.name.lexeme(); + + if !settings.allow_duplicate_variables && self.assignments.contains_key(variable) { + return Err(assignment.name.token.error(DuplicateVariable { variable })); } if self .assignments - .get(assignment.name.lexeme()) + .get(variable) .map_or(true, |original| assignment.depth <= original.depth) { self.assignments.insert(assignment.clone()); } + + if unexports.contains(variable) { + return Err(assignment.name.token.error(ExportUnexported { variable })); + } } AssignmentResolver::resolve_assignments(&self.assignments)?; @@ -167,6 +178,7 @@ impl<'src> Analyzer<'src> { recipes, settings, source: root.into(), + unexports, warnings, }) } diff --git a/src/command_ext.rs b/src/command_ext.rs index a317e6f..6bd7208 100644 --- a/src/command_ext.rs +++ b/src/command_ext.rs @@ -1,25 +1,41 @@ use super::*; pub(crate) trait CommandExt { - fn export(&mut self, settings: &Settings, dotenv: &BTreeMap, scope: &Scope); + fn export( + &mut self, + settings: &Settings, + dotenv: &BTreeMap, + scope: &Scope, + unexports: &HashSet, + ); - fn export_scope(&mut self, settings: &Settings, scope: &Scope); + fn export_scope(&mut self, settings: &Settings, scope: &Scope, unexports: &HashSet); } impl CommandExt for Command { - fn export(&mut self, settings: &Settings, dotenv: &BTreeMap, scope: &Scope) { + fn export( + &mut self, + settings: &Settings, + dotenv: &BTreeMap, + scope: &Scope, + unexports: &HashSet, + ) { for (name, value) in dotenv { self.env(name, value); } if let Some(parent) = scope.parent() { - self.export_scope(settings, parent); + self.export_scope(settings, parent, unexports); } } - fn export_scope(&mut self, settings: &Settings, scope: &Scope) { + fn export_scope(&mut self, settings: &Settings, scope: &Scope, unexports: &HashSet) { if let Some(parent) = scope.parent() { - self.export_scope(settings, parent); + self.export_scope(settings, parent, unexports); + } + + for unexport in unexports { + self.env_remove(unexport); } for binding in scope.bindings() { diff --git a/src/compile_error.rs b/src/compile_error.rs index e2e5781..b60835c 100644 --- a/src/compile_error.rs +++ b/src/compile_error.rs @@ -131,6 +131,9 @@ impl Display for CompileError<'_> { DuplicateVariable { variable } => { write!(f, "Variable `{variable}` has multiple definitions") } + DuplicateUnexport { variable } => { + write!(f, "Variable `{variable}` is unexported multiple times") + } ExpectedKeyword { expected, found } => { let expected = List::or_ticked(expected); if found.kind == TokenKind::Identifier { @@ -143,6 +146,9 @@ impl Display for CompileError<'_> { write!(f, "Expected keyword {expected} but found `{}`", found.kind) } } + ExportUnexported { variable } => { + write!(f, "Variable {variable} is both exported and unexported") + } ExtraLeadingWhitespace => write!(f, "Recipe line has extra leading whitespace"), FunctionArgumentCountMismatch { function, diff --git a/src/compile_error_kind.rs b/src/compile_error_kind.rs index 5a9b7a4..49b5624 100644 --- a/src/compile_error_kind.rs +++ b/src/compile_error_kind.rs @@ -52,10 +52,16 @@ pub(crate) enum CompileErrorKind<'src> { DuplicateVariable { variable: &'src str, }, + DuplicateUnexport { + variable: &'src str, + }, ExpectedKeyword { expected: Vec, found: Token<'src>, }, + ExportUnexported { + variable: &'src str, + }, ExtraLeadingWhitespace, FunctionArgumentCountMismatch { function: &'src str, diff --git a/src/evaluator.rs b/src/evaluator.rs index 1e029c7..1c6e6ae 100644 --- a/src/evaluator.rs +++ b/src/evaluator.rs @@ -8,6 +8,7 @@ pub(crate) struct Evaluator<'src: 'run, 'run> { pub(crate) scope: Scope<'src, 'run>, pub(crate) search: &'run Search, pub(crate) settings: &'run Settings<'run>, + unsets: &'run HashSet, } impl<'src, 'run> Evaluator<'src, 'run> { @@ -19,6 +20,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { scope: Scope<'src, 'run>, search: &'run Search, settings: &'run Settings<'run>, + unsets: &'run HashSet, ) -> RunResult<'src, Scope<'src, 'run>> { let mut evaluator = Self { assignments: Some(assignments), @@ -28,6 +30,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { scope, search, settings, + unsets, }; for assignment in assignments.values() { @@ -217,7 +220,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { cmd.arg(command); cmd.args(args); cmd.current_dir(&self.search.working_directory); - cmd.export(self.settings, self.dotenv, &self.scope); + cmd.export(self.settings, self.dotenv, &self.scope, self.unsets); cmd.stdin(Stdio::inherit()); cmd.stderr(if self.config.verbosity.quiet() { Stdio::null() @@ -261,6 +264,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { scope: &'run Scope<'src, 'run>, search: &'run Search, settings: &'run Settings, + unsets: &'run HashSet, ) -> RunResult<'src, (Scope<'src, 'run>, Vec)> { let mut evaluator = Self { assignments: None, @@ -270,6 +274,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { scope: scope.child(), search, settings, + unsets, }; let mut scope = scope.child(); @@ -316,6 +321,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { scope: &'run Scope<'src, 'run>, search: &'run Search, settings: &'run Settings, + unsets: &'run HashSet, ) -> Self { Self { assignments: None, @@ -325,6 +331,7 @@ impl<'src, 'run> Evaluator<'src, 'run> { scope: Scope::child(scope), search, settings, + unsets, } } } diff --git a/src/item.rs b/src/item.rs index a39b5d2..b72ec8d 100644 --- a/src/item.rs +++ b/src/item.rs @@ -20,6 +20,9 @@ pub(crate) enum Item<'src> { }, Recipe(UnresolvedRecipe<'src>), Set(Set<'src>), + Unexport { + name: Name<'src>, + }, } impl<'src> Display for Item<'src> { @@ -61,6 +64,7 @@ impl<'src> Display for Item<'src> { } Self::Recipe(recipe) => write!(f, "{}", recipe.color_display(Color::never())), Self::Set(set) => write!(f, "{set}"), + Self::Unexport { name } => write!(f, "unexport {name}"), } } } diff --git a/src/justfile.rs b/src/justfile.rs index e86f6c8..150a405 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -24,6 +24,7 @@ pub(crate) struct Justfile<'src> { pub(crate) settings: Settings<'src>, #[serde(skip)] pub(crate) source: PathBuf, + pub(crate) unexports: HashSet, pub(crate) warnings: Vec, } @@ -113,6 +114,7 @@ impl<'src> Justfile<'src> { scope, search, &self.settings, + &self.unexports, ) } @@ -163,7 +165,7 @@ impl<'src> Justfile<'src> { let scope = scope.child(); - command.export(&self.settings, &dotenv, &scope); + command.export(&self.settings, &dotenv, &scope, &self.unexports); let status = InterruptHandler::guard(|| command.status()).map_err(|io_error| { Error::CommandInvoke { @@ -286,6 +288,7 @@ impl<'src> Justfile<'src> { scope: invocation.scope, search, settings: invocation.settings, + unexports: &self.unexports, }; Self::run_recipe( @@ -441,6 +444,7 @@ impl<'src> Justfile<'src> { context.scope, search, context.settings, + context.unexports, )?; let scope = outer.child(); @@ -452,6 +456,7 @@ impl<'src> Justfile<'src> { &scope, search, context.settings, + context.unexports, ); if !context.config.no_dependencies { diff --git a/src/keyword.rs b/src/keyword.rs index d5f268a..d57032a 100644 --- a/src/keyword.rs +++ b/src/keyword.rs @@ -25,6 +25,7 @@ pub(crate) enum Keyword { Shell, Tempdir, True, + Unexport, WindowsPowershell, WindowsShell, X, diff --git a/src/lib.rs b/src/lib.rs index 10cd391..1d0817d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,7 +42,7 @@ pub(crate) use { std::{ borrow::Cow, cmp, - collections::{BTreeMap, BTreeSet, HashMap}, + collections::{BTreeMap, BTreeSet, HashMap, HashSet}, env, ffi::OsString, fmt::{self, Debug, Display, Formatter}, diff --git a/src/node.rs b/src/node.rs index e4c4d03..2fd73e3 100644 --- a/src/node.rs +++ b/src/node.rs @@ -54,6 +54,11 @@ impl<'src> Node<'src> for Item<'src> { } Self::Recipe(recipe) => recipe.tree(), Self::Set(set) => set.tree(), + Self::Unexport { name } => { + let mut unexport = Tree::atom(Keyword::Unexport.lexeme()); + unexport.push_mut(name.lexeme().replace('-', "_")); + unexport + } } } } diff --git a/src/parser.rs b/src/parser.rs index 9f7e883..857284d 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -340,6 +340,11 @@ impl<'run, 'src> Parser<'run, 'src> { self.presume_keyword(Keyword::Export)?; items.push(Item::Assignment(self.parse_assignment(true)?)); } + Some(Keyword::Unexport) => { + self.presume_keyword(Keyword::Unexport)?; + let name = self.parse_name()?; + items.push(Item::Unexport { name }); + } Some(Keyword::Import) if self.next_are(&[Identifier, StringToken]) || self.next_are(&[Identifier, Identifier, StringToken]) diff --git a/src/recipe.rs b/src/recipe.rs index 4d17103..bad0463 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -169,6 +169,7 @@ impl<'src, D> Recipe<'src, D> { scope, context.search, context.settings, + context.unexports, ); if self.shebang { @@ -279,7 +280,7 @@ impl<'src, D> Recipe<'src, D> { cmd.stdout(Stdio::null()); } - cmd.export(context.settings, context.dotenv, scope); + cmd.export(context.settings, context.dotenv, scope, context.unexports); match InterruptHandler::guard(|| cmd.status()) { Ok(exit_status) => { @@ -425,7 +426,7 @@ impl<'src, D> Recipe<'src, D> { command.args(positional); } - command.export(context.settings, context.dotenv, scope); + command.export(context.settings, context.dotenv, scope, context.unexports); // run it! match InterruptHandler::guard(|| command.status()) { diff --git a/src/recipe_context.rs b/src/recipe_context.rs index f1ed5c6..a1e5434 100644 --- a/src/recipe_context.rs +++ b/src/recipe_context.rs @@ -7,4 +7,5 @@ pub(crate) struct RecipeContext<'src: 'run, 'run> { pub(crate) scope: &'run Scope<'src, 'run>, pub(crate) search: &'run Search, pub(crate) settings: &'run Settings<'src>, + pub(crate) unexports: &'run HashSet, } diff --git a/tests/completions.rs b/tests/completions.rs index 6ba3e49..96d35ca 100644 --- a/tests/completions.rs +++ b/tests/completions.rs @@ -29,10 +29,10 @@ fn bash() { #[test] fn replacements() { for shell in ["bash", "elvish", "fish", "powershell", "zsh"] { - let status = Command::new(executable_path("just")) + let output = Command::new(executable_path("just")) .args(["--completions", shell]) - .status() + .output() .unwrap(); - assert!(status.success()); + assert!(output.status.success()); } } diff --git a/tests/json.rs b/tests/json.rs index 218b625..d44e0da 100644 --- a/tests/json.rs +++ b/tests/json.rs @@ -59,6 +59,7 @@ fn alias() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -98,6 +99,7 @@ fn assignment() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -151,6 +153,7 @@ fn body() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -216,6 +219,7 @@ fn dependencies() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -319,6 +323,7 @@ fn dependency_argument() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -384,6 +389,7 @@ fn duplicate_recipes() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -427,6 +433,7 @@ fn duplicate_variables() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -473,6 +480,7 @@ fn doc_comment() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -505,6 +513,7 @@ fn empty_justfile() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -658,6 +667,7 @@ fn parameters() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -744,6 +754,7 @@ fn priors() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -790,6 +801,7 @@ fn private() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -836,6 +848,7 @@ fn quiet() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -897,6 +910,7 @@ fn settings() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -946,6 +960,7 @@ fn shebang() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -992,6 +1007,7 @@ fn simple() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -1041,6 +1057,7 @@ fn attribute() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }), ); @@ -1103,6 +1120,7 @@ fn module() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], }, }, @@ -1124,6 +1142,7 @@ fn module() { "windows_powershell": false, "windows_shell": null, }, + "unexports": [], "warnings": [], })) .unwrap() diff --git a/tests/lib.rs b/tests/lib.rs index cceda57..18da62d 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -104,6 +104,7 @@ mod summary; mod tempdir; mod timestamps; mod undefined_variables; +mod unexport; mod unstable; #[cfg(target_family = "windows")] mod windows_shell; diff --git a/tests/unexport.rs b/tests/unexport.rs new file mode 100644 index 0000000..9ca93e8 --- /dev/null +++ b/tests/unexport.rs @@ -0,0 +1,104 @@ +use super::*; + +#[test] +fn unexport_environment_variable_linewise() { + Test::new() + .justfile( + " + unexport JUST_TEST_VARIABLE + + @recipe: + echo ${JUST_TEST_VARIABLE:-unset} + ", + ) + .env("JUST_TEST_VARIABLE", "foo") + .stdout("unset\n") + .run(); +} + +#[test] +fn unexport_environment_variable_shebang() { + Test::new() + .justfile( + " + unexport JUST_TEST_VARIABLE + + recipe: + #!/usr/bin/env bash + echo ${JUST_TEST_VARIABLE:-unset} + ", + ) + .env("JUST_TEST_VARIABLE", "foo") + .stdout("unset\n") + .run(); +} + +#[test] +fn duplicate_unexport_fails() { + Test::new() + .justfile( + " + unexport JUST_TEST_VARIABLE + + recipe: + echo \"variable: $JUST_TEST_VARIABLE\" + + unexport JUST_TEST_VARIABLE + ", + ) + .env("JUST_TEST_VARIABLE", "foo") + .stderr( + " + error: Variable `JUST_TEST_VARIABLE` is unexported multiple times + ——▶ justfile:6:10 + │ + 6 │ unexport JUST_TEST_VARIABLE + │ ^^^^^^^^^^^^^^^^^^ + ", + ) + .status(1) + .run(); +} + +#[test] +fn export_unexport_conflict() { + Test::new() + .justfile( + " + unexport JUST_TEST_VARIABLE + + recipe: + echo variable: $JUST_TEST_VARIABLE + + export JUST_TEST_VARIABLE := 'foo' + ", + ) + .stderr( + " + error: Variable JUST_TEST_VARIABLE is both exported and unexported + ——▶ justfile:6:8 + │ + 6 │ export JUST_TEST_VARIABLE := 'foo' + │ ^^^^^^^^^^^^^^^^^^ + ", + ) + .status(1) + .run(); +} + +#[test] +fn unexport_doesnt_override_local_recipe_export() { + Test::new() + .justfile( + " + unexport JUST_TEST_VARIABLE + + recipe $JUST_TEST_VARIABLE: + @echo \"variable: $JUST_TEST_VARIABLE\" + ", + ) + .args(["recipe", "value"]) + .stdout("variable: value\n") + .status(0) + .run(); +}