-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(unnecessary_fold): lint on folds with Add::add/Mul::mul
#16124
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
base: master
Are you sure you want to change the base?
Conversation
|
Reminder, once the PR becomes ready for a review, use |
92cad4a to
22d0416
Compare
|
@rustbot ready |
… `fold`s with `Add::add`/`Mul::mul`
|
|
||
| // - the final expression in the body of a function with a simple return type | ||
| (ExprUseNode::Return(_), DefinedTy::Mir { ty: fn_return_ty, .. }) | ||
| if fn_return_ty.skip_binder().is_simple_text() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this supposed to be checking for? The return type of a function must be a fully defined type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this supposed to be checking for?
Opaque types (like impl Add/Mul, see {add,mul}_turbofish_necessary), and generic types with trait bounds (something like fn sum<T: Add>(ts: &[T]) -> T), it seems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change it to checking for opaque types then. Ty::walk will get you all the types you need to check.
The type of generic parameters can't be changed by inference so you can ignore those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type of generic parameters can't be changed by inference
I think they can, if you replace fn foo() -> impl Add with fn foo<T: Add>() -> T
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those aren't the same thing. In fn foo<T>() -> T has the return type determined by the caller; inside the body it must be the abstract type T. With -> impl Trait the return type is determined by the callee with the actual type inferred from whatever the body returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ty::walkwill get you all the types you need to check
Do you think descending into the generics is necessary, given that .sum()/.product() usually return primitive types? Though I guess nothing would stop an external implementation of Add/Mul from doing something like that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sum and Product are implemented for Option and Result, so yes.
22d0416 to
e4cfba0
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
bfdd5b5 to
ce5a4a1
Compare
ce5a4a1 to
254e0cb
Compare
Resurrects #13475
changelog: [
unnecessary_fold]: lint onfolds withAdd::add/Mul::mul