Expand format_args! with more details
#14820
Conversation
rustbot
added
the
S-waiting-on-review
| @@ -171,5 +171,5 @@ | |||
| <span class="macro">assert</span><span class="macro_bang">!</span><span class="parenthesis macro">(</span><span class="bool_literal macro">true</span><span class="comma macro">,</span> <span class="string_literal macro">"</span><span class="format_specifier">{</span><span class="format_specifier">}</span><span class="string_literal macro"> asdasd"</span><span class="comma macro">,</span> <span class="numeric_literal macro">1</span><span class="parenthesis macro">)</span><span class="semicolon">;</span> | |||
| <span class="macro">toho</span><span class="macro_bang">!</span><span class="parenthesis macro">(</span><span class="string_literal macro">"</span><span class="format_specifier">{</span><span class="format_specifier">}</span><span class="string_literal macro">fmt"</span><span class="comma macro">,</span> <span class="numeric_literal macro">0</span><span class="parenthesis macro">)</span><span class="semicolon">;</span> | |||
| <span class="macro unsafe">asm</span><span class="macro_bang">!</span><span class="parenthesis macro">(</span><span class="string_literal macro">"mov eax, </span><span class="format_specifier">{</span><span class="numeric_literal">0</span><span class="format_specifier">}</span><span class="string_literal macro">"</span><span class="parenthesis macro">)</span><span class="semicolon">;</span> | |||
| <span class="macro">format_args</span><span class="macro_bang">!</span><span class="parenthesis macro">(</span><span class="none macro">concat</span><span class="punctuation macro">!</span><span class="parenthesis macro">(</span><span class="string_literal macro">"</span><span class="format_specifier">{</span><span class="format_specifier">}</span><span class="string_literal macro">"</span><span class="parenthesis macro">)</span><span class="comma macro">,</span> <span class="string_literal macro">"{}"</span><span class="parenthesis macro">)</span><span class="semicolon">;</span> | |||
| <span class="macro">format_args</span><span class="macro_bang">!</span><span class="parenthesis macro">(</span><span class="none macro">concat</span><span class="punctuation macro">!</span><span class="parenthesis macro">(</span><span class="string_literal macro">"</span><span class="format_specifier">{</span><span class="format_specifier">}</span><span class="string_literal macro">"</span><span class="parenthesis macro">)</span><span class="comma macro">,</span> <span class="string_literal macro">"</span><span class="format_specifier">{</span><span class="format_specifier">}</span><span class="string_literal macro">"</span><span class="parenthesis macro">)</span><span class="semicolon">;</span> | |||
There was a problem hiding this comment.
This test regressed, but I can't see why. The recursive expansion of format_args!(concat!("{}"), "{}") is correct but it still highlights the {} in the second argument.
There was a problem hiding this comment.
That looks like we aren't mapping down the argument into the format_args expansion anymore, resulting in the highlight. Our current logic for checking if a string is a format_args template argument is just checking if its in a token tree of a format_args (or similar) named macro call,
There was a problem hiding this comment.
The strange thing is that if I remove the concat! it will work correctly (so mapping down is working in at least some cases), and that concat! should not play any role here...
There was a problem hiding this comment.
Huh, maybe our token mapping infra is not handling eager macros properly?
There was a problem hiding this comment.
Yes, it's probably something related to eager macros. In this code:
let a = 2;
format_args!("{} {} {:?}", a, concat!("{}", "xxx"), a);go to definition on first a works but on second a doesn't.
There was a problem hiding this comment.
I think there is a deep issue with eager macro expansion, and fixing it is out of scope for this PR. Do you consider this a blocker?
There was a problem hiding this comment.
Depends on whether this affects more things or not. But if its just caused by having anothereager macro call in the format_args macro it seems fine . Though what exactly do we gain from this change right now?
There was a problem hiding this comment.
But if its just caused by having anothereager macro call in the format_args macro it seems fine.
I think it is not limited to the eager macros. Any argument after a macro argument in any eager macro will lose its span, and this PR is making the format_args! eager (we can keep format_args! lazy but it has its own problems: false positive expansion diagnostic when someone construct the first argument with macros).
Though what exactly do we gain from this change right now?
For users, a diagnostics when there are less arguments provided in a format_args! based macro + correct and meaningful inline-macro and Expand macro recursively output + more precise closure captures in case of #11260 which theoretically may cause false positives for mir based diagnostics. For me, I can panic-debugging the mir interpreter, seeing the variable values by panicking them.
There was a problem hiding this comment.
Aight, that's a lot of benefits from this. I think it's fine to merge then, but do create an issue tracking this eager expansion problem once merged please (once bors is alive again).
|
@bors r+ |
Expand `format_args!` with more details
|
@bors r- This has some problems with raw strings. |
|
☀️ Try build successful - checks-actions |
1 similar comment
|
☀️ Try build successful - checks-actions |
|
@bors r+ |
Expand `format_args!` with more details
|
💔 Test failed - checks-actions |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
No description provided.