Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make format_args!() its own AST node (ast::ExprKind::FormatArgs) #541

Closed
1 of 3 tasks
m-ou-se opened this issue Aug 8, 2022 · 3 comments
Closed
1 of 3 tasks

Make format_args!() its own AST node (ast::ExprKind::FormatArgs) #541

m-ou-se opened this issue Aug 8, 2022 · 3 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Aug 8, 2022

Proposal

I originally wrote about this idea here: rust-lang/rust#78356 (comment)

Today, format_args!() expands to a (often rather big) call expression of fmt::Arguments::new…(…). Instead, I think it'd be better if it expanded to its own special AST node (e.g. ast::ExprKind::FormatArgs), which is only lowered to the actual fmt::Argument::new call in a later stage when the types and values of the arguments can be inspected (hir? mir?).

This means it'll be much easier to implement optimizations like the ones I described in rust-lang/rust#78356: Flattening and simplifying format_args!(). Not only to make things like format_args!("a {}", "b") compile down to the same code as format_args!("a b") (which const evaluation might also be able to achieve at some point), but also to make format_args!("a {} {}", "b", x) compile down to the same as format_args!("a b {}", x), and also to flatten nested format_args. For example, instead of needing format_args_nl!(…), a format_args!("{}\n", format_args!(…)) could result in the exact same code.

These optimization possibilities remove the need for some bad use cases of concat!() we currently see in the ecosystem. A crate might have a log!() macro that expands to something like write!(log, concat!("info: ", first_arg, "\n"), other_args…), which has the unfortunate consequence that log!(1) works, since concat converts that 1 to "1" implicitly. It also breaks implicit capturing like log!("hello {name}"). The "proper" solution is to instead expand to write!(log, "info: {}\n", format_args!(all_args…)), which, today, is far less efficient. (Which is the reason we have format_args_nl!() today for println.)

Separately from the optimization possibilities, having an intermediate representation of a parsed format_args!() invocation (with all the numeric and named and implicitly captured placeholders 'resolved', after any lints/errors/etc.), makes it much easier to try out different fmt::Arguments implementations. Today, changing the structure/implementation of fmt::Arguments is quite a big task, since it requires changing large parts of the format_args builtin macro. This makes experimenting with new, more efficient, implementations harder than necessary, which is a bit of a blocker for rust-lang/rust#99012.

In addition, it'd also make diagnostics/lints around formatting much easier, since a format_args!() invocation can be recognized as a single node, without having to pattern-match on the exact expansion of that macro. For example, check out Clippy's current code for recognizing format_args!(): https://github.com/rust-lang/rust-clippy/blob/97a0cf2de2367187c1f9eb9d9f6d333cb8bc8b8f/clippy_utils/src/macros.rs#L353-L552. Changing the implementation of fmt::Arguments also involves updating that part of Clippy at the same time, which adds even more resistance to any work on rust-lang/rust#99012.

And as a bonus, -Zunpretty=expanded would be a lot less noisy if every println/format/format_args call didn't result in a huge unreadable expansion. The ast::ExprKind::FormatArgs node would simply be displayed as format_args!(…).

cc @eddyb @oli-obk, I think I discussed this idea with both of you some time last year.

Mentors or Reviewers

If you have a reviewer or mentor in mind for this work, mention then
here. You can put your own name here if you are planning to mentor the
work.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
@m-ou-se m-ou-se added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Aug 8, 2022
@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2022

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Aug 8, 2022
@m-ou-se m-ou-se mentioned this issue Aug 8, 2022
Open
46 tasks
@oli-obk
Copy link
Contributor

oli-obk commented Aug 24, 2022

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Aug 24, 2022
@m-ou-se m-ou-se mentioned this issue Aug 25, 2022
Merged
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 25, 2022
@apiraino
Copy link
Contributor

apiraino commented Sep 8, 2022

@rustbot label -final-comment-period +major-change-accepted

@apiraino apiraino closed this as completed Sep 8, 2022
@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Sep 8, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 8, 2022
@est31 est31 mentioned this issue Sep 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 28, 2022
@bors
Auto merge of rust-lang#100996 - m-ou-se:format-args-2, r=estebank
d6734be 
Rewrite and refactor format_args!() builtin macro.

This is a near complete rewrite of `compiler/rustc_builtin_macros/src/format.rs`.

This gets rid of the massive unmaintanable [`Context` struct](https://github.com/rust-lang/rust/blob/76531befc4b0352247ada67bd225e8cf71ee5686/compiler/rustc_builtin_macros/src/format.rs#L176-L263), and splits the macro expansion into three parts:

1. First, `parse_args` will parse the `(literal, arg, arg, name=arg, name=arg)` syntax, but doesn't parse the template (the literal) itself.
2. Second, `make_format_args` will parse the template, the format options, resolve argument references, produce diagnostics, and turn the whole thing into a `FormatArgs` structure.
3. Finally, `expand_parsed_format_args` will turn that `FormatArgs` structure into the expression that the macro expands to.

In other words, the `format_args` builtin macro used to be a hard-to-maintain 'single pass compiler', which I've split into a three phase compiler with a parser/tokenizer (step 1), semantic analysis (step 2), and backend (step 3). (It's compilers all the way down. ^^)

This can serve as a great starting point for rust-lang#99012, which will only need to change the implementation of 3, while leaving step 1 and 2 unchanged.

It also makes rust-lang/compiler-team#541 easier, which could then upgrade the new `FormatArgs` struct to an `ast` node and remove step 3, moving that step to later in the compilation process.

It also fixes a few diagnostics bugs.

This also [significantly reduces](https://gist.github.com/m-ou-se/b67b2d54172c4837a5ab1b26fa3e5284) the amount of generated code for cases with arguments in non-default order without formatting options, like `"{1} {0}"` or `"{a} {}"`, etc.
@Alexendoo Alexendoo mentioned this issue Nov 26, 2022
Open
@m-ou-se m-ou-se mentioned this issue Jan 11, 2023
Merged
@nbdd0121 nbdd0121 mentioned this issue Jan 13, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 26, 2023
@bors
Auto merge of rust-lang#106745 - m-ou-se:format-args-ast, r=oli-obk
3e97763 
Move format_args!() into AST (and expand it during AST lowering)

Implements rust-lang/compiler-team#541

This moves FormatArgs from rustc_builtin_macros to rustc_ast_lowering. For now, the end result is the same. But this allows for future changes to do smarter things with format_args!(). It also allows Clippy to directly access the ast::FormatArgs, making things a lot easier.

This change turns the format args types into lang items. The builtin macro used to refer to them by their path. After this change, the path is no longer relevant, making it easier to make changes in `core`.

This updates clippy to use the new language items, but this doesn't yet make clippy use the ast::FormatArgs structure that's now available. That should be done after this is merged.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jan 27, 2023
@bors
Auto merge of #106745 - m-ou-se:format-args-ast, r=oli-obk
a64940f 
Move format_args!() into AST (and expand it during AST lowering)

Implements rust-lang/compiler-team#541

This moves FormatArgs from rustc_builtin_macros to rustc_ast_lowering. For now, the end result is the same. But this allows for future changes to do smarter things with format_args!(). It also allows Clippy to directly access the ast::FormatArgs, making things a lot easier.

This change turns the format args types into lang items. The builtin macro used to refer to them by their path. After this change, the path is no longer relevant, making it easier to make changes in `core`.

This updates clippy to use the new language items, but this doesn't yet make clippy use the ast::FormatArgs structure that's now available. That should be done after this is merged.
bors added a commit to rust-lang/miri that referenced this issue Jan 31, 2023
@bors
Auto merge of #106745 - m-ou-se:format-args-ast, r=oli-obk
cca1337 
Move format_args!() into AST (and expand it during AST lowering)

Implements rust-lang/compiler-team#541

This moves FormatArgs from rustc_builtin_macros to rustc_ast_lowering. For now, the end result is the same. But this allows for future changes to do smarter things with format_args!(). It also allows Clippy to directly access the ast::FormatArgs, making things a lot easier.

This change turns the format args types into lang items. The builtin macro used to refer to them by their path. After this change, the path is no longer relevant, making it easier to make changes in `core`.

This updates clippy to use the new language items, but this doesn't yet make clippy use the ast::FormatArgs structure that's now available. That should be done after this is merged.
thomcc pushed a commit to tcdi/postgrestd that referenced this issue May 31, 2023
@bors
Auto merge of #106745 - m-ou-se:format-args-ast, r=oli-obk
5d3d055 
Move format_args!() into AST (and expand it during AST lowering)

Implements rust-lang/compiler-team#541

This moves FormatArgs from rustc_builtin_macros to rustc_ast_lowering. For now, the end result is the same. But this allows for future changes to do smarter things with format_args!(). It also allows Clippy to directly access the ast::FormatArgs, making things a lot easier.

This change turns the format args types into lang items. The builtin macro used to refer to them by their path. After this change, the path is no longer relevant, making it easier to make changes in `core`.

This updates clippy to use the new language items, but this doesn't yet make clippy use the ast::FormatArgs structure that's now available. That should be done after this is merged.
calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this issue Jun 20, 2023
@bors
Auto merge of #106745 - m-ou-se:format-args-ast, r=oli-obk
6d13315 
Move format_args!() into AST (and expand it during AST lowering)

Implements rust-lang/compiler-team#541

This moves FormatArgs from rustc_builtin_macros to rustc_ast_lowering. For now, the end result is the same. But this allows for future changes to do smarter things with format_args!(). It also allows Clippy to directly access the ast::FormatArgs, making things a lot easier.

This change turns the format args types into lang items. The builtin macro used to refer to them by their path. After this change, the path is no longer relevant, making it easier to make changes in `core`.

This updates clippy to use the new language items, but this doesn't yet make clippy use the ast::FormatArgs structure that's now available. That should be done after this is merged.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
@bors
Auto merge of #100996 - m-ou-se:format-args-2, r=estebank
5981777 
Rewrite and refactor format_args!() builtin macro.

This is a near complete rewrite of `compiler/rustc_builtin_macros/src/format.rs`.

This gets rid of the massive unmaintanable [`Context` struct](https://github.com/rust-lang/rust/blob/76531befc4b0352247ada67bd225e8cf71ee5686/compiler/rustc_builtin_macros/src/format.rs#L176-L263), and splits the macro expansion into three parts:

1. First, `parse_args` will parse the `(literal, arg, arg, name=arg, name=arg)` syntax, but doesn't parse the template (the literal) itself.
2. Second, `make_format_args` will parse the template, the format options, resolve argument references, produce diagnostics, and turn the whole thing into a `FormatArgs` structure.
3. Finally, `expand_parsed_format_args` will turn that `FormatArgs` structure into the expression that the macro expands to.

In other words, the `format_args` builtin macro used to be a hard-to-maintain 'single pass compiler', which I've split into a three phase compiler with a parser/tokenizer (step 1), semantic analysis (step 2), and backend (step 3). (It's compilers all the way down. ^^)

This can serve as a great starting point for rust-lang/rust#99012, which will only need to change the implementation of 3, while leaving step 1 and 2 unchanged.

It also makes rust-lang/compiler-team#541 easier, which could then upgrade the new `FormatArgs` struct to an `ast` node and remove step 3, moving that step to later in the compilation process.

It also fixes a few diagnostics bugs.

This also [significantly reduces](https://gist.github.com/m-ou-se/b67b2d54172c4837a5ab1b26fa3e5284) the amount of generated code for cases with arguments in non-default order without formatting options, like `"{1} {0}"` or `"{a} {}"`, etc.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
@bors
Auto merge of #100996 - m-ou-se:format-args-2, r=estebank
2043890 
Rewrite and refactor format_args!() builtin macro.

This is a near complete rewrite of `compiler/rustc_builtin_macros/src/format.rs`.

This gets rid of the massive unmaintanable [`Context` struct](https://github.com/rust-lang/rust/blob/76531befc4b0352247ada67bd225e8cf71ee5686/compiler/rustc_builtin_macros/src/format.rs#L176-L263), and splits the macro expansion into three parts:

1. First, `parse_args` will parse the `(literal, arg, arg, name=arg, name=arg)` syntax, but doesn't parse the template (the literal) itself.
2. Second, `make_format_args` will parse the template, the format options, resolve argument references, produce diagnostics, and turn the whole thing into a `FormatArgs` structure.
3. Finally, `expand_parsed_format_args` will turn that `FormatArgs` structure into the expression that the macro expands to.

In other words, the `format_args` builtin macro used to be a hard-to-maintain 'single pass compiler', which I've split into a three phase compiler with a parser/tokenizer (step 1), semantic analysis (step 2), and backend (step 3). (It's compilers all the way down. ^^)

This can serve as a great starting point for rust-lang/rust#99012, which will only need to change the implementation of 3, while leaving step 1 and 2 unchanged.

It also makes rust-lang/compiler-team#541 easier, which could then upgrade the new `FormatArgs` struct to an `ast` node and remove step 3, moving that step to later in the compilation process.

It also fixes a few diagnostics bugs.

This also [significantly reduces](https://gist.github.com/m-ou-se/b67b2d54172c4837a5ab1b26fa3e5284) the amount of generated code for cases with arguments in non-default order without formatting options, like `"{1} {0}"` or `"{a} {}"`, etc.
@programmerjake programmerjake mentioned this issue May 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@oli-obk @m-ou-se @apiraino @rustbot