Deduplicate identifier printing a bit #69387
Conversation
rust-highfive
added
the
S-waiting-on-review
| @@ -2,7 +2,7 @@ mod a {} | |||
|
|
|||
| macro_rules! m { | |||
| () => { | |||
| use a::$crate; //~ ERROR unresolved import `a::$crate` | |||
| use a::$crate; //~ ERROR unresolved import `a::crate` | |||
There was a problem hiding this comment.
I am a bit surprised to see this, though I didn't try to trace the calls to the new infrastructure. I think it's a correct change though strange (to me) that how we print paths differs from how we print the errors below, I guess. But ultimately seems good -- it's a more correct error AFAICT.
Out of interest, in a cross-crate use, does the expansion break down and expand to a::::crate_name here?
There was a problem hiding this comment.
In one case it happens to be printed through ident (ast::Ident) and in another through ident.name (ast::Name aka Symbol).
This is all annoying.
Previously format!("{}", ident) and format!("{}", ident.name) were interchangeable, but in #67010 @estebank wanted to print identifiers in error messages as raw if they are likely raw, so they are now different.
Now @olegnn in #68963 wants to print Symbols in the same way, with rawness guessing.
This, however needs to be opt-in because Symbol is not generally an identifier, raw or not, it's simply an interned string.
There are seems to be three cases for identifier printing with different requirements:
- Token printing. It is important for this one to be precise, no rawness guessing, no
$crateconversions. This is how proc macros know what tokens are passed to them.
This is also used for error messages in lexer/parser where we are dealing with tokens (throughfn token_to_string). This is an excellent candidate for aDisplayimpl forToken, which doesn't exist yet. - AST printing. We need to print larger pieces of (possibly expanded) AST for both people, and for proc macros (due to issues). Both people and proc macros are then trying to parse and further compile that pretty-printed code again, so this printing has to recover rawness (which is not stored in AST, but can be guessed very well) and convert
$crates, which simply don't parse otherwise. - Printing individual identifiers in error messages. That's the use case that @estebank had in mind when implementing Accurately portray raw identifiers in error messages #67010.
Apparently the rawness should be guessed as well in this case. Should the$crates be converted though? I have no idea.
If the logic is that the printed error message should look as similar as possible to the source code, then it shouldn't.
I agree that this use case is better served by aDisplayimpl (the most ergonomic way) than by an explicit function, because error reporting code is more often written by people than pretty-printing code which is already written once.
There was a problem hiding this comment.
I guess I'll change the PR to account for the "print ast::Ident in a way that is as similar as possible to its originally written token in source code" use case, which is important for error messages.
There was a problem hiding this comment.
And also move token_to_string to Display for Token while I'm here.
Nonterminal printing needs to pretty-print AST, so it won't fly.
|
☔ The latest upstream changes (presumably #69334) made this pull request unmergeable. Please resolve the merge conflicts. |
petrochenkov
force-pushed
the
idprint
branch
from
af9ff5d to
e355a33
Compare
|
Updated with more comments and #69387 (comment) addressed. |
|
@bors r+ Looks good, thanks for the clarifications! |
|
📌 Commit e355a33 has been approved by |
bors
added
S-waiting-on-bors
…crum Deduplicate identifier printing a bit rust-lang#67010 introduced a couple more subtly different ways to print an identifier. This PR attempts to restore the order. The most basic identifier printing interface is `Formatter`-based now, so `String`s are not allocated unless required. r? @Mark-Simulacrum
Rollup of 7 pull requests Successful merges: - #67637 (Add primitive module to libcore) - #69387 (Deduplicate identifier printing a bit) - #69412 (Mark attributes consumed by `check_mod_attrs` as normal) - #69423 (syntax: Remove `Nt(Impl,Trait,Foreign)Item`) - #69429 (remove redundant clones and import) - #69457 (Clean up e0370 e0371) - #69468 ([master] Backport release notes of 1.41.1) Failed merges: r? @ghost
…acrum rustc_span: Add `Symbol::to_ident_string` for use in diagnostic messages Covers the same error reporting use case (rust-lang#69387 (comment)) as the `Display` impl for `Ident`. cc rust-lang#69053 Follow-up to rust-lang#69387. r? @Mark-Simulacrum
…acrum rustc_span: Add `Symbol::to_ident_string` for use in diagnostic messages Covers the same error reporting use case (rust-lang#69387 (comment)) as the `Display` impl for `Ident`. cc rust-lang#69053 Follow-up to rust-lang#69387. r? @Mark-Simulacrum
#67010 introduced a couple more subtly different ways to print an identifier.
This PR attempts to restore the order.
The most basic identifier printing interface is
Formatter-based now, soStrings are not allocated unless required.r? @Mark-Simulacrum