Don't trigger field_reassign_with_default in macros

[flip1995]

Fixes #7155

Producing a good suggestion for this lint is already hard when no macros
are involved. With macros the lint message and the suggestion are just
confusing. Since both, producing a good suggestion and figuring out if
this pattern can be re-written inside a macro is nearly impossible, just
bail out.

changelog: [field_reassign_with_default] No longer triggers in macros

---

No that our reviewing queue is under control, I want to start hacking on Clippy myself again. Starting with an easy issue to get back in :)

[rust-highfive]

r? @phansch

(rust-highfive has picked a reviewer for you, use r? to override)

[camsteffen]

This could still be applicable in macros, but not applicable unless span.ctxt() is equal for all of let a = b, b, a.c = d, d.

Then this is the most macro-ified yet lintable version.

(..) => {
	  let $x = Default::default();
	  $x.$y = Default::default();
}

In other words, the example in the bug is not lint-able because the field re-assignment (a.c = d) occurs in a different context (expansion) than where let a = b occurs.

[flip1995]

Yeah, I thought of doing this. But the lint message + suggestion is then all over the place. It tells you that:

1. There is something going on with default assignment
2. This happens in a macro
3. You should replace the code inside the macro (this suggestion is currently also broken)
4. You should remove the remaining field reassignments, which points at the macro and not inside the macro.

The suggestion generation and detection in this lint is already really complicated. Now special casing every part of the suggestion on different macro contexts would make this code absolutely unreadable IMO. I don't think this is worth for this lint.

Also if you expand a $()* inside the same part of the macro, everything comes from the same macro context, doesn't it? And you can't fix that, like the lint would suggest you to do. So:

(..) => {
	  let $x = Default::default();
          $(
	      $x.$y = Default::default();
          )*
}

would have everything in the same expansion context, but you can't rewrite it. Playground

I'll adapt the test case, since this better shows the problem with linting this inside macros.

[camsteffen]

I see. I think it may be possible to do without special casing for macros. I would start by not using snippet_with_macro_callsite. But I'm just speculating. Totally fine to not do it.

[flip1995]

I would start by not using snippet_with_macro_callsite.

Yeah, but this is necessary, so that if you assign vec![1] to a field, so that it doesn't get expanded in the suggestion.

[camsteffen]

Maybe snippet(span.adjust(other_span.ctxt().outer_expn()))

[camsteffen]

Er hygiene::walk_chain(span, other_span.ctxt())

[camsteffen]

Meh nevermind. There could be $(..)? between the statements.

[flip1995]

There could be $(..)? between the statements.

Yeah, macros are just too complicated to get lints right that check patterns over multiple statements (especially since we don't know how the unexpanded code looked like in the HIR). As I said, even if you expand with $(..)* you can't lint.

[giraffate]

ping from triage @phansch. Can you have time to review this?

[xFrednet]

LGTM. I gathered from the previous discussion that everyone is okay with just not linting this inside macros.

Hey @camsteffen, I've confirmed with @phansch that it's okay if we pick up his PRs. @flip1995 usually double-checks my reviews, but this would be a bit counterproductive in this case. Could you maybe rereview it and merge the PR if you also approve of it? 🙃

[camsteffen]

Sure thing.

@bors r=xFrednet,camsteffen

[bors]

📌 Commit 0854f0c has been approved by xFrednet,camsteffen

[bors]

⌛ Testing commit 0854f0c with merge f1f5ccd...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet,camsteffen
Pushing f1f5ccd to master...