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

println!, eprintln! and writeln! accept other literals not just format strings #30143

Closed
ollie27 opened this issue Dec 1, 2015 · 13 comments · Fixed by #53256
Closed

println!, eprintln! and writeln! accept other literals not just format strings #30143

ollie27 opened this issue Dec 1, 2015 · 13 comments · Fixed by #53256
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ollie27
Copy link
Member

ollie27 commented Dec 1, 2015

The following all compile and run for example:

println!(0);
writeln!(b, true);
println!('a');

I would expect them to produce "error: format argument must be a string literal." like print!(0); does.

This is because they use things like concat!($fmt, "\n") but concat! accepts other literals not just str.

I assume this isn't intentional.

@steveklabnik steveklabnik added A-libs A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Dec 1, 2015
@jFransham
Copy link
Contributor

One way to fix this is to replace print!'s definition with

macro_rules! print {
    ($fmt:tt $($arg:tt)*) => ($crate::io::_print(format_args!(concat!($fmt) $($arg)*)));
}

I would prefer to force println! and friends to take strings but at least this keeps it consistent.

EDIT: I honestly don't like how concat! automatically stringifies, it seems kinda ugly and I feel like there should be a separate macro that does compile-time stringification. This would almost certainly be super backwards-incompatible though.

@nagisa
Copy link
Member

nagisa commented Dec 1, 2015

I feel like there should be a separate macro that does compile-time stringification

stringify!

@jFransham
Copy link
Contributor

@nagisa awesome! Why do the places that need concat!'s stringifying functionality not use concat!(stringify!()) so concat! doesn't duplicate functionality? That would solve this issue.

EDIT: Turns out stringify! is different to concat!, the latter only accepts literals whereas the former will verbatim stringify token trees, such as assert_eq!(stringify!(a + b), "a + b").

@durka
Copy link
Contributor

durka commented Dec 1, 2015

Seems like it'd be a breaking change to "fix" this.

@bluss
Copy link
Member

bluss commented Dec 2, 2015

For unreachable!() it is explicit and by design, so that part is no problem.

($msg:expr) => ({
        unreachable!("{}", $msg)
    });

@ollie27 ollie27 changed the title println!, writeln! and unreachable! accept other literals not just format strings println! and writeln! accept other literals not just format strings Dec 2, 2015
@ollie27
Copy link
Member Author

ollie27 commented Dec 2, 2015

@bluss good point, unreachable! actually has it's own unintuitive behaviour similar but different to panic! #22932.

@raindev
Copy link
Contributor

raindev commented Dec 20, 2015

Why do we consider that the code like println!('a') works to be a bug? I understand that forbidding it will make println! more consistent and explicit but we're trading in some convenience here. The other option could be to make print! behave in same way.

@ollie27
Copy link
Member Author

ollie27 commented Dec 23, 2015

@raindev it's not documented so technically something needs to fixed, even if it's just the documentation.

If we make print! behave the same then surely we'd have to change format! and format_args! as well to keep things consistent and I don't think that's a good idea as this isn't even useful.

I think this should either be fixed as a breaking change, I doubt it will break much if any real world code, or just depreciated.

@raindev
Copy link
Contributor

raindev commented Dec 23, 2015

@ollie27 thanks for the clarifications, thats makes sense now.

I'm not sure format! and format_args! should behave exactly as print! and println! as the two groups of macros serve different purposes. Maybe one more special case doesn't worth it however.

@huonw huonw added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 6, 2016
@aturon aturon added rust-2-breakage-wishlist In the wishlist of breaking changes that requires rust 2.0 and removed I-nominated rust-2-breakage-wishlist In the wishlist of breaking changes that requires rust 2.0 labels Jan 8, 2016
@aturon
Copy link
Member

aturon commented Jan 8, 2016

Libs team consensus: while it may be desirable to clean this up, this is not breakage we're willing to take on during the Rust 1.x series. I've added a new tag, closed-as-desirable-breakage, to help us track issues like this that we might want to revisit if we ever produce a 2.0 release.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 12, 2016

related issue: assert! accepts non-bools

@ollie27
Copy link
Member Author

ollie27 commented May 12, 2017

Sadly, eprintln has been implemented with this bug, so is also affected: #41192.

@ollie27 ollie27 changed the title println! and writeln! accept other literals not just format strings println!, eprintln! and writeln! accept other literals not just format strings May 12, 2017
@eira-fransham
Copy link

This would be improved by #35625

estebank pushed a commit to estebank/rust that referenced this issue Jul 21, 2018
Address rust-lang#30143 as well. `writeln!()` hasn't been changed.
bors added a commit that referenced this issue Aug 16, 2018
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
@fee1-dead fee1-dead removed the rust-2-breakage-wishlist In the wishlist of breaking changes that requires rust 2.0 label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.