Skip to content

Commit 3120b98

Browse files
blyxyasblyxyas
authored
Optimize allow_unwrap_types evaluation to eliminate performance regression (rust-lang#16652)
Fixes rust-lang#16605 This PR addresses the 6.2% compilation performance regression introduced by the original `allow_unwrap_types` implementation (which natively allocated AST strings on every `unwrap` call via `bumpalo`). The implementation has been refactored to pre-resolve types securely without a performance penalty: **Pre-Resolution Cache via `LateLintPass`**: Instead of executing `ty.to_string()` and `lookup_path_str` inside the immediate `unwrap_expect_used` hot check for every method call encountered, `allow-unwrap-types` configuration strings are now parsed exactly *once* per crate compilation during the `LateLintPass::check_crate` hook in `clippy_lints/src/methods/mod.rs`. **`DefId` Native Storage**: The parsed `DefId` representations are stored in-memory using highly efficient caching maps directly on the main `Methods` struct: - `unwrap_allowed_ids: FxHashSet<DefId>`: Allows instant O(1) lookups for standard types. - `unwrap_allowed_aliases: Vec<DefId>`: Tracks type aliases requiring signature substitution checking later. **Execution Relief**: Inside `unwrap_expect_used::check()`, string manipulation is completely removed and `ty::Adt` checking uses lightning-fast `.contains(&adt.did())` operations to categorically avoid regression allocation loops. This implementation strongly parallels the pre-resolution type-routing logic in `clippy_config::types::create_disallowed_map` without altering the core `clippy.toml` config requirements. <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_SUMMARY_START --> ### Summary Notes - [Beta-nomination](rust-lang#16652 (comment)) by [samueltardieu](https://github.com/samueltardieu) *Managed by `@rustbot`—see [help](https://forge.rust-lang.org/triagebot/note.html) for details* <!-- TRIAGEBOT_SUMMARY_END --> <!-- TRIAGEBOT_END --> changelog: none
2 parents e492d02 + 7ac8be4 commit 3120b98

File tree

2 files changed

+64
-48
lines changed

2 files changed

+64
-48
lines changed

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4761,6 +4761,8 @@ pub struct Methods {
47614761
allowed_dotfiles: FxHashSet<&'static str>,
47624762
format_args: FormatArgsStorage,
47634763
allow_unwrap_types: Vec<String>,
4764+
unwrap_allowed_ids: FxHashSet<rustc_hir::def_id::DefId>,
4765+
unwrap_allowed_aliases: Vec<rustc_hir::def_id::DefId>,
47644766
}
47654767

47664768
impl Methods {
@@ -4778,6 +4780,8 @@ impl Methods {
47784780
allowed_dotfiles,
47794781
format_args,
47804782
allow_unwrap_types: conf.allow_unwrap_types.clone(),
4783+
unwrap_allowed_ids: FxHashSet::default(),
4784+
unwrap_allowed_aliases: Vec::new(),
47814785
}
47824786
}
47834787
}
@@ -4953,6 +4957,19 @@ pub fn method_call<'tcx>(recv: &'tcx Expr<'tcx>) -> Option<(Symbol, &'tcx Expr<'
49534957
}
49544958

49554959
impl<'tcx> LateLintPass<'tcx> for Methods {
4960+
fn check_crate(&mut self, cx: &LateContext<'tcx>) {
4961+
for s in &self.allow_unwrap_types {
4962+
let def_ids = clippy_utils::paths::lookup_path_str(cx.tcx, clippy_utils::paths::PathNS::Type, s);
4963+
for def_id in def_ids {
4964+
if cx.tcx.def_kind(def_id) == rustc_hir::def::DefKind::TyAlias {
4965+
self.unwrap_allowed_aliases.push(def_id);
4966+
} else {
4967+
self.unwrap_allowed_ids.insert(def_id);
4968+
}
4969+
}
4970+
}
4971+
}
4972+
49564973
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
49574974
if expr.span.from_expansion() {
49584975
return;
@@ -4976,7 +4993,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
49764993
self.allow_expect_in_tests,
49774994
self.allow_unwrap_in_consts,
49784995
self.allow_expect_in_consts,
4979-
&self.allow_unwrap_types,
4996+
&self.unwrap_allowed_ids,
4997+
&self.unwrap_allowed_aliases,
49804998
);
49814999
},
49825000
ExprKind::MethodCall(..) => {
@@ -5730,7 +5748,8 @@ impl Methods {
57305748
false,
57315749
self.allow_expect_in_consts,
57325750
self.allow_expect_in_tests,
5733-
&self.allow_unwrap_types,
5751+
&self.unwrap_allowed_ids,
5752+
&self.unwrap_allowed_aliases,
57345753
unwrap_expect_used::Variant::Expect,
57355754
);
57365755
expect_fun_call::check(cx, &self.format_args, expr, method_span, recv, arg);
@@ -5743,7 +5762,8 @@ impl Methods {
57435762
true,
57445763
self.allow_expect_in_consts,
57455764
self.allow_expect_in_tests,
5746-
&self.allow_unwrap_types,
5765+
&self.unwrap_allowed_ids,
5766+
&self.unwrap_allowed_aliases,
57475767
unwrap_expect_used::Variant::Expect,
57485768
);
57495769
},
@@ -5764,7 +5784,8 @@ impl Methods {
57645784
false,
57655785
self.allow_unwrap_in_consts,
57665786
self.allow_unwrap_in_tests,
5767-
&self.allow_unwrap_types,
5787+
&self.unwrap_allowed_ids,
5788+
&self.unwrap_allowed_aliases,
57685789
unwrap_expect_used::Variant::Unwrap,
57695790
);
57705791
},
@@ -5776,7 +5797,8 @@ impl Methods {
57765797
true,
57775798
self.allow_unwrap_in_consts,
57785799
self.allow_unwrap_in_tests,
5779-
&self.allow_unwrap_types,
5800+
&self.unwrap_allowed_ids,
5801+
&self.unwrap_allowed_aliases,
57805802
unwrap_expect_used::Variant::Unwrap,
57815803
);
57825804
},

‎clippy_lints/src/methods/unwrap_expect_used.rs‎

Lines changed: 37 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ pub(super) fn check(
4444
is_err: bool,
4545
allow_unwrap_in_consts: bool,
4646
allow_unwrap_in_tests: bool,
47-
allow_unwrap_types: &[String],
47+
unwrap_allowed_ids: &rustc_data_structures::fx::FxHashSet<rustc_hir::def_id::DefId>,
48+
unwrap_allowed_aliases: &[rustc_hir::def_id::DefId],
4849
variant: Variant,
4950
) {
5051
let ty = cx.typeck_results().expr_ty(recv).peel_refs();
@@ -66,51 +67,39 @@ pub(super) fn check(
6667

6768
let method_suffix = if is_err { "_err" } else { "" };
6869

69-
let ty_name = ty.to_string();
70-
if allow_unwrap_types
71-
.iter()
72-
.any(|allowed_type| ty_name.starts_with(allowed_type) || ty_name == *allowed_type)
70+
if let ty::Adt(adt, _) = ty.kind()
71+
&& unwrap_allowed_ids.contains(&adt.did())
7372
{
7473
return;
7574
}
7675

77-
for s in allow_unwrap_types {
78-
let def_ids = clippy_utils::paths::lookup_path_str(cx.tcx, clippy_utils::paths::PathNS::Type, s);
79-
for def_id in def_ids {
80-
if let ty::Adt(adt, _) = ty.kind()
81-
&& adt.did() == def_id
82-
{
83-
return;
84-
}
85-
if cx.tcx.def_kind(def_id) == DefKind::TyAlias {
86-
let alias_ty = cx.tcx.type_of(def_id).instantiate_identity();
87-
if let (ty::Adt(adt, substs), ty::Adt(alias_adt, alias_substs)) = (ty.kind(), alias_ty.kind())
88-
&& adt.did() == alias_adt.did()
89-
{
90-
let mut all_match = true;
91-
for (arg, alias_arg) in substs.iter().zip(alias_substs.iter()) {
92-
if let (Some(arg_ty), Some(alias_arg_ty)) = (arg.as_type(), alias_arg.as_type()) {
93-
if matches!(alias_arg_ty.kind(), ty::Param(_)) {
94-
continue;
95-
}
96-
if let (ty::Adt(arg_adt, _), ty::Adt(alias_arg_adt, _)) =
97-
(arg_ty.peel_refs().kind(), alias_arg_ty.peel_refs().kind())
98-
{
99-
if arg_adt.did() != alias_arg_adt.did() {
100-
all_match = false;
101-
break;
102-
}
103-
} else if arg_ty != alias_arg_ty {
104-
all_match = false;
105-
break;
106-
}
107-
}
76+
for &def_id in unwrap_allowed_aliases {
77+
let alias_ty = cx.tcx.type_of(def_id).instantiate_identity();
78+
if let (ty::Adt(adt, substs), ty::Adt(alias_adt, alias_substs)) = (ty.kind(), alias_ty.kind())
79+
&& adt.did() == alias_adt.did()
80+
{
81+
let mut all_match = true;
82+
for (arg, alias_arg) in substs.iter().zip(alias_substs.iter()) {
83+
if let (Some(arg_ty), Some(alias_arg_ty)) = (arg.as_type(), alias_arg.as_type()) {
84+
if matches!(alias_arg_ty.kind(), ty::Param(_)) {
85+
continue;
10886
}
109-
if all_match {
110-
return;
87+
if let (ty::Adt(arg_adt, _), ty::Adt(alias_arg_adt, _)) =
88+
(arg_ty.peel_refs().kind(), alias_arg_ty.peel_refs().kind())
89+
{
90+
if arg_adt.did() != alias_arg_adt.did() {
91+
all_match = false;
92+
break;
93+
}
94+
} else if arg_ty != alias_arg_ty {
95+
all_match = false;
96+
break;
11197
}
11298
}
11399
}
100+
if all_match {
101+
return;
102+
}
114103
}
115104
}
116105

@@ -149,7 +138,8 @@ pub(super) fn check_call(
149138
allow_unwrap_in_tests: bool,
150139
allow_expect_in_consts: bool,
151140
allow_expect_in_tests: bool,
152-
allow_unwrap_types: &[String],
141+
unwrap_allowed_ids: &rustc_data_structures::fx::FxHashSet<rustc_hir::def_id::DefId>,
142+
unwrap_allowed_aliases: &[rustc_hir::def_id::DefId],
153143
) {
154144
let Some(recv) = args.first() else {
155145
return;
@@ -167,7 +157,8 @@ pub(super) fn check_call(
167157
false,
168158
allow_unwrap_in_consts,
169159
allow_unwrap_in_tests,
170-
allow_unwrap_types,
160+
unwrap_allowed_ids,
161+
unwrap_allowed_aliases,
171162
Variant::Unwrap,
172163
);
173164
},
@@ -179,7 +170,8 @@ pub(super) fn check_call(
179170
false,
180171
allow_expect_in_consts,
181172
allow_expect_in_tests,
182-
allow_unwrap_types,
173+
unwrap_allowed_ids,
174+
unwrap_allowed_aliases,
183175
Variant::Expect,
184176
);
185177
},
@@ -191,7 +183,8 @@ pub(super) fn check_call(
191183
true,
192184
allow_unwrap_in_consts,
193185
allow_unwrap_in_tests,
194-
allow_unwrap_types,
186+
unwrap_allowed_ids,
187+
unwrap_allowed_aliases,
195188
Variant::Unwrap,
196189
);
197190
},
@@ -203,7 +196,8 @@ pub(super) fn check_call(
203196
true,
204197
allow_expect_in_consts,
205198
allow_expect_in_tests,
206-
allow_unwrap_types,
199+
unwrap_allowed_ids,
200+
unwrap_allowed_aliases,
207201
Variant::Expect,
208202
);
209203
},

0 commit comments

Comments
 (0)