Add allow-unwrap-types configuration for unwrap_used and expect_used#16605
Add allow-unwrap-types configuration for unwrap_used and expect_used#16605llogiq merged 1 commit intorust-lang:masterfrom
allow-unwrap-types configuration for unwrap_used and expect_used#16605Conversation
rustbot
added
the
S-waiting-on-review
|
r? @llogiq rustbot has assigned @llogiq. Use Why was this reviewer chosen?The reviewer was selected based on:
|
rustbot
removed
the
S-waiting-on-review
|
We'll have to take another look into this, it's added 30 million instructions TwT (profiled in @sengmonkham Do you see any way to optimize this PR? From my profilling, it's mainly due to that |
|
This should be pre-resolving the types the same way the |
Thanks for looking into this and catching the performance regression. The 6.2% regression is because the hot evaluation loop in I can optimize this by pre-resolving the types exactly the same way the
This should bring the performance cost of this configuration practically down to zero during the hot method checks. Please let me know if this sounds good to you. I'll immediately start working on a PR. |
|
Sounds good, let's try that (๑•ω•́ฅ✧ Make sure to ping me in that new PR to profile. |
…gression (#16652) Fixes #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](#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
…gression (rust-lang#16652) Fixes rust-lang/rust-clippy#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/rust-clippy#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
5 participants
#16535
This PR introduces a new configuration option,
allow-unwrap-types, which accepts a list of type paths (e.g.,["std::sync::LockResult"]). When configured, calling.unwrap()or.expect()on values of these types will no longer trigger theunwrap_usedorexpect_usedlints.This is particularly useful for reducing noisy lints on patterns where unwrapping is widely considered acceptable or expected by design, such as obtaining a
std::sync::LockResultfromMutex::lock().Changes Made
Introduced the
allow_unwrap_types: Vec<String>field inclippy_config.Updated the
unwrap_expect_usedlint to check the underlying evaluated expressiontyagainst the configured allowed types.Handled robust type alias resolution so that aliases like
std::sync::LockResultaccurately map to their underlyingAdttypes during lint evaluation.Added UI tests in
tests/ui-toml/unwrap_used_allowed/to verify both allowed and disallowed cases.changelog: [
unwrap_used], [expect_used]: Addallow-unwrap-typesconfiguration to allow unwrapping specified types.