Minic rustc's new format_args! expansion
#20056
Conversation
crates/test-utils/src/minicore.rs
Outdated
| @@ -31,6 +31,7 @@ | |||
| //! eq: sized | |||
| //! error: fmt | |||
| //! fmt: option, result, transmute, coerce_unsized, copy, clone, derive | |||
| //! fmt_since_1_89_0: fmt | |||
There was a problem hiding this comment.
I think it's a bit weird if activating just fmt will activate the old fmt, better for it to activate the newest.
There was a problem hiding this comment.
Fair point. I'll fix those names along with the method names in expr_store/lower.rs
| // Assume that rustc version >= 1.89.0 iff lang item `format_arguments` exists | ||
| // but `format_unsafe_arg` does not | ||
| let fmt_args = | ||
| crate::lang_item::lang_item(self.db, self.module.krate(), LangItem::FormatArguments); |
There was a problem hiding this comment.
I'm a bit worried about the impact of searching for lang items in body lowering, as body lowering should stay fast.
I guess it's fine because lang items generally remain unchanged so they will be cached.
There was a problem hiding this comment.
Yes, I agree to your both points: body lowering should be fast and in general, lang item caches persists.
But I think there would be no regression wrt this since we are already doing lang item query in current implementation:
rust-analyzer/crates/hir-def/src/expr_store/lower.rs
Lines 2866 to 2875 in 0ddaf2c
| } | ||
|
|
||
| /// `format_args!` expansion implementation for rustc versions < `1.89.0` | ||
| fn collect_format_args_impl( |
There was a problem hiding this comment.
Maybe better to call this collect_format_args_old()? Although I'd prefer the names to tell the toolchain version, I don't want collect_format_args_really_new() if we get yet another version.
| .alloc_expr_desugared(Expr::Call { callee: new_v1_formatted, args: args.into() }); | ||
|
|
||
| // We collect the unused expressions here so that we still infer them instead of | ||
| // dropping them out of the expression tree. |
There was a problem hiding this comment.
This is not good as it means they will be under an unsafe block, so will not need unsafe.
But the test testing that did not fire... So I don't know what's going on.
There was a problem hiding this comment.
Oh, you're right! I missed this point because it didn't failed orphan unsafe test.
I checked why the test didn't failed and found out that the following code:
fn main() {
let p = 3735928559 as *const i32;
format_args!("", *p);
}is expanded into:
fn main() {
let p = 3735928559 as *const i32;
{
let args = (&*p, );
//^^^ Orphans are once assigned into first `arg`
let args = [];
unsafe {
*p;
builtin#lang(Arguments::new_v1_formatted)(
&[],
&args,
&[],
)
}
};
}And thus, deleting those unnecessary and incorrect lines
4f59f2f to
2b33b86
Compare
| fn main() { | ||
| let are = "are"; | ||
| let count = 10; | ||
| builtin#format_args("\u{1b}hello {count:02} {} friends, we {are:?} {0}{last}", "fancy", last = "!"); | ||
| builtin#format_args("\u{1b}hello {count:02} {} friends, we {are:?} {0}{last}", "fancy", orphan = (), last = "!"); |
There was a problem hiding this comment.
I added a new arg orphan to check expansion behaviour or orphans
| @@ -1474,20 +1474,18 @@ fn main() { | |||
| } | |||
| "#, | |||
| expect![[r#" | |||
| me foo() fn(&self) | |||
There was a problem hiding this comment.
So, with the new format_args!, the completion works even with formatting with invalid matching braces, but I'm not sure whether I should take this as a regression or improvement
2b33b86 to
1ccd29f
Compare
| r#"//- minicore: todo, unimplemented | ||
| // FIXME: Since we are lacking of `super let`, term search fails due to borrowck failure. | ||
| // Should implement super let and remove `fmt_before_1_89_0` | ||
| r#"//- minicore: todo, unimplemented, fmt_before_1_89_0 |
There was a problem hiding this comment.
Those regressions are caused by failed borrowcks here, rooted from new format_args! expansions without super let or other proper alternative expansions:
rust-analyzer/crates/hir/src/term_search/tactics.rs
Lines 54 to 56 in 0ddaf2c
I'll implement super let as a followup and fix these by then
1ccd29f to
4f8767d
Compare
Fixes #19929 and fixes #20051