Migrate write.rs to rustc_ast::FormatArgs

[Alexendoo]

changelog: none

Part 1 of #10233

The additions to clippy_utils are the main novelty of this PR, there's no removals yet since other parts still rely on FormatArgsExpn

The changes to write.rs itself are relatively straightforward this time around, as there's no lints in it that rely on type checking format params

r? @flip1995

[nyurik]

This is awesome, thanks for the implementation! The code looks reasonable, and I learnt a few things.

If my understanding is correct, you essentially store every format_args invocation in a hashmap as span -> AST during the early pass, and later get the AST during the late pass, and you do it in the context of the write lint. My guess is that some other lints may need the same functionality, and it would be a bit wasteful for each lint to generate a full copy of such mapping for all format_args. Is there a way to have a shared cache of those, where each lint that needs it can set some flag to enable this feature if that lint is being used?

[llogiq]

Yeah, I suggested on zulip to have one internal lint pass that stores the mapping in a shared hash map.

[Alexendoo]

It's being populated by the Write early pass but other late lints can use it as well, as the thread local lives in clippy_utils

Putting the populating part in its own internal lint would make that clearer so I'll look at doing that

[nyurik]

Looks good!

Somewhat related -- does clippy optimize lint running if a lint is disabled? Or does it run all lints all the time, but hides output of the disabled lints? If first, all arg formatting lints would need some way to indicate they are dependent on this internal lint.

[flip1995]

does clippy optimize lint running if a lint is disabled? Or does it run all lints all the time, but hides output of the disabled lints?

rust-lang/rust#106983 Currently the latter. In the future maybe also the former.

Also in the future lint passes might run in parallel. This might lead to problems with the thread_local. But I think we should be fine here, because one is a early lint and the others are all late lints. And I think it's safe to assume that early and late lints will never run in parallel. (Also this is far in the future).

I still have to do a proper review.

[Alexendoo]

It shouldn't be too much of an issue if it does optimise that in the future because you'd have to go pretty far out of your way to disable the collector lint, it's not part of any group like clippy::all so you'd have to #[allow] it by name, and the name isn't listed on the website or in clippy-driver -W help

Is clippy tested anywhere with parallel_compiler? That could be an issue if the threads change there, though it should still compile fine

[Alexendoo]

Worth mentioning that I pulled this from the existing FormatArgsExpn::find_nested and the very start of FormatArgsExpn::parse:

rust-clippy/clippy_utils/src/macros.rs

Lines 989 to 1004 in 8a98609

	pub fn find_nested(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, expn_id: ExpnId) -> Option<Self> {
	for_each_expr(expr, |e| {
	let e_ctxt = e.span.ctxt();
	if e_ctxt == expr.span.ctxt() {
	ControlFlow::Continue(Descend::Yes)
	} else if e_ctxt.outer_expn().is_descendant_of(expn_id) {
	if let Some(args) = FormatArgsExpn::parse(cx, e) {
	ControlFlow::Break(args)
	} else {
	ControlFlow::Continue(Descend::No)
	}
	} else {
	ControlFlow::Continue(Descend::No)
	}
	})
	}

rust-clippy/clippy_utils/src/macros.rs

Lines 893 to 896 in 8a98609

	pub fn parse(cx: &LateContext<'_>, expr: &'tcx Expr<'tcx>) -> Option<Self> {
	let macro_name = macro_backtrace(expr.span)
	.map(|macro_call| cx.tcx.item_name(macro_call.def_id))
	.find(|&name| matches!(name, sym::const_format_args | sym::format_args | sym::format_args_nl))?;

[nyurik]

Friendly ping @flip1995 :)

[nyurik]

👋

[flip1995]

I don't really like the thread_local variable to store the state, but I can't come up with a better implementation. So let's merge this to move things forward.

[flip1995]

@bors r+

Thanks!

[bors]

📌 Commit d1407e5 has been approved by flip1995

It is now in the queue for this repository.

[bors]

⌛ Testing commit d1407e5 with merge 717577f...

[bors]

💔 Test failed - checks-action_test

[Alexendoo]

Damn proc macros

[Alexendoo]

Moved that test to tests/ui/crashes as it's a false positive now, or at least the suggestion would cause an error. It was an ICE until a week ago (#10401) so it's not too much of a regression at least

[flip1995]

Why is this linted now and wasn't before this refactor?

[Alexendoo]

Previously it parsed the string from the source text in order to hand it off to rustc_parse_format, but for an invalid string it would bail early

rust-clippy/clippy_utils/src/macros.rs

Lines 416 to 424 in 56fbfe5

	// Sometimes the original string comes from a macro which accepts a malformed string, such as in a
	// #[display(""somestring)] attribute (accepted by the `displaythis` crate). Reconstructing the
	// string from the span will not be possible, so we will just return None here.
	let mut unparsable = false;
	unescape_literal(inner, mode, &mut |_, ch| match ch {
	Ok(ch) => unescaped.push(ch),
	Err(e) if !e.is_fatal() => (),
	Err(_) => unparsable = true,
	});

After the refactor it doesn't need to use rustc_parse_format so there's no parsing of the source string, for valid strings it's a FP in both versions, i.e.

println!(with_span!("" ""));

currently produces

error: empty string literal in `println!`
  --> $DIR/x.rs:8:5
   |
LL |     println!(with_span!("" ""));
   |     ^^^^^^^^^^^^^^^^^^^^--^^^^^
   |                         |
   |                         help: remove the empty string

and is unchanged after this PR

[flip1995]

Thanks for the explanation! I'm fine with this "regression" since it is only for invalid code anyway IIUC.

Let's get this merged to get it in with this sync.

@bors +

[flip1995]

@bors r+ (forgot the r...)

[bors]

📌 Commit fa0c3cc has been approved by flip1995

It is now in the queue for this repository.

[bors]

⌛ Testing commit fa0c3cc with merge 3c06e0b...

[bors]

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