Emit specific message for time<=0.3.35 #129343

estebank · 2024-08-21T03:20:49Z

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 · 2024-08-21T03:20:56Z

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 · 2024-08-21T03:23:10Z

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 · 2024-08-21T03:25:51Z

compiler/rustc_trait_selection/src/error_reporting/infer/need_type_info.rs

+            if component == "registry"
+                && let Some(next) = components.next()
+                && let std::path::Component::Normal(next) = next
+                && next == "src"
+                && let Some(next) = components.next()
+                && let std::path::Component::Normal(next) = next
+                && next.to_string_lossy().starts_with("index.")
+                && let Some(next) = components.next()
+                && let std::path::Component::Normal(next) = next
+                && let next = next.to_string_lossy()
+                && next.starts_with("time-0.")
+                && let Some(version) = next.split('-').skip(1).next()
+                && let mut segments = version.split('.')
+                && let Some(major) = segments.next()
+                && major == "0"
+                && let Some(minor) = segments.next()
+                && minor == "3"


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.

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.

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?

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.

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/rustc_trait_selection/src/error_reporting/infer/need_type_info.rs

compiler-errors · 2024-08-21T04:23:54Z

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 · 2024-08-21T04:24:42Z

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.

compiler/rustc_trait_selection/src/error_reporting/infer/need_type_info.rs

estebank · 2024-08-21T04:33:41Z

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 · 2024-08-21T04:39:43Z

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

r? compiler

jieyouxu · 2024-08-21T05:03:05Z

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 · 2024-08-21T05:05:35Z

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

compiler/rustc_trait_selection/src/error_reporting/infer/need_type_info.rs

jieyouxu · 2024-08-21T05:12:18Z

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.

compiler/rustc_trait_selection/src/error_reporting/infer/need_type_info.rs

m-ou-se · 2024-08-22T11:04:37Z

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 · 2024-08-22T15:23:01Z

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

see what I did there? ↩

bors · 2024-08-28T23:38:57Z

📌 Commit b013a3d has been approved by estebank

It is now in the queue for this repository.

estebank · 2024-08-28T23:39:15Z

@bors r-

estebank · 2024-08-28T23:39:31Z

@bors r=jieyouxu

bors · 2024-08-28T23:39:33Z

📌 Commit b013a3d has been approved by jieyouxu

It is now in the queue for this repository.

Emit specific message for time<=0.3.35 ``` 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 rust-lang#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.

…kingjubilee Rollup of 14 pull requests Successful merges: - rust-lang#128192 (rustc_target: Add various aarch64 features) - rust-lang#129170 (Add an ability to convert between `Span` and `visit::Location`) - rust-lang#129343 (Emit specific message for time<=0.3.35) - rust-lang#129378 (Clean up cfg-gating of ProcessPrng extern) - rust-lang#129401 (Partially stabilize `feature(new_uninit)`) - rust-lang#129467 (derive(SmartPointer): assume pointee from the single generic and better error messages) - rust-lang#129494 (format code in tests/ui/threads-sendsync) - rust-lang#129617 (Update books) - rust-lang#129673 (Add fmt::Debug to sync::Weak<T, A>) - rust-lang#129683 (copysign with sign being a NaN can have non-portable results) - rust-lang#129689 (Move `'tcx` lifetime off of impl and onto methods for `CrateMetadataRef`) - rust-lang#129695 (Fix path to run clippy on rustdoc) - rust-lang#129712 (Correct trusty targets to be tier 3) - rust-lang#129715 (Update `compiler_builtins` to `0.1.123`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#129343 - estebank:time-version, r=jieyouxu Emit specific message for time<=0.3.35 ``` 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 rust-lang#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.

apiraino · 2024-08-29T14:13:06Z

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

[beta] backports - Emit specific message for `time<0.3.35` inference failure rust-lang#129343 - Use a reduced recursion limit in the MIR inliner's cycle breaker rust-lang#129714 - rustdoc: do not run doctests with invalid langstrings rust-lang#128838 r? cuviper

rustbot assigned compiler-errors Aug 21, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 21, 2024

estebank force-pushed the time-version branch from b4414a1 to 5446e27 Compare August 21, 2024 03:24

compiler-errors requested changes Aug 21, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2024

This comment has been minimized.

Sign in to view

compiler-errors reviewed Aug 21, 2024

View reviewed changes

compiler/rustc_trait_selection/src/error_reporting/infer/need_type_info.rs Outdated Show resolved Hide resolved

estebank added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Aug 21, 2024

rustbot assigned jieyouxu and unassigned compiler-errors Aug 21, 2024

jieyouxu added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2024

jieyouxu reviewed Aug 21, 2024

View reviewed changes

compiler/rustc_trait_selection/src/error_reporting/infer/need_type_info.rs Outdated Show resolved Hide resolved

yotamofek reviewed Aug 21, 2024

View reviewed changes

compiler/rustc_trait_selection/src/error_reporting/infer/need_type_info.rs Outdated Show resolved Hide resolved

estebank mentioned this pull request Aug 22, 2024

Revert "alloc: implement FromIterator for Box<str>" #129379

Closed

rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Aug 22, 2024

estebank force-pushed the time-version branch from bbf8317 to ad82adc Compare August 22, 2024 15:24

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 28, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2024

This was referenced Aug 29, 2024

Rollup of 14 pull requests #129719

Closed

Rollup of 14 pull requests #129721

Merged

bors merged commit 26f75a6 into rust-lang:master Aug 29, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 29, 2024

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 29, 2024

cuviper mentioned this pull request Aug 29, 2024

[beta] backports #129758

Merged

cuviper modified the milestones: 1.82.0, 1.81.0 Aug 29, 2024

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit specific message for time<=0.3.35 #129343

Emit specific message for time<=0.3.35 #129343

estebank commented Aug 21, 2024 •

edited

Loading

rustbot commented Aug 21, 2024

estebank commented Aug 21, 2024

compiler-errors Aug 21, 2024

compiler-errors Aug 21, 2024

compiler-errors Aug 21, 2024

estebank Aug 21, 2024

compiler-errors Aug 21, 2024 •

edited

Loading

This comment has been minimized.

compiler-errors commented Aug 21, 2024 •

edited

Loading

compiler-errors commented Aug 21, 2024

estebank commented Aug 21, 2024

compiler-errors commented Aug 21, 2024

jieyouxu commented Aug 21, 2024 •

edited

Loading

jieyouxu commented Aug 21, 2024

jieyouxu commented Aug 21, 2024 •

edited

Loading

m-ou-se commented Aug 22, 2024

apiraino commented Aug 22, 2024 •

edited

Loading

bors commented Aug 28, 2024

estebank commented Aug 28, 2024

estebank commented Aug 28, 2024

bors commented Aug 28, 2024

apiraino commented Aug 29, 2024

Emit specific message for time<=0.3.35 #129343

Emit specific message for time<=0.3.35 #129343

Conversation

estebank commented Aug 21, 2024 • edited Loading

rustbot commented Aug 21, 2024

estebank commented Aug 21, 2024

compiler-errors Aug 21, 2024

Choose a reason for hiding this comment

compiler-errors Aug 21, 2024

Choose a reason for hiding this comment

compiler-errors Aug 21, 2024

Choose a reason for hiding this comment

estebank Aug 21, 2024

Choose a reason for hiding this comment

compiler-errors Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

compiler-errors commented Aug 21, 2024 • edited Loading

compiler-errors commented Aug 21, 2024

estebank commented Aug 21, 2024

compiler-errors commented Aug 21, 2024

jieyouxu commented Aug 21, 2024 • edited Loading

jieyouxu commented Aug 21, 2024

jieyouxu commented Aug 21, 2024 • edited Loading

Summary for T-compiler

m-ou-se commented Aug 22, 2024

apiraino commented Aug 22, 2024 • edited Loading

Footnotes

bors commented Aug 28, 2024

estebank commented Aug 28, 2024

estebank commented Aug 28, 2024

bors commented Aug 28, 2024

apiraino commented Aug 29, 2024

estebank commented Aug 21, 2024 •

edited

Loading

compiler-errors Aug 21, 2024 •

edited

Loading

compiler-errors commented Aug 21, 2024 •

edited

Loading

jieyouxu commented Aug 21, 2024 •

edited

Loading

jieyouxu commented Aug 21, 2024 •

edited

Loading

apiraino commented Aug 22, 2024 •

edited

Loading