Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unhelpful error messages with env-vars that don't set all fields of a config struct #13688

Closed
Nemo157 opened this issue Apr 2, 2024 · 9 comments · Fixed by #13956
Closed
Labels
A-configuration Area: cargo config files and env vars A-diagnostics Area: Error and warning messages generated by Cargo itself. A-environment-variables Area: environment variables C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@Nemo157
Copy link
Member

Nemo157 commented Apr 2, 2024

Problem

I have been using the gitoxide integration for a while, via setting env-vars so it doesn't impact running the stable compiler:

CARGO_UNSTABLE_GITOXIDE_CHECKOUT=true
CARGO_UNSTABLE_GITOXIDE_FETCH=true
CARGO_UNSTABLE_GITOXIDE_INTERNAL_USE_GIT2=false
CARGO_UNSTABLE_GITOXIDE_SHALLOW_DEPS=true
CARGO_UNSTABLE_GITOXIDE_SHALLOW_INDEX=true

After updating to a version that includes #13592 any usage of cargo starts failing with

> cargo test -- --ignored zero_byte
error: missing field `list_files`

which makes no mention of where this field is missing

Steps

> CARGO_UNSTABLE_GITOXIDE_FETCH=true cargo build
error: missing field `list_files`

Possible Solution(s)

No response

Notes

#13687 will likely fix this specific issue by making the fields of this struct default, but the general issue of the error message not having enough context will remain.

Version

cargo 1.79.0-nightly (a59aba136 2024-03-28)
release: 1.79.0-nightly
commit-hash: a59aba136aab5510c16b0750a36cbd9916f91796
commit-date: 2024-03-28
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.2 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: NixOS 23.11.0 [64-bit]
@Nemo157 Nemo157 added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Apr 2, 2024
@weihanglo
Copy link
Member

Thanks for the report. I guess this is not a general issue but -Zgit/-Zgitoxide specific one?

@weihanglo weihanglo added A-diagnostics Area: Error and warning messages generated by Cargo itself. Z-gitoxide Nightly: gitoxide integration labels Apr 2, 2024
@Nemo157
Copy link
Member Author

Nemo157 commented Apr 2, 2024

Those do appear to be the only structs in the unstable config. I'm not sure where the general config-env-var parsing happens so I'm not certain if they're the only ones that are affected.

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 2, 2024

Found a way to trigger it with a stable config too:

> CARGO_TERM_PROGRESS_WIDTH=2 cargo check
error: missing field `when`

@weihanglo weihanglo added A-configuration Area: cargo config files and env vars A-environment-variables Area: environment variables S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed Z-gitoxide Nightly: gitoxide integration S-triage Status: This issue is waiting on initial triage. labels Apr 2, 2024
@linyihai
Copy link
Contributor

linyihai commented Apr 2, 2024

FYI, I have searched the code base, and have not found the place that triggers missing field, guess that this is related to the parsing process of the third-party library serde?

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 2, 2024

Yeah, the env-vars are read as part of a deserializer that merges the config files, env-vars and cli flags into one, then the error is produced by the Deserialize impl from ProgressConfig, you also get the same unhelpful error using the cli flag or config file:

> cargo check --config 'term.progress.width=2'
error: missing field `when`
> mkdir -p .cargo && echo 'term.progress.width=2' > .cargo/config.toml
> cargo check
error: missing field `when`

What would be most useful is more context on the error: what's the full config key that's being parsed and failed, and where are the other fields coming from (they can even come from multiple locations:

> CARGO_TERM_PROGRESS_WIDTH=2 cargo check --config 'term.progress.when="always"'
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s

@epage
Copy link
Contributor

epage commented Apr 2, 2024

Whats interesting about the config case is that its term.progress.when. Serde only reports when. We only know that the caller looked up term. I'm unsure if we can connect the dots to report term.progress.when without specializing the error reporting to each individual struct which is just playing whack-a-mole (like in #13687).

@epage
Copy link
Contributor

epage commented Apr 2, 2024

See #13694 to see what that looks like

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 2, 2024

I'm unsure if we can connect the dots to report term.progress.when

It looks like this is being tracked in the Deserializer as it enters into parsing each field of the structs:

self.de.key.push(field);

it feels like it should be possible to attach this context somewhere at this level when the error first passes back to the deserializer.

@epage
Copy link
Contributor

epage commented Apr 3, 2024

Good point. I do something similar in toml_edit, see https://github.com/toml-rs/toml/blob/48f968b449d64a3b7bcefab37c3f08438ba857ec/crates/toml_edit/src/de/table.rs#L171

However, I hit my time box I set aside for this as I need to be wrapping up some existing tasks and putting most of my focus to my 2024 Edition work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-diagnostics Area: Error and warning messages generated by Cargo itself. A-environment-variables Area: environment variables C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
4 participants