Diagnostic for forgetting a writer in write! is bad

[mejrs]

Code

fn main() {
    let x = 1;
    let y = 2;
    write!("{}_{}", x, y);
}

Current output

Compiling playground v0.0.1 (/playground)
error: format argument must be a string literal
 --> src/main.rs:4:21
  |
4 |     write!("{}_{}", x, y);
  |                     ^
  |
help: you might be missing a string literal to format with
  |
4 |     write!("{}_{}", "{} {}", x, y);
  |                     ++++++++

error[E0599]: no method named `write_fmt` found for reference `&'static str` in the current scope
 --> src/main.rs:4:5
  |
4 |     write!("{}_{}", x, y);
  |     ^^^^^^^^^^^^^^^^^^^^^ method not found in `&str`
  |
  = note: this error originates in the macro `write` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0599`.
error: could not compile `playground` due to 2 previous errors

Desired output

error: cannot write into `&'static str`
 --> src/main.rs:4:21
  |
4 |     write!("{}_{}", x, y);
  |             ^^^^
  |
help: you might be missing a writer to write into
  +    let mut writer = String::new();
  |
4 |     write!(&mut writer, "{}_{}", x, y);
  |            ++++++++++++

Rationale and extra context

No response

Other cases

No response

Anything else?

It could also recommend using format!

[mj10021]

This seems like a good first issue am I missing something? I am happy to claim it. The write! docs say that write! should accept a writer, a format string, and a list of arguments, so the error output should probably remind that

[mj10021]

@rustbot claim

[mejrs]

This seems like a good first issue am I missing something? I am happy to claim it. The write! docs say that write! should accept a writer, a format string, and a list of arguments, so the error output should probably remind that

That's great, let us know if you're having any trouble 🙂

[mj10021]

Ok! So this is where I am with it:

the macro for write! is the following:

    ($dst:expr, $($arg:tt)*) => { 
        $dst.write_fmt($crate::format_args!($($arg)*))
    };

The issue being that if a writer is forgotten, the macro gets expanded with .write_fmt() being called on the formatting string, and the rest of the arguments get passed into format_args!, which then returns an error that the formatting string maybe was forgotten.

In most other erroneous cases, .write_fmt() will get called on something that is not a writer and will return a more helpful error that whatever first argument was passed to write! does not have a method write_fmt.

I had two ideas, the first being to modify the error from /rustc_builtin_macros/format.rs, which is where the missing string literal to format with error originates. My idea is that if a string literal is missing AND the format_args was called from a write! then there probably is a missing writer.

My second idea is to add another pattern match to the write! macro itself like:

($oops:literal, $($args:tt)*) => {
    $crate::panic!("you are missing a writer")
};

I am not sure which approach is more appropriate, or how exactly to determine the macro that called format_args! to clarify the error. Do either of these approaches seem worthwhile? I think the write! macro should be the parent of format_args in AST and that is how I would access it?

Thanks :)
James

[mejrs]

how exactly to determine the macro that called format_args! to clarify the error.

You will have to determine whether you are in an expansion of the macro. See https://github.com/rust-lang/rust/blob/master/src/tools/clippy/clippy_lints/src/write.rs for an example of how you could do that and check that it's write/writeln/etc. Most likely you can use SyntaxContext::outer_expn and/or SyntaxContext::outer_expn_data to do that.

My second idea is to add another pattern match to the write! macro itself like:

I think this is not a good idea

• that arm will show up in the documentation, and probably confuse people
• it also isn't helpful when it's used on something other than a literal.

I think I would have the following approach:

If the method is named write_fmt and if we're in a write macro expansion don't emit the "no method named x found for ..." error at compiler\rustc_hir_typeck\src\method\suggest.rs, but instead emit an error like...

x must implement `io::Write`, `fmt::Write` or have a `write_fmt` method to be used as a writer

being more specific if it's a string literal...

you probably want to insert a writer in front of this format string

[mj10021]

Got it, thank you so much for pointing me in the right direction 🙂

As far as the write_fmt, this is where I am so far:

        let is_write_macro = sugg_span.ctxt().outer_expn_data().kind
            == ExpnKind::Macro(MacroKind::Bang, Symbol::intern("write"));
        let mut err;
        let is_write_fmt = item_name.name == Symbol::intern("write_fmt");
        if is_write_fmt && is_write_macro && args.is_some() {
            let helper = tcx.sess.source_map().span_to_snippet(args.unwrap().0.span).unwrap();
            let writer_note;
            match args.unwrap().0.kind {
                ExprKind::Lit(_) => {
                    writer_note = Some("you might want to insert a writer in front of this format string")
                }
                _ => writer_note = None,
            };
            err = struct_span_err!(
                tcx.sess,
                args.unwrap().0.span,
                E0599,
                "cannot write into '{}'",
                ty_str_reported,
            );
            err.note(format!(
                "{} must implement 'io::Write', 'fmt::Write' or have a 'write_fmt' method",
                helper
            ));
            if writer_note.is_some() {
                err.help(writer_note.unwrap());
            }
        }

Which gives the following output:

error[E0599]: cannot write into '&'static str'
 --> src/macros/mod.rs:484:12
  |
6 |     write!("{}_{}", x, y);
  |     -------^^^^^^^------- method not found in `&str`
  |
  = note: "{}_{}" must implement 'io::Write', 'fmt::Write' or have a 'write_fmt' method
  = help: you might want to insert a writer in front of this format string

Where I am checking to see if the function is write_fmt and the macro is write! and then creating the relevant error message.

I still need to create the recommended substitution from your example, I think that would be from rustc_errors::CodeSuggestions, is that correct?

Then for the format.rs error about the missing format string, using ecx.expansion_cause() gives me the whole line of the write! macro write!("{}_{}", x, y), I can think of some hacky ways to pull write! out of the outer span, is there a better way to extract the outer macro or should I just focus on improving the suggest.rs portion of the diagnostic?

Thanks again!
James

[jyn514]

@mj10021 I suggest posting on https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp so more people see your message and can help you.

[WaffleLapkin]

@mj10021 first of all, I would suggest using the fact that the macro has #[rustc_diagnostic_item = "write_macro"] so you can match on the def id like this for example:

let is_write_macro = sugg_span
    .ctxt()
    .outer_expn_data()
    .macro_def_id
    .map_or(false, |did| tcx.is_diagnostic_item(sym::write_macro, did));

This will be more reliable than matching on the name, as it won't fire on user provided macros with the same name, for example.

---

Secondly I wouldn't suggest using span_to_snippet, currently it is seen more as an anti-pattern. Instead you could probably use span_note to highlight the argument and say this argument must implement ... (there are other alternatives like printing the type instead, but using span_note is a good idea anyway, I think).

---

Thirdly about your question about suggestions: the easiest way is to use span_suggestion, you can get the span using the span of the first argument and then shrink_to_lo to get the span just before the literal, to add &mut writer, there. However, I'm not sure if such suggestion is a good idea, after all writer may not be present, or may be named differently, etc; a suggestion that is almost always wrong seems awkward (this reminds me that suggestions should be more powerful than they are currently in rustc…).

[mj10021]

@jyn514 Thanks for the tip ❤️, I've been lurking on Zulip for a little bit but wasn't sure if it was the right place for questions once there was already an issue on github

@WaffleLapkin Amazing thank you so much! I get what you're saying about the suggestion being unclear since the writer name will almost always be different... I also was thinking it might be a little awkward. Anyway I implemented the changes as you suggested and now I think the error message makes it pretty clear that a writer is missing:

error[E0599]: cannot write into '&'static str'
 --> src/macros/mod.rs:484:12
  |
6 |     write!("{}_{}", x, y);
  |     -------^^^^^^^------- method not found in `&str`
  |
note: must implement 'io::Write', 'fmt::Write' or have a 'write_fmt' method
 --> src/macros/mod.rs:484:12
  |
6 |     write!("{}_{}", x, y);
  |            ^^^^^^^
help: you might want to insert a writer in front of this format string
 --> src/macros/mod.rs:484:12
  |
6 |     write!("{}_{}", x, y);
  |            ^

error: aborting due to 2 previous errors

Otherwise is there anything that could be improved with the message?

I am not sure if E0599 is still the correct error code but I think that it is because the error is still technically no method found?

[WaffleLapkin]

@mj10021 looks good :)

I think at this point you can open a PR, if any tweaks are needed we'll tell you in the review process. Feel free to r? @WaffleLapkin, if you want me to review it.

[mj10021]

Ok awesome thanks, just submitted PR