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

Add foreign formatting directive detection. #37613

Merged
merged 1 commit into from
Nov 12, 2016

Conversation

DanielKeep
Copy link
Contributor

This teaches format_args! how to interpret format printf- and
shell-style format directives. This is used in cases where there are
unused formatting arguments, and the reason for that might be because
the programmer is trying to use the wrong kind of formatting string.

This was prompted by an issue encountered by simulacrum on the #rust IRC
channel. In short: although println! told them that they weren't using
all of the conversion arguments, the problem was in using printf-syle
directives rather than ones println! would undertand.

Where possible, format_args! will tell the programmer what they should
use instead. For example, it will suggest replacing %05d with {:0>5},
or %2$.*3$s with {1:.3$}. Even if it cannot suggest a replacement,
it will explicitly note that Rust does not support that style of directive,
and direct the user to the std::fmt documentation.


Example: given:

fn main() {
    println!("%.*3$s %s!\n", "Hello,", "World", 4);
    println!("%1$*2$.*3$f", 123.456);
}

The compiler outputs the following:

error: multiple unused formatting arguments
 --> local/fmt.rs:2:5
  |
2 |     println!("%.*3$s %s!\n", "Hello,", "World", 4);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
note: argument never used
 --> local/fmt.rs:2:30
  |
2 |     println!("%.*3$s %s!\n", "Hello,", "World", 4);
  |                              ^^^^^^^^
note: argument never used
 --> local/fmt.rs:2:40
  |
2 |     println!("%.*3$s %s!\n", "Hello,", "World", 4);
  |                                        ^^^^^^^
note: argument never used
 --> local/fmt.rs:2:49
  |
2 |     println!("%.*3$s %s!\n", "Hello,", "World", 4);
  |                                                 ^
  = help: `%.*3$s` should be written as `{:.2$}`
  = help: `%s` should be written as `{}`
  = note: printf formatting not supported; see the documentation for `std::fmt`
  = note: this error originates in a macro outside of the current crate

error: argument never used
 --> local/fmt.rs:6:29
  |
6 |     println!("%1$*2$.*3$f", 123.456);
  |                             ^^^^^^^
  |
  = help: `%1$*2$.*3$f` should be written as `{0:1$.2$}`
  = note: printf formatting not supported; see the documentation for `std::fmt`

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@DanielKeep DanielKeep force-pushed the eww-you-got-printf-in-your-format branch from 34d195f to 8830dd1 Compare November 6, 2016 11:02
@aturon
Copy link
Member

aturon commented Nov 8, 2016

cc @rust-lang/lang @rust-lang/libs -- not sure what the right jurisdiction here is :)

This seems like an amazing change to me!

@alexcrichton
Copy link
Member

Sounds like a good idea to me!

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 10, 2016
@alexcrichton
Copy link
Member

I'm going to unilaterally usurp this from the lang team and declare it a libs PR! This seems like a large enough feature though that we'd want to consider it carefully, though, in which case...

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 10, 2016

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@brson
Copy link
Contributor

brson commented Nov 11, 2016

This looks reasonable to me, but although it contains lots of unit tests, I'd like to see at least one compile-fail test showing that the compiler does indeed emit these errors.

@DanielKeep
Copy link
Contributor Author

@brson I looked at modifying an existing compile-fail test for format!, but couldn't work out how to annotate the errors with the extra fields. Is there a reference or example somewhere?

@eddyb
Copy link
Member

eddyb commented Nov 11, 2016

@DanielKeep You should probably use an ui test instead, which can test the exact error output.

@DanielKeep DanielKeep force-pushed the eww-you-got-printf-in-your-format branch from 8830dd1 to e5f07a6 Compare November 11, 2016 02:35
This teaches `format_args!` how to interpret format printf- and
shell-style format directives.  This is used in cases where there are
unused formatting arguments, and the reason for that *might* be because
the programmer is trying to use the wrong kind of formatting string.

This was prompted by an issue encountered by simulacrum on the #rust IRC
channel.  In short: although `println!` told them that they weren't using
all of the conversion arguments, the problem was in using printf-syle
directives rather than ones `println!` would undertand.

Where possible, `format_args!` will tell the programmer what they should
use instead.  For example, it will suggest replacing `%05d` with `{:0>5}`,
or `%2$.*3$s` with `{1:.3$}`.  Even if it cannot suggest a replacement,
it will explicitly note that Rust does not support that style of directive,
and direct the user to the `std::fmt` documentation.
@DanielKeep DanielKeep force-pushed the eww-you-got-printf-in-your-format branch from e5f07a6 to 455723c Compare November 11, 2016 04:23
@DanielKeep
Copy link
Contributor Author

Added a ui test, as suggested.

@brson
Copy link
Contributor

brson commented Nov 11, 2016

Thanks @DanielKeep !

@rfcbot
Copy link

rfcbot commented Nov 12, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 12, 2016

📌 Commit 455723c has been approved by alexcrichton

eddyb added a commit to eddyb/rust that referenced this pull request Nov 12, 2016
…ur-format, r=alexcrichton

Add foreign formatting directive detection.

This teaches `format_args!` how to interpret format printf- and
shell-style format directives.  This is used in cases where there are
unused formatting arguments, and the reason for that *might* be because
the programmer is trying to use the wrong kind of formatting string.

This was prompted by an issue encountered by simulacrum on the #rust IRC
channel.  In short: although `println!` told them that they weren't using
all of the conversion arguments, the problem was in using printf-syle
directives rather than ones `println!` would undertand.

Where possible, `format_args!` will tell the programmer what they should
use instead.  For example, it will suggest replacing `%05d` with `{:0>5}`,
or `%2$.*3$s` with `{1:.3$}`.  Even if it cannot suggest a replacement,
it will explicitly note that Rust does not support that style of directive,
and direct the user to the `std::fmt` documentation.

-----

**Example**: given:

```rust
fn main() {
    println!("%.*3$s %s!\n", "Hello,", "World", 4);
    println!("%1$*2$.*3$f", 123.456);
}
```

The compiler outputs the following:

```text
error: multiple unused formatting arguments
 --> local/fmt.rs:2:5
  |
2 |     println!("%.*3$s %s!\n", "Hello,", "World", 4);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
note: argument never used
 --> local/fmt.rs:2:30
  |
2 |     println!("%.*3$s %s!\n", "Hello,", "World", 4);
  |                              ^^^^^^^^
note: argument never used
 --> local/fmt.rs:2:40
  |
2 |     println!("%.*3$s %s!\n", "Hello,", "World", 4);
  |                                        ^^^^^^^
note: argument never used
 --> local/fmt.rs:2:49
  |
2 |     println!("%.*3$s %s!\n", "Hello,", "World", 4);
  |                                                 ^
  = help: `%.*3$s` should be written as `{:.2$}`
  = help: `%s` should be written as `{}`
  = note: printf formatting not supported; see the documentation for `std::fmt`
  = note: this error originates in a macro outside of the current crate

error: argument never used
 --> local/fmt.rs:6:29
  |
6 |     println!("%1$*2$.*3$f", 123.456);
  |                             ^^^^^^^
  |
  = help: `%1$*2$.*3$f` should be written as `{0:1$.2$}`
  = note: printf formatting not supported; see the documentation for `std::fmt`
```
bors added a commit that referenced this pull request Nov 12, 2016
@bors bors merged commit 455723c into rust-lang:master Nov 12, 2016
@bors
Copy link
Contributor

bors commented Nov 12, 2016

☔ The latest upstream changes (presumably #37730) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants