OpenE2K/rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Merge pull request #2381 from HMPerson1/remove_is_unit_expr

Browse Source 
Replace `is_unit_expr`
This commit is contained in:
Oliver Schneider 2018-01-19 09:14:04 +01:00 committed by GitHub
commit 26c415ab2d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 252 additions and 334 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

140
clippy_lints/src/is_unit_expr.rs
View File

@ -1,140 +0,0 @@
use rustc::lint::*;
use syntax::ast::*;
use syntax::ext::quote::rt::Span;
use utils::{span_lint, span_note_and_lint};
/// **What it does:** Checks for
/// - () being assigned to a variable
/// - () being passed to a function
///
/// **Why is this bad?** It is extremely unlikely that a user intended to
/// assign '()' to valiable. Instead,
/// Unit is what a block evaluates to when it returns nothing. This is
/// typically caused by a trailing
/// unintended semicolon.
///
/// **Known problems:** None.
///
/// **Example:**
/// * `let x = {"foo" ;}` when the user almost certainly intended `let x
/// ={"foo"}`
declare_lint! {
pub UNIT_EXPR,
Warn,
"unintended assignment or use of a unit typed value"
}
#[derive(Copy, Clone)]
enum UnitCause {
SemiColon,
EmptyBlock,
}
#[derive(Copy, Clone)]
pub struct UnitExpr;
impl LintPass for UnitExpr {
fn get_lints(&self) -> LintArray {
lint_array!(UNIT_EXPR)
}
}
impl EarlyLintPass for UnitExpr {
fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) {
if let ExprKind::Assign(ref _left, ref right) = expr.node {
check_for_unit(cx, right);
}
if let ExprKind::MethodCall(ref _left, ref args) = expr.node {
for arg in args {
check_for_unit(cx, arg);
}
}
if let ExprKind::Call(_, ref args) = expr.node {
for arg in args {
check_for_unit(cx, arg);
}
}
}
fn check_stmt(&mut self, cx: &EarlyContext, stmt: &Stmt) {
if let StmtKind::Local(ref local) = stmt.node {
if local.pat.node == PatKind::Wild {
return;
}
if let Some(ref expr) = local.init {
check_for_unit(cx, expr);
}
}
}
}
fn check_for_unit(cx: &EarlyContext, expr: &Expr) {
match is_unit_expr(expr) {
Some((span, UnitCause::SemiColon)) => span_note_and_lint(
cx,
UNIT_EXPR,
expr.span,
"This expression evaluates to the Unit type ()",
span,
"Consider removing the trailing semicolon",
),
Some((_span, UnitCause::EmptyBlock)) => span_lint(
cx,
UNIT_EXPR,
expr.span,
"This expression evaluates to the Unit type ()",
),
None => (),
}
}
fn is_unit_expr(expr: &Expr) -> Option<(Span, UnitCause)> {
match expr.node {
ExprKind::Block(ref block) => match check_last_stmt_in_block(block) {
Some(UnitCause::SemiColon) =>
Some((block.stmts[block.stmts.len() - 1].span, UnitCause::SemiColon)),
Some(UnitCause::EmptyBlock) =>
Some((block.span, UnitCause::EmptyBlock)),
None => None
}
ExprKind::If(_, ref then, ref else_) => {
let check_then = check_last_stmt_in_block(then);
if let Some(ref else_) = *else_ {
let check_else = is_unit_expr(else_);
if let Some(ref expr_else) = check_else {
return Some(*expr_else);
}
}
match check_then {
Some(c) => Some((expr.span, c)),
None => None,
}
},
ExprKind::Match(ref _pattern, ref arms) => {
for arm in arms {
if let Some(r) = is_unit_expr(&arm.body) {
return Some(r);
}
}
None
},
_ => None,
}
}
fn check_last_stmt_in_block(block: &Block) -> Option<UnitCause> {
if block.stmts.is_empty() { return Some(UnitCause::EmptyBlock); }
let final_stmt = &block.stmts[block.stmts.len() - 1];
// Made a choice here to risk false positives on divergent macro invocations
// like `panic!()`
match final_stmt.node {
StmtKind::Expr(_) => None,
StmtKind::Semi(ref expr) => match expr.node {
ExprKind::Break(_, _) | ExprKind::Continue(_) | ExprKind::Ret(_) => None,
_ => Some(UnitCause::SemiColon),
},
_ => Some(UnitCause::SemiColon), // not sure what's happening here
}
}

9
clippy_lints/src/lib.rs
View File

@ -111,7 +111,6 @@ pub mod if_not_else;
pub mod infinite_iter;
pub mod int_plus_one;
pub mod invalid_ref;
pub mod is_unit_expr;
pub mod items_after_statements;
pub mod large_enum_variant;
pub mod len_zero;
@ -248,6 +247,10 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
"string_to_string",
"using `string::to_string` is common even today and specialization will likely happen soon",
);
store.register_removed(
"unit_expr",
"superseded by `let_unit_value` and `unit_arg`",
);
// end deprecated lints, do not remove this comment, it’s used in `update_lints`
reg.register_late_lint_pass(box serde_api::Serde);
@ -268,7 +271,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_late_lint_pass(box approx_const::Pass);
reg.register_late_lint_pass(box misc::Pass);
reg.register_early_lint_pass(box precedence::Precedence);
reg.register_early_lint_pass(box is_unit_expr::UnitExpr);
reg.register_early_lint_pass(box needless_continue::NeedlessContinue);
reg.register_late_lint_pass(box eta_reduction::EtaPass);
reg.register_late_lint_pass(box identity_op::IdentityOp);
@ -365,6 +367,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_early_lint_pass(box const_static_lifetime::StaticConst);
reg.register_late_lint_pass(box fallible_impl_from::FallibleImplFrom);
reg.register_late_lint_pass(box replace_consts::ReplaceConsts);
reg.register_late_lint_pass(box types::UnitArg);
reg.register_lint_group("clippy_restrictions", vec![
arithmetic::FLOAT_ARITHMETIC,
@ -478,7 +481,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
if_let_redundant_pattern_matching::IF_LET_REDUNDANT_PATTERN_MATCHING,
infinite_iter::INFINITE_ITER,
invalid_ref::INVALID_REF,
is_unit_expr::UNIT_EXPR,
large_enum_variant::LARGE_ENUM_VARIANT,
len_zero::LEN_WITHOUT_IS_EMPTY,
len_zero::LEN_ZERO,
@ -608,6 +610,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
types::OPTION_OPTION,
types::TYPE_COMPLEXITY,
types::UNIT_CMP,
types::UNIT_ARG,
types::UNNECESSARY_CAST,
unicode::ZERO_WIDTH_SPACE,
unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,

162
clippy_lints/src/types.rs
View File

@ -361,25 +361,22 @@ declare_lint! {
fn check_let_unit(cx: &LateContext, decl: &Decl) {
if let DeclLocal(ref local) = decl.node {
match cx.tables.pat_ty(&local.pat).sty {
ty::TyTuple(slice, _) if slice.is_empty() => {
if in_external_macro(cx, decl.span) || in_macro(local.pat.span) {
return;
}
if higher::is_from_for_desugar(decl) {
return;
}
span_lint(
cx,
LET_UNIT_VALUE,
decl.span,
&format!(
"this let-binding has unit value. Consider omitting `let {} =`",
snippet(cx, local.pat.span, "..")
),
);
},
_ => (),
if is_unit(cx.tables.pat_ty(&local.pat)) {
if in_external_macro(cx, decl.span) || in_macro(local.pat.span) {
return;
}
if higher::is_from_for_desugar(decl) {
return;
}
span_lint(
cx,
LET_UNIT_VALUE,
decl.span,
&format!(
"this let-binding has unit value. Consider omitting `let {} =`",
snippet(cx, local.pat.span, "..")
),
);
}
}
}
@ -434,31 +431,118 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnitCmp {
}
if let ExprBinary(ref cmp, ref left, _) = expr.node {
let op = cmp.node;
if op.is_comparison() {
match cx.tables.expr_ty(left).sty {
ty::TyTuple(slice, _) if slice.is_empty() => {
let result = match op {
BiEq | BiLe | BiGe => "true",
_ => "false",
};
span_lint(
cx,
UNIT_CMP,
expr.span,
&format!(
"{}-comparison of unit values detected. This will always be {}",
op.as_str(),
result
),
);
},
_ => (),
}
if op.is_comparison() && is_unit(cx.tables.expr_ty(left)) {
let result = match op {
BiEq | BiLe | BiGe => "true",
_ => "false",
};
span_lint(
cx,
UNIT_CMP,
expr.span,
&format!(
"{}-comparison of unit values detected. This will always be {}",
op.as_str(),
result
),
);
}
}
}
}
/// **What it does:** Checks for passing a unit value as an argument to a function without using a unit literal (`()`).
///
/// **Why is this bad?** This is likely the result of an accidental semicolon.
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// foo({
/// let a = bar();
/// baz(a);
/// })
/// ```
declare_lint! {
pub UNIT_ARG,
Warn,
"passing unit to a function"
}
pub struct UnitArg;
impl LintPass for UnitArg {
fn get_lints(&self) -> LintArray {
lint_array!(UNIT_ARG)
}
}
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnitArg {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
if in_macro(expr.span) {
return;
}
match expr.node {
ExprCall(_, ref args) | ExprMethodCall(_, _, ref args) => {
for arg in args {
if is_unit(cx.tables.expr_ty(arg)) && !is_unit_literal(arg) {
let map = &cx.tcx.hir;
// apparently stuff in the desugaring of `?` can trigger this
// so check for that here
// only the calls to `Try::from_error` is marked as desugared,
// so we need to check both the current Expr and its parent.
if !is_questionmark_desugar_marked_call(expr) {
if_chain!{
let opt_parent_node = map.find(map.get_parent_node(expr.id));
if let Some(hir::map::NodeExpr(parent_expr)) = opt_parent_node;
if is_questionmark_desugar_marked_call(parent_expr);
then {}
else {
// `expr` and `parent_expr` where _both_ not from
// desugaring `?`, so lint
span_lint_and_sugg(
cx,
UNIT_ARG,
arg.span,
"passing a unit value to a function",
"if you intended to pass a unit value, use a unit literal instead",
"()".to_string(),
);
}
}
}
}
}
},
_ => (),
}
}
}
fn is_questionmark_desugar_marked_call(expr: &Expr) -> bool {
use syntax_pos::hygiene::CompilerDesugaringKind;
if let ExprCall(ref callee, _) = expr.node {
callee.span.is_compiler_desugaring(CompilerDesugaringKind::QuestionMark)
} else {
false
}
}
fn is_unit(ty: Ty) -> bool {
match ty.sty {
ty::TyTuple(slice, _) if slice.is_empty() => true,
_ => false,
}
}
fn is_unit_literal(expr: &Expr) -> bool {
match expr.node {
ExprTup(ref slice) if slice.is_empty() => true,
_ => false,
}
}
pub struct CastPass;
/// **What it does:** Checks for casts from any numerical to a float type where

79
tests/ui/is_unit_expr.rs
View File

@ -1,79 +0,0 @@
#![warn(unit_expr)]
#[allow(unused_variables)]
fn main() {
// lint should note removing the semicolon from "baz"
let x = {
"foo";
"baz";
};
// lint should ignore false positive.
let y = if true {
"foo"
} else {
return;
};
// lint should note removing semicolon from "bar"
let z = if true {
"foo";
} else {
"bar";
};
let a1 = Some(5);
// lint should ignore false positive
let a2 = match a1 {
Some(x) => x,
_ => {
return;
},
};
// lint should note removing the semicolon after `x;`
let a3 = match a1 {
Some(x) => {
x;
},
_ => {
0;
},
};
loop {
let a2 = match a1 {
Some(x) => x,
_ => {
break;
},
};
let a2 = match a1 {
Some(x) => x,
_ => {
continue;
},
};
}
}
pub fn foo() -> i32 {
let a2 = match None {
Some(x) => x,
_ => {
return 42;
},
};
55
}
pub fn issue_2160() {
let x1 = {};
let x2 = if true {} else {};
let x3 = match None { Some(_) => {}, None => {}, };
}

73
tests/ui/is_unit_expr.stderr
View File

@ -1,73 +0,0 @@
error: This expression evaluates to the Unit type ()
--> $DIR/is_unit_expr.rs:8:13
|
8 | let x = {
| _____________^
9 | | "foo";
10 | | "baz";
11 | | };
| |_____^
|
= note: `-D unit-expr` implied by `-D warnings`
note: Consider removing the trailing semicolon
--> $DIR/is_unit_expr.rs:10:9
|
10 | "baz";
| ^^^^^^
error: This expression evaluates to the Unit type ()
--> $DIR/is_unit_expr.rs:22:13
|
22 | let z = if true {
| _____________^
23 | | "foo";
24 | | } else {
25 | | "bar";
26 | | };
| |_____^
|
note: Consider removing the trailing semicolon
--> $DIR/is_unit_expr.rs:25:9
|
25 | "bar";
| ^^^^^^
error: This expression evaluates to the Unit type ()
--> $DIR/is_unit_expr.rs:40:14
|
40 | let a3 = match a1 {
| ______________^
41 | | Some(x) => {
42 | | x;
43 | | },
... |
46 | | },
47 | | };
| |_____^
|
note: Consider removing the trailing semicolon
--> $DIR/is_unit_expr.rs:42:13
|
42 | x;
| ^^
error: This expression evaluates to the Unit type ()
--> $DIR/is_unit_expr.rs:76:14
|
76 | let x1 = {};
| ^^
error: This expression evaluates to the Unit type ()
--> $DIR/is_unit_expr.rs:77:14
|
77 | let x2 = if true {} else {};
| ^^^^^^^^^^^^^^^^^^
error: This expression evaluates to the Unit type ()
--> $DIR/is_unit_expr.rs:78:14
|
78 | let x3 = match None { Some(_) => {}, None => {}, };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 6 previous errors

55
tests/ui/unit_arg.rs Normal file
View File

@ -0,0 +1,55 @@
#![warn(unit_arg)]
#![allow(no_effect)]
use std::fmt::Debug;
fn foo<T: Debug>(t: T) {
println!("{:?}", t);
}
fn foo3<T1: Debug, T2: Debug, T3: Debug>(t1: T1, t2: T2, t3: T3) {
println!("{:?}, {:?}, {:?}", t1, t2, t3);
}
struct Bar;
impl Bar {
fn bar<T: Debug>(&self, t: T) {
println!("{:?}", t);
}
}
fn bad() {
foo({});
foo({ 1; });
foo(foo(1));
foo({
foo(1);
foo(2);
});
foo3({}, 2, 2);
let b = Bar;
b.bar({ 1; });
}
fn ok() {
foo(());
foo(1);
foo({ 1 });
foo3("a", 3, vec![3]);
let b = Bar;
b.bar({ 1 });
b.bar(());
question_mark();
}
fn question_mark() -> Result<(), ()> {
Ok(Ok(())?)?;
Ok(Ok(()))??;
Ok(())
}
fn main() {
bad();
ok();
}

68
tests/ui/unit_arg.stderr Normal file
View File

@ -0,0 +1,68 @@
error: passing a unit value to a function
--> $DIR/unit_arg.rs:23:9
|
23 | foo({});
| ^^
|
= note: `-D unit-arg` implied by `-D warnings`
help: if you intended to pass a unit value, use a unit literal instead
|
23 | foo(());
| ^^
error: passing a unit value to a function
--> $DIR/unit_arg.rs:24:9
|
24 | foo({ 1; });
| ^^^^^^
help: if you intended to pass a unit value, use a unit literal instead
|
24 | foo(());
| ^^
error: passing a unit value to a function
--> $DIR/unit_arg.rs:25:9
|
25 | foo(foo(1));
| ^^^^^^
help: if you intended to pass a unit value, use a unit literal instead
|
25 | foo(());
| ^^
error: passing a unit value to a function
--> $DIR/unit_arg.rs:26:9
|
26 | foo({
| _________^
27 | | foo(1);
28 | | foo(2);
29 | | });
| |_____^
help: if you intended to pass a unit value, use a unit literal instead
|
26 | foo(());
| ^^
error: passing a unit value to a function
--> $DIR/unit_arg.rs:30:10
|
30 | foo3({}, 2, 2);
| ^^
help: if you intended to pass a unit value, use a unit literal instead
|
30 | foo3((), 2, 2);
| ^^
error: passing a unit value to a function
--> $DIR/unit_arg.rs:32:11
|
32 | b.bar({ 1; });
| ^^^^^^
help: if you intended to pass a unit value, use a unit literal instead
|
32 | b.bar(());
| ^^
error: aborting due to 6 previous errors