cargo-upgrade --to-lockfile fails to upgrade renamed dependencies

[CAD97]

Example (from nushell):

〉playground〉cargo add tokio@0.1.0 --optional --rename tokio_01
    Updating crates.io index
      Adding tokio v0.1.0 to optional dependencies.
〉playground〉cargo update
    Updating crates.io index
# [snip]
〉playground〉open Cargo.lock | from toml | get package | where name == 'tokio'
╭───┬──────────────────────────────────────────────────────────────────┬─────────────────┬───────┬───────────────────────────────────────────────────────┬─────────╮
│ # │                             checksum                             │  dependencies   │ name  │                        source                         │ version │
├───┼──────────────────────────────────────────────────────────────────┼─────────────────┼───────┼───────────────────────────────────────────────────────┼─────────┤
│ 0 │ 5a09c0b5bb588872ab2f09afa13ee6e9dac11e10a0ec9e8e3ba39a5a5d530af6 │ [list 16 items] │ tokio │ registry+https://github.com/rust-lang/crates.io-index │ 0.1.22  │
╰───┴──────────────────────────────────────────────────────────────────┴─────────────────┴───────┴───────────────────────────────────────────────────────┴─────────╯
〉playground〉open Cargo.toml | get dependencies | get tokio_01
╭──────────┬───────╮
│ optional │ true  │
│ package  │ tokio │
│ version  │ 0.1.0 │
╰──────────┴───────╯
〉playground〉open Cargo.lock | from toml | get package | where name == 'tokio'
╭───┬──────────────────────────────────────────────────────────────────┬─────────────────┬───────┬───────────────────────────────────────────────────────┬─────────╮
│ # │                             checksum                             │  dependencies   │ name  │                        source                         │ version │
├───┼──────────────────────────────────────────────────────────────────┼─────────────────┼───────┼───────────────────────────────────────────────────────┼─────────┤
│ 0 │ 5a09c0b5bb588872ab2f09afa13ee6e9dac11e10a0ec9e8e3ba39a5a5d530af6 │ [list 16 items] │ tokio │ registry+https://github.com/rust-lang/crates.io-index │ 0.1.22  │
╰───┴──────────────────────────────────────────────────────────────────┴─────────────────┴───────┴───────────────────────────────────────────────────────┴─────────╯
〉playground〉cargo upgrade --to-lockfile
    Checking playground's dependencies
note: Re-run with `--pinned` to upgrade pinned version requirements
D:\git\cad97\playground〉open Cargo.toml | get dependencies | get tokio_01
╭──────────┬───────╮
│ optional │ true  │
│ package  │ tokio │
│ version  │ 0.1.0 │
╰──────────┴───────╯

[epage]

Can you confirm which version of cargo-upgrade you are using?

[CAD97]

I meant to include that in my demo but it didn't make it in 🙃

〉playground〉cargo upgrade -vV
cargo-upgrade 0.10.2
〉playground〉cargo -vV
cargo 1.62.1 (a748cf5a3 2022-06-08)
release: 1.62.1
commit-hash: a748cf5a3e666bc2dcdf54f37adef8ef22196452
commit-date: 2022-06-08
host: x86_64-pc-windows-msvc
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:Schannel)
os: Windows 10.0.22000 (Windows 10 Education) [64-bit]

[epage]

Oh right, forgot about the rename behavior.

If you run cargo-upgrade with --verbose, you'll get a warning that tokio_01 was left unchanged because we assume renamred dependencies are pinned dependencies.

We hint at this behavior with the note

note: Re-run with --pinned to upgrade pinned version requirements

As I'm thinking through this, some things I'm going to be considering (and welcome others thoughts on)

• Is cargo upgrade --to-lockfile --pinned sufficient for this resolving this?
• Why was --pinned not tried and what could improve discoverability?
• Does it make sense to treat renamed as pinned? I think in general, this example re-affirmed that with tokio_01 as the name implying that i is meant to not be upgraded to tokio 0.2
• Does the rename rule make sense for --to-lockfile?
  • Strictly speaking, probably not.
  • Would it cause confusion though to condition this on --to-lockfile's presence?
• Should --to-lockfile just imply --pinned?

[CAD97]

--pinned was tried... in the actual repo, not this example. --pinned functioned properly in the test example now that I tried it.

I think why it "failed" in the actual repo is that the precision was 0.1 and not 0.1.0, so cargo-upgrade decided to preserve that precision.

So I think the takeaway here would be

• Treat renames as "major version pinned" but not "minor version pinned"
  • Thus skip them in normal operation of not upgrading within a semver range, but upgrade them --to-lockfile
• --to-lockfile implying --pinned is probably absolutely fine
  • The pinned version is necessarily the one in the lockfile
  • For = pins, leave as is (assuming it matches the lockfile)
  • For < pins, upgrade the lower bound only
  • Thus making renames behave exactly as a "pinned ^"
• I was absolutely bamboozled by --to-lockfile not upgrading dependency requirements to the full precision in the lockfile.
  • Suggested resolution: add a --precise flag to upgrade to a full major.minor.patch requirement, and note if dependency requirements weren't upgraded (just when using --to-lockfile?) all the way to the lockfile
• Axiom: cargo upgrade --to-lockfile should make it so people depending on me get at least the versions in my lockfile / update --minimal-versions does not change the versions of direct dependencies
  • This is what I (apparently wrongly) assumed was the case and left me blaming cargo-upgrade for not upholding that axiom

[CAD97]

I remembered the big reason why I was blaming cargo-upgrade for having a bug: cargo upgrade -p tokio --to-lockfile --pinned (or equivalent) was running cleanly "no upgrade needed" despite me dancing between maximal and minimal versions in the lockfile and seeing the dependency version change. If upgrading a single dependency, upgrade absolutely should be verbose about what requirement was in the lockfile, what requirement is now in the lockfile, and what the full upgraded version is, even if no change occurs. Upgrading multiple deps should not note no-changes, but when only considering a single dep, that's the only state/information the user asked for.

---

Another note on renames; ideally:

In the specific case of

[dependencies."lib.name"]
package = "package.name"

this should not be considered a pinned dependency, and should be treated as a normal dependency.

There's two reasonable ways that this can come into being:

• The manifest has a number of other renamed dependencies, so the same form is used throughout for consistency
• lib.name != package.name, and the rename is included in the manifest to make this discrepancy locally evident
  • this means that name = { package = "name" might be a rename, because lib.name != package.name

Similarly, renaming to __dep maybe should be ignored, since __ is the convention to not show features even though it's publicly available. Though at the same time, __dep = { package = "dep", optional = true } may be the pattern for "phantom dependencies," so perhaps they should be considered pinned? Probably safer to not have the special case for certain renames not being considered pinned, and just have all (actually) renamed dependencies be pinned.

[epage]

e. If upgrading a single dependency, upgrade absolutely should be verbose about what requirement was in the lockfile, what requirement is now in the lockfile, and what the full upgraded version is, even if no change occurs

I think something that will help is that I'm considering adding a cargo outdated like report which will include the reason why a dependency was not upgraded or not fully upgraded.

Similarly, renaming to __dep maybe should be ignored, since __ is the convention to not show features even though it's publicly available

FYI the cargo team has not yet decided if it will adopt this docs.rs convention, see rust-lang/cargo#10794 (comment)

[epage]

I was absolutely bamboozled by --to-lockfile not upgrading dependency requirements to the full precision in the lockfile.
...
Suggested resolution: add a --precise flag to upgrade to a full major.minor.patch requirement

I lean towards keeping the current behavior and not adding a flag. I think the table view will help raise visibility of what is happening to be sufficient.

If someone doesn't like the existing behavior, its a one time change to their lock file (set full precision)

A flag effectively would automate that one-time change and doesn't seem to justify the cost of increasing the surface area of the UX.

[CAD97]

If someone doesn't like the existing behavior, its a one time change to their lock file [sic; manifest] (set full precision)

A flag effectively would automate that one-time change and doesn't seem to justify the cost of increasing the surface area of the UX.

While it is true it is a one-time cost to make the migration, in a workspace such as tokio-rs/tracing#2246, it's not a trivial change to do by hand due to there being multiple manifests involved. For that PR I did a s/= "(\d+\.\d+)"/$1.0/g search-replace to increase precision across the workspace.

A more difficult task would be doing so for a single dependency rather than all dependencies. Ideally, in such a minver correctness diff, instead of {upgrade to lockfile; downgrade to msrv} you'd upgrade each dependency only as-needed, which is significantly more work, especially if it involves manual steps¹ (e.g. to increase dependency precision).

Personally I would prefer --to-lockfile to increase precision (such that the dependency specification actually requires the version in the lockfile), but I can at least understand the choice otherwise :)

Footnotes

1. If cargo-upgrade supported increasing the precision, the process would be automatable. (Though perhaps it'd be better off using toml-edit directly rather than going through cargo-upgrade?) Roughly

   • for features in [--no-default-features, , --all-features]
   • while !cargo minimal-versions check $features
   • extract what crate failed to compile
   • if !cargo update -p $failed; cargo upgrade $failed --to-lockfile
   • find the direct dependency edge: cargo minimal-versions tree -i $failed
   • if !cargo update -p $direct; cargo upgrade $direct --to-lockfile
   • throw "manual intervention required to avoid minver-incorrect $failed"

   One of these days I'm going to get annoyed enough to write this tool. ↩