feat: support array of any types in Cargo config #16103
Conversation
rustbot
added
A-configuration
weihanglo
force-pushed
the
array-of-any-types
branch
from
57ee900 to
0850805
Compare
weihanglo
changed the title
| Caused by: | ||
| expected string but found integer at index 0 | ||
| [ERROR] invalid type: integer `1`, expected a string |
There was a problem hiding this comment.
This is the error message regression that the next commit want to deal with
weihanglo
force-pushed
the
array-of-any-types
branch
from
0850805 to
48fed23
Compare
src/cargo/util/context/de.rs
Outdated
| seed.deserialize(de) | ||
| .map_err(|e| { | ||
| // This along with ArrayItemKeyPath provide a better error context of the | ||
| // ConfigValue definition + the key path within an array item that native | ||
| // TOML key path can't express. For example, `foo.bar[3].baz`. | ||
| key_path.push_index(i); | ||
| e.with_array_item_key_context(&key_path, definition) | ||
| }) | ||
| .map(Some) |
There was a problem hiding this comment.
While it indeed fixes the this might the less good approach dealing with the diagnostic issue in general. The current approach only track the key path for array items when deserialzation fails. If caller requests for Vec<Value<String>> and performs post-deserialization validation, the key path info is missing for a better error context.
We might want to add an extra optional field array_item_path to Value<T> to preserve the information though. That is why I feel like may we should put off the second half of this PR.
There was a problem hiding this comment.
Think more about it. The other option is doing nothing and staying with the current collect-on-error implementation. The caller should be able to compute the index of an item given it has the full picture of the array.
There was a problem hiding this comment.
Some design decisions:
This is insta- stabilizing config features that have no use and we could overlook the stabilization conversation on the design for this when making use of it because it exists and is "stable".
There was a problem hiding this comment.
Do we want to tie it with a feature like -Zconfig-include? I just feel the PR will be huge. Maybe we can put the support only on nightly until we are going to stabilize a use case of it, though I am not sure how the code would look like with that.
This demonstrates what we have supported today: no nested arrays
Actually it is not supported yet. Will do in the next few commits.
weihanglo
force-pushed
the
array-of-any-types
branch
from
48fed23 to
fc4fa03
Compare
Only add the support to TOML-based Cargo config. Environment variables still only supports array of strings. `-Zadvanced-env` is not yet supported. There is a regression in `cargo_alias_config::alias_malformed_config_list` that the key path error report is missing. This may require a revamp over ConfigKey.
weihanglo
force-pushed
the
array-of-any-types
branch
from
fc4fa03 to
4fae025
Compare
3 participants
What does this PR try to resolve?
This adds the support of array of any types in Cargo config.
The motivation of this is mostly from the proposed
-Zconfig-includesyntax (#7723 (comment))which before this Cargo only supported array of string.
Some design decisions:
CARGO_*env support added — Only the--configCLI and file-based configuration start supporting array of any types.CARGO_*config environment still doesn't. The only type you can set through environment variable are still primitive types. For advanced env usage, we left it as follow-up to deal with in-Zadvanced-envcargo config getnow prints inline array/tables for array of nested array/tables.fixes #16111
How to test and review this PR?
Starting from the fifth commit, those changes are for handling diagnostic regression, as now
Config::from_tomlaccepts any types and the invalid type error is delayed from the "TOML->ConfigValue" deserialization to "ConfigValue->Target" Type deserialization. Not sure if we should split it to a separate PR, but the solution here I admit is a bit nasty.