Add explicit_default_arguments lint#16123
Add explicit_default_arguments lint#16123vogelbirb wants to merge 14 commits intorust-lang:masterfrom
explicit_default_arguments lint#16123Conversation
This comment has been minimized.
This comment has been minimized.
|
Lintcheck changes for 90ff88e
This comment will be updated if you push new changes |
|
Do you think the function |
explicit_default_arguments lint
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi, @Jarcho. I just wanted to follow up on this PR, it's ready for review and has been waiting for a few weeks now. I took into account your advice on Zulip. It would be great if you could take a look at it. Thanks. |
This comment has been minimized.
This comment has been minimized.
Most notably, `ExprKind::Path` is still a work in progress. Also added a check for equality constraints in `impl Trait` and trait objects. Unlike consts, statics were missing so a check for them was added.
|
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. |
|
r? clippy |
|
☔ The latest upstream changes (possibly #16687) made this pull request unmergeable. Please resolve the merge conflicts. |
| let TyKind::Path( | ||
| qpath @ QPath::Resolved( | ||
| _, | ||
| Path { | ||
| segments: | ||
| [ | ||
| .., | ||
| PathSegment { | ||
| args: Some(hir_ty_generics), | ||
| .. | ||
| }, | ||
| ], | ||
| .. | ||
| }, | ||
| ), | ||
| ) = hir_ty.kind |
There was a problem hiding this comment.
the indentation is terrifying.. maybe use a let-chain instead?
| let TyKind::Path( | |
| qpath @ QPath::Resolved( | |
| _, | |
| Path { | |
| segments: | |
| [ | |
| .., | |
| PathSegment { | |
| args: Some(hir_ty_generics), | |
| .. | |
| }, | |
| ], | |
| .. | |
| }, | |
| ), | |
| ) = hir_ty.kind | |
| let hir_ty_generics = if let TyKind::Path(ref qpath) = hir_ty.kind | |
| && let QPath::Resolved(_, path) = qpath | |
| && let Some(last_segment) = path.segments.last() | |
| && let Some(hir_ty_generics) = last_segment.args | |
| { | |
| hir_ty_generics | |
| } else { | |
| return; | |
| }; |
| redundant_tys_spans.push(redundant_ty.span()); | ||
| } | ||
| } | ||
| let no_generics_hir_ty_snippet = snippet_opt(cx, hir_ty.span.until(hir_ty_generics.span_ext)).unwrap(); |
There was a problem hiding this comment.
unwrapping in clippy is generally not a good idea... it's preferable to use snippet, since it accepts a fallback string in case source code is not available (which can happen)
| if i > 0 { | ||
| generics_string.push_str(", "); | ||
| } | ||
| generics_string.push_str(&snippet_opt(cx, arg.span()).unwrap()); |
| let (defaults, aliased_to_alias) = match_generics(cx, aliased_ty_args.as_slice(), alias_ty_params.as_slice()); | ||
|
|
||
| let mut redundant_tys_spans = Vec::new(); | ||
| for (i, generic_arg) in resolved_ty_args.iter().enumerate() { |
There was a problem hiding this comment.
could replace with .into_iter() to avoid copying below
| hir_ty.span, | ||
| format!( | ||
| "redudant usage of default argument `{}`", | ||
| snippet_opt(cx, redundant_ty_span).unwrap() |
| if expr.span.from_expansion() { | ||
| return; | ||
| } | ||
| let ty = cx.typeck_results().expr_ty(expr); |
There was a problem hiding this comment.
does this one get used anywhere?
| check_typair(cx, (cx.typeck_results().node_type(ty.hir_id), *ty)); | ||
| for ty_pair in path_generic_args(iter::once(segment)) | ||
| .into_iter() | ||
| .map(|hir_ty| (cx.typeck_results().node_type(hir_ty.hir_id), hir_ty)) | ||
| { | ||
| check_typair(cx, ty_pair); | ||
| } | ||
| }, | ||
| QPath::Resolved(ty, path) => { | ||
| if let Some(ty) = ty { | ||
| check_typair(cx, (cx.typeck_results().node_type(ty.hir_id), *ty)); | ||
| } | ||
| for ty_pair in path_generic_args(path.segments) | ||
| .into_iter() | ||
| .map(|hir_ty| (cx.typeck_results().node_type(hir_ty.hir_id), hir_ty)) | ||
| { | ||
| check_typair(cx, ty_pair); | ||
| } | ||
| }, | ||
| _ => {}, | ||
| }, | ||
| ExprKind::Closure(Closure { | ||
| fn_decl: FnDecl { inputs, output, .. }, | ||
| .. | ||
| }) => { | ||
| for input in *inputs { | ||
| check_typair(cx, (cx.typeck_results().node_type(input.hir_id), *input)); | ||
| } | ||
| if let FnRetTy::Return(ty) = *output { | ||
| check_typair(cx, (cx.typeck_results().node_type(ty.hir_id), *ty)); | ||
| } | ||
| }, | ||
| ExprKind::Cast(_, &ty) | ||
| | ExprKind::Type(_, &ty) | ||
| | ExprKind::Let(&LetExpr { ty: Some(&ty), .. }) | ||
| | ExprKind::UnsafeBinderCast(_, _, Some(&ty)) | ||
| | ExprKind::OffsetOf(&ty, _) => check_typair(cx, (cx.typeck_results().expr_ty(expr), ty)), |
There was a problem hiding this comment.
you might want to do let typeck = cx.typeck_results(); to save yourself some characters across this match
| error: redudant usage of default argument `i32` | ||
| --> tests/ui/explicit_default_arguments.rs:28:28 | ||
| | | ||
| LL | fn multi_all_defaults() -> Multi<i32, String> { | ||
| | ^^^^^^^^^^^^^^^^^^ help: try: `Multi` | ||
|
|
||
| error: redudant usage of default argument `String` | ||
| --> tests/ui/explicit_default_arguments.rs:28:28 | ||
| | | ||
| LL | fn multi_all_defaults() -> Multi<i32, String> { | ||
| | ^^^^^^^^^^^^^^^^^^ help: try: `Multi` |
There was a problem hiding this comment.
- these two should ideally be one lint
- you don't need to repeat the name of the argument -- highlight its span instead
| error: redudant usage of default argument `i32` | |
| --> tests/ui/explicit_default_arguments.rs:28:28 | |
| | | |
| LL | fn multi_all_defaults() -> Multi<i32, String> { | |
| | ^^^^^^^^^^^^^^^^^^ help: try: `Multi` | |
| error: redudant usage of default argument `String` | |
| --> tests/ui/explicit_default_arguments.rs:28:28 | |
| | | |
| LL | fn multi_all_defaults() -> Multi<i32, String> { | |
| | ^^^^^^^^^^^^^^^^^^ help: try: `Multi` | |
| error: redundant usage of default arguments | |
| --> tests/ui/explicit_default_arguments.rs:28:28 | |
| | | |
| LL | fn multi_all_defaults() -> Multi<i32, String> { | |
| | ^^^ ^^^^^^ | |
| | | |
| help: consider removing them | |
| | | |
| LL - fn multi_all_defaults() -> Multi<i32, String> { | |
| LL + fn multi_all_defaults() -> Multi { | |
| | |
| hir_ty.hir_id, | ||
| hir_ty.span, | ||
| format!( | ||
| "redudant usage of default argument `{}`", |
There was a problem hiding this comment.
| "redudant usage of default argument `{}`", | |
| "redundant usage of default argument `{}`", |
| snippet_opt(cx, redundant_ty_span).unwrap() | ||
| ), | ||
| |diag| { | ||
| diag.span_suggestion(hir_ty.span, "try", &sugg_text_snippet, Applicability::MachineApplicable); |
There was a problem hiding this comment.
Consider replacing hir_ty_generics.span_ext instead. That way, you will no longer need to include no_generics_hir_ty_snippet in sugg_text_snippet. As an added bonus, the suggestion should have a nicer diff (only the generics will be highlighted)
|
Reminder, once the PR becomes ready for a review, use |
|
Thanks for the thorough review. There's a lot to address, so I'll get to it as soon as I have the time. |
View all comments
This lint checks if the generic arguments of a type alias are set to the default.
fixes #14848
r? @Jarcho
Relevant Zulip threads: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/How.20to.20access.20generic.20params.20of.20a.20.60DefId.60.3F/with/547384320/, https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/Is.20there.20an.20easier.20way.20to.20walk.20through.20the.20predicates.3F/with/560255667