Merge pull request rust-lang#3553 from bash/minor-refactorings

Minor refactorings

src/attr.rs

@@ -53,12 +53,11 @@ fn is_derive(attr: &ast::Attribute) -> bool {
 }
 
 /// Returns the arguments of `#[derive(...)]`.
-fn get_derive_spans(attr: &ast::Attribute) -> Option<Vec<Span>> {
+fn get_derive_spans<'a>(attr: &'a ast::Attribute) -> Option<impl Iterator<Item = Span> + 'a> {
     attr.meta_item_list().map(|meta_item_list| {
         meta_item_list
-            .iter()
+            .into_iter()
             .map(|nested_meta_item| nested_meta_item.span)
-            .collect()
     })
 }
 
@@ -411,10 +410,11 @@ impl<'a> Rewrite for [ast::Attribute] {
             // Handle derives if we will merge them.
             if context.config.merge_derives() && is_derive(&attrs[0]) {
                 let derives = take_while_with_pred(context, attrs, is_derive);
-                let mut derive_spans = vec![];
-                for derive in derives {
-                    derive_spans.append(&mut get_derive_spans(derive)?);
-                }
+                let derive_spans: Vec<_> = derives
+                    .iter()
+                    .filter_map(get_derive_spans)
+                    .flatten()
+                    .collect();
                 let derive_str =
                     format_derive(&derive_spans, attr_prefix(&attrs[0]), shape, context)?;
                 result.push_str(&derive_str);

src/checkstyle.rs

@@ -1,3 +1,4 @@
+use std::fmt::{self, Display};
 use std::io::{self, Write};
 use std::path::Path;
 
@@ -9,9 +10,9 @@ use crate::rustfmt_diff::{DiffLine, Mismatch};
 /// future version of Rustfmt.
 pub(crate) fn header() -> String {
     let mut xml_heading = String::new();
-    xml_heading.push_str("<?xml version=\"1.0\" encoding=\"utf-8\"?>");
+    xml_heading.push_str(r#"<?xml version="1.0" encoding="utf-8"?>"#);
     xml_heading.push_str("\n");
-    xml_heading.push_str("<checkstyle version=\"4.3\">");
+    xml_heading.push_str(r#"<checkstyle version="4.3">"#);
     xml_heading
 }
 
@@ -31,17 +32,16 @@ pub(crate) fn output_checkstyle_file<T>(
 where
     T: Write,
 {
-    write!(writer, "<file name=\"{}\">", filename.display())?;
+    write!(writer, r#"<file name="{}">"#, filename.display())?;
     for mismatch in diff {
         for line in mismatch.lines {
             // Do nothing with `DiffLine::Context` and `DiffLine::Resulting`.
-            if let DiffLine::Expected(ref str) = line {
-                let message = xml_escape_str(str);
+            if let DiffLine::Expected(message) = line {
                 write!(
                     writer,
-                    "<error line=\"{}\" severity=\"warning\" message=\"Should be `{}`\" \
-                     />",
-                    mismatch.line_number, message
+                    r#"<error line="{}" severity="warning" message="Should be `{}`" />"#,
+                    mismatch.line_number,
+                    XmlEscaped(&message)
                 )?;
             }
         }
@@ -50,19 +50,53 @@ where
     Ok(())
 }
 
-// Convert special characters into XML entities.
-// This is needed for checkstyle output.
-fn xml_escape_str(string: &str) -> String {
-    let mut out = String::new();
-    for c in string.chars() {
-        match c {
-            '<' => out.push_str("&lt;"),
-            '>' => out.push_str("&gt;"),
-            '"' => out.push_str("&quot;"),
-            '\'' => out.push_str("&apos;"),
-            '&' => out.push_str("&amp;"),
-            _ => out.push(c),
+/// Convert special characters into XML entities.
+/// This is needed for checkstyle output.
+struct XmlEscaped<'a>(&'a str);
+
+impl<'a> Display for XmlEscaped<'a> {
+    fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        for char in self.0.chars() {
+            match char {
+                '<' => write!(formatter, "&lt;"),
+                '>' => write!(formatter, "&gt;"),
+                '"' => write!(formatter, "&quot;"),
+                '\'' => write!(formatter, "&apos;"),
+                '&' => write!(formatter, "&amp;"),
+                _ => write!(formatter, "{}", char),
+            }?;
         }
+
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn special_characters_are_escaped() {
+        assert_eq!(
+            "&lt;&gt;&quot;&apos;&amp;",
+            format!("{}", XmlEscaped(r#"<>"'&"#)),
+        );
+    }
+
+    #[test]
+    fn special_characters_are_escaped_in_string_with_other_characters() {
+        assert_eq!(
+            "The quick brown &quot;🦊&quot; jumps &lt;over&gt; the lazy 🐶",
+            format!(
+                "{}",
+                XmlEscaped(r#"The quick brown "🦊" jumps <over> the lazy 🐶"#)
+            ),
+        );
+    }
+
+    #[test]
+    fn other_characters_are_not_escaped() {
+        let string = "The quick brown 🦊 jumps over the lazy 🐶";
+        assert_eq!(string, format!("{}", XmlEscaped(string)));
     }
-    out
 }

src/formatting.rs

@@ -458,8 +458,7 @@ struct FormatLines<'a> {
     errors: Vec<FormattingError>,
     issue_seeker: BadIssueSeeker,
     line_buffer: String,
-    // `true` if the current line contains a string literal.
-    is_string: bool,
+    current_line_contains_string_literal: bool,
     format_line: bool,
     allow_issue_seek: bool,
     config: &'a Config,
@@ -483,7 +482,7 @@ impl<'a> FormatLines<'a> {
             allow_issue_seek: !issue_seeker.is_disabled(),
             issue_seeker,
             line_buffer: String::with_capacity(config.max_width() * 2),
-            is_string: false,
+            current_line_contains_string_literal: false,
             format_line: config.file_lines().contains_line(name, 1),
             config,
         }
@@ -547,7 +546,7 @@ impl<'a> FormatLines<'a> {
                 && !self.is_skipped_line()
                 && self.should_report_error(kind, &error_kind)
             {
-                let is_string = self.is_string;
+                let is_string = self.current_line_contains_string_literal;
                 self.push_err(error_kind, kind.is_comment(), is_string);
             }
         }
@@ -561,7 +560,7 @@ impl<'a> FormatLines<'a> {
         self.newline_count += 1;
         self.last_was_space = false;
         self.line_buffer.clear();
-        self.is_string = false;
+        self.current_line_contains_string_literal = false;
     }
 
     fn char(&mut self, c: char, kind: FullCodeCharKind) {
@@ -574,7 +573,7 @@ impl<'a> FormatLines<'a> {
         self.last_was_space = c.is_whitespace();
         self.line_buffer.push(c);
         if kind.is_string() {
-            self.is_string = true;
+            self.current_line_contains_string_literal = true;
         }
     }
 
@@ -589,12 +588,14 @@ impl<'a> FormatLines<'a> {
     }
 
     fn should_report_error(&self, char_kind: FullCodeCharKind, error_kind: &ErrorKind) -> bool {
-        let allow_error_report =
-            if char_kind.is_comment() || self.is_string || error_kind.is_comment() {
-                self.config.error_on_unformatted()
-            } else {
-                true
-            };
+        let allow_error_report = if char_kind.is_comment()
+            || self.current_line_contains_string_literal
+            || error_kind.is_comment()
+        {
+            self.config.error_on_unformatted()
+        } else {
+            true
+        };
 
         match error_kind {
             ErrorKind::LineOverflow(..) => {