Emit specific message for time<=0.3.35

[estebank]

error[E0282]: type annotations needed for `Box<_>`
  --> /home/gh-estebank/.cargo/registry/src/index.crates.io-6f17d22bba15001f/time-0.3.34/src/format_description/parse/mod.rs:83:9
   |
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
   = note: this is an inference error on `time` caused by a change in Rust 1.80.0; update `time` to version `>=0.3.36`

Partially mitigate the fallout from #127343. Although the biggest benefit of this would have been if we had had this in 1.80 before it became stable, the long-tail of that change will be felt for a long time, so better late than never.

We can also emit an even more targeted error instead of this inference failure.

[rustbot]

r? @compiler-errors

rustbot has assigned @compiler-errors.
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

[estebank]

This PR is missing a test because it relies on cargo paths and synthesizing that in a make will still be ugly. I verified with a minimal local project:

$ cargo new time-hack
Cargo.toml:

[package]
name = "time-hack"
version = "0.1.0"
edition = "2021"

[dependencies]
time = { version = "=0.3.34", features = ["parsing"] }

The output is the one in the PR description.

[compiler-errors]

Hard-coding an error message like this based off of path matching can likely be written using regex instead. This let chain is inscrutable to the point that I'd rather we not merge this if it can't be rewritten.

[compiler-errors]

Or, alternatively, I'd rather we not do path parsing here and just match on a combination of crate name and some other condition we can deduce from the program itself.

[compiler-errors]

like, are we even always going to be compiling time from a crates.io registry? Does this work correctly for vendored deps? This certainly doesn't work for non-cargo build tools, right?

[estebank]

Pattern matching on a path sounds more brittle. This check is horrible, I agree, but it at least handles paths across windows/unix correctly, and by being this specific in the number of checks we're less likely to wrongly trigger if someone just happens to put code in a directory called time-0.blah. I think we can likely split some of the elements into one or two closures to make the chain easier to read, though.

[compiler-errors]

This is already incredibly brittle. See that comment I just left. I'd rather we not make this hack rely on some special structure of the crates.io registry, or even depending on cargo itself (this is rustc after all), or not go with this at all if that's impossible.

[compiler-errors]

I also don't see why this isn't something that cargo should be detecting any time that a registry dependency fails to compile.

Any time we have a registry dep fail to compile, it should probably be saying something like "hey, try updating the version of crate xyz you depend on", since the fact that it was both published to the registry but now fails to compile suggests that there's probably something that happened in the mean time.

These "somethings" could include:

• Inference failures like this
• Soundness fixes
• Future-compat warnings that were turned into hard errors
• Accidental stabilizations that were rolled back

There are many reasons why a crate compiles on rust 1.x but not 1.x+1 that we regularly accept for one reason or the other, and I still don't think that we should or need to have a crate-specific hack for time in this case. Especially unless we're "timeboxing" such a hack to be removed once the only important difference -- that is, the impact of the regression -- has subsided.

[compiler-errors]

If you disagree, though, then I guess you probably should compiler-nominate this for team discussion, because I would opt to not approve this now, but I'd rather not be the single blocker for this hack.

[estebank]

I also don't see why this isn't something that cargo should be detecting any time that a registry dependency fails to compile.

I agree, our user experience around updates doesn't live up to the bar we set for ourselves.

There are many reasons why a crate compiles on rust 1.x but not 1.x+1 that we regularly accept for one reason or the other, and I still don't think that we should or need to have a crate-specific hack for time in this case.

If it was up to me, we'd have 1.80.2 without the new impl yesterday. This was a fuck up and we need to do something to mitigate the fallout as much as we can. 5% of all of crates.io broke, and we didn't even bother mitigating the fallout. An explicit check for this error is the very least we should do, and we can't wait for a full design-discussion-implementation-stabilization cycle of a cargo feature to that "right".

[compiler-errors]

i'll let someone else review this after the team decides on the action here

r? compiler

[jieyouxu]

I appreciate the efforts to help mitigate the fallout from the inference regression, but I'm also not very comfortable adding another crate-specific hack. If anything, we ought to be removing crate-specific hacks. I would prefer if T-compiler worked with T-libs-api and T-release to create a postmortem for the time regression to make our release process more robust. To help catch these inference regressions happen before they happen in the future, instead of adding hacks for mitigations after the fact. Since this has been nominated for T-compiler decision, I'll wait for that decision and respect that decision, even though I would prefer not having this hack myself.

[jieyouxu]

In any case, I'll write up a summary for T-compiler triage meeting.

[jieyouxu]

Summary for T-compiler

This PR (#129343) proposes to special-case an inference error diagnostic note for the time crate to help mitigate ecosystem fallout from #127343:

error[E0282]: type annotations needed for `Box<Vec<_>>`
  --> $DIR/detect-old-time-version-format_description-parse.rs:4:9
   |
LL |     let items = Box::new(vec![]);
   |         ^^^^^   ---------------- type must be known at this point
   |
   = note: this is an inference error on crate `time` caused by a change in Rust 1.80.0; update `time` to version `>=0.3.36`

Nominating for T-compiler discussion to decide if we want to merge this crate-specific diagnostic note.

[m-ou-se]

5% of all of crates.io broke

I don't agree with this framing. Cargo does not use the lock files from packages on crates.io by default. So if you pull anything from crates.io with cargo, it'll simply use the newest (semver compatible) version of the time crate, which compiles fine.

People working in projects with a Cargo.lock with an old time crate file are broken, yes (until they run cargo update), but that's not the same as 'packages on crates.io broke'.

If it was up to me, we'd have 1.80.2 without the new impl yesterday

That would be a breaking change too. That would break anyone who uses the new impls in 1.80.0 when they update their compiler again, but without a "cargo update" solution.

[apiraino]

Discussed in t-compiler triage on zulip.

This change is a bit controversial, we don't like hard-coding stuff for specific crates but it seems it's not the first time¹ and after all it's just a diagnostic message. That we pinky promise to remove in around six months or so. I hereby @apiraino take responsability to remind t-compiler in case the revert is forgotten 🙂

@rustbot label -I-compiler-nominated

Footnotes

1. see what I did there? ↩

[estebank]

@bors r-

[estebank]

@bors r=jieyouxu

[bors]

📌 Commit b013a3d has been approved by jieyouxu

It is now in the queue for this repository.

[apiraino]

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted