syntax: Expand format!() deterministically

Previously, format!("{a}{b}", a=foo(), b=bar()) has foo() and bar() run in a
nondeterminisc order. This is clearly a non-desirable property, so this commit
uses iteration over a list instead of iteration over a hash map to provide
deterministic code generation of these format arguments.

diff --git a/src/libsyntax/ext/deriving/show.rs b/src/libsyntax/ext/deriving/show.rs
index 5286720b9fc08d08c790ae1cda502c2db525e053..4b9925c8d9f3e9a4875d623dac5ba4afe34bc500 100644 (file)
--- a/src/libsyntax/ext/deriving/show.rs
+++ b/src/libsyntax/ext/deriving/show.rs
@@ -135,5 +135,6 @@ fn show_substructure(cx: &mut ExtCtxt, span: Span,
     // phew, not our responsibility any more!
     format::expand_preparsed_format_args(cx, span,
                                          format_closure,
-                                         format_string, exprs, HashMap::new())
+                                         format_string, exprs, ~[],
+                                         HashMap::new())
 }

diff --git a/src/libsyntax/ext/format.rs b/src/libsyntax/ext/format.rs
index 1b73d42c79aa4b98137573377422b4eec84f230a..2642ee00458c12015861328f26194e030f5b1cff 100644 (file)
--- a/src/libsyntax/ext/format.rs
+++ b/src/libsyntax/ext/format.rs
@@ -43,9 +43,13 @@ struct Context<'a> {
     // them.
     args: ~[@ast::Expr],
     arg_types: ~[Option<ArgumentType>],
-    // Parsed named expressions and the types that we've found for them so far
+    // Parsed named expressions and the types that we've found for them so far.
+    // Note that we keep a side-array of the ordering of the named arguments
+    // found to be sure that we can translate them in the same order that they
+    // were declared in.
     names: HashMap<~str, @ast::Expr>,
     name_types: HashMap<~str, ArgumentType>,
+    name_ordering: ~[~str],
 
     // Collection of the compiled `rt::Piece` structures
     pieces: ~[@ast::Expr],
@@ -63,12 +67,15 @@ struct Context<'a> {
 ///
 /// If parsing succeeds, the second return value is:
 ///
-///     Some((fmtstr, unnamed arguments, named arguments))
-fn parse_args(ecx: &mut ExtCtxt, sp: Span,
-              tts: &[ast::TokenTree]) -> (@ast::Expr, Option<(@ast::Expr, ~[@ast::Expr],
-                                                              HashMap<~str, @ast::Expr>)>) {
+///     Some((fmtstr, unnamed arguments, ordering of named arguments,
+///           named arguments))
+fn parse_args(ecx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
+    -> (@ast::Expr, Option<(@ast::Expr, ~[@ast::Expr], ~[~str],
+                            HashMap<~str, @ast::Expr>)>)
+{
     let mut args = ~[];
     let mut names = HashMap::<~str, @ast::Expr>::new();
+    let mut order = ~[];
 
     let mut p = rsparse::new_parser_from_tts(ecx.parse_sess(),
                                              ecx.cfg(),
@@ -125,12 +132,13 @@ fn parse_args(ecx: &mut ExtCtxt, sp: Span,
                     continue
                 }
             }
+            order.push(name.to_str());
             names.insert(name.to_str(), e);
         } else {
             args.push(p.parse_expr());
         }
     }
-    return (extra, Some((fmtstr, args, names)));
+    return (extra, Some((fmtstr, args, order, names)));
 }
 
 impl<'a> Context<'a> {
@@ -661,10 +669,11 @@ fn to_expr(&self, extra: @ast::Expr) -> @ast::Expr {
             locals.push(self.format_arg(e.span, Exact(i),
                                         self.ecx.expr_ident(e.span, name)));
         }
-        for (name, &e) in self.names.iter() {
-            if !self.name_types.contains_key(name) {
-                continue
-            }
+        for name in self.name_ordering.iter() {
+            let e = match self.names.find(name) {
+                Some(&e) if self.name_types.contains_key(name) => e,
+                Some(..) | None => continue
+            };
 
             let lname = self.ecx.ident_of(format!("__arg{}", *name));
             pats.push(self.ecx.pat_ident(e.span, lname));
@@ -810,8 +819,9 @@ pub fn expand_args(ecx: &mut ExtCtxt, sp: Span,
                    tts: &[ast::TokenTree]) -> base::MacResult {
 
     match parse_args(ecx, sp, tts) {
-        (extra, Some((efmt, args, names))) => {
-            MRExpr(expand_preparsed_format_args(ecx, sp, extra, efmt, args, names))
+        (extra, Some((efmt, args, order, names))) => {
+            MRExpr(expand_preparsed_format_args(ecx, sp, extra, efmt, args,
+                                                order, names))
         }
         (_, None) => MRExpr(ecx.expr_uint(sp, 2))
     }
@@ -823,6 +833,7 @@ pub fn expand_args(ecx: &mut ExtCtxt, sp: Span,
 pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, sp: Span,
                                     extra: @ast::Expr,
                                     efmt: @ast::Expr, args: ~[@ast::Expr],
+                                    name_ordering: ~[~str],
                                     names: HashMap<~str, @ast::Expr>) -> @ast::Expr {
     let arg_types = vec::from_fn(args.len(), |_| None);
     let mut cx = Context {
@@ -832,6 +843,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, sp: Span,
         names: names,
         name_positions: HashMap::new(),
         name_types: HashMap::new(),
+        name_ordering: name_ordering,
         nest_level: 0,
         next_arg: 0,
         pieces: ~[],

diff --git a/src/test/run-pass/ifmt.rs b/src/test/run-pass/ifmt.rs
index ca798a77a4fd12387e1eb9f891b05300e69603f0..564f7b43426395d756c2a23b730c8290ac6236b2 100644 (file)
--- a/src/test/run-pass/ifmt.rs
+++ b/src/test/run-pass/ifmt.rs
@@ -139,6 +139,7 @@ pub fn main() {
 
     test_write();
     test_print();
+    test_order();
 
     // make sure that format! doesn't move out of local variables
     let a = ~3;
@@ -202,3 +203,18 @@ fn test_format_args() {
     let s = format_args!(fmt::format, "hello {}", "world");
     t!(s, "hello world");
 }
+
+fn test_order() {
+    // Make sure format!() arguments are always evaluated in a left-to-right
+    // ordering
+    fn foo() -> int {
+        static mut FOO: int = 0;
+        unsafe {
+            FOO += 1;
+            FOO
+        }
+    }
+    assert_eq!(format!("{} {} {a} {b} {} {c}",
+                       foo(), foo(), foo(), a=foo(), b=foo(), c=foo()),
+               ~"1 2 4 5 3 6");
+}