Add option_zip_none lint - #17465
Conversation
|
Thanks for the pull request, and welcome! You should hear from one of our reviewers after this PR gets at least 2 reviews from the community. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
892c510 to
c3ee38c
Compare
c3ee38c to
8a32bab
Compare
There was a problem hiding this comment.
I also tried this code:
trait MyZip {
fn zip(self, other: Option<()>) -> &'static str;
}
impl<T> MyZip for &Option<T> {
fn zip(self, other: Option<()>) -> &'static str {
"not Option::zip"
}
}
fn main() {
let opt = Some(1);
let r = (&opt).zip(None::<()>);
}It lints this case although it shouldn't, (but would anyone do that though?), otherwise looks good.
|
@Gri-ffin Is the current logic sufficient? |
|
You can tighten the lint by checking that the method resolves to |
95dd73b to
24c498b
Compare
bcf352a to
aecbce4
Compare
aecbce4 to
d8a7bff
Compare
d8a7bff to
8b8e0a1
Compare
c87d507 to
230e87b
Compare
230e87b to
04697cc
Compare
|
@ada4a I tried applying your suggested simplification, but it caused the compile-test suite to fail. |
04697cc to
06cee0b
Compare
06cee0b to
693dbbf
Compare
693dbbf to
4b8f202
Compare
|
@ada4a I tried swapping allow to expect, but it causes an unfulfilled_lint_expectations error. |
|
@ada4a I have updated the suggestions and adjusted the test suite to ''//@no-rustfix'' to handle the multi-type suggestion overlap. I replaced allow in the other file because it threw no error while it causes an unfulfilled_lint_expectations error in manual_option_rs file. |
| } | ||
|
|
||
| let applicability = if suggested_map { | ||
| Applicability::Unspecified |
There was a problem hiding this comment.
Could you give the reasoning behind this?
| suggested_map = true; | ||
| } else if !recv_is_none | ||
| && arg_is_none | ||
| && let Some(recv_snip) = snippet_opt(cx, recv.span) |
There was a problem hiding this comment.
Actually, in this case you don't even need recv_snip – instead, you could only replace call_span, which is one the fields of ExprKind::MethodCall, and points to everything after the dot (zip(None) in this case), with map(|n| (n, None)).
This has the additional benefit of making the diff more fine-grained: it will only point at the part after the dot.
| LL | let _: Option<((), i32)> = Option::zip(None::<()>, Some(1)); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: otherwise, simplify this to `None`: `None::<()>` |
There was a problem hiding this comment.
the help message still includes otherwise, , for some reason...
| LL - let _ = (&standard_opt).zip(None::<()>); | ||
| LL + let _ = (&standard_opt).map(|n| (n, None::<()>)); | ||
| | | ||
| help: simplify this to `None` |
There was a problem hiding this comment.
this also doesn't look right..
| mod edge_case { | ||
| trait MyZip { | ||
| fn zip(self, other: Option<()>) -> &'static str; | ||
| } | ||
|
|
||
| impl<T> MyZip for &Option<T> { | ||
| fn zip(self, _other: Option<()>) -> &'static str { | ||
| "not Option::zip" | ||
| } | ||
| } | ||
|
|
||
| pub fn test_custom_trait() { | ||
| let opt = Some(1); | ||
| let _ = (&opt).zip(None::<()>); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you please add a similar case where you define an item (probably an enum) called MyOption, with an inherent (i.e. defined in a regular impl block) method called zip, and check that that doesn't trigger the lint either?
This comment has been minimized.
This comment has been minimized.
|
|
||
| let applicability = if suggested_map { Applicability::Unspecified } else { app }; | ||
|
|
||
| diag.span_suggestion(expr.span, fallback_msg, none_snippet.to_string(), applicability); |
There was a problem hiding this comment.
nit: span_suggestion takes impl Into<String>, so you can omit .to_string()
| diag.span_suggestion(expr.span, fallback_msg, none_snippet.to_string(), applicability); | |
| diag.span_suggestion(expr.span, fallback_msg, none_snippet, applicability); |
| let mut suggested_map = false; | ||
|
|
||
| let none_snippet = if recv_is_none { | ||
| snippet_with_context(cx, recv.span, expr.span.ctxt(), "None", &mut app).0 |
There was a problem hiding this comment.
getting the context of a span is a somewhat expensive operation -- could you please get it once, store it in a variable (which we usually call ctxt), and use that everywhere?
| } | ||
| enum MyOption { |
There was a problem hiding this comment.
| } | |
| enum MyOption { | |
| } | |
| enum MyOption { |
There was a problem hiding this comment.
At this point I think everything here is actually fixable? Because we do emit the None suggestion
|
|
||
| if let ExprKind::MethodCall(_, _, _, call_span) = expr.kind { | ||
| if recv_is_none && !arg_is_none { | ||
| let arg_snip = snippet_with_context(cx, arg.span, expr.span.ctxt(), "..", &mut app).0; |
There was a problem hiding this comment.
nit: since arg is a simple expression, its fallback snippet should be _ (or (_), if you wish, since it's going to be a method call receiver)
|
@ada4a I actually tried consolidating everything into the option_zip_none.rs file, but doing so breaks rustfix with an E0308: mismatched types error. I haven't removed the unfixable.rs file so that tests remain green. |
View all comments
fixes #17462
Adds a new
suspiciouslate lintoption_zip_nonethat checks for calls of the formOption::zip(None).As noted in the issue, this always yields
Noneand is almost certainly a logic bug where the user intended to either just writeNoneor use.map(|x| (x, None))..stderrfile)cargo testpasses locallycargo dev update_lintscargo dev fmtchangelog: [
option_zip_none]: Add new lint to check forOption::zip(None)