update macro rewrite functions to return RewriteResult

[ding-young]

Tracked by #6206

Description

• update both rewrite_macro and rewrite_macro_def to return RewriteResult
• add a new enum variant that represents macro rewrite failure

[ytmimi]

Looks like some of the self_tests are failing because of a long comment. In general its probably better to place comments directly above the code that you're trying to explain given the max_width constraints that rustfmt has.

---- test::self_tests stdout ----
error[internal]: line formatted, but exceeded maximum width (maximum: 100 (see `max_width` option), found: 105)
    --> src/macros.rs:1043:1043:101
     |
1043 |     let parsed_args = MacroArgParser::new().parse(token_stream).macro_error()?; // TODO macro parse error
     |                                                                                                     ^^^^^
     |
     = note: set `error_on_unformatted = false` to suppress the warning against comments or string literals

[ding-young]

        match format_lazy_static(context, shape, ts.clone(), mac.span()) {
            Ok(rw) => return Ok(rw),
            Err(err) => match err {
                RewriteError::MacroFailure { macro_error_kind, span: _ } 
                if macro_error_kind== MacroErrorKind::ParseFailure => {},
                _ => return Err(err),
            },
        }

I think we can improve error reporting on calling format_lazy_static this way.

If we fail to parse macro with known (expected) syntax, we should still need to try formatting like following example.

lazy_static!(xxx, yyyy, zzzzz)

However, if we already succeeds to parse lazy_static with known syntax but fails to format due to other reason, it's better to tell users about it.

Since this causes change in formatting logic I want to hear your opinion about it.

[ytmimi]

Interesting idea. I think we can give this a try!

[ytmimi]

Question: Do we have plans to make rewrite_assign_rhs return a RewriteResut?

[ding-young]

Definitely yes. However, when I tried it a few weeks ago, I found that we might not be able to fully avoid using unknown_error() inside rewrite_assign_rhs due to challenges in grabbing the correct span. Anyway, I’ll give it another try and update you on the outcome.

[ding-young]

I replaced empty string with "(unknown reason)", and full error msg would look like

Failed to format given macro(unknown reason) at: Span {....} 

I’d appreciate any other suggestions regarding the error message, though.

[ytmimi]

I didn't realize that this Display impl would feed into RewriteError::MacroFailures Display impl. I think that not specifying a reason would have been fine now that I think about it more.

[ding-young]

Then I'll just revert it to empty string :)

[ding-young]

Thank you for the review! I’ve made the requested changes.

[ytmimi]

nit: Is there a specific reason we're importing this as std::fmt::{self}; and not std::fmt;? We can probably just impl std::fmt::Display for MacroErrorKind below and avoid the use altogether.

[ding-young]

I agree that it’s better to avoid the additional use here. Thank you for pointing it out. I updated the code.

[ytmimi]

Thanks for making those changes. PR looks good now!

[ytmimi]

Running the Diff-Check

Edit: Job passed ✅