Repository: nushell/engine-q (rust)

Issue:
Creating environment variable with dynamic name panics
```
> let x = "FOO"

> let-env $x = "foo"
thread 'main' panicked at 'internal error: missing variable', crates/nu-command/src/env/let_env.rs:38:14
stack backtrace:
   0: rust_begin_unwind
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/panicking.rs:107:14
   2: core::panicking::panic_display
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/panicking.rs:63:5
   3: core::option::expect_failed
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/option.rs:1637:5
   4: <nu_command::env::let_env::LetEnv as nu_protocol::engine::command::Command>::run
   5: nu_engine::eval::eval_call
   6: nu_engine::eval::eval_expression_with_input
   7: nu_engine::eval::eval_block
   8: nu::utils::eval_source
   9: nu::repl::evaluate
  10: nu::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
```

This works fine in Nushell

Pull request:
Allow let-env to be dynamic
# Description

Fixes #935 
  
# Tests

Make sure you've run and fixed any issues with these commands:

- [x] `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes)
- [x] `cargo clippy --all --all-features -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style
- [x] `cargo build; cargo test --all --all-features` to check that all the tests pass


Patch:
diff --git a/crates/nu-command/src/env/let_env.rs b/crates/nu-command/src/env/let_env.rs
index 829abf1..4035959 100644
--- a/crates/nu-command/src/env/let_env.rs
+++ b/crates/nu-command/src/env/let_env.rs
@@ -1,4 +1,4 @@
-use nu_engine::{current_dir, eval_expression_with_input};
+use nu_engine::{current_dir, eval_expression_with_input, CallExt};
 use nu_protocol::ast::Call;
 use nu_protocol::engine::{Command, EngineState, Stack};
 use nu_protocol::{Category, PipelineData, Signature, SyntaxShape, Value};
@@ -33,9 +33,7 @@ impl Command for LetEnv {
         call: &Call,
         input: PipelineData,
     ) -> Result<nu_protocol::PipelineData, nu_protocol::ShellError> {
-        let env_var = call.positional[0]
-            .as_string()
-            .expect("internal error: missing variable");
+        let env_var = call.req(engine_state, stack, 0)?;
 
         let keyword_expr = call.positional[1]
             .as_keyword()
diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs
index 55cab1a..87cddb1 100644
--- a/crates/nu-command/src/filesystem/ls.rs
+++ b/crates/nu-command/src/filesystem/ls.rs
@@ -6,8 +6,8 @@ use nu_engine::CallExt;
 use nu_protocol::ast::Call;
 use nu_protocol::engine::{Command, EngineState, Stack};
 use nu_protocol::{
-    Category, DataSource, IntoInterruptiblePipelineData, PipelineData, PipelineMetadata,
-    ShellError, Signature, Span, Spanned, SyntaxShape, Value,
+    Category, DataSource, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData,
+    PipelineMetadata, ShellError, Signature, Span, Spanned, SyntaxShape, Value,
 };
 use pathdiff::diff_paths;
 #[cfg(unix)]
@@ -67,27 +67,70 @@ impl Command for Ls {
         let full_paths = call.has_flag("full-paths");
         let du = call.has_flag("du");
         let ctrl_c = engine_state.ctrlc.clone();
-
         let call_span = call.head;
         let cwd = current_dir(engine_state, stack)?;
-
         let pattern_arg = call.opt::<Spanned<String>>(engine_state, stack, 0)?;
 
-        let pattern = if let Some(pattern) = pattern_arg {
-            pattern
-        } else {
-            Spanned {
-                item: cwd.join("*").to_string_lossy().to_string(),
-                span: call_span,
+        let (path, p_tag) = match pattern_arg {
+            Some(p) => {
+                let p_tag = p.span;
+                let mut p = PathBuf::from(p.item);
+                if p.is_dir() {
+                    if permission_denied(&p) {
+                        #[cfg(unix)]
+                        let error_msg = format!(
+                            "The permissions of {:o} do not allow access for this user",
+                            p.metadata()
+                                .expect(
+                                    "this shouldn't be called since we already know there is a dir"
+                                )
+                                .permissions()
+                                .mode()
+                                & 0o0777
+                        );
+                        #[cfg(not(unix))]
+                        let error_msg = String::from("Permission denied");
+                        return Err(ShellError::SpannedLabeledError(
+                            "Permission denied".to_string(),
+                            error_msg,
+                            p_tag,
+                        ));
+                    }
+                    if is_empty_dir(&p) {
+                        return Ok(Value::nothing(call_span).into_pipeline_data());
+                    }
+                    p.push("*");
+                }
+                (p, p_tag)
+            }
+            None => {
+                if is_empty_dir(current_dir(engine_state, stack)?) {
+                    return Ok(Value::nothing(call_span).into_pipeline_data());
+                } else {
+                    (PathBuf::from("./*"), call_span)
+                }
             }
         };
 
-        let (prefix, glob) = nu_engine::glob_from(&pattern, &cwd, call_span)?;
+        let hidden_dir_specified = is_hidden_dir(&path);
+
+        let glob_path = Spanned {
+            item: path.display().to_string(),
+            span: p_tag,
+        };
+        let (prefix, paths) = nu_engine::glob_from(&glob_path, &cwd, call_span)?;
+
+        let mut paths_peek = paths.peekable();
+        if paths_peek.peek().is_none() {
+            return Err(ShellError::LabeledError(
+                format!("No matches found for {}", &path.display().to_string()),
+                "no matches found".to_string(),
+            ));
+        }
 
-        let hidden_dir_specified = is_hidden_dir(&pattern.item);
         let mut hidden_dirs = vec![];
 
-        Ok(glob
+        Ok(paths_peek
             .into_iter()
             .filter_map(move |x| match x {
                 Ok(path) => {
@@ -163,6 +206,20 @@ impl Command for Ls {
     }
 }
 
+fn permission_denied(dir: impl AsRef<Path>) -> bool {
+    match dir.as_ref().read_dir() {
+        Err(e) => matches!(e.kind(), std::io::ErrorKind::PermissionDenied),
+        Ok(_) => false,
+    }
+}
+
+fn is_empty_dir(dir: impl AsRef<Path>) -> bool {
+    match dir.as_ref().read_dir() {
+        Err(_) => true,
+        Ok(mut s) => s.next().is_none(),
+    }
+}
+
 fn is_hidden_dir(dir: impl AsRef<Path>) -> bool {
     #[cfg(windows)]
     {


Test patch:
diff --git a/crates/nu-command/tests/commands/ls.rs b/crates/nu-command/tests/commands/ls.rs
index eda52a6..91f9a13 100644
--- a/crates/nu-command/tests/commands/ls.rs
+++ b/crates/nu-command/tests/commands/ls.rs
@@ -67,8 +67,6 @@ fn lists_regular_files_using_question_mark_wildcard() {
     })
 }
 
-// FIXME: jt: needs more work
-#[ignore]
 #[test]
 fn lists_all_files_in_directories_from_stream() {
     Playground::setup("ls_test_4", |dirs, sandbox| {
@@ -90,7 +88,7 @@ fn lists_all_files_in_directories_from_stream() {
             r#"
                 echo dir_a dir_b
                 | each { ls $it }
-                | length
+                | flatten | length
             "#
         ));
 
@@ -115,8 +113,6 @@ fn does_not_fail_if_glob_matches_empty_directory() {
     })
 }
 
-// FIXME: jt: needs more work
-#[ignore]
 #[test]
 fn fails_when_glob_doesnt_match() {
     Playground::setup("ls_test_5", |dirs, sandbox| {
@@ -292,8 +288,6 @@ fn lists_files_including_starting_with_dot() {
     })
 }
 
-// FIXME: jt: needs more work
-#[ignore]
 #[test]
 fn list_all_columns() {
     Playground::setup("ls_test_all_columns", |dirs, sandbox| {
@@ -306,14 +300,14 @@ fn list_all_columns() {
         // Normal Operation
         let actual = nu!(
             cwd: dirs.test(),
-            "ls | get | to md"
+            "ls | columns | to md"
         );
         let expected = ["name", "type", "size", "modified"].join("");
         assert_eq!(actual.out, expected, "column names are incorrect for ls");
         // Long
         let actual = nu!(
             cwd: dirs.test(),
-            "ls -l | get | to md"
+            "ls -l | columns | to md"
         );
         let expected = {
             #[cfg(unix)]
@@ -322,10 +316,10 @@ fn list_all_columns() {
                     "name",
                     "type",
                     "target",
-                    "num_links",
-                    "inode",
                     "readonly",
                     "mode",
+                    "num_links",
+                    "inode",
                     "uid",
                     "group",
                     "size",
diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs
index aae132b..eb38281 100644
--- a/src/tests/test_engine.rs
+++ b/src/tests/test_engine.rs
@@ -230,3 +230,8 @@ fn export_def_env() -> TestResult {
         "BAZ",
     )
 }
+
+#[test]
+fn dynamic_let_env() -> TestResult {
+    run_test(r#"let x = "FOO"; let-env $x = "BAZ"; $env.FOO"#, "BAZ")
+}