Change environment variables to be case-preserving

[YizhePKU]

This PR changes $env to be case-preserving instead of case-sensitive. That is, it preserves the case of the environment variable when it is first assigned, but subsequent retrieval and update ignores the case.

Notably, both $env.PATH and $env.Path can now be used to read or set the environment variable, but child processes will always see the correct case based on the platform.

Fixes #11268.

---

This feature was surprising simple to implement, because most of the infrastructure to support case-insensitive cell path access already exists. The get command extracts data using a cell path in a case-insensitive way (!), but accepts a --sensitive flag. (I think this should be flipped around?)

[fdncred]

Hopefully this will close this issue once and for all. My only question is whether all supported operating systems should be allowed case-insensitive access to env vars?

[kubouch]

I'll mention this PR here since it implemented the same thing but hasn't been finished: #11432

[YizhePKU]

My only question is whether all supported operating systems should be allowed case-insensitive access to env vars?

We already have get and get --sensitive if the user wants to explictly choose between case-sensitive and case-insensitive. Do you mean case-insensitive by default on all systems? Is there a good reason for that? I feel like it would be surprising for non-Windows users.

I'll mention this PR here since it implemented the same thing but hasn't been finished: #11432

I was aware of that. The author of that PR has abandoned it for quite a while now.

[YizhePKU]

I just noticed that $env.0 = "foo" doesn't work, even though the original implementation intended it. This is probably a bug. It is unrelated to this PR though, it already breaks without this PR.

[devyn]

I think the challenging behaviour here is that we usually try to make sure that the same Nushell scripts can run on all operating systems, as long as they're not written in a very OS-specific way. So having this just be the case for Windows would make it possible for a Windows-based developer to write a script that won't work on other OSes, because they assume they can just do e.g. $env.path to get the PATH var, which would work for them but not elsewhere.

I think if we're going to do it for Windows we probably need to do it for everyone. It's probably not the worst thing though - even though environment variables are typically case sensitive on Unixes, I can't think of single case where that actually matters to someone trying to read an environment variable. Worst case, they can use get and make it explicit.

[YizhePKU]

So having this just be the case for Windows would make it possible for a Windows-based developer to write a script that won't work on other OSes, because they assume they can just do e.g. $env.path to get the PATH var, which would work for them but not elsewhere.

I think "cross-platform support" doesn't necessary means "the same script always has the same behavior on every OS". For example, filenames on Windows are case-insensitive, so a script that writes to foo.txt and then reads from FOO.txt behaves differently depending on the platform. I wouldn't try to make filenames case-insensitive on every platform just so that the behavior is the same.

This PR makes it so that $env.PATH works on all platforms. Of course there are ways to write things that don't work on all platforms, but I think that's part of the responsibility of the script author.

That said, it would be nice to improve documentation and/or issue a warning when $env.path is used instead of $env.PATH.

[fdncred]

I think "cross-platform support" doesn't necessary means "the same script always has the same behavior on every OS".

Correct, it probably doesn't necessarily mean that but that's what we aim for with nushell. We want everything possible to work across MacOS, Linux, and Windows. Some things aren't possible, but I think this one is.

I agree with devyn and would prefer this to work the same across platforms. We'd appreciate it if you can please make that one additional change. I think we'd be happy to land this if that happens because many people have been "bitten" by this issue where the path env var doesn't work the same across platforms.

[YizhePKU]

OK, that's fine with me.

[YizhePKU]

I've opened a PR on the book that explains the new behavior.

[fdncred]

Thanks. Excited to use this!

[IanManske]

Thanks for this PR, I think this is great addition.

One question/problem that I'll submit to everyone is: how should the case preservation interact with the other methods of setting environment variables:

1. with-env and the env shorthand env_var=value command. E.g., in FOO=bar foo=bar command which case is put into the environment, or are both FOO and foo put into $env?
2. load-env, same spiel: load-env { FOO: bar, foo: bar }.
3. loading the initial environment on startup. Though unlikely, two different environment variables that only differ in case are possible on unix, so how are these two variables translated into the initial $env?

Tagging you as well @sholderbach, since you worked on the recent environment changes.

[fdncred]

@IanManske My vote is for those commands to be case preserving. If a user wants foo instead of FOO it would be nice to allow that.

[IanManske]

I was thinking that as well, but the issue is after, for example, load-env { FOO: bar, foo: bar }, what does $env contain? Does it contain FOO, foo, or both? (Sorry if I wasn't clear before.) Currently, load-env and with-env do not allow duplicate keys/columns/env variables.

[fdncred]

hmmm, seems like case preserving would allow both FOO and foo, but I wonder if that would be confusing?

[devyn]

Personally I think it would be most cross platform to have it use the case of an existing var if there is one. Otherwise $env.path = would work on windows but not unixes

[fdncred]

Personally I think it would be most cross platform to have it use the case of an existing var if there is one. Otherwise $env.path = would work on windows but not unixes

I think it's supposed to work this way now. $env.path works on macos. $env.foo = "test" works fine on macos too and it has a lowercase foo but if I then do $env.FOO = "test2" it reassigns foo to test2. I think that's right too because all env vars are case preserving now. So, now that I think more about it, we shouldn't be able to have FOO and foo at the same time.

I can also do $env.path | append foo and it updates $env.PATH on macos, which is case-preserving and working the way I think it should.

[YizhePKU]

I think we should make it an invariant that "$env cannot contains keys like FOO and foo at the same time". This would be the simplest option.

with-env and load-env should put only one of the variables into $env. When loading the initial environment on Unix, only one of the variables should be loaded.

This does mean that Nushell cannot read both FOO and foo from the environment, but such cases are rare. I think it's worth it.

[sholderbach]

As we currently reject duplicate env vars passed to with-env and load-env with the unique key semantics of record they would have to be changed to error out when the passed record violates whatever semantics we end up with here.

Is the change of implementing the Windows-based case-preserving behavior on the level of Nushell causing problems for folks trying to interact with edge cases on Unix?

[devyn]

I do worry that being overly strict with it would cause issues for someone trying to do something that is possible on Unix. I don't think it should be impossible to add both to env but I think it should have to be very intentional

[YizhePKU]

I don't think it should be impossible to add both to env but I think it should have to be very intentional.

While we do have get --sensitive for case-sensitive read, AFAIK there's no case-sensitive write in Nushell yet. Without that, it wouldn't be very useful to have both cases in $env.

[devyn]

Well, $env is kind of special. Perhaps with-env can take --sensitive

Record cell path updates are normally sensitive I think, but I haven't confirmed that

[Dorumin]

Has any thought been put into how this update interacts with ENV_CONVERSIONS?

nushell/crates/nu-utils/src/sample_config/default_env.nu

Lines 65 to 74 in c54d223

	$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) }
	}
	"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) }
	}
	}

nushell/crates/nu-engine/src/env.rs

Lines 341 to 347 in c54d223

	let conversions = stack.get_env_var(engine_state, ENV_CONVERSIONS);
	let conversion = conversions
	.as_ref()
	.and_then(|val| val.as_record().ok())
	.and_then(|record| record.get(name))
	.and_then(|val| val.as_record().ok())
	.and_then(|record| record.get(direction));

Which one gets called, maybe both can if it's case-preserving, but I'd hope that record can also benefit from insensitive lookups.

[YizhePKU]

Has any thought been put into how this update interacts with ENV_CONVERSIONS?

Currently it doesn't affact ENV_CONVERSIONS at all. We still use case-sensitive comparision to convert to/from the environment. I wonder if that's OK?

[fdncred]

In my perfect world, no one would have to ever know or have to remember what case path needs to be in, whether it is PATH or Path or PaTh or path, all would work everywhere equally. Nushell would know and update the environment appropriately.