Repository: nushell/nushell (language: rust)

Issue:
Built-in handling of `$env.PATH` doesn't expand paths when converting to a string
### Describe the bug

Previous to 0.101.0, the default behaviour for converting `$env.PATH` to a string before calling an external command was to not only handle the `PATH`/`Path` case issue, but also to expand individual paths using `path expand`. E.g:

```
$env.ENV_CONVERSIONS = {
    "PATH": {
        from_string: { |s| $s | split row (char esep) | path expand --no-symlink }
        to_string: { |v| $v | path expand --no-symlink | str join (char esep) }
    }
}
```

After 0.101.0, the case-sensitivity issue is handled internally, but `$env.ENV_CONVERSIONS` is now empty by default, so no path expansion is performed.

### How to reproduce

1. Add a path to `$env.PATH` that requires expansion, for instance: `$env.PATH = ($env.PATH | prepend [ "~/.cargo/bin" ])`.
2. Run a program that needs to `exec` a command found in the above path (in my case, I was running `trunk` which was trying to run `cargo metadata`)
3. Observe the failure, as the executed program will have a `$PATH` variable containing the unexpanded `~/.cargo/bin` path instead of the fully resolved one.

### Expected behavior

I expected the default behaviour in 0.101.0 to match that of 0.100.0 and prior. 

### Configuration

| key                | value                                                            |
| ------------------ | ---------------------------------------------------------------- |
| version            | 0.101.0                                                          |
| major              | 0                                                                |
| minor              | 101                                                              |
| patch              | 0                                                                |
| branch             | makepkg                                                          |
| commit_hash        | fb2610904997f54f50ad52095948bede81695f31                         |
| build_os           | linux-x86_64                                                     |
| build_target       | x86_64-unknown-linux-gnu                                         |
| rust_version       | rustc 1.83.0 (90b35a623 2024-11-26) (Arch Linux rust 1:1.83.0-1) |
| cargo_version      | cargo 1.83.0 (5ffbef321 2024-10-29)                              |
| build_time         | 2024-12-23 06:13:49 +00:00                                       |
| build_rust_channel | release                                                          |
| allocator          | mimalloc                                                         |
| features           | default, sqlite, trash                                           |
| installed_plugins  |                                                                  |


Discussion / hints:
See also [this thread](https://discord.com/channels/601130461678272522/1325968335057190912) on discord
Thanks for finding and reporting this.  I definitely didn't consider the expansion when removing this - I wrongly assumed the internal function had the same capabilities, and I'm finding that wasn't the case in several different aspects.

I'm fairly sure we can handle this part internally as well, but as you've found, a temporary workaround can be to still populate the `ENV_CONVERSIONS` with the path.  I'll try to get a fix before 0.102, unless someone else wants to sign up in the meantime.

Worst case, we'll put the `ENV_CONVERSIONS` back in `default_env.nu` before 0.102.
No worries. The workaround I went with (should have mentioned it) is to simply add the already expanded path in my config (i.e `$"($env.HOME)/.cargo/bin"`), which is arguably what I should have done in the first place :)
Another workaround is to use `std path add`, which performs the expansion before adding the path:
```nushell
use std ["path add"]
path add "~/.cargo/bin"
```
FYI: The removal of the default breaks the current `virtualenv` [activate.nu](https://github.com/pypa/virtualenv/blob/a6a5c45b0c9da5a8823c03fd57e47e5a3773a081/src/virtualenv/activation/nushell/activate.nu#L57) (used with `overlay add venv/bin/activate.nu`)
@oderwat Is the breakage already occuring for the current state or are you warning against a potential breakage with this proposal?

I thought we had a test setup for the `virtualenv` integration that should catch regressions, if that test is not catching something we should reinforce that testing.
@oderwat I'm not having any issue with some basic `overlay use venv/bin/activate.nu` testing on main.  Can you file an issue with platform and repro steps?
I checked back what may be the reason that if fails for me. I think it happens with a special kind of setup which used a nix flake based shell environment using `.envrc` with `use flake`.  I tried the activation with a previously created `virtualenv` and got:

```
Error: nu::shell::env_var_not_a_string

  × 'PATH' is not representable as a string.
    ╭─[/Users/anon/workspace/mlx/venv/bin/activate.nu:57:30]
 56 │     let new_env = {
 57 │         $path_name         : $new_path
    ·                              ────┬────
    ·                                  ╰── value not representable as a string
 58 │         VIRTUAL_ENV        : $virtual_env
    ╰────
  help: The 'PATH' environment variable must be a string or be convertible to a string.
            Either make sure 'PATH' is a string, or add a 'to_string' entry for it in ENV_CONVERSIONS.
```

Then I created a new virtualenv manually and had the same problem. Rewriting the `$new_path` to `($new_path | str join ':')` solved that and then I looked into the ENV_CONVERSIONS that is mentioned in the error message. That was the reason I ende up here.

Since then, I switched to use:

.envrc
```
use flake
export VIRTUAL_ENV=venv
layout python3
```

This seems to be independent of adding the PATH conversion.

Fix patch:
diff --git a/crates/nu-engine/src/env.rs b/crates/nu-engine/src/env.rs
index 345d81d6f78cb..03119995009ec 100644
--- a/crates/nu-engine/src/env.rs
+++ b/crates/nu-engine/src/env.rs
@@ -139,18 +139,18 @@ pub fn env_to_string(
                     // Try to convert PATH/Path list to a string
                     match value {
                         Value::List { vals, .. } => {
-                            let paths = vals
+                            let paths: Vec<String> = vals
                                 .iter()
-                                .map(Value::coerce_str)
-                                .collect::<Result<Vec<_>, _>>()?;
+                                .filter_map(|v| v.coerce_str().ok())
+                                .map(|s| nu_path::expand_tilde(&*s).to_string_lossy().into_owned())
+                                .collect();
 
-                            match std::env::join_paths(paths.iter().map(AsRef::as_ref)) {
-                                Ok(p) => Ok(p.to_string_lossy().to_string()),
-                                Err(_) => Err(ShellError::EnvVarNotAString {
+                            std::env::join_paths(paths.iter().map(AsRef::<str>::as_ref))
+                                .map(|p| p.to_string_lossy().to_string())
+                                .map_err(|_| ShellError::EnvVarNotAString {
                                     envvar_name: env_name.to_string(),
                                     span: value.span(),
-                                }),
-                            }
+                                })
                         }
                         _ => Err(ShellError::EnvVarNotAString {
                             envvar_name: env_name.to_string(),


Test patch: