Update to Rust 1.90 #6958

robertbastian · 2025-09-18T22:49:23Z

1.90 improves dead code detection, so I had to do some cleanups to pass CI.

sffc

I am happy that DCE is more aggressive, but I think it's swinging a bit far, and we really should avoid reaching for allow(dead_code).

I didn't comment everywhere there was a problem because there were too many.

sffc · 2025-09-19T08:03:54Z

components/calendar/benches/date.rs

 // (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).

-mod fixtures;
+use serde::{Deserialize, Serialize};


Suggestion: make the change smaller by keeping the fixture definitions in the separate file, and just not including them in the file where they are not used

the problem is that multiple bench files include the same fixtures module, and some fixtures are unused in each benchmark. this puts the fixtures for each benchmark in the benchmark

Oh I see, that's terrible...

I think well commented allow(dead_code) in a central file might be better

there's no shared code so this is better

sffc · 2025-09-19T08:06:01Z

components/datetime/src/provider/time_zones.rs

-    #[cfg_attr(feature = "datagen", databake(path = icu_datetime::provider::time_zones))]
    #[cfg_attr(feature = "serde", derive(serde::Deserialize))]
    #[yoke(prove_covariance_manually)]
+    #[allow(dead_code)]


Issue: allowing dead code is bad; why do you need if?

this is the one place where I don't know why, Rust says it's never constructed so it doesn't seem to detect when it's constructed from deserialization through Yoke

If that's so, it sounds like a bug, potentially a bad bug if rustc ever tries to use the same heuristic to strip symbols from the binary or something

definitely not the same "heuristics", DCE happens way later when types and source locations are long gone

sffc · 2025-09-19T08:08:45Z

provider/source/src/cldr_serde/currencies/supplemental.rs

-//! <https://github.com/unicode-cldr/cldr-core/blob/master/supplemental/currencyData.json>
-
-use serde::Deserialize;
-use std::collections::BTreeMap;


Issue: if we're not using these, it's a bug. Ideally the datagen code would have been added in the same PR as the safe serde code, but that doesn't mean we should just delete the impls. Please at least make them cfg test with a little test?

I have no idea what these are, I'm certainly not going to start writing tests for them.

One of these files is not even included as module anywhere.

They are for parsing supplemental currency data. I've been asking for @younies to finish up the currency component and he probably added these impls but doesn't use them yet, but we will be needing them.

sffc · 2025-09-19T08:10:20Z

provider/source/src/datetime/legacy.rs

 //! TODO(#5613): Even though these markers are no longer exported, we need them in order to export
 //! semantic skeleton data markers. This should be refactored to skip the intermediate data struct.

+#![allow(dead_code)] // so many unused fields


Issue: I want to eventually get rid of this whole file. If fields are actually dead, delete them?

I did do that whole work, but it's super messy because you end up with structs with single or even no fields, and I think it makes the tests a lot harder to parse because you lose a lot of semantics of what these are. So I reverted to this.

I haven't seen all of what we'd be losing?

Cfg(test) isn't sufficient?

let's keep this massive cleanup for a different PR please

Cfg(test) isn't sufficient?

there are fields here that are used in code, there are fields here that are used only in tests, there are fields here that are unused. to properly clean this up, the tests need to be rewritten

These blanket allows make future improvements harder because we won't notice when we actually can delete code

A file containing code we actually want to delete is like the worst file to put a blanket suppression

sffc · 2025-09-19T08:12:45Z

provider/source/src/properties/uprops_serde.rs


+#[cfg(any(feature = "use_wasm", feature = "use_icu4c"))]
 pub(crate) mod code_point_prop {
-    #[derive(serde::Deserialize)]


Issue: again, cldr_serde is the one place where I think we should keep the whole struct. Also, it's not completely dead code: it makes the Deserialize impls check that those fields are well formed.

eh, I don't think we generally assert fields that we don't use, I don't think it's that valuable

sffc · 2025-09-19T08:14:50Z

utils/zerovec/src/varzerovec/serde.rs

    }

    #[derive(serde::Serialize, serde::Deserialize)]
+    #[expect(dead_code)]


Nit: comment why

because we test the derive macro but never construct these

I meant add a comment in the code

so I put a comment on the DeriveTest structs saying "only tests derive macro"?

#[expect(dead_code)] // tests derive macro

seems stupid but if you insist

sffc

Please make allow dead_code as narrow as possible, on individual fields even, and leave code comments each time explaining why it is needed

robertbastian · 2025-09-19T08:47:33Z

leave code comments each time explaining why it is needed

yeah I'm just not going to do that for the legacy datetime stuff

if this is a blocker then I'll close this PR and we can update the toolchain once that code has been cleaned up

Update to Rust 1.90

7d13d85

robertbastian force-pushed the 90 branch from 0e20b98 to 7d13d85 Compare September 19, 2025 07:46

robertbastian marked this pull request as ready for review September 19, 2025 07:53

robertbastian requested review from Manishearth, sffc, younies, zbraniecki and a team as code owners September 19, 2025 07:53

sffc requested changes Sep 19, 2025

View reviewed changes

robertbastian requested a review from sffc September 19, 2025 08:23

sffc requested changes Sep 19, 2025

View reviewed changes

robertbastian closed this Sep 19, 2025

Update to Rust 1.90 #6958

Update to Rust 1.90 #6958

Uh oh!

Conversation

robertbastian commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

robertbastian commented Sep 19, 2025

Uh oh!

Uh oh!

robertbastian commented Sep 18, 2025 •

edited

Loading

sffc Sep 19, 2025 •

edited

Loading

robertbastian Sep 19, 2025 •

edited

Loading