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

Improve suggestion for missing fmt str in println #52394

Merged
merged 19 commits into from
Jul 22, 2018
Merged

Improve suggestion for missing fmt str in println #52394

merged 19 commits into from
Jul 22, 2018

Conversation

estebank
Copy link
Contributor

Avoid using concat!(fmt, "\n") to improve the diagnostics being
emitted when the first println!() argument isn't a formatting string
literal.

Fix #52347.

@estebank
Copy link
Contributor Author

r? @oli-obk

CC @csmoe I tried a couple of approaches, going down the same path that you mentioned in #52347, and the current PR was the least disruptive change I found.

This will need a perf run to see how much of an impact the change to println!() actually is :-/

@bors try

@bors
Copy link
Contributor

bors commented Jul 15, 2018

⌛ Trying commit a99c2da with merge effb34d...

bors added a commit that referenced this pull request Jul 15, 2018
@bors
Auto merge of #52394 - estebank:println, r=<try>
effb34d 
Improve suggestion for missing fmt str in println

Avoid using `concat!(fmt, "\n")` to improve the diagnostics being
emitted when the first `println!()` argument isn't a formatting string
literal.

Fix #52347.
@estebank
Copy link
Contributor Author

cc @ollie27

@rust-highfive

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jul 15, 2018

☀️ Test successful - status-travis
State: approved= try=True

@estebank
Copy link
Contributor Author

@rust-timer build effb34d

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@estebank
Copy link
Contributor Author

cc @kennytm

@estebank
Copy link
Contributor Author

Note to self: Need to fix run-make-fulldeps/hir-tree to not match on the trailing \n, given the change to println.

@kennytm
Copy link
Member

kennytm commented Jul 15, 2018

@rust-timer build effb34d

@rust-timer
Copy link
Collaborator

Success: Queued effb34d with parent 49f1e5d, comparison URL.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 15, 2018
ollie27
ollie27 reviewed Jul 15, 2018
View reviewed changes
src/libstd/macros.rs Outdated
($fmt:expr, $($arg:tt)*) => (print!(concat!($fmt, "\n"), $($arg)*));
($fmt:expr) => ({
print!($fmt);
print!("\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using two print!s will mean stdout will be unlocked before printing the \n. Replacing these two rules with

($($arg:tt)*) => (print!("{}\n", format_args!($($arg)*)));

will avoid that issue.

src/libsyntax_ext/concat.rs Outdated
}
}
}
if missing_literal.len() > 0 {
let mut err = cx.struct_span_err(missing_literal, "expected a literal");
err.note("only `&str` literals can be passed to `concat!()`");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most literals can be passed to concat!, not just &str.

Copy link
Contributor Author

@estebank estebank Jul 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but the wording "only literals..." could be harder to understand to newcomers... What do you think of

only literals (like `"foo"`, `42` and `3.14`) can be passed to `concat!()`

@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2018
@estebank
Copy link
Contributor Author

@kennytm can we get another perf run? After the last change I doubt this will introduce any regression.

@kennytm
Copy link
Member

kennytm commented Jul 15, 2018

@bors try

@bors
Copy link
Contributor

bors commented Jul 15, 2018

⌛ Trying commit d9450a6 with merge ac3b07a...

bors added a commit that referenced this pull request Jul 15, 2018
@bors
Auto merge of #52394 - estebank:println, r=<try>
ac3b07a 
Improve suggestion for missing fmt str in println

Avoid using `concat!(fmt, "\n")` to improve the diagnostics being
emitted when the first `println!()` argument isn't a formatting string
literal.

Fix #52347.
@rust-highfive

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jul 15, 2018

☀️ Test successful - status-travis
State: approved= try=True

@kennytm
Copy link
Member

kennytm commented Jul 16, 2018

@rust-timer build ac3b07a

@rust-timer
Copy link
Collaborator

Success: Queued ac3b07a with parent fb8bde0, comparison URL.

@kennytm
Copy link
Member

kennytm commented Jul 16, 2018

Perf is ready. Looks even worse than the previous one.

@kennytm kennytm removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 16, 2018
@estebank
Copy link
Contributor Author

In that case I guess I'll have to come up with something more involved if we want this :-/

@estebank
Copy link
Contributor Author

Cc @dtolnay after the talk in Rust Meetup. Note to self, see if marching quote literals is possible.

Esteban Küber added 2 commits July 21, 2018 15:50
Gate format_args_nll behind feature flag
00d5000
Change eprintln!()
a7a6837 
Address rust-lang#30143 as well. `writeln!()` hasn't been changed.
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jul 22, 2018

📌 Commit dc563d9 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2018
@bors
Copy link
Contributor

bors commented Jul 22, 2018

⌛ Testing commit dc563d9 with merge 3d51086...

bors added a commit that referenced this pull request Jul 22, 2018
@bors
Auto merge of #52394 - estebank:println, r=oli-obk
3d51086 
Improve suggestion for missing fmt str in println

Avoid using `concat!(fmt, "\n")` to improve the diagnostics being
emitted when the first `println!()` argument isn't a formatting string
literal.

Fix #52347.
@bors
Copy link
Contributor

bors commented Jul 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 3d51086 to master...

@bors bors merged commit dc563d9 into rust-lang:master Jul 22, 2018
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #52394!

Tested on commit 3d51086.
Direct link to PR: #52394

💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jul 22, 2018
@rust-highfive
📣 Toolstate changed by rust-lang/rust#52394!
c309115 
Tested on commit rust-lang/rust@3d51086.
Direct link to PR: <rust-lang/rust#52394>

💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
@bors bors mentioned this pull request Jul 22, 2018
Merged
@kennytm
Copy link
Member

kennytm commented Jul 22, 2018 

[01:26:48] failures:
[01:26:48]     [ui] ui/matches.rs
[01:26:48]     [ui] ui/print.rs
[01:26:48]     [ui] ui/print_literal.rs
[01:26:48]     [ui] ui/println_empty_string.rs

@ollie27 ollie27 mentioned this pull request Aug 10, 2018
Merged
bors added a commit that referenced this pull request Aug 16, 2018
@bors
Auto merge of #53256 - ollie27:writeln, r=KodrAus
5050349 
Don't accept non-string literals for the format string in writeln

This is to improve diagnostics.

`println` and `eprintln` were already fixed by #52394.

Fixes #30143
@pnkfelix pnkfelix mentioned this pull request Oct 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ollie27 ollie27 ollie27 left review comments

@oli-obk oli-obk oli-obk approved these changes

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@estebank @bors @rust-highfive @rust-timer @kennytm @dtolnay @oli-obk @ollie27