Inconsistent data in cargo metadata

[matklad]

Problem

cargo metadata produces inconsistent dependency graph (there's an edge to the node which does not exist)

Steps

$ git clone https://github.com/ChaseElectr/analyzer-panic
$ cd analyzer-panic
$ cargo metadata

Notice how the packages section has

"diesel 1.4.3 (git+https://github.com/ChaseElectr/diesel?branch=1.4.x#066bc412d7ead423e71de811b1dac9428a63712c)"

but the resolve section has

"diesel 1.4.3 (git+https://github.com/ChaseElectr/diesel?branch=1.4.x#24af5f700c111c2b995953a40f7a262d0872a00d)"

Notes

This is using master (c326fcb) cargo. Here's the gist with various printouts:

https://gist.github.com/matklad/eed0d46f427af5abf3c024cc07c59663

(I've added some printfs to the code which produces metadata)

This is what I gather from that output:

• Cargo.lock uses diesel #24af5f7, I assume this is what we want to see.
• cargo metadata uses the right #24af5f7 in the deps section, and the wrong one in the packages section.
• the package_map contains the wrong diesel (this I think the bug)
• the node_map contains both diesels.

I haven't looked into this further yet, as I am hoping someone from cargo team immediately knows what's the issue :)

[ChaseElectr]

I split my demo's commit to make it clear which modify made it wrong, you could check that :p

[ehuss]

I'm inclined to close this. After further inspection, it looks like the issue is related to specifying both a branch and a revision at the same time. Cargo specifically warns about this:

warning: dependency (diesel) specification is ambiguous. Only one of `branch`, `tag` or `rev` is allowed. This will be considered an error in future versions
warning: dependency (diesel_derives) specification is ambiguous. Only one of `branch`, `tag` or `rev` is allowed. This will be considered an error in future versions

Of course it's probably best for Cargo not to stumble on it, but since it isn't supported to start with, I'm not sure it is worth putting time into. We could change the warning to an error, since it has been around for a few years.

Does that sounds reasonable?

[matklad]

Yeah, I think upgrading to an error would be totally OK here. Well, maybe adding some asserts internally to elevate this to an ICE of sorts would also be prudent, but only if that doesn’t require much new logic.

[UebelAndre]

Hey, any updates here? 😅 🙏