Destabilise the internals of `format_args!`

[huonw]

#22899 should mean that contents of core::fmt::rt::v1 used by format_args! can probably be made #[unstable].

cc @eddyb

[eddyb]

😍

[rprichard]

@alexcrichton Does this need to be finished before 1.0?

Some other observations:

• Formatter::align returns a value of rt::v1::Alignment type. The method is still unstable with the reason "method was just created". Does it actually make sense to stabilize a method whose return type is unstable? (The method is documented, but the return type is not, so there's a 404 error in the API docs.)
• Formatter::flags is stable and returns u32, but the flag values themselves aren't documented, AFAICT, so there's no stable way to use the flags. The core::fmt::FlagV1 enum is private.
• I'm experimenting with some changes to the core::fmt representation that I think will improve performance, but it completely changes the representation of Arguments and Formatter and removes the undocumented but stable methods from Arguments. In addition to destabilizing the contents of core::fmt::rt::v1, can we also make Arguments::new_v1[_formatted] and ArgumentV1 et al unstable?
• Stabilizing the v1 representation seems to have performance consequences. Supposing we introduced a new rt::v2::Argument, AFAICT, Arguments would need to use an enum to represent both rt::v1::Argument and rt::v2::Argument. Perhaps we'd then like to make Arguments::new_v1_formatted convert the v1 representation to the v2 representation, but I don't think it could due to lifetime issues. Do we really need to support old and new representations in the same version of libcore?

[alexcrichton]

@rprichard yeah it would be nice to get this done, but it's not necessarily the end of the world if it slips through the cracks for now. The align method will not be stabilized as-is (returning that type), and flags may just be stable by mistake as you can't actually interpret the return value.

Due to the #[allow_internal_unstable] attribute/feature I believe we can destabilize as much of this as possible.

[rprichard]

@alexcrichton Thanks for looking at this.

It looks like #[allow_internal_unstable] is already specified on the format_args! macro, but when I tried marking the v1 things unstable, I still got stability errors. I think I can fix those by using a different Span on the v1 paths that format_args! generates. (Specifically, use the Span passed to expand_format_args rather than that of the format expression or arguments. An argument can refer to something unstable, and that must still fail.)

[alexcrichton]

Ah yeah I'm not too familiar with how the #[allow_internal_unstable] machinery works, but @huonw may be able to help out with problems.

[huonw]

@rprichard ah, yeah, I guess you do need the span of the macro (i.e the one passed into the function) rather than the expression/arguments.

[rprichard]

@huonw Currently, I've modified format_args! to use a combination of the macro span, the format literal span, and the argument spans. It more-or-less works, but I still get stability errors if format_args! is expanded from another macro (e.g. write!). (Even if I mark the containing macro as #[allow_internal_unstable], I still get the stability errors.)

I then tried writing a simple syntax plugin (easier to experiment with, faster iteration times), and I've noticed that with a simple macro that evaluates to its last argument, I don't get stability errors, even if that argument is unstable! Here's the code for the plugin I'm working with.

[huonw]

Ah! I think plugins are handled differently to the built-in ones in a fairly subtle (and apparently buggy) way. Specifically the registry overrides the definition span, which is used to check for #[allow_internal_unstable], and, it seems that self.krate_span used in the registry covers the whole of the main file (it should just be the #[plugin(...)] attribute) and hence every expression is included inside that span.

If I make the following change the stability error occurs as expected:

--- a/foo_macros.rs
+++ b/foo_macros.rs
@@ -99,6 +99,6 @@ fn expand_foo(cx: &mut ExtCtxt, sp: Span, args: &[ast::TokenTree])
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
     let expander: MacroExpanderFn = expand_foo;
-    reg.register_syntax_extension(token::intern("foo"),
-                                  NormalTT(Box::new(expander), None, true));
+    reg.syntax_exts.push((token::intern("foo"),
+                          NormalTT(Box::new(expander), None, true)));
 }

(Abusing the fact that the registry has public fields to stop it changing the None to a Some.)

[huonw]

(FWIW, your plugin + my bug-workaround seems to behave correctly wrt being inside a macro_rules marked with #[allow_internal_unstable] vs. not.)

[huonw]

(Filed #23964.)

[rprichard]

@huonw With your change to plugin_registrar, I'm now able to reproduce the problem I'm having with format_args! where #[allow_internal_unstable] works, but not when the macro is used inside another macro.

I made a reduced plugin demonstrating the problem.

[huonw]

This appears to be because the span passed into the syntax extension appears to be the span of the outer macro invocation, rather than the span of the internal plugin invocation, which is what the stability checking assumes. Hm.

[rprichard]

Yes, when we expand an expression macro, we use cx.original_span(), which returns the outer-most call site, rather than the immediate call site. This behavior originated from the #5794 fix.

It looks like using cx.call_site() instead of the macro span fixes my test case. It's interesting that cx.call_site() has a backtrace, though, whereas the macro span doesn't. (Does it ever have a backtrace?) IIUC, backtraces are assigned after macro expansion via noop_fold_expr / new_span.

I also thought it was interesting that when we expand an expression macro, we replace the fully expanded Expr's span with that of the original invocation. IIUC, this loses the backtrace, but if we stopped doing the replacement, some backtraces would be wrong. (i.e. The top of the stack could be the outer-most call site, followed by the inner-most expansion, then the second-most-inner expansion, all the way to the outer-most call site again.)

OTOH, maybe this change makes sense:

diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs
index ee2cf90..fdb5716 100644
--- a/src/libsyntax/ext/expand.rs
+++ b/src/libsyntax/ext/expand.rs
@@ -80,7 +80,7 @@ pub fn expand_expr(e: P<ast::Expr>, fld: &mut MacroExpander) -> P<ast::Expr> {
             fully_expanded.map(|e| ast::Expr {
                 id: ast::DUMMY_NODE_ID,
                 node: e.node,
-                span: span,
+                span: fld.new_span(span),
             })
         }

[rprichard]

I also thought it was interesting that when we expand an expression macro, we replace the fully expanded Expr's span with that of the original invocation. IIUC, this loses the backtrace...

To elaborate more, even if the expanded macro is #[allow_internal_unstable], span_allows_unstable will still reject the fully expanded Expr's span, because that span's backtrace is empty. I think...