$env.config now always holds a record with only valid values

[webbedspace]

Description

Closes #7059. Rather than generate a new Record each time $env.config is accessed (as described in that issue), instead $env.config = now A) parses the input record, then B) un-parses it into a clean Record with only the valid values, and stores that as an env-var. The reasoning for this is that I believe config_to_nu_record() (the method that performs step B) will be useful in later PRs. (See below)

As a result, this also "fixes" the following "bug":

〉$env.config = 'butts'
$env.config is not a record
〉$env.config
butts

Instead, $env.config = 'butts' now turns $env.config into the default (not the default config.nu, but Config::default(), which notably has empty keybindings, color_config, menus and hooks vecs).

This doesn't attempt to fix #7110. cc @Kangaxx-0

Example of new behaviour

OLD:

〉$env.config = ($env.config | merge { foo: 1 })
$env.config.foo is an unknown config setting
〉$env.config.foo
1

NEW:

〉$env.config = ($env.config | merge { foo: 1 })
Error:
  × Config record contains invalid values or unknown settings

Error:
  × Error while applying config changes
   ╭─[entry #1:1:1]
 1 │ $env.config = ($env.config | merge { foo: 1 })
   ·                                           ┬
   ·                                           ╰── $env.config.foo is an unknown config setting
   ╰────
  help: This value has been removed from your $env.config record.

〉$env.config.foo
Error: nu::shell::column_not_found (link)

  × Cannot find column
   ╭─[entry #1:1:1]
 1 │ $env.config = ($env.config | merge { foo: 1 })
   ·                              ──┬──
   ·                                ╰── value originates here
   ╰────
   ╭─[entry #2:1:1]
 1 │ $env.config.foo
   ·             ─┬─
   ·              ╰── cannot find column 'foo'
   ╰────

Example of new errors

OLD:

$env.config.cd.baz is an unknown config setting
$env.config.foo is an unknown config setting
$env.config.bar is an unknown config setting
$env.config.table.qux is an unknown config setting
$env.config.history.qux is an unknown config setting

NEW:

Error: 
  × Config record contains invalid values or unknown settings

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:267:1]
 267 │     abbreviations: true # allows `cd s/o/f` to expand to `cd some/other/folder`
 268 │     baz: 3,
     ·          ┬
     ·          ╰── $env.config.cd.baz is an unknown config setting
 269 │   }
     ╰────
  help: This value has been removed from your $env.config record.

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:269:1]
 269 │   }
 270 │   foo: 1,
     ·        ┬
     ·        ╰── $env.config.foo is an unknown config setting
 271 │   bar: 2,
     ╰────
  help: This value has been removed from your $env.config record.

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:270:1]
 270 │   foo: 1,
 271 │   bar: 2,
     ·        ┬
     ·        ╰── $env.config.bar is an unknown config setting
     ╰────
  help: This value has been removed from your $env.config record.

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:279:1]
 279 │     }
 280 │     qux: 4,
     ·          ┬
     ·          ╰── $env.config.table.qux is an unknown config setting
 281 │   }
     ╰────
  help: This value has been removed from your $env.config record.

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:285:1]
 285 │     file_format: "plaintext" # "sqlite" or "plaintext"
 286 │  qux: 2
     ·       ┬
     ·       ╰── $env.config.history.qux is an unknown config setting
 287 │   }
     ╰────
  help: This value has been removed from your $env.config record.

User-Facing Changes

See above.

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

• cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
• cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

[rgwood]

Thanks for the PR!

I find the new behaviour a little unintuitive. It's surprising to me that $env.config = 'butts' resets the entire $env.config record to a default value. I think I would expect that to either fail or overwrite the whole record with a string (the current behaviour).

Maybe I haven't fully wrapped my head around this problem 🤔

[rgwood]

I'm also a little concerned that it's getting harder to add new config points; I assume that any new config will now need to be added to config_to_nu_record() as well as the existing places.

Not necessarily a dealbreaker, maybe that's a cost we have to pay, just something that jumped out at me.

[Kangaxx-0]

Thanks for the PR!

I find the new behaviour a little unintuitive. It's surprising to me that $env.config = 'butts' resets the entire $env.config record to a default value. I think I would expect that to either fail or overwrite the whole record with a string (the current behaviour).

Maybe I haven't fully wrapped my head around this problem 🤔

I called out this in our discord, Value.into_config() does return a new default Config with only apply the change to updated field, the rest will be replaced to default config. I do not think this PR changes this behavior

[Kangaxx-0]

This into_config will still return a new Config, eventually, you will have another default config and the only change to that updated field

[kubouch]

I'm not a big fan of this change because it adds 400+ lines of code to maintain and as Reilly said, adding new config options would involve one more step.

Currently, invalid config options print out error messages which I find sufficient. I imagine that with the improvements to the type system, we could be able to tighten it better. For example, allowing the config to only be a record and we could even go as far as enforcing all the record key names and their types.

@Kangaxx-0 I think this PR is solving a bit different issue but you're right, what you showed on Discord didn't seem right.

[Kangaxx-0]

I'm not a big fan of this change because it adds 400+ lines of code to maintain and as Reilly said, adding new config options would involve one more step.

Currently, invalid config options print out error messages which I find sufficient. I imagine that with the improvements to the type system, we could be able to tighten it better. For example, allowing the config to only be a record and we could even go as far as enforcing all the record key names and their types.

@Kangaxx-0 I think this PR is solving a bit different issue but you're right, what you showed on Discord didn't seem right.

Yes, I was talking a different issue, however, due to the change of this commit , things got more complicated, and I wished this pr could get my work easier.

Allow me to talk through the example $env.config.history.max_size = 1 again

• Expectation
  I believe how this would work is - It only updates the current state config.

• Parsing:

  • The left part is a CellPath and the right part is an Int of 1

• Eval:

  • After the above commit, Int 1 has to be converted into Record, otherwise, into_config will end up with error of $env.config is not a record at line of https://github.com/nushell/nushell/blob/main/crates/nu-protocol/src/config.rs#L825.

To reuse the existing config, nu needs to know which config env var is being updated upfront, but again, it is not easy with current implementation because $env.config.history.max_size is not the real key, the expected one is max_history_size https://github.com/nushell/nushell/blob/main/crates/nu-protocol/src/config.rs#L299.

That being said, for user's input $env.config.history.max_size = 1, Nu needs to look up the existing config ,and update its max_history_size to 1 which requires 2 conversions/castings:

1. Convert Int to record to match the recent change
2. Convert CellPath tails config.history.max_size to max_history_size

If we could have the single source of truth to represent and save Config, we might not need the conversions at all. This is something I want to be on the same page with you guys. Like you said - For example, allowing the config to only be a record and we could even go as far as enforcing all the record key names and their types.

[webbedspace]

I find the new behaviour a little unintuitive. It's surprising to me that $env.config = 'butts' resets the entire $env.config record to a default value.

OK, I've changed it now so that .into_config() now mutates a clone of the previous existing config (passed in) rather than Config::default().

I'm not a big fan of this change because it adds 400+ lines of code to maintain and as Reilly said, adding new config options would involve one more step.

I suppose I could rework this so that .into_config() just scrubs (a mutable clone of) the passed-in Record as well as returning the resulting Config… or possibly just calls env_vars.insert() itself rather than pass it back out.

That being said, for user's input $env.config.history.max_size = 1, Nu needs to look up the existing config ,and update its max_history_size to 1 which requires 2 conversions/castings:

1. Convert `Int` to `record` to match the recent change

2. Convert CellPath tails `config.history.max_size` to `max_history_size`

If we could have the single source of truth to represent and save Config, we might not need the conversions at all.

My take is that while this is the most efficient way of doing this, it's still the most brittle. What I would do instead is:

1. Take the record saved in $env.config
2. Apply the .history.max_size = 1 mutation to it
3. Do config = record.into_config() to propagate the mutation onto Config ← this step means every single config value gets re-updated, but this means less code needs to be written.

[Kangaxx-0]

Apply the .history.max_size = 1 mutation to it

Do you have a rough idea how it works? I could not come up with a clean solution due to the new grouped structure

This is the evaluation code, $env.config.history.max_size is a FullCellPath with tails = [config, history, max_size], above code matches the first item which is config, stack.add_env_var insert a new item which key is config and value is Int.

merge_env call, it goes into if condition because the key is config, however, v is just a Int of whatever number it contains, the next into_config would fail.

To fix this, we somehow need to re-construct a proper record (probably nested, depends on the structure) which matches the group config change, otherwise, nu will never know which config field to update.

The old structure would not be a big problem because it was one-layered record, the casting or conversion would not be too messy

[webbedspace]

I've had a look at the code section you've pointed out (thank you very much) and I've devised a solution in a separate PR: #7318

[kubouch]

@webbedspace We chatted with the core team and agreed we're not comfortable merging this because of the extra overhead for developers when changing the config.

We could still keep the .into_config(&config) change if it helps anything (it seems like it prevents resetting the config to a default when changing one field?).

One way forward would be improving the into_config() to print a proper error message instead of eprintln! using report_error() or report_error_new(). Also, @fdncred mentioned the "config manager" we used to have earlier. It should be possible to write one using Nushell def-env commands and it could implement this functionality of enforcing only valid config entries.

[webbedspace]

We could still keep the .into_config(&config) change if it helps anything (it seems like it prevents resetting the config to a default when changing one field?).

That's fine. I'll do it when I get time.

[kubouch]

OK, I'll put it as draft for now, then.

[webbedspace]

OK, new approach - I reworked this so that .into_config() just scrubs (a mutable clone of) the passed-in Record. This has pretty much the same effect as what the first post says, at greatly reduced LOC cost.

[kubouch]

OK, this looks better, you don't need to add the config option 2x now. I think we can merge it. One thing I would still change is instead of eprintln!, print out a proper error message with report_error() or report_error_new(). Also, the error message should explain that the invalid option was removed from the $env.config because it's a non-standard behavior that does not happen with other environment variables. It should be an easy change now that you have the invalid! macro.

It would also be good to add test for these. One way to do it might be to use the "REPL simulator" (like this but the test bin might need updating with the merge_env() so we can leave it for later). Could you at least give a few before/after examples of the new behavior in the PR description in case we need to get back to it later (and use them for release notes)?

[webbedspace]

One thing I would still change is instead of eprintln!, print out a proper error message with report_error() or report_error_new().

So, like, I cannot do this because report_error() is in nu_cli, which depends on nu_protocol:

I'll do the other stuff though.

[kubouch]

Ah, makes sense. Then, instead of report_error(), you can let the ShellError bubble out of into_config() and merge_env(), it will be reported in the REPL. This condition might need updating to keep working as intended then.

[webbedspace]

First problem I can see with that is that invalid config values should NOT result in into_config() not returning a config. Current Nushell behaviour is that having some typo like "always_trash": tru in your config.nu's set-env config record shouldn't throw out literally all the other config settings you put on the record.

[kubouch]

Yeah, that makes sense. Then, into_config() could return (Config, Option<ShellError>). This pattern is used in the parser a lot.

[webbedspace]

It should be an easy change now that you have the invalid! macro.

Actually, I had to change it entirely because in order to create good errors, I needed each value's span. ☠ Thanks for the jinx.

Tests will be added… very soon.

[webbedspace]

This got more complicated. I realised a few things while making the tests: one, I could add more macros to make the into_config() block even shorter, and two, that what SHOULD be happening when a config value errors, and NOT a config key, is for the "previous value" to be reinstated on the input record. I'd forgotten about this, and moreover I needed to reinstate some of the code from the old config_to_nu_record() commit to make it work correctly. But, as you'll be able to see, I've integrated it into into_config() in the least space-wasteful manner I could come up with.

By the way, I'll fix the failing tests when I get some more time (very shortly).

[kubouch]

I think more macros are not necessary. Especially if the macro is used only once, then there is no point in having the macro. The problem is not the number of LOC but what was there initially that you'd need to define config entries in two places instead of only one (now eliminated). So I wouldn't go too crazy with the macros because they can actually decrease the readability if overused.

You introduced some type checking and also mentioned checking the values for only the valid ones. My hope is that the type checking and key matching would eventually be handled by our type system so I wouldn't put any more effort into that. Making sure only valid values are inserted would be good to check (unless we implement enums in the language) but I'd leave that for a next PR and try to land this one first. It's already quite big.

One comment: The message $env.config.foo is an unknown config setting would better point at the foo key, not the value, but not a big deal. Definitely a good enough improvement over the old version.

The examples in the PR descriptions are super helpful, thanks for that.

[webbedspace]

I like to think I chose relatively good things to encapsulate in macros. try_bool! and try_int! are fairly easy abstractions over "check if the config value is a bool then error otherwise" and "check if the config value is an int then error otherwise". The others later in the file are for ensuring that the slightly lengthy code for reconstructing inner records (which has to be reused when the outer record is reconstructed) isn't repeated.

One comment: The message $env.config.foo is an unknown config setting would better point at the foo key, not the value, but not a big deal. Definitely a good enough improvement over the old version.

This is impossible because Value::Record { } only stores the columns as Rust Strings, NOT Values, so they have no Span at all.

PR will be done as soon as I stop having a devil of a time figuring out why the test code doesn't seem to cause into_config() to fire at all, even though it works in the REPL…

[kubouch]

OK, yeah, sounds reasonable. There is not much we can do about the key spans right now.

REPL operates in parse-eval cycles followed by merge_env() but when you run a script, that's only one parse-eval cycle and merge_env() is not necessary. In your tests, you change the $env.config but the merge_env() is not called because there is no need to merge the env back to the engine state because there is no further parse-eval cycle to be done. I think you'd need to use the "REPL simulator" I linked to earlier. It's very easy to use, I believe I linked to an example of how it can be used in tests.

[webbedspace]

OK, I get it. I actually had swapped nu_repl_code() in on my side, but since I put the env mutation condition on the last line of the array, (see below) I guess the merging wasn't taking effect. Thanks a lot.

[kubouch]

You might need to add an extra empty line to the test code to trigger the last merge_env().

[webbedspace]

I couldn't make the last 3 tests work because they depend on observing Nushell behaviour after a config-setting error has occurred (see each for details) which isn't currently possible to test with nu_repl() because it exits after the first error, so I've marked them #[ignore] (with a message telling when they can be re-enabled). The rest are fine, though.

[kubouch]

OK, thanks! We can figure out how to extend the test runner to continue after errors later, there could be a flag or something.

[rgwood]

Thanks for putting in a lot of work to get this over the line, @webbedspace and @kubouch!