Skip to content

refactor: use type-based checks in arithmetic_side_effects #16375

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.

Sign up for GitHub

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

Jump to bottom
Open
pantha704 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor: use type-based checks in arithmetic_side_effects #16375

pantha704 wants to merge 2 commits into from
+182 −140

Conversation

@pantha704
Copy link

@pantha704 pantha704 commented Jan 10, 2026 •
edited by rustbot
Loading

This PR refactors arithmetic_side_effects to use type-based checks instead of string representation matching, as suggested in #16359.

Changes

Replaced string-based type comparison in has_allowed_binary() and has_allowed_unary() with proper type system checks:

  • Float types: Use ty::FloatTy::F32 | ty::FloatTy::F64 matching via ty.kind()
  • String type: Use opt_diag_name() to check for sym::String
  • Saturating/Wrapping: Use opt_diag_name() for sym::Saturating | sym::Wrapping

This follows the pattern used in disallowed_types lint.

Benefits

  • Correctly handles type aliases (core::num::Wrapping == std::num::Wrapping)
  • More efficient than string comparison
  • No behavior change for existing functionality

String-based matching is preserved as fallback for user-configured types from clippy.toml.

Testing

All arithmetic_side_effects tests pass.

changelog: [arithmetic_side_effects]: use type-based matching instead of strings

Fixes #16359

@pantha704
refactor: use type-based checks in arithmetic_side_effects
119df19 
Instead of using string representation of types to compare them,
use the compiler's type system directly via ty.kind() and
opt_diag_name(). This approach:

- Correctly handles type aliases (core vs std paths)
- Is more efficient than string comparison
- Follows the pattern used in disallowed_types lint

The change affects has_allowed_binary() and has_allowed_unary()
which now check:
- Float types via ty::FloatTy::F32/F64 matching
- String type via diagnostic item
- Saturating/Wrapping via diagnostic items

Falls back to string matching for user-configured types from
clippy.toml to maintain backward compatibility.

Fixes rust-lang#16359

changelog: [`arithmetic_side_effects`]: use type-based matching instead of strings
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 10, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2026

r? @dswij

rustbot has assigned @dswij.
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

@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2026

⚠️ Warning ⚠️

  • There are issue links (such as #123) in the commit messages of the following commits.
    Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

samueltardieu
samueltardieu requested changes Jan 10, 2026
View reviewed changes
Copy link
Member

@samueltardieu samueltardieu left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a first step, but all string comparisons have to get away.

Also, can you please include examples where type aliases are used, both for primitive and composite types?

r? samueltardieu
@rustbot author

View changes since this review

clippy_lints/src/operators/arithmetic_side_effects.rs
Comment on lines 82 to 85
let lhs_ty_string = lhs_ty.to_string();
let lhs_ty_string_elem = lhs_ty_string.split('<').next().unwrap_or_default();
let rhs_ty_string = rhs_ty.to_string();
let rhs_ty_string_elem = rhs_ty_string.split('<').next().unwrap_or_default();
Copy link
Member

@samueltardieu samueltardieu Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those to_string() and split() need to be gone as well, as well as the maps containing strings.

@rustbot rustbot assigned samueltardieu and unassigned dswij Jan 10, 2026
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 10, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@pantha704
Copy link
Author

pantha704 commented Jan 10, 2026

Sure! working on it.

@pantha704
Copy link
Author

pantha704 commented Jan 10, 2026

I've completed the implementation for built-in type checks (String, Saturating, Wrapping) using is_diag_item, and added type alias tests.

For user-configured types from clippy.toml, the current config allows simple names like
Foo and i32 which can't be resolved to DefIds at init time. Removing the string-based fallback would require changing the config format to use fully-qualified paths.

Should I:

  • Keep the string fallback for user configs (current behavior)
  • Change the config format (breaking change)

or some other approach that you'd recommend?

@samueltardieu
Copy link
Member

samueltardieu commented Jan 18, 2026

For user-configured types from clippy.toml, the current config allows simple names like Foo and i32 which can't be resolved to DefIds at init time. Removing the string-based fallback would require changing the config format to use fully-qualified paths.

Should I:

* Keep the string fallback for user configs (current behavior)
* Change the config format (breaking change)

or some other approach that you'd recommend?

The lint documentation is unclear indeed. Types are named from the top of the main module because they are compared against the output of rustc_middle::Ty::to_string().

Maybe clippy_utils::paths::lookup_path() (or something similar) could be enhanced to also search types starting from the current crate root. That should not break existing settings, and would allow naming types starting at the top of the current crate.

What do you think?

@pantha704
Copy link
Author

pantha704 commented Jan 21, 2026

Thanks for the suggestion! I'll look into enhancing lookup_path() to also search from the current crate root. I'll implement it and see if it works for the existing test cases.

@pantha704
Copy link
Author

pantha704 commented Jan 21, 2026

Pushed! Enhanced lookup_path() to search LOCAL_CRATE for single-segment names, fixed type detection with is_diag_item(), and added type alias tests. All passing.

Still have the to_string()/split() for user configs - should I tackle that here or save it for a follow-up?

@pantha704 pantha704 force-pushed the refactor/arithmetic-side-effects-type-based branch from 24e4352 to 76c4428 Compare January 21, 2026 04:46
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samueltardieu samueltardieu Awaiting requested review from samueltardieu

Requested changes must be addressed to merge this pull request.

Assignees

@samueltardieu samueltardieu

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rewrite arithmetic_side_effects to not use matching on string representation of types

4 participants

@pantha704 @rustbot @samueltardieu @dswij