Skip to content
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

add a pair of whitespace after remove parentheses #63163

Merged
merged 4 commits into from
Aug 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 83 additions & 27 deletions src/librustc_lint/unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use syntax::print::pprust;
use syntax::symbol::{kw, sym};
use syntax::symbol::Symbol;
use syntax::util::parser;
use syntax_pos::Span;
use syntax_pos::{Span, BytePos};

use rustc::hir;

Expand Down Expand Up @@ -353,31 +353,46 @@ declare_lint! {
declare_lint_pass!(UnusedParens => [UNUSED_PARENS]);

impl UnusedParens {

fn is_expr_parens_necessary(inner: &ast::Expr, followed_by_block: bool) -> bool {
followed_by_block && match inner.node {
ast::ExprKind::Ret(_) | ast::ExprKind::Break(..) => true,
_ => parser::contains_exterior_struct_lit(&inner),
}
}

fn check_unused_parens_expr(&self,
cx: &EarlyContext<'_>,
value: &ast::Expr,
msg: &str,
followed_by_block: bool) {
cx: &EarlyContext<'_>,
value: &ast::Expr,
msg: &str,
followed_by_block: bool,
left_pos: Option<BytePos>,
right_pos: Option<BytePos>) {
match value.node {
ast::ExprKind::Paren(ref inner) => {
let necessary = followed_by_block && match inner.node {
ast::ExprKind::Ret(_) | ast::ExprKind::Break(..) => true,
_ => parser::contains_exterior_struct_lit(&inner),
};
if !necessary {
if !Self::is_expr_parens_necessary(inner, followed_by_block) {
let expr_text = if let Ok(snippet) = cx.sess().source_map()
.span_to_snippet(value.span) {
snippet
} else {
pprust::expr_to_string(value)
};
Self::remove_outer_parens(cx, value.span, &expr_text, msg);
let keep_space = (
left_pos.map(|s| s >= value.span.lo()).unwrap_or(false),
right_pos.map(|s| s <= value.span.hi()).unwrap_or(false),
);
Self::remove_outer_parens(cx, value.span, &expr_text, msg, keep_space);
}
}
ast::ExprKind::Let(_, ref expr) => {
// FIXME(#60336): Properly handle `let true = (false && true)`
// actually needing the parenthesis.
self.check_unused_parens_expr(cx, expr, "`let` head expression", followed_by_block);
self.check_unused_parens_expr(
cx, expr,
"`let` head expression",
followed_by_block,
None, None
);
}
_ => {}
}
Expand All @@ -394,11 +409,15 @@ impl UnusedParens {
} else {
pprust::pat_to_string(value)
};
Self::remove_outer_parens(cx, value.span, &pattern_text, msg);
Self::remove_outer_parens(cx, value.span, &pattern_text, msg, (false, false));
}
}

fn remove_outer_parens(cx: &EarlyContext<'_>, span: Span, pattern: &str, msg: &str) {
fn remove_outer_parens(cx: &EarlyContext<'_>,
span: Span,
pattern: &str,
msg: &str,
keep_space: (bool, bool)) {
let span_msg = format!("unnecessary parentheses around {}", msg);
let mut err = cx.struct_span_lint(UNUSED_PARENS, span, &span_msg);
let mut ate_left_paren = false;
Expand All @@ -424,11 +443,27 @@ impl UnusedParens {
},
_ => false,
}
}).to_owned();
});

let replace = {
let mut replace = if keep_space.0 {
let mut s = String::from(" ");
s.push_str(parens_removed);
s
} else {
String::from(parens_removed)
};

if keep_space.1 {
replace.push(' ');
}
replace
};

err.span_suggestion_short(
span,
"remove these parentheses",
parens_removed,
replace,
Applicability::MachineApplicable,
);
err.emit();
Expand All @@ -438,14 +473,35 @@ impl UnusedParens {
impl EarlyLintPass for UnusedParens {
fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) {
use syntax::ast::ExprKind::*;
let (value, msg, followed_by_block) = match e.node {
If(ref cond, ..) => (cond, "`if` condition", true),
While(ref cond, ..) => (cond, "`while` condition", true),
ForLoop(_, ref cond, ..) => (cond, "`for` head expression", true),
Match(ref head, _) => (head, "`match` head expression", true),
Ret(Some(ref value)) => (value, "`return` value", false),
Assign(_, ref value) => (value, "assigned value", false),
AssignOp(.., ref value) => (value, "assigned value", false),
let (value, msg, followed_by_block, left_pos, right_pos) = match e.node {
If(ref cond, ref block, ..) => {
let left = e.span.lo() + syntax_pos::BytePos(2);
let right = block.span.lo();
(cond, "`if` condition", true, Some(left), Some(right))
}

While(ref cond, ref block, ..) => {
let left = e.span.lo() + syntax_pos::BytePos(5);
let right = block.span.lo();
(cond, "`while` condition", true, Some(left), Some(right))
},

ForLoop(_, ref cond, ref block, ..) => {
(cond, "`for` head expression", true, None, Some(block.span.lo()))
}

Match(ref head, _) => {
let left = e.span.lo() + syntax_pos::BytePos(5);
(head, "`match` head expression", true, Some(left), None)
}

Ret(Some(ref value)) => {
let left = e.span.lo() + syntax_pos::BytePos(3);
(value, "`return` value", false, Some(left), None)
}

Assign(_, ref value) => (value, "assigned value", false, None, None),
AssignOp(.., ref value) => (value, "assigned value", false, None, None),
// either function/method call, or something this lint doesn't care about
ref call_or_other => {
let (args_to_check, call_kind) = match *call_or_other {
Expand All @@ -467,12 +523,12 @@ impl EarlyLintPass for UnusedParens {
}
let msg = format!("{} argument", call_kind);
for arg in args_to_check {
self.check_unused_parens_expr(cx, arg, &msg, false);
self.check_unused_parens_expr(cx, arg, &msg, false, None, None);
}
return;
}
};
self.check_unused_parens_expr(cx, &value, msg, followed_by_block);
self.check_unused_parens_expr(cx, &value, msg, followed_by_block, left_pos, right_pos);
}

fn check_pat(&mut self, cx: &EarlyContext<'_>, p: &ast::Pat) {
Expand All @@ -492,7 +548,7 @@ impl EarlyLintPass for UnusedParens {
fn check_stmt(&mut self, cx: &EarlyContext<'_>, s: &ast::Stmt) {
if let ast::StmtKind::Local(ref local) = s.node {
if let Some(ref value) = local.init {
self.check_unused_parens_expr(cx, &value, "assigned value", false);
self.check_unused_parens_expr(cx, &value, "assigned value", false, None, None);
}
}
}
Expand Down
27 changes: 27 additions & 0 deletions src/test/ui/lint/unused_parens_json_suggestion.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// compile-flags: --error-format pretty-json -Zunstable-options
// build-pass (FIXME(62277): could be check-pass?)
// run-rustfix

// The output for humans should just highlight the whole span without showing
// the suggested replacement, but we also want to test that suggested
// replacement only removes one set of parentheses, rather than naïvely
// stripping away any starting or ending parenthesis characters—hence this
// test of the JSON error format.

#![warn(unused_parens)]
#![allow(unreachable_code)]

fn main() {
// We want to suggest the properly-balanced expression `1 / (2 + 3)`, not
// the malformed `1 / (2 + 3`
let _a = 1 / (2 + 3);
f();
}

fn f() -> bool {
loop {
if (break { return true }) {
}
}
false
}
2 changes: 2 additions & 0 deletions src/test/ui/lint/unused_parens_json_suggestion.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// compile-flags: --error-format pretty-json -Zunstable-options
// build-pass (FIXME(62277): could be check-pass?)
// run-rustfix

// The output for humans should just highlight the whole span without showing
// the suggested replacement, but we also want to test that suggested
Expand All @@ -8,6 +9,7 @@
// test of the JSON error format.

#![warn(unused_parens)]
#![allow(unreachable_code)]

fn main() {
// We want to suggest the properly-balanced expression `1 / (2 + 3)`, not
Expand Down
28 changes: 14 additions & 14 deletions src/test/ui/lint/unused_parens_json_suggestion.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
"spans": [
{
"file_name": "$DIR/unused_parens_json_suggestion.rs",
"byte_start": 611,
"byte_end": 624,
"line_start": 15,
"line_end": 15,
"byte_start": 654,
"byte_end": 667,
"line_start": 17,
"line_end": 17,
"column_start": 14,
"column_end": 27,
"is_primary": true,
Expand All @@ -36,10 +36,10 @@
"spans": [
{
"file_name": "$DIR/unused_parens_json_suggestion.rs",
"byte_start": 457,
"byte_end": 470,
"line_start": 10,
"line_end": 10,
"byte_start": 472,
"byte_end": 485,
"line_start": 11,
"line_end": 11,
"column_start": 9,
"column_end": 22,
"is_primary": true,
Expand All @@ -66,10 +66,10 @@
"spans": [
{
"file_name": "$DIR/unused_parens_json_suggestion.rs",
"byte_start": 611,
"byte_end": 624,
"line_start": 15,
"line_end": 15,
"byte_start": 654,
"byte_end": 667,
"line_start": 17,
"line_end": 17,
"column_start": 14,
"column_end": 27,
"is_primary": true,
Expand All @@ -91,13 +91,13 @@
}
],
"rendered": "warning: unnecessary parentheses around assigned value
--> $DIR/unused_parens_json_suggestion.rs:15:14
--> $DIR/unused_parens_json_suggestion.rs:17:14
|
LL | let _a = (1 / (2 + 3));
| ^^^^^^^^^^^^^ help: remove these parentheses
|
note: lint level defined here
--> $DIR/unused_parens_json_suggestion.rs:10:9
--> $DIR/unused_parens_json_suggestion.rs:11:9
|
LL | #![warn(unused_parens)]
| ^^^^^^^^^^^^^
Expand Down
62 changes: 62 additions & 0 deletions src/test/ui/lint/unused_parens_remove_json_suggestion.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// compile-flags: --error-format pretty-json -Zunstable-options
// build-pass
// run-rustfix

// The output for humans should just highlight the whole span without showing
// the suggested replacement, but we also want to test that suggested
// replacement only removes one set of parentheses, rather than naïvely
// stripping away any starting or ending parenthesis characters—hence this
// test of the JSON error format.

#![warn(unused_parens)]
#![allow(unreachable_code)]

fn main() {

let _b = false;

if _b {
println!("hello");
}

f();

}

fn f() -> bool {
let c = false;

if c {
println!("next");
}

if c {
println!("prev");
}

while false && true {
if c {
println!("norm");
}

}

while true && false {
for _ in 0 .. 3 {
println!("e~")
}
}

for _ in 0 .. 3 {
while true && false {
println!("e~")
}
}


loop {
if (break { return true }) {
}
}
false
}
Loading