Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regression: mismatched types (panic info hook stuff) #128890

Closed
BoxyUwU opened this issue Aug 9, 2024 · 15 comments · Fixed by Lokathor/yacurses#8
Closed

regression: mismatched types (panic info hook stuff) #128890

BoxyUwU opened this issue Aug 9, 2024 · 15 comments · Fixed by Lokathor/yacurses#8
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Aug 9, 2024

[INFO] [stdout] error[E0308]: mismatched types
[INFO] [stdout]    --> src/lib.rs:122:36
[INFO] [stdout]     |
[INFO] [stdout] 122 |       std::panic::set_hook(replace(&mut self.old_hook, Box::new(|_| {})));
[INFO] [stdout]     |                            ------- ^^^^^^^^^^^^^^^^^^ expected `PanicHookInfo<'_>`, found `PanicInfo<'_>`
[INFO] [stdout]     |                            |
[INFO] [stdout]     |                            arguments to this function are incorrect
[INFO] [stdout]     |
[INFO] [stdout]     = note: expected mutable reference `&mut Box<dyn for<'a, 'b> std::ops::Fn(&'a PanicHookInfo<'b>) + Send + Sync>`
[INFO] [stdout]                found mutable reference `&mut Box<(dyn for<'a, 'b> std::ops::Fn(&'a PanicInfo<'b>) + Send + Sync + 'static)>`
@BoxyUwU BoxyUwU added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 9, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 9, 2024
@BoxyUwU BoxyUwU removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 9, 2024
@compiler-errors
Copy link
Member

Regressed in #115974, cc @m-ou-se @Amanieu

@workingjubilee
Copy link
Member

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 12, 2024
@m-ou-se m-ou-se added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 14, 2024
@Amanieu
Copy link
Member

Amanieu commented Aug 14, 2024

We discussed this in the libs meeting today. This is more breakage than we expected when we originally landed this, so we're going to revert this for now in beta and discuss ways to land this without as much breakage (possibly via an edition change).

@VorpalBlade
Copy link

possibly via an edition change

At this point that means this will be blocked for another 3 years though.

@ChrisDenton
Copy link
Member

It seems like there's three main crates that error:

  • yacurses 0.2.4 (current version)
  • tezos-smart-rollup-entrypoint 0.2.2 (current version)
  • crashy 0.1 (old version)

There was also a test only failure in static_init 1.0.3 (current version, not updated in 2 years). And something called baller-bot v0.0.0 failed which I couldn't immediately find the source of.

@Lokathor
Copy link
Contributor

Lokathor commented Aug 14, 2024

Hi, yacurses dev here, if there's some update you want me to publish I'd be happy to, if that would help move things forward.

Within the context of yacurses, the panic hook replacer is because the panic info needs to be printed after curses mode is deactivated instead of before, otherwise the panic message gets eaten by curses mode. Exactly the types involved or whatever really aren't important, so I'd be happy to merge a PR and push a new version. I don't think the crate is particularly used, I certainly don't use it myself any more.

@ChrisDenton
Copy link
Member

As far as I can see, yacurses is not a no_std crate so that failure could be fixed simple by importing std::panic::PanicInfo instead of importing it from core (as is currently the case).

@workingjubilee
Copy link
Member

@ChrisDenton the ballerbot repo is this one: https://github.com/SNIAPA/screeps-ballerbot

@m-ou-se
Copy link
Member

m-ou-se commented Aug 15, 2024

Reverting this and waiting for another way to handle this will likely just result in this change never happening, permanently blocking things like #66745 and #125175

@m-ou-se
Copy link
Member

m-ou-se commented Aug 15, 2024

It seems like there's three main crates that error:

* yacurses 0.2.4 (current version)

* tezos-smart-rollup-entrypoint 0.2.2 (current version)

* crashy 0.1 (old version)

There was also a test only failure in static_init 1.0.3 (current version, not updated in 2 years). And something called baller-bot v0.0.0 failed which I couldn't immediately find the source of.

The crashy crate has also already been updated in their repository: https://gitlab.com/bitpowder/indigo-ng/-/blob/5302aab72fbe4e495d0fb759e2a0294d908f6860/crashy/src/lib.rs#L293

Overall, I don't agree the breakage is big enough to warrant something like an edition hack (or permanently stalling related features).

@workingjubilee
Copy link
Member

workingjubilee commented Aug 16, 2024

When I pointed out this was cratered, FCPed with knowledge of the cost, and accepted, yet offered no remark otherwise? I meant that as a way of suggesting that this issue should probably simply be closed, because we already decided this was okay.

I struggle to make sense of T-libs-api FCPing this, knowing that a few edge-case crates were broken, yet balking at this amount. Of course finding N crates broken in the craterbot check run becomes N+M in the beta's craterbot build-and-test run.

I especially struggle to make sense of it while T-libs-api also takes no action except saying "deal with it" on countless other cases of breaking inference like #127343 which have proven to be way more disruptive to the actual ecosystem, because they hit core ecosystem crates. Not to dig on stuff like yacurses and the other crates here, but collectively they are dwarfed by the sheer number of instances of time in lockfiles. And that PR was not cratered and FCPed with full knowledge of the breakage.

And that much wider breakage was for the sake of a FromIterator impl... a pretty convenience... and not something that actually allows us to solve fundamental problems with making sense of our error-reporting story in Rust.

@workingjubilee
Copy link
Member

workingjubilee commented Aug 16, 2024

And if we're going to rank the value of something mostly for its pretty conveniences, the pretty conveniences that #115974 enables (assuming I correctly understand that #126732 effectively depends on it) are pretty pretty and pretty convenient: SeleDreams/libnds-sys@252a940

@Amanieu
Copy link
Member

Amanieu commented Aug 20, 2024

We discussed this again in the libs-api meeting today. Considering the fact that the impact is minor and the affected crates are already fixed, we chose to not revert this in the end. We had some concerns about std no longer being a strict superset of core, but this alias is clearly marked as deprecated and can be removed in a future edition.

@Amanieu Amanieu closed this as completed Aug 20, 2024
@apiraino
Copy link
Contributor

ok thanks Amanieu and T-libs

@rustbot label -P-high -I-libs-api-nominated

@rustbot rustbot removed P-high High priority I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Aug 21, 2024
@Mark-Simulacrum Mark-Simulacrum added this to the 1.81.0 milestone Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.