Improves handling of statement macros. #24155

chris-chambers · 2015-04-07T14:23:10Z

Statement macros are now treated somewhat like item macros, in that a statement macro can now expand into a series of statements, rather than just a single statement.

This allows statement macros to be nested inside other kinds of macros and expand properly, where previously the expansion would only work when no nesting was present.

See:

src/test/run-pass/macro-stmt_macro_in_expr_macro.rs
src/test/run-pass/macro-nested_stmt_macro.rs

This changes the interface of the MacResult trait. make_stmt has become make_stmts and now returns a vector, rather than a single item. Plugin writers who were implementing MacResult will have breakage, as well as anyone using MacEager::stmt.

See:

src/libsyntax/ext/base.rs

This also causes a minor difference in behavior to the diagnostics produced by certain malformed macros.

See:

src/test/compile-fail/macro-incomplete-parse.rs

rust-highfive · 2015-04-07T14:23:22Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kmcallister (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

Statement macros are now treated somewhat like item macros, in that a statement macro can now expand into a series of statements, rather than just a single statement. This allows statement macros to be nested inside other kinds of macros and expand properly, where previously the expansion would only work when no nesting was present. See: src/test/run-pass/macro-stmt_macro_in_expr_macro.rs src/test/run-pass/macro-nested_stmt_macro.rs This changes the interface of the MacResult trait. make_stmt has become make_stmts and now returns a vector, rather than a single item. Plugin writers who were implementing MacResult will have breakage, as well as anyone using MacEager::stmt. See: src/libsyntax/ext/base.rs This also causes a minor difference in behavior to the diagnostics produced by certain malformed macros. See: src/test/compile-fail/macro-incomplete-parse.rs

chris-chambers · 2015-04-07T15:46:36Z

I just found issue #10681 that should be fixed by this PR.

chris-chambers · 2015-04-10T00:31:38Z

I hope I am not being presumptuous here, but it seems like @sfackler was interested in this. So I am going to:

r? @sfackler

Hopefully that does what I want! This is my first (possible) contribution to rust.

This PR has been hanging around unreviewed a little while now. And then the assigned reviewer removed themselves. If revisions are needed, or if the change is not wanted, please let me know.

sfackler · 2015-04-10T16:14:12Z

Yep, sorry, I will take a look tonight.

sfackler · 2015-04-11T01:57:47Z

Does this close #10681?

chris-chambers · 2015-04-11T02:05:48Z

Yes, I believe it would. I haven't run tests against the exact code presented there, but the two test files added in this PR are very similar to that.

sfackler · 2015-04-11T02:08:18Z

src/libsyntax/ext/expand.rs

+    // semicolon to the final statement produced by expansion.
+    if style == MacStmtWithSemicolon && fully_expanded.len() > 0 {
+        let last_index = fully_expanded.len() - 1;
+        fully_expanded.into_iter().enumerate().map(|(i, stmt)|


You should be able to replace this slightly weird map/collect by popping the last one off, updating it and pushing it back on.

I was hoping someone would have advice about that. I wasn't 100% sure about the api of SmallVector, and the docs for unstable things were highly in flux at that moment. Does it implement pop directly?

SmallVector isn't even an unstable part of the stdlib - it's part of libsyntax itself. It looks like it doesn't implement pop right now: https://github.com/rust-lang/rust/blob/master/src/libsyntax/util/small_vector.rs but the impl should be pretty simple:

pub fn pop(&mut self) -> Option<T> { let old = mem::replace(&mut self.repr, Zero); let (ret, new_repr) = match old { Zero => (None, Zero), One(v) => (Some(v), Zero), Many(mut vec) => { let ret = vec.pop(); (ret, Many(vec)) } } self.repr = new_repr; ret }

sfackler · 2015-04-11T02:09:56Z

Looks good to me other than that one nit. Thanks for doing this!

Implements pop() on SmallVector, and uses it to expand the final semicolon in a statement macro expansion more efficiently. Corrects the placement of the call to fld.cx.bt_pop(). It must run unconditionally to reverse the corresponding push.

chris-chambers · 2015-04-11T02:58:54Z

I think this should cover everything. I'm still waiting on a full compile and test run, though.

sfackler · 2015-04-11T03:48:35Z

src/libsyntax/ext/expand.rs

-                    _ => node /* might already have a semi */
-                },
-                span: span
+        match fully_expanded.pop() {


I think this can be an if let Some(stmt) = fully_expanded.pop() { .. } instead of a match.

Oh yes, that would be cleaner.

sfackler · 2015-04-11T04:32:41Z

src/libsyntax/util/small_vector.rs

+                    _ => unreachable!()
+                }
+            }
+            Many(..) => {


You can simplify this a bit more since we don't actually need to swap anyore:

Many(ref mut vs) => vs.pop()

Edit: though that may treat the repr as borrowed in the other branches...

Ah, right you are. Updated.

SmallVector::pop no longer worries about converting a Many repr downward to One or Zero. expand_stmt makes use of `if let` for style purposes.

sfackler · 2015-04-11T04:46:04Z

@bors: r+ 22eb319

Woo, thanks for doing this!

chris-chambers · 2015-04-11T04:46:36Z

My pleasure! Glad to get it in there.

bors · 2015-04-11T05:04:36Z

⌛ Testing commit 22eb319 with merge 5692553...

chris-chambers · 2015-04-11T05:46:49Z

My compile and tests finally got far enough along to reveal that the bt_pop is probably in the wrong position now. Maybe my first instinct was correct. I'm looking into it.

chris-chambers · 2015-04-11T05:56:20Z

Ok, yes, from what I see here, expand_mac_invoc is the source of the matching bt_push, and it only happens in the case where it will eventually return Some(..). In all cases where it returns None, there is no bt_push, so in the None branch in expand_stmt, there should be no bt_pop.

bors · 2015-04-11T05:57:55Z

💔 Test failed - auto-mac-32-opt

sfackler · 2015-04-11T06:02:22Z

@bors r+ 77627ea

Statement macros are now treated somewhat like item macros, in that a statement macro can now expand into a series of statements, rather than just a single statement. This allows statement macros to be nested inside other kinds of macros and expand properly, where previously the expansion would only work when no nesting was present. See: - `src/test/run-pass/macro-stmt_macro_in_expr_macro.rs` - `src/test/run-pass/macro-nested_stmt_macro.rs` This changes the interface of the MacResult trait. make_stmt has become make_stmts and now returns a vector, rather than a single item. Plugin writers who were implementing MacResult will have breakage, as well as anyone using MacEager::stmt. See: - `src/libsyntax/ext/base.rs` This also causes a minor difference in behavior to the diagnostics produced by certain malformed macros. See: - `src/test/compile-fail/macro-incomplete-parse.rs`

bors · 2015-04-11T08:07:35Z

⌛ Testing commit 77627ea with merge 0be4e0e...

bors · 2015-04-11T11:06:13Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

durka · 2015-07-30T21:04:26Z

Even after this PR there seem to be some issues with hygiene in multi-statement macros.

Try uncommenting the second macro here: http://is.gd/sGjll7
It doesn't work, because hygiene seems to run separately on the two statements.

Is this expected behavior?

chris-chambers · 2015-07-31T02:08:29Z

I agree that this is not desirable behavior. Given the behavior of this code (a single-statement macro) http://is.gd/gaw6zc, one would expect the second definition of m! to work. I will look into this when I have a chance, but I am in the middle of a move.

Opening a new issue may get it resolved before I get the chance.

chris-chambers · 2015-07-31T03:04:44Z

Ok, I got a chance to look at this briefly. Three ideas (none of which is glorious):

Add a mechanism to specify what hygiene space will be used in a particular expansion (this could cause fallout at a large number of call sites). And apply the same hygiene space to items in a multi-statement macro.
Add a dummy AST node that represents a bundle of items, similar to a lexical scope, except that all names in the root of the bundle would escape into the parent scope. Then wrap/unwrap as statement macros are expanded.
Post-process after expanding each multi-statement macro to put all the expanded symbols into the same hygiene space. (It may be impossible to reliably determine what constitutes a root symbol after full expansion has taken place.)

I am hoping that there are better ideas out there.

rust-highfive assigned kmcallister Apr 7, 2015

chris-chambers force-pushed the stmt_macros branch 2 times, most recently from ab72f7a to 1934386 Compare April 7, 2015 14:29

kmcallister removed their assignment Apr 9, 2015

rust-highfive assigned sfackler Apr 10, 2015

sfackler reviewed Apr 11, 2015
View reviewed changes

Eliminates a pointless is_empty test.

fae29e4

sfackler reviewed Apr 11, 2015
View reviewed changes

chris-chambers force-pushed the stmt_macros branch 3 times, most recently from cbcd310 to a41c98b Compare April 11, 2015 04:14

sfackler reviewed Apr 11, 2015
View reviewed changes

chris-chambers force-pushed the stmt_macros branch from a41c98b to 22eb319 Compare April 11, 2015 04:42

Simplifications to statement macro handling.

22eb319

SmallVector::pop no longer worries about converting a Many repr downward to One or Zero. expand_stmt makes use of `if let` for style purposes.

Moves expand_stmt's bt_pop so that it balances correctly.

77627ea

bors merged commit 77627ea into rust-lang:master Apr 11, 2015

chris-chambers deleted the stmt_macros branch April 11, 2015 14:18

durka mentioned this pull request Sep 9, 2015

Allow multi-statement macros #10681

Closed

durka mentioned this pull request Feb 24, 2016

Hygiene broken in multi-statement macros #31856

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves handling of statement macros. #24155

Improves handling of statement macros. #24155

chris-chambers commented Apr 7, 2015

rust-highfive commented Apr 7, 2015

chris-chambers commented Apr 7, 2015

chris-chambers commented Apr 10, 2015

sfackler commented Apr 10, 2015

sfackler commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

sfackler Apr 11, 2015

chris-chambers Apr 11, 2015

sfackler Apr 11, 2015

sfackler commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

sfackler Apr 11, 2015

chris-chambers Apr 11, 2015

sfackler Apr 11, 2015

chris-chambers Apr 11, 2015

sfackler commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

bors commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

bors commented Apr 11, 2015

sfackler commented Apr 11, 2015

bors commented Apr 11, 2015

bors commented Apr 11, 2015

durka commented Jul 30, 2015

chris-chambers commented Jul 31, 2015

chris-chambers commented Jul 31, 2015

Improves handling of statement macros. #24155

Improves handling of statement macros. #24155

Conversation

chris-chambers commented Apr 7, 2015

rust-highfive commented Apr 7, 2015

chris-chambers commented Apr 7, 2015

chris-chambers commented Apr 10, 2015

sfackler commented Apr 10, 2015

sfackler commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

sfackler Apr 11, 2015

Choose a reason for hiding this comment

chris-chambers Apr 11, 2015

Choose a reason for hiding this comment

sfackler Apr 11, 2015

Choose a reason for hiding this comment

sfackler commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

sfackler Apr 11, 2015

Choose a reason for hiding this comment

chris-chambers Apr 11, 2015

Choose a reason for hiding this comment

sfackler Apr 11, 2015

Choose a reason for hiding this comment

chris-chambers Apr 11, 2015

Choose a reason for hiding this comment

sfackler commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

bors commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

chris-chambers commented Apr 11, 2015

bors commented Apr 11, 2015

sfackler commented Apr 11, 2015

bors commented Apr 11, 2015

bors commented Apr 11, 2015

durka commented Jul 30, 2015

chris-chambers commented Jul 31, 2015

chris-chambers commented Jul 31, 2015