Add PRINTLN_EMPTY_STRING lint. #2146
Conversation
clippy_lints/src/print.rs
Outdated
| "print" => check_print(cx, span, args), | ||
| "println" => check_println(cx, span, args), | ||
| _ => (), | ||
| } |
There was a problem hiding this comment.
I was getting a cyclomatic_complexity lint, so I refactored this part.
MaloJaffre
force-pushed
the
println_empty_string
branch
2 times, most recently
from
fac1419 to
88d9ec9
Compare
clippy_lints/src/print.rs
Outdated
| let ExprPath(ref qpath) = args_fun.node, | ||
| let Some(const_def_id) = opt_def_id(resolve_node(cx, qpath, args_fun.hir_id)), | ||
| match_def_path(cx.tcx, const_def_id, &paths::FMT_ARGUMENTS_NEWV1), | ||
| args_args.len() == 2, |
There was a problem hiding this comment.
Can you deduplicate the above lines? check_print has the same lines.
clippy_lints/src/print.rs
Outdated
| // HACK: check size of the macro invocation: | ||
| // println!(); // 11 chars: don't lint | ||
| // println!("") // 12 chars: lint | ||
| (span.hi() - span.lo()).0 > 11, |
There was a problem hiding this comment.
hahahaha! That is awesome, but I think it would be better if you checked whether the code snippet matches println!("")
tests/ui/println_empty_string.rs
Outdated
| fn main() { | ||
|
|
||
| println!(); | ||
| println!("") // no semicolon intended |
clippy_lints/src/print.rs
Outdated
| (span.hi() - span.lo()).0 > 11, | ||
| // check the presence of that string | ||
| let Ok(snippet) = cx.sess().codemap().span_to_snippet(span), | ||
| snippet.contains("\"\""), |
There was a problem hiding this comment.
I actually meant that you could just do snippet == "println!(\"\")".
There was a problem hiding this comment.
I've tested that this snippet can contain whitespace or a semicolon, and your method doesn't detect them. Is there a better way I can test the presence of an empty argument?
There was a problem hiding this comment.
I don't know of any, but you're right, this should work for all realistic cases
|
Thanks for the review @oli-obk! |
MaloJaffre
changed the title
MaloJaffre
force-pushed
the
println_empty_string
branch
from
4e0d932 to
22f3ca0
Compare
|
Thanks, this looks good to me now. |
Since the merge of rust-lang#2146, `Cargo.lock` is no longer checked in the git repository, but `cargo` generates it anyway, if we are not in the main rust repository. Ignore it to avoid eventual confusion when contributing directly to clippy.
I have no idea of how I could properly check that there is really an empty string argument in a
printlninvocation.I think this will need the pre-expanded AST, which I can't access even in an
EarlyLintPass.See the
HACKinprint.rsfor details.Will fix #2135 when ready.