feat (patch): Display where the patch was defined in patch-related error messages

[omskscream]

What does this PR try to resolve?

Closes #13952

This PR adds location information of where each 'patch' was defined to the dependency override algorithm. Currently there're three possible sources: manifest Cargo.toml, one of the configuration files (listed here) or CLI option --config 'patch.crates-io.rand.path="rand"' (source).

Preserving location of where the patch was defined should simplify troubleshooting of incorrect or incompatible dependency override configuration.

This PR does not change:

• the way of toml files parsing
• the way of working with dependency locks
• the way of patch overlaps resolving (end of cargo::core::Workspace::root_patch function)
• related error messages happening in a different phases of the build, e.g. toml parsing

How to test and review this PR?

I suppose test changes should demonstrate the intended behavior change. Few earlier commits do not contain test changes because they do not change visible behavior of cargo but allow to proceed with the ones that do.

[rustbot]

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[weihanglo]

Thanks for the PR!

Haven't really started the review, though I feel like for [patch] from config we might want to reuse the config definition

[epage]

Note that while we ask for small commits, we ask for them to be atomic, including not having dead code, see https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request

For instance, Patch is added in a commit but not used.

[omskscream]

including not having dead code

Thank you for this comment! I was looking for the cargo specific commit guidance, but somehow missed this, even though it's in the repo itself.
Let me rebase things to make it more accurate.

[omskscream]

I feel like for [patch] from config we might want to reuse the config definition

Thank you for this comment!

I thought about this while working on the implementation.
Firstly, my understanding was that the [patch] might be defined in fewer places that context::Definition covers, e.g. patch can't be defined in environmental variables. So if it's not CLI or config file, we can display "in configuration" as a fallback location, and that shouldn't happen.
Secondly, I started with a bit simpler handling of patches in general, and to separate configuration files from CLI in the messages, and that was kept since the prototype but not needed, generally speaking.
Also, it seems that derived Copy trait for PatchLocation (added in early prototype) was conflicting with Definition, however it is not actually needed in my final changes.

Taking into account the above, I now don't see anything specific that is against using Definition, I can see a way to reuse it while keeping behavior the same.
I'll update this PR with all the changes and rebase again, short snippet of the idea:

pub enum PatchLocation {
    /// Defined in a manifest.
    Manifest(PathBuf),
    /// Defined in cargo configuration.
    Config(Definition),
}

[epage]

, e.g. patch can't be defined in environmental variables.

Is that assumption true? I'm not seeing documentation or any tests for it, so I;'m unsure.

Secondly, I started with a bit simpler handling of patches in general, and to separate configuration files from CLI in the messages, and that was kept since the prototype but not needed, generally speaking.

I see a lot of value in reusing our existing definition and keeping code paths as similar as possible. I'd like to better understand why CLI would be handled differently.

[omskscream]

I'm not seeing documentation or any tests for it, so I'm unsure

I wasn't sure for the same reasons, so I'll try adding more patch specific tests for different config locations. Not sure if it will make sense to test Definition::BuiltIn kind of patch definition -- it isn't from any external source, and I have doubts that cargo will ever introduce a default patch 🤔

why CLI would be handled differently

I think initially it was a way to separate file-based patch locations from "ephemeral" in my mind to clarify the requirements. Maybe it was unnecessary, but helped me to grasp the concept and proceed with the pull request 🙂

[weihanglo]

Not sure if it will make sense to test Definition::BuiltIn kind of patch definition

No need to test this.

why CLI would be handled differently

I guess the impression was also from the [patch] section doc that only talks about CLI and file configs but environment variable, and the [patch] doc in config also doesn't say whether environment variable is supported.

[weihanglo]

That is unfortunate 😞.

I guess it was ignored because the subtable got interpreted as [patch.crates.io]. If it were a complete registry name without any punctuation, it may work.

[epage]

non-blocking (likely shouldn't even be attempted in this PR to limit scope creep): these error messages are not in the style we're aiming for. The path is inserted in this PR where it makes sense for english but it further exacerbates the problem and could potentially make the lines really long, breaking up the text and making it harder to read.

Eventually we want to migrate to rustc's error reporter (#15944) which will help a lot (e,g, the patch location would be a source-less Origin) but that is a ways off.

It would be nice for us (doesn't have to be this PR's author) to do a follow up to clean these up.

[epage]

Unless there is a driving reason to do so, I think it would be better to not add this additional context through context as it makes things less flexible for how we could improve the wording of the error messages, see also a39d5c0#r2628751019

[omskscream]

I was thinking about unifying the error context for similar patch resolve problems. There are two similar messages with the same additional context in the very same function on lines L447 and L466

I fixed preposition in -> for in the additional commit to match those two exactly. Although, I see the reasoning. Will make sense to remove this extra context from here and from two other places mentioned above as well?

[epage]

Those should be cleaned up (oof, one even duplicates url) but that would not be blocking for this PR and would likely be best left to another PR to avoid adding extra review rounds just to get those "right"

[epage]

Thanks for your work on this! Adding this context will be really helpful to users!

[epage]

FYI feel free to rewrite commits. It is usually easier to come back and review when I'm able to look at atomic commits rather than having to keep in mind how future commits might change what I'm looking at.