Rust-clippy: writeln_empty_string contains hardcoded suggestion

[mati865]

Actual output:

warning: using `writeln!(v, "")`
   --> src/main.rs:106:9
    |
106 |         writeln!(out, "").unwrap();
    |         ^^^^^^^^^^^^^^^^^ help: replace it with: `writeln!(v)`
    |
    = note: #[warn(writeln_empty_string)] on by default
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#writeln_empty_string

Expected output:

warning: using `writeln!(out, "")`
   --> src/main.rs:106:9
    |
106 |         writeln!(out, "").unwrap();
    |         ^^^^^^^^^^^^^^^^^ help: replace it with: `writeln!(out)`
    |
    = note: #[warn(writeln_empty_string)] on by default
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#writeln_empty_string

See lines:
https://github.com/rust-lang-nursery/rust-clippy/pull/2630/files#diff-2e8e94b55639a4808cb93bcc616d2d92R238
https://github.com/rust-lang-nursery/rust-clippy/pull/2630/files#diff-2e8e94b55639a4808cb93bcc616d2d92R240

cc @oli-obk for missing it during review :rofl:

[flip1995]

To improve this, we first need to get the span of the first argument of the writeln! macro. After that, it can be easily included in the suggestion (with the utils::snippet function).

[elpiel]

I want to pick this up. I will dig into it.

[flip1995]

Great! If you have any questions feel free to ask here or open a WIP PR. We're happy to help!

[elpiel]

I did take a look yesterday. I can't seem to understand where I need to get the parameter variable string. I did found the syntax::ast::Mac_ enum, but I am not sure if it's the correct one and what to do with it.

PS: I haven't written anything so that's why I am writing in the Issue. Maybe an example in the code that does something similar will be a sufficient for the begining

[mikerite]

Before you can get started, you need to be able to:

1. Build the project
2. Run your build on an example file

Have you managed to do the above?

[oli-obk]

@elpiel I don't think you need to extract anything from the macro itself (which is proably not available at this point anyway). All the data is already available at the printing sites shown in the first post of this issue. What needs doing is replacing the v with the result of calling the snippet function on the span of write_args[0]. You can have a look at the other calls to snippet and snippet_opt to see examples how other lints do it.

[flip1995]

@oli-obk you rewrote the print/write lints not long ago and write_args[0] doesn't exist anymore. #2949

@elpiel You should be able to get the first argument of the write(ln)! macro from the check_tts function:
https://github.com/rust-lang-nursery/rust-clippy/blob/99a087bea59c8f808b5485c6113edf9ce774e94a/clippy_lints/src/write.rs#L230-L231

Instead of skipping the write target, you could additionally return an Option<Expr> and get the span of the first argument from there. I haven't tested this, but it should work :smile:.

See also https://doc.rust-lang.org/nightly/nightly-rustc/syntax/parse/parser/struct.Parser.html#method.parse_expr

[elpiel]

OK, so I am a little bit lost. And I though this would be easy :smile_cat:
From what I see, currently the Some(fmtstr) is return if all the arguments were consumed.
Before that we have a loop checking if there is fmt_parser.errors and returns None in this case.

So here comes the question: How should I return both Option<String> for frmstr and the Option<Expr> for the expression?

Do you do that in a Tuple, a new struct or?
PS: Probably it shouldn't be a struct, but still not sure how the return the Option<Expr> along side wth the Option<String>

[flip1995]

OK, so I am a little bit lost. And I though this would be easy 😸

Yep I thought this was easier too. But I only looked at the old code and assumed that it would just be a view lines, like @oli-obk described it. Sorry for that :smile:. But now you can learn even more with your first lint! 🎉 You can just ask questions if you have any!

Do you do that in a Tuple, a new struct or?

Yes you can do this with a tuple, by using the return type (Option<Expr>, Option<String>). You can then match over the tuple here:
https://github.com/rust-lang-nursery/rust-clippy/blob/99a087bea59c8f808b5485c6113edf9ce774e94a/clippy_lints/src/write.rs#L203-L205
and create the suggestion from there.

You practically have to care only about the two cases

1. (Some(expr), Some(fmtstr)), where you should be able to use the span of the expr for the suggestion.
2. (None, Some(fmtstr)), where you use the current suggestion with the placeholder. (I'm not sure if this case could be reached, but lets cover it anyway)

and else don't lint.

[elpiel]

I did get somewhere, but I am clueless how to solve the issues with ? and why this is not working:

error[E0599]: no method named `into_vec` found for type `syntax::ptr::P<syntax::ast::Expr>` in the current scope
   --> clippy_lints/src/write.rs:240:27
    |
240 |         Some(p) => Some(p.into_vec().0),
    |                           ^^^^^^^^

error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `std::ops::Try`)
   --> clippy_lints/src/write.rs:244:5
    |
244 |     parser.expect(&token::Comma).map_err(|mut err| err.cancel()).ok()?;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot use the `?` operator in a function that returns `(std::option::Option<std::string::String>, std::option::Option<syntax::ast::Expr>)`

The documentation for syntax::ptr::P says that there should be a into_vec and I think that's the only way to extract the Expr from this smart pointer.
Also should I remove the if for is_write and leave the parse_expr where it is?

[flip1995]

Do you mind opening a WIP PR, so I could see your code? I have an idea what the problem could be, but that would just be a wild guess and probably not really helpful.

[elpiel]

I opened a WIP PR #3059.