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.

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

[WIP] Add implicit named arguments for fmt macros #65338

Closed

Conversation

davidhewitt
Copy link
Contributor

It struck me the other day that it would be really nice to omit the named argument to println! for lots of simple use cases, e.g.:

let recipient = "World";
println!("Hello, {}", recipient);                     // Most common style (I think)
println!("Hello, {recipient}", recipient=recipient);  // We can do this already, clearer but longer

// So how about this being sugar for the above?
println!("Hello, {recipient}");

I believe adding this shorthand will make a lot of invocations to the format family of macros marginally shorter, and slightly easier to read. There's also a lot of precedent for it in high-level languages e.g. Javascript backticks, Python f-strings.

I actually opened a thread about this on internals a while ago: https://internals.rust-lang.org/t/println-use-named-arguments-from-scope/10633

The response on the thread was that it generally seems reasonable and ergonomic to extend these macros to implicitly use simple identifiers. We didn't think it was desirable to support arbitrary expressions. So I've gone ahead and implemented this little PR to write a test implementation. If this needs an RFC I can backtrack and write one.

If the concensus is that this is a nice addition to the macros, then I'll need to do a little more work:

  • Add documentation to the std::fmt module to cover this new functionality
  • Add some more thorough testing
  • Error messages: if the identifier doesn't exist in scope we get the obvious enough "cannot find value recipient in this scope". In this simple first pass of the PR the error span is the whole format string for the macro. It really should be just underneath the invalid identifier.
  • Possible extension to allow fields e.g. println!("Hello, {self.recipient}"), which would be even more flexible though could occasionally have side effects from dereferencing.

Hope you guys like the idea 😄

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-10-12T14:22:08.9784877Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-12T14:22:09.0190060Z ##[command]git config gc.auto 0
2019-10-12T14:22:09.0286008Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-12T14:22:09.0352815Z ##[command]git config --get-all http.proxy
2019-10-12T14:22:09.0503491Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65338/merge:refs/remotes/pull/65338/merge
---
2019-10-12T14:28:52.7363120Z    Compiling serde_json v1.0.40
2019-10-12T14:28:54.5328277Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-10-12T14:29:05.9093288Z     Finished release [optimized] target(s) in 1m 28s
2019-10-12T14:29:05.9172467Z tidy check
2019-10-12T14:29:06.0458722Z tidy error: /checkout/src/libsyntax_ext/format.rs:452: line longer than 100 chars
2019-10-12T14:29:08.0318558Z some tidy checks failed
2019-10-12T14:29:08.0322588Z Found 482 error codes
2019-10-12T14:29:08.0323201Z Found 0 error codes with no tests
2019-10-12T14:29:08.0323564Z Done!
2019-10-12T14:29:08.0323564Z Done!
2019-10-12T14:29:08.0323776Z 
2019-10-12T14:29:08.0323968Z 
2019-10-12T14:29:08.0325418Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-10-12T14:29:08.0326134Z 
2019-10-12T14:29:08.0326312Z 
2019-10-12T14:29:08.0329777Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-10-12T14:29:08.0330049Z Build completed unsuccessfully in 0:01:31
2019-10-12T14:29:08.0330049Z Build completed unsuccessfully in 0:01:31
2019-10-12T14:29:08.0379116Z == clock drift check ==
2019-10-12T14:29:08.0397784Z   local time: Sat Oct 12 14:29:08 UTC 2019
2019-10-12T14:29:08.2010849Z   network time: Sat, 12 Oct 2019 14:29:08 GMT
2019-10-12T14:29:08.2014682Z == end clock drift check ==
2019-10-12T14:29:09.0684137Z ##[error]Bash exited with code '1'.
2019-10-12T14:29:09.0713567Z ##[section]Starting: Checkout
2019-10-12T14:29:09.0715810Z ==============================================================================
2019-10-12T14:29:09.0715877Z Task         : Get sources
2019-10-12T14:29:09.0715918Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril Centril added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 12, 2019
@Centril Centril added this to the 1.40 milestone Oct 12, 2019
@Centril
Copy link
Contributor

Centril commented Oct 12, 2019

r? @petrochenkov (hygiene & stuff...)

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 12, 2019

I love the idea (at least in the single identifier variation), and it proved itself quite useful in struct literal expressions like let s = S { field };.
And I remember wanting it for formatting strings specifically too.

It's still think it may be a noticeable enough addition to deserve an RFC.

@petrochenkov
Copy link
Contributor

The hygiene question is pretty simple here - inherit the identifier span from the string literal (at least the hygienic context part of it, the location can probably be made more precise).
Looks like the PR does exactly that.

@petrochenkov
Copy link
Contributor

Most of built-in macros are basically library proc macros now, so they should be under T-libs rather than T-lang?
Adding both teams and marking as waiting on team to decide whether this need an RFC or not.

@petrochenkov petrochenkov added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2019
@davidhewitt
Copy link
Contributor Author

I love the idea (at least in the single identifier variation), and it proved itself quite useful in struct literal expressions like let s = S { field };.

Cool! It took me a while to realise what you meant by the shorthand being very similar in struct literal expressions. Yeah I guess it is (and funnily also a shorthand available in Javascript!) 😄

The hygiene question is pretty simple here - inherit the identifier span from the string literal (at least the hygienic context part of it, the location can probably be made more precise).
Looks like the PR does exactly that.

Yep, I think I see what I need to do to narrow down the location but would need a bit more change to the code than what I've done so far, so will wait until we know exactly what's agreed upon.

Adding both teams and marking as waiting on team to decide whether this need an RFC or not.

👍

@KodrAus
Copy link
Contributor

KodrAus commented Oct 17, 2019

This seems like a great enhancement to me! I think this is something that would need an RFC. That would also be a chance for libraries maintaining their own macros that mimic format_args behaviour to chime in.

@withoutboats
Copy link
Contributor

We discussed this in the lang team meeting. In general, we are enthusiastic about this idea. We all have that experience that most of the time we use {identifier} formats, we end up writing identifier=identifier. However, we agree with @KodrAus that this should have an RFC before we land it.

@davidhewitt
Copy link
Contributor Author

\o/ I'll get to work on an RFC at the weekend!

@petrochenkov
Copy link
Contributor

Closing in favor of the upcoming RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants