Rustup

[flip1995]

changelog: none

[rust-highfive]

r? @Manishearth

(rust_highfive has picked a reviewer for you, use r? to override)

[flip1995]

Probably blocked on rust-lang/rust#73257

[Manishearth]

r=me when ready!

[flip1995]

@bors r=Manishearth (let's try and see if we're waiting unnecessarily, the queue is empty anyway)

[bors]

📌 Commit 08e044e has been approved by Manishearth

[bors]

⌛ Testing commit 08e044e with merge 596c54f...

[bors]

💔 Test failed - checks-action_test

[flip1995]

I'll continue working on fixing the fallout of rust-lang/rust#72389 tomorrow. Now binary operators will be marked as expansions. This broke every lint, that tested for expansion with span.from_expansion(). This will require careful evaluation of every span.from_expansion() call in the Clippy code base.

[flip1995]

TODO:

• UNIT_CMP
• BOOL_COMPARISON
• ITERATOR_STEP_BY_ZERO
• STRING_ADD

In these cases neither utils::in_macro, nor span.from_expansion() works as expected. We should prepare ourselfs for multiple FN and FP issues because of macros in the near future.

[Manishearth]

@flip1995 I think we should be open to asking for that PR to be reverted while we fix this.

[Manishearth]

It's a diagnostics PR. If it is causing mass lint breakage, that's a problem, and perhaps its approach is not the best one.

[flip1995]

Yeah, I also thought about this.

There is currently the issue, that it seems that expn info is overridden. See the string_add example for this: https://github.com/rust-lang/rust-clippy/runs/780962847#step:10:1751

We're checking if the string addition comes from an external macro (what it does)

rust-clippy/tests/ui/string_add.rs

Line 25 in 7427065

string_add!();

rust-clippy/tests/ui/auxiliary/macro_rules.rs

Lines 35 to 41 in 7427065

	#[macro_export]
	macro_rules! string_add {
	() => {
	let y = "".to_owned();
	let z = y + "...";
	};
	}

But since we're checking this on the expr with expr.kind == ExprKind::BinaryOp, it will get the expansion context of ExpnKind::Desugaring(DesugaringKind::Operator), instead of ExpnKind::Macro. This means that everytime we find an expr with expr.kind == BinaryOp, we can't determine, if it is from a macro expansion, unless we check every parent expression (which would definitely be a very bad idea, since I can only imagine how many bugs this would introduce and how inefficient it would be).

cc @Aaron1011

[Aaron1011]

It sounds like the root issue is that ExpnKind::Desugaring and ExpnKind::Macro are mutually exclusive. What we really want is to be able to mark a Span with some desugaring information, and independently set whether or not it's coming from a macro.

I think the simplest way of achieving this would be to add an Option<DesugaringKind> to ExpnData, and delete the Desugaring variant from ExpnKind. I'll open a PR to rustc.

[Manishearth]

@flip1995 Oh, the loss of this functionality is definitely a bug in rust-lang/rust#72389 , since macros affect other diagnostics in rustc too.

[Aaron1011]

@flip1995: After thinking about this more, I think walking up the 'expansion stack' is actually the correct thing to do.

Consider the following code:

fn main() {
    if true || true {}
}

When we lower the if expression, we'll end up with both a DesugaringKind::CondTemporary and a DesugaringKind::Operator applied to the expression true || true. If we store the DesugaringKind alongside the existing ExpnData, we'll either need to overwrite one of the DesugaringKinds, or maintain a list of applied DesugaringKinds per ExpnData.

I think it would be simpler to have Clippy walk up the 'expansion stack' to check if an ExpnKind::Macro or ExpnKind::AstPass is present. We should be able to eliminate any performance impact by caching this in ExpnData on creation.

[matthiaskrgr]

The PR that fixed the ci-ice has been merged now 🎉

[phansch]

I believe this PR is currently waiting on rust-lang/rust#73594, right?

[Manishearth]

Yes

[flip1995]

@bors r+

Next up: sync Clippy back in Rust repo

[bors]

📌 Commit 51592f8 has been approved by flip1995

[bors]

⌛ Testing commit 51592f8 with merge fa0f6a8...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing fa0f6a8 to master...