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.

Sign up for GitHub

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

Jump to bottom

Don't trigger use_debug lint in Debug impl #5047

Merged
merged 2 commits into from
Mar 3, 2020
Merged

Don't trigger use_debug lint in Debug impl #5047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a628733
Don't lint debug formatting in debug impl
flip1995 Jan 14, 2020
a540b5c
Update stderr
flip1995 Jan 14, 2020
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
2 changes: 1 addition & 1 deletion clippy_lints/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ mod reexport {
///
/// Used in `./src/driver.rs`.
pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore, conf: &Conf) {
store.register_pre_expansion_pass(|| box write::Write);
store.register_pre_expansion_pass(|| box write::Write::default());
store.register_pre_expansion_pass(|| box redundant_field_names::RedundantFieldNames);
let single_char_binding_names_threshold = conf.single_char_binding_names_threshold;
store.register_pre_expansion_pass(move || box non_expressive_names::NonExpressiveNames {
Expand Down
276 changes: 156 additions & 120 deletions clippy_lints/src/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ use std::borrow::Cow;
use std::ops::Range;

use crate::utils::{snippet_with_applicability, span_lint, span_lint_and_sugg, span_lint_and_then};
use rustc_ast::ast::{Expr, ExprKind, Mac, StrLit, StrStyle};
use rustc_ast::ast::{Expr, ExprKind, Item, ItemKind, Mac, StrLit, StrStyle};
use rustc_ast::token;
use rustc_ast::tokenstream::TokenStream;
use rustc_errors::Applicability;
use rustc_lexer::unescape::{self, EscapeError};
use rustc_lint::{EarlyContext, EarlyLintPass};
use rustc_parse::parser;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::symbol::Symbol;
use rustc_span::{BytePos, Span};

Expand Down Expand Up @@ -175,7 +175,12 @@ declare_clippy_lint! {
"writing a literal with a format string"
}

declare_lint_pass!(Write => [
#[derive(Default)]
pub struct Write {
in_debug_impl: bool,
}

impl_lint_pass!(Write => [
PRINT_WITH_NEWLINE,
PRINTLN_EMPTY_STRING,
PRINT_STDOUT,
Expand All @@ -187,10 +192,34 @@ declare_lint_pass!(Write => [
]);

impl EarlyLintPass for Write {
fn check_item(&mut self, _: &EarlyContext<'_>, item: &Item) {
if let ItemKind::Impl {
of_trait: Some(trait_ref),
..
} = &item.kind
{
let trait_name = trait_ref
.path
.segments
.iter()
.last()
.expect("path has at least one segment")
.ident
.name;
if trait_name == sym!(Debug) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, to avoid the FN, couldn't we compare the full path here instead of just the last segment? i.e if it's core::fmt::Debug or std::fmt::Debug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would need to turn it in a LatePassLint for that. I think we can live with this FN though, since it shouldn't be that common that someone defines a Debug trait

self.in_debug_impl = true;
}
}
}

fn check_item_post(&mut self, _: &EarlyContext<'_>, _: &Item) {
self.in_debug_impl = false;
}

fn check_mac(&mut self, cx: &EarlyContext<'_>, mac: &Mac) {
if mac.path == sym!(println) {
span_lint(cx, PRINT_STDOUT, mac.span(), "use of `println!`");
if let (Some(fmt_str), _) = check_tts(cx, &mac.args.inner_tokens(), false) {
if let (Some(fmt_str), _) = self.check_tts(cx, &mac.args.inner_tokens(), false) {
if fmt_str.symbol == Symbol::intern("") {
span_lint_and_sugg(
cx,
Expand All @@ -205,7 +234,7 @@ impl EarlyLintPass for Write {
}
} else if mac.path == sym!(print) {
span_lint(cx, PRINT_STDOUT, mac.span(), "use of `print!`");
if let (Some(fmt_str), _) = check_tts(cx, &mac.args.inner_tokens(), false) {
if let (Some(fmt_str), _) = self.check_tts(cx, &mac.args.inner_tokens(), false) {
if check_newlines(&fmt_str) {
span_lint_and_then(
cx,
Expand All @@ -226,7 +255,7 @@ impl EarlyLintPass for Write {
}
}
} else if mac.path == sym!(write) {
if let (Some(fmt_str), _) = check_tts(cx, &mac.args.inner_tokens(), true) {
if let (Some(fmt_str), _) = self.check_tts(cx, &mac.args.inner_tokens(), true) {
if check_newlines(&fmt_str) {
span_lint_and_then(
cx,
Expand All @@ -247,7 +276,7 @@ impl EarlyLintPass for Write {
}
}
} else if mac.path == sym!(writeln) {
if let (Some(fmt_str), expr) = check_tts(cx, &mac.args.inner_tokens(), true) {
if let (Some(fmt_str), expr) = self.check_tts(cx, &mac.args.inner_tokens(), true) {
if fmt_str.symbol == Symbol::intern("") {
let mut applicability = Applicability::MachineApplicable;
let suggestion = expr.map_or_else(
Expand Down Expand Up @@ -296,129 +325,136 @@ fn newline_span(fmtstr: &StrLit) -> Span {
sp.with_lo(newline_sp_hi - newline_sp_len).with_hi(newline_sp_hi)
}

/// Checks the arguments of `print[ln]!` and `write[ln]!` calls. It will return a tuple of two
/// `Option`s. The first `Option` of the tuple is the macro's format string. It includes
/// the contents of the string, whether it's a raw string, and the span of the literal in the
/// source. The second `Option` in the tuple is, in the `write[ln]!` case, the expression the
/// `format_str` should be written to.
///
/// Example:
///
/// Calling this function on
/// ```rust
/// # use std::fmt::Write;
/// # let mut buf = String::new();
/// # let something = "something";
/// writeln!(buf, "string to write: {}", something);
/// ```
/// will return
/// ```rust,ignore
/// (Some("string to write: {}"), Some(buf))
/// ```
#[allow(clippy::too_many_lines)]
fn check_tts<'a>(cx: &EarlyContext<'a>, tts: &TokenStream, is_write: bool) -> (Option<StrLit>, Option<Expr>) {
use fmt_macros::{
AlignUnknown, ArgumentImplicitlyIs, ArgumentIs, ArgumentNamed, CountImplied, FormatSpec, Parser, Piece,
};
let tts = tts.clone();

let mut parser = parser::Parser::new(&cx.sess.parse_sess, tts, None, false, false, None);
let mut expr: Option<Expr> = None;
if is_write {
expr = match parser.parse_expr().map_err(|mut err| err.cancel()) {
Ok(p) => Some(p.into_inner()),
Err(_) => return (None, None),
impl Write {
/// Checks the arguments of `print[ln]!` and `write[ln]!` calls. It will return a tuple of two
/// `Option`s. The first `Option` of the tuple is the macro's format string. It includes
/// the contents of the string, whether it's a raw string, and the span of the literal in the
/// source. The second `Option` in the tuple is, in the `write[ln]!` case, the expression the
/// `format_str` should be written to.
///
/// Example:
///
/// Calling this function on
/// ```rust
/// # use std::fmt::Write;
/// # let mut buf = String::new();
/// # let something = "something";
/// writeln!(buf, "string to write: {}", something);
/// ```
/// will return
/// ```rust,ignore
/// (Some("string to write: {}"), Some(buf))
/// ```
#[allow(clippy::too_many_lines)]
fn check_tts<'a>(
&self,
cx: &EarlyContext<'a>,
tts: &TokenStream,
is_write: bool,
) -> (Option<StrLit>, Option<Expr>) {
use fmt_macros::{
AlignUnknown, ArgumentImplicitlyIs, ArgumentIs, ArgumentNamed, CountImplied, FormatSpec, Parser, Piece,
};
// might be `writeln!(foo)`
if parser.expect(&token::Comma).map_err(|mut err| err.cancel()).is_err() {
return (None, expr);
}
}
let tts = tts.clone();

let fmtstr = match parser.parse_str_lit() {
Ok(fmtstr) => fmtstr,
Err(_) => return (None, expr),
};
let tmp = fmtstr.symbol.as_str();
let mut args = vec![];
let mut fmt_parser = Parser::new(&tmp, None, Vec::new(), false);
while let Some(piece) = fmt_parser.next() {
if !fmt_parser.errors.is_empty() {
return (None, expr);
}
if let Piece::NextArgument(arg) = piece {
if arg.format.ty == "?" {
// FIXME: modify rustc's fmt string parser to give us the current span
span_lint(cx, USE_DEBUG, parser.prev_token.span, "use of `Debug`-based formatting");
let mut parser = parser::Parser::new(&cx.sess.parse_sess, tts, None, false, false, None);
let mut expr: Option<Expr> = None;
if is_write {
expr = match parser.parse_expr().map_err(|mut err| err.cancel()) {
Ok(p) => Some(p.into_inner()),
Err(_) => return (None, None),
};
// might be `writeln!(foo)`
if parser.expect(&token::Comma).map_err(|mut err| err.cancel()).is_err() {
return (None, expr);
}
args.push(arg);
}
}
let lint = if is_write { WRITE_LITERAL } else { PRINT_LITERAL };
let mut idx = 0;
loop {
const SIMPLE: FormatSpec<'_> = FormatSpec {
fill: None,
align: AlignUnknown,
flags: 0,
precision: CountImplied,
precision_span: None,
width: CountImplied,
width_span: None,
ty: "",
ty_span: None,

let fmtstr = match parser.parse_str_lit() {
Ok(fmtstr) => fmtstr,
Err(_) => return (None, expr),
};
if !parser.eat(&token::Comma) {
return (Some(fmtstr), expr);
let tmp = fmtstr.symbol.as_str();
let mut args = vec![];
let mut fmt_parser = Parser::new(&tmp, None, Vec::new(), false);
while let Some(piece) = fmt_parser.next() {
if !fmt_parser.errors.is_empty() {
return (None, expr);
}
if let Piece::NextArgument(arg) = piece {
if !self.in_debug_impl && arg.format.ty == "?" {
// FIXME: modify rustc's fmt string parser to give us the current span
span_lint(cx, USE_DEBUG, parser.prev_token.span, "use of `Debug`-based formatting");
}
args.push(arg);
}
}
let token_expr = if let Ok(expr) = parser.parse_expr().map_err(|mut err| err.cancel()) {
expr
} else {
return (Some(fmtstr), None);
};
match &token_expr.kind {
ExprKind::Lit(_) => {
let mut all_simple = true;
let mut seen = false;
for arg in &args {
match arg.position {
ArgumentImplicitlyIs(n) | ArgumentIs(n) => {
if n == idx {
all_simple &= arg.format == SIMPLE;
seen = true;
}
},
ArgumentNamed(_) => {},
let lint = if is_write { WRITE_LITERAL } else { PRINT_LITERAL };
let mut idx = 0;
loop {
const SIMPLE: FormatSpec<'_> = FormatSpec {
fill: None,
align: AlignUnknown,
flags: 0,
precision: CountImplied,
precision_span: None,
width: CountImplied,
width_span: None,
ty: "",
ty_span: None,
};
if !parser.eat(&token::Comma) {
return (Some(fmtstr), expr);
}
let token_expr = if let Ok(expr) = parser.parse_expr().map_err(|mut err| err.cancel()) {
expr
} else {
return (Some(fmtstr), None);
};
match &token_expr.kind {
ExprKind::Lit(_) => {
let mut all_simple = true;
let mut seen = false;
for arg in &args {
match arg.position {
ArgumentImplicitlyIs(n) | ArgumentIs(n) => {
if n == idx {
all_simple &= arg.format == SIMPLE;
seen = true;
}
},
ArgumentNamed(_) => {},
}
}
}
if all_simple && seen {
span_lint(cx, lint, token_expr.span, "literal with an empty format string");
}
idx += 1;
},
ExprKind::Assign(lhs, rhs, _) => {
if let ExprKind::Lit(_) = rhs.kind {
if let ExprKind::Path(_, p) = &lhs.kind {
let mut all_simple = true;
let mut seen = false;
for arg in &args {
match arg.position {
ArgumentImplicitlyIs(_) | ArgumentIs(_) => {},
ArgumentNamed(name) => {
if *p == name {
seen = true;
all_simple &= arg.format == SIMPLE;
}
},
if all_simple && seen {
span_lint(cx, lint, token_expr.span, "literal with an empty format string");
}
idx += 1;
},
ExprKind::Assign(lhs, rhs, _) => {
if let ExprKind::Lit(_) = rhs.kind {
if let ExprKind::Path(_, p) = &lhs.kind {
let mut all_simple = true;
let mut seen = false;
for arg in &args {
match arg.position {
ArgumentImplicitlyIs(_) | ArgumentIs(_) => {},
ArgumentNamed(name) => {
if *p == name {
seen = true;
all_simple &= arg.format == SIMPLE;
}
},
}
}
if all_simple && seen {
span_lint(cx, lint, rhs.span, "literal with an empty format string");
}
}
if all_simple && seen {
span_lint(cx, lint, rhs.span, "literal with an empty format string");
}
}
}
},
_ => idx += 1,
},
_ => idx += 1,
}
}
}
}
Expand Down
8 changes: 1 addition & 7 deletions tests/ui/print.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,6 @@ LL | write!(f, "{:?}", 43.1415)
|
= note: `-D clippy::use-debug` implied by `-D warnings`

error: use of `Debug`-based formatting
--> $DIR/print.rs:18:19
|
LL | write!(f, "{:?}", 42.718)
| ^^^^^^

error: use of `println!`
--> $DIR/print.rs:23:5
|
Expand Down Expand Up @@ -56,5 +50,5 @@ error: use of `Debug`-based formatting
LL | print!("Hello {:#?}", "#orld");
| ^^^^^^^^^^^^^

error: aborting due to 9 previous errors
error: aborting due to 8 previous errors