Skip to content

Commit 017c504

Browse files
committed
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.
1 parent ec57db0 commit 017c504

File tree

3 files changed

+42
-13
lines changed

3 files changed

+42
-13
lines changed

src/libsyntax/ext/deriving/show.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,5 +135,6 @@ fn show_substructure(cx: &mut ExtCtxt, span: Span,
135135
// phew, not our responsibility any more!
136136
format::expand_preparsed_format_args(cx, span,
137137
format_closure,
138-
format_string, exprs, HashMap::new())
138+
format_string, exprs, ~[],
139+
HashMap::new())
139140
}

src/libsyntax/ext/format.rs

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,13 @@ struct Context<'a> {
4343
// them.
4444
args: ~[@ast::Expr],
4545
arg_types: ~[Option<ArgumentType>],
46-
// Parsed named expressions and the types that we've found for them so far
46+
// Parsed named expressions and the types that we've found for them so far.
47+
// Note that we keep a side-array of the ordering of the named arguments
48+
// found to be sure that we can translate them in the same order that they
49+
// were declared in.
4750
names: HashMap<~str, @ast::Expr>,
4851
name_types: HashMap<~str, ArgumentType>,
52+
name_ordering: ~[~str],
4953

5054
// Collection of the compiled `rt::Piece` structures
5155
pieces: ~[@ast::Expr],
@@ -63,12 +67,15 @@ struct Context<'a> {
6367
///
6468
/// If parsing succeeds, the second return value is:
6569
///
66-
/// Some((fmtstr, unnamed arguments, named arguments))
67-
fn parse_args(ecx: &mut ExtCtxt, sp: Span,
68-
tts: &[ast::TokenTree]) -> (@ast::Expr, Option<(@ast::Expr, ~[@ast::Expr],
69-
HashMap<~str, @ast::Expr>)>) {
70+
/// Some((fmtstr, unnamed arguments, ordering of named arguments,
71+
/// named arguments))
72+
fn parse_args(ecx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
73+
-> (@ast::Expr, Option<(@ast::Expr, ~[@ast::Expr], ~[~str],
74+
HashMap<~str, @ast::Expr>)>)
75+
{
7076
let mut args = ~[];
7177
let mut names = HashMap::<~str, @ast::Expr>::new();
78+
let mut order = ~[];
7279

7380
let mut p = rsparse::new_parser_from_tts(ecx.parse_sess(),
7481
ecx.cfg(),
@@ -125,12 +132,13 @@ fn parse_args(ecx: &mut ExtCtxt, sp: Span,
125132
continue
126133
}
127134
}
135+
order.push(name.to_str());
128136
names.insert(name.to_str(), e);
129137
} else {
130138
args.push(p.parse_expr());
131139
}
132140
}
133-
return (extra, Some((fmtstr, args, names)));
141+
return (extra, Some((fmtstr, args, order, names)));
134142
}
135143

136144
impl<'a> Context<'a> {
@@ -661,10 +669,11 @@ impl<'a> Context<'a> {
661669
locals.push(self.format_arg(e.span, Exact(i),
662670
self.ecx.expr_ident(e.span, name)));
663671
}
664-
for (name, &e) in self.names.iter() {
665-
if !self.name_types.contains_key(name) {
666-
continue
667-
}
672+
for name in self.name_ordering.iter() {
673+
let e = match self.names.find(name) {
674+
Some(&e) if self.name_types.contains_key(name) => e,
675+
Some(..) | None => continue
676+
};
668677

669678
let lname = self.ecx.ident_of(format!("__arg{}", *name));
670679
pats.push(self.ecx.pat_ident(e.span, lname));
@@ -810,8 +819,9 @@ pub fn expand_args(ecx: &mut ExtCtxt, sp: Span,
810819
tts: &[ast::TokenTree]) -> base::MacResult {
811820

812821
match parse_args(ecx, sp, tts) {
813-
(extra, Some((efmt, args, names))) => {
814-
MRExpr(expand_preparsed_format_args(ecx, sp, extra, efmt, args, names))
822+
(extra, Some((efmt, args, order, names))) => {
823+
MRExpr(expand_preparsed_format_args(ecx, sp, extra, efmt, args,
824+
order, names))
815825
}
816826
(_, None) => MRExpr(ecx.expr_uint(sp, 2))
817827
}
@@ -823,6 +833,7 @@ pub fn expand_args(ecx: &mut ExtCtxt, sp: Span,
823833
pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, sp: Span,
824834
extra: @ast::Expr,
825835
efmt: @ast::Expr, args: ~[@ast::Expr],
836+
name_ordering: ~[~str],
826837
names: HashMap<~str, @ast::Expr>) -> @ast::Expr {
827838
let arg_types = vec::from_fn(args.len(), |_| None);
828839
let mut cx = Context {
@@ -832,6 +843,7 @@ pub fn expand_preparsed_format_args(ecx: &mut ExtCtxt, sp: Span,
832843
names: names,
833844
name_positions: HashMap::new(),
834845
name_types: HashMap::new(),
846+
name_ordering: name_ordering,
835847
nest_level: 0,
836848
next_arg: 0,
837849
pieces: ~[],

src/test/run-pass/ifmt.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ pub fn main() {
139139

140140
test_write();
141141
test_print();
142+
test_order();
142143

143144
// make sure that format! doesn't move out of local variables
144145
let a = ~3;
@@ -202,3 +203,18 @@ fn test_format_args() {
202203
let s = format_args!(fmt::format, "hello {}", "world");
203204
t!(s, "hello world");
204205
}
206+
207+
fn test_order() {
208+
// Make sure format!() arguments are always evaluated in a left-to-right
209+
// ordering
210+
fn foo() -> int {
211+
static mut FOO: int = 0;
212+
unsafe {
213+
FOO += 1;
214+
FOO
215+
}
216+
}
217+
assert_eq!(format!("{} {} {a} {b} {} {c}",
218+
foo(), foo(), foo(), a=foo(), b=foo(), c=foo()),
219+
~"1 2 4 5 3 6");
220+
}

0 commit comments

Comments
 (0)