Intern PackageId

[dwijnand]

Refs #6207

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[dwijnand]

Interning SourceId broke some tests in the git source test suite (updating git precise revision). I don't know why it's happening and whether it's just a test artefact or not, so I just pulled that commit out for now (it's in a separate branch of mine).

[Eh2406]

Thank you for doing this! I don't know why the test are failing. It doesn't look to be the hash problems we found in #5153 and #5287, but I could be wrong.

[dwijnand]

Damn, it's the same tests that failed. Well at least I know where to look now.

[alexcrichton]

FWIW this looks good to me so far!

[dwijnand]

@alexcrichton / @Eh2406 Could you help me understand the failure here, please?

I've been studying and debugging git::two_deps_only_update_one for a number of hours today, and I think it's down to these log entries, right?

dep1 head sha: 41927a80
running `/d/cargo/target/debug/cargo update -p dep1`
    Updating git repository `file:///d/cargo/target/cit/t0/dep1`
 TRACE cargo::ops::resolve > previous: graph: Graph {
  - foo v0.5.0 (/d/cargo/target/cit/t0/foo)
    - dep1 v0.5.0 (file:///d/cargo/target/cit/t0/dep1#fb979dad)
  - dep1 v0.5.0 (file:///d/cargo/target/cit/t0/dep1#fb979dad)
}

features: {
}
 DEBUG cargo::ops::resolve > ignoring any lock pointing directly at dep1 v0.5.0 (file:///d/cargo/target/cit/t0/dep1#fb979dad)
 DEBUG cargo::ops::resolve > attempting to prefer foo v0.5.0 (/d/cargo/target/cit/t0/foo)
 DEBUG cargo::core::resolver > initial activation: foo v0.5.0 (/d/cargo/target/cit/t0/foo)
 TRACE cargo::core::resolver > activating foo v0.5.0 (/d/cargo/target/cit/t0/foo)
 TRACE cargo::core::resolver > foo[0]>dep1 1 candidates
 TRACE cargo::core::resolver > foo[0]>dep1 0 prev activations
 TRACE cargo::core::resolver > foo[0]>dep1 trying 0.5.0
 TRACE cargo::core::resolver > activating dep1 v0.5.0 (file:///d/cargo/target/cit/t0/dep1#fb979dad)
 TRACE cargo::core::resolver > resolved: graph: Graph {
  - dep1 v0.5.0 (file:///d/cargo/target/cit/t0/dep1#fb979dad)
  - foo v0.5.0 (/d/cargo/target/cit/t0/foo)
    - dep1 v0.5.0 (file:///d/cargo/target/cit/t0/dep1#fb979dad)
}

features: {
}

(RUST_LOG=cargo::ops::cargo_generate_lockfile=debug,cargo::core::resolver=trace,cargo::ops::resolve=trace,cargo::ops::resolvers::context=trace cargo test git::two_deps_only_update_one

The new sha for the HEAD of the dep1 dependency is at the top (41927a80) but the resolver reuses the source id from the lock file. Does that seem wrong?

[dwijnand]

Indeed, the same thing on master show the sha changing:

    Updating git repository `file:///d/cargo-master/target/cit/t0/dep1`
 TRACE cargo::ops::resolve > previous: graph: Graph {
  - dep1 v0.5.0 (file:///d/cargo-master/target/cit/t0/dep1#4ac9593f)
  - foo v0.5.0 (/d/cargo-master/target/cit/t0/foo)
    - dep1 v0.5.0 (file:///d/cargo-master/target/cit/t0/dep1#4ac9593f)
}

features: {
}
 DEBUG cargo::ops::resolve > ignoring any lock pointing directly at dep1 v0.5.0 (file:///d/cargo-master/target/cit/t0/dep1#4ac9593f)
 DEBUG cargo::ops::resolve > attempting to prefer foo v0.5.0 (/d/cargo-master/target/cit/t0/foo)
 DEBUG cargo::core::resolver > initial activation: foo v0.5.0 (/d/cargo-master/target/cit/t0/foo)
 TRACE cargo::core::resolver > activating foo v0.5.0 (/d/cargo-master/target/cit/t0/foo)
 TRACE cargo::core::resolver > foo[0]>dep1 1 candidates
 TRACE cargo::core::resolver > foo[0]>dep1 0 prev activations
 TRACE cargo::core::resolver > foo[0]>dep1 trying 0.5.0
 TRACE cargo::core::resolver > activating dep1 v0.5.0 (file:///d/cargo-master/target/cit/t0/dep1#666d8cf3)
 TRACE cargo::core::resolver > resolved: graph: Graph {
  - foo v0.5.0 (/d/cargo-master/target/cit/t0/foo)
    - dep1 v0.5.0 (file:///d/cargo-master/target/cit/t0/dep1#666d8cf3)
  - dep1 v0.5.0 (file:///d/cargo-master/target/cit/t0/dep1#666d8cf3)
}

features: {
}

[Eh2406]

So I think what is going on is that we end up with the two different PackageId one for 4ac9593f and one for 666d8cf3. The only difference is the source_id, but the two source_id's only differ by the sha. But the two source_id's Hash to the same value, and they eq with each other. So the interning decides that they are exchangeable.

I don't know how we solve this. Is there some way to let the interning know the "real" hash/eq for the source_id? I don't see how. I wish we had a cleaner way to separate all the different hacks for the different uses of hash/eq. I don't see a cleaner design without specialization.

I do have one (probably bad) idea. A PackageId is just a SourceId, a InternedString, and a semver::Version. If we intern the semver::Version then all of them are copy. Maybe we can decide that 3 pointers don't need to be interned, and just be declared copy.

[bors]

☔ The latest upstream changes (presumably #6347) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

@Eh2406 is right in that these are two SourceId instances that hash to the same value / are equal, but we rely on the fact that they behave differently at runtime to do various operations.

I think the PackageId interning could perhaps take this into account though? It won't be as bland as #[derive(Hash)] but with some handwritten impls I think we can get the same result?

[dwijnand]

That fixed the git tests, but now random other tests are failing:

    lockfile_compat::listed_checksum_bad_if_we_cannot_compute
    overrides::override_adds_some_deps
    update::conservative
    update::transitive_minor_update

I think this needs a rethink. This "sometimes equals, sometimes not" strategy is bound to leave holes. Can we not have distinct types for the different use cases?

[Eh2406]

I love the idea of getting away from "sometimes equals, sometimes not" mess. I don't know how infectious a new type would be, how many things have a PackageId as a field and sow would also need a new type, I honestly don't know. Worth a try as a separate PR, or at least a tracking issue.

[dwijnand]

Nice one, @Eh2406! The test suite now passes.

[Eh2406]

Looks correct. The test pass, and Alex said (on the other PR) to trust the tests. So:
@bors: r+

[bors]

📌 Commit efd03bd has been approved by Eh2406

[bors]

⌛ Testing commit efd03bd with merge 151c225...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: Eh2406
Pushing 151c225 to master...