Skip to content

Commit

Permalink
Improve heuristics whether format_args string is a source literal
Browse files Browse the repository at this point in the history
Previously, it only checked whether there was _a_ literal at the span of
the first argument, not whether the literal actually matched up. This
caused issues when a proc macro was generating a different literal with
the same span.

This requires an annoying special case for literals ending in `\n`
because otherwise `println` wouldn't give detailed diagnostics anymore
which would be bad.
  • Loading branch information
Noratrieb committed Dec 28, 2022
1 parent 1322e47 commit e6c02aa
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 2 deletions.
37 changes: 36 additions & 1 deletion compiler/rustc_parse_format/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub use Flag::*;
pub use Piece::*;
pub use Position::*;

use rustc_lexer::unescape;
use std::iter;
use std::str;
use std::string;
Expand Down Expand Up @@ -306,7 +307,7 @@ impl<'a> Parser<'a> {
append_newline: bool,
mode: ParseMode,
) -> Parser<'a> {
let (width_map, is_literal) = find_width_map_from_snippet(snippet, style);
let (width_map, is_literal) = find_width_map_from_snippet(s, snippet, style);
Parser {
mode,
input: s,
Expand Down Expand Up @@ -844,6 +845,7 @@ impl<'a> Parser<'a> {
/// written code (code snippet) and the `InternedString` that gets processed in the `Parser`
/// in order to properly synthesise the intra-string `Span`s for error diagnostics.
fn find_width_map_from_snippet(
input: &str,
snippet: Option<string::String>,
str_style: Option<usize>,
) -> (Vec<InnerWidthMapping>, bool) {
Expand All @@ -856,8 +858,27 @@ fn find_width_map_from_snippet(
return (vec![], true);
}

// Strip quotes.
let snippet = &snippet[1..snippet.len() - 1];

// Macros like `println` add a newline at the end. That technically doens't make them "literals" anymore, but it's fine
// since we will never need to point our spans there, so we lie about it here by ignoring it.
// Since there might actually be newlines in the source code, we need to normalize away all trailing newlines.
// If we only trimmed it off the input, `format!("\n")` would cause a mismatch as here we they actually match up.
// Alternatively, we could just count the trailing newlines and only trim one from the input if they don't match up.
let input_no_nl = input.trim_end_matches('\n');
let Ok(unescaped) = unescape_string(snippet) else {
return (vec![], false);
};

let unescaped_no_nl = unescaped.trim_end_matches('\n');

if unescaped_no_nl != input_no_nl {
// The source string that we're pointing at isn't our input, so spans pointing at it will be incorrect.
// This can for example happen with proc macros that respan generated literals.
return (vec![], false);
}

let mut s = snippet.char_indices();
let mut width_mappings = vec![];
while let Some((pos, c)) = s.next() {
Expand Down Expand Up @@ -936,9 +957,23 @@ fn find_width_map_from_snippet(
_ => {}
}
}

(width_mappings, true)
}

fn unescape_string(string: &str) -> Result<string::String, unescape::EscapeError> {
let mut buf = string::String::new();
let mut error = Ok(());
unescape::unescape_literal(string, unescape::Mode::Str, &mut |_, unescaped_char| {
match unescaped_char {
Ok(c) => buf.push(c),
Err(err) => error = Err(err),
}
});

error.map(|_| buf)
}

// Assert a reasonable size for `Piece`
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Piece<'_>, 16);
Expand Down
14 changes: 13 additions & 1 deletion src/test/ui/fmt/auxiliary/format-string-proc-macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@

extern crate proc_macro;

use proc_macro::{Literal, Span, TokenStream, TokenTree};
use proc_macro::{Delimiter, Group, Ident, Literal, Punct, Spacing, Span, TokenStream, TokenTree};
use std::iter::FromIterator;

#[proc_macro]
pub fn foo_with_input_span(input: TokenStream) -> TokenStream {
Expand All @@ -26,3 +27,14 @@ pub fn err_with_input_span(input: TokenStream) -> TokenStream {

TokenStream::from(TokenTree::Literal(lit))
}

#[proc_macro]
pub fn respan_to_invalid_format_literal(input: TokenStream) -> TokenStream {
let mut s = Literal::string("{");
s.set_span(input.into_iter().next().unwrap().span());
TokenStream::from_iter([
TokenTree::from(Ident::new("format", Span::call_site())),
TokenTree::from(Punct::new('!', Spacing::Alone)),
TokenTree::from(Group::new(Delimiter::Parenthesis, TokenTree::from(s).into())),
])
}
10 changes: 10 additions & 0 deletions src/test/ui/fmt/respanned-literal-issue-106191.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// aux-build:format-string-proc-macro.rs

extern crate format_string_proc_macro;

fn main() {
format_string_proc_macro::respan_to_invalid_format_literal!("¡");
//~^ ERROR invalid format string: expected `'}'` but string was terminated
format_args!(r#concat!("¡ {"));
//~^ ERROR invalid format string: expected `'}'` but string was terminated
}
19 changes: 19 additions & 0 deletions src/test/ui/fmt/respanned-literal-issue-106191.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
error: invalid format string: expected `'}'` but string was terminated
--> $DIR/respanned-literal-issue-106191.rs:6:65
|
LL | format_string_proc_macro::respan_to_invalid_format_literal!("¡");
| ^^^ expected `'}'` in format string
|
= note: if you intended to print `{`, you can escape it using `{{`

error: invalid format string: expected `'}'` but string was terminated
--> $DIR/respanned-literal-issue-106191.rs:8:18
|
LL | format_args!(r#concat!("¡ {"));
| ^^^^^^^^^^^^^^^^^^^^^^^ expected `'}'` in format string
|
= note: if you intended to print `{`, you can escape it using `{{`
= note: this error originates in the macro `concat` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

0 comments on commit e6c02aa

Please sign in to comment.