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...

[rprichard]

Using cx.call_site() fixes the stability errors in normal compilation, but then the [pretty] tests fail (because the expanded source refers to unstable things, and there are no #[allow_internal_unstable] attributes left).

[huonw]

Does it ever have a backtrace?

I guess it might if the macro is defined/imported inside the expansion of another macro. (e.g. macro_rules! foo ( () => { #[macro_use] extern crate bar; } ).)

(I'm not sure I'm interpreting the question right.)

OTOH, maybe this change makes sense:

Hm, I'm struggling to visualise what effect that will have, do you happen to have a before/after diagnostics listing? (Also, including cx.call_site() one.)

Using cx.call_site() fixes the stability errors in normal compilation, but then the [pretty] tests fail (because the expanded source refers to unstable things, and there are no #[allow_internal_unstable] attributes left).

I think those tests will just have to be marked as expected-fails (with // xfail-pretty) as the macro-expansion-based stability is fundamentally "broken" post-expansion. It is mainly designed as a hack to allow 1.0 to be usable without having to stabilise the very insides of certain macros.

[rprichard]

Does it ever have a backtrace?

I guess it might if the macro is defined/imported inside the expansion of another macro. (e.g. macro_rules! foo ( () => { #[macro_use] extern crate bar; } ).)

(I'm not sure I'm interpreting the question right.)

I think my question was confused -- by "macro span," I was thinking of the span passed to a syntax extension. That span is sometimes cx.original_span(), which never has a backtrace. expand_item_mac OTOH passes it.span to the syntax extension, which I suspect doesn't typically have a backtrace.

I'm wondering whether, say, the expression passed to expand_expr ever has a span with a backtrace. If I understand correctly, the span backtraces are assigned in post-order, via MacroFolder::new_span. If format_args! emit expressions using cx.call_site(), it would break the supposed invariant, because cx.call_site() has a backtrace, and the expanded formatting expression is passed to expand_expr! again (e.g. because the arguments could invoke macros). If it mattered, expand_format_args could strip off the backtrace from cx.call_site().

My current guess is that a macro definition span has no backtrace, but I can't find a test case where it would matter.

OTOH, maybe this change makes sense:

Hm, I'm struggling to visualise what effect that will have, do you happen to have a before/after diagnostics listing?

Adding the fld.new_span call in expand_expr affects the backtrace for code like this (copied from my new compile-fail/macro-backtrace-nested.rs test):

macro_rules! nested_expr {
    () => (fake)
}

macro_rules! call_nested_expr_sum { //~ NOTE in expansion of
    () => { 1 + nested_expr!(); } //~ ERROR unresolved name
}

fn main() {
    call_nested_expr_sum!(); //~ NOTE expansion site
}

Without the fld.new_span call, the error is:

rprichard@ryan:~/mess$ rustc test.rs
test.rs:6:17: 6:32 error: unresolved name `fake`
test.rs:6     () => { 1 + nested_expr!(); } //~ ERROR unresolved name
                          ^~~~~~~~~~~~~~~
error: aborting due to previous error

With the fld.new_span call, there is a macro backtrace:

rprichard@ryan:~/mess$ ~/work/rust-staging2/build/install/bin/rustc test.rs
test.rs:6:17: 6:32 error: unresolved name `fake`
test.rs:6     () => { 1 + nested_expr!(); } //~ ERROR unresolved name
                          ^~~~~~~~~~~~~~~
test.rs:5:1: 7:2 note: in expansion of call_nested_expr_sum!
test.rs:10:5: 10:29 note: expansion site
error: aborting due to previous error

(Also, including cx.call_site() one.)

I'm actually not sure what the total impact is of using cx.call_site() instead of the format string. With my current implementation, it affects this test case:

fn main() {
    let x = format_args!("abc");
}

from:

rprichard@ryan:~/mess$ rustc test.rs
test.rs:2:26: 2:31 error: borrowed value does not live long enough
test.rs:2     let x = format_args!("abc");
                                   ^~~~~
note: in expansion of format_args!
test.rs:2:13: 2:33 note: expansion site
test.rs:2:32: 3:2 note: reference must be valid for the block suffix following statement 0 at 2:31...
test.rs:2     let x = format_args!("abc");
test.rs:3 }
test.rs:2:5: 2:32 note: ...but borrowed value is only valid for the statement at 2:4
test.rs:2     let x = format_args!("abc");
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~
test.rs:2:5: 2:32 help: consider using a `let` binding to increase its lifetime
test.rs:2     let x = format_args!("abc");
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

to:

rprichard@ryan:~/mess$ ~/work/rust-staging1/build/install/bin/rustc test.rs
test.rs:2:13: 2:33 error: borrowed value does not live long enough
test.rs:2     let x = format_args!("abc");
                      ^~~~~~~~~~~~~~~~~~~~
note: in expansion of format_args!
test.rs:2:13: 2:33 note: expansion site
test.rs:2:32: 3:2 note: reference must be valid for the block suffix following statement 0 at 2:31...
test.rs:2     let x = format_args!("abc");
test.rs:3 }
test.rs:2:5: 2:32 note: ...but borrowed value is only valid for the statement at 2:4
test.rs:2     let x = format_args!("abc");
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~
test.rs:2:5: 2:32 help: consider using a `let` binding to increase its lifetime
test.rs:2     let x = format_args!("abc");
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

The difference is the first span that the borrow checker highlights.

I was a little concerned that the format_args! call site is less useful as an error span than the format expression, but I haven't thought of any problem cases yet. In the case above, the change is an improvement.

I think those tests will just have to be marked as expected-fails (with // xfail-pretty) as the macro-expansion-based stability is fundamentally "broken" post-expansion.

It affects a lot of tests -- in my testing, 550 of 2032 pretty tests failed. I'm currently destabilizing all the v1 things, so assert_eq! is enough to break a test.

[rprichard]

This issue is blocked on #23616.

[huonw]

My current guess is that a macro definition span has no backtrace, but I can't find a test case where it would matter.

I can't think of somewhere it would matter either... if there are problems later we can fix them as they come.

Adding the fld.new_span call in expand_expr affects the backtrace for code like this (copied from my new compile-fail/macro-backtrace-nested.rs test):

Thanks, looks good.

It affects a lot of tests -- in my testing, 550 of 2032 pretty tests failed. I'm currently destabilizing all the v1 things, so assert_eq! is enough to break a test.

Argh. That's not so great. I'm inclined to agree with Niko: #23616 (comment). Unfortunately, I guess that still requires adding feature flags to 550 tests, and that is some druggery I wouldn't wish on anyone; although it seems to me that large parts might automatable (e.g. insert #![feature(...)] after line 8 (just after the license block) for the feature gates that are added).

(If we do decide to follow Niko's suggestion, it's totally cool if you're not up to doing this boring part.)

[rprichard]

@alexcrichton recently destabilized the internals of panic!, which also required dealing with a bunch of pretty-printing tests (commit). It looks like we already have to opt-in to allow a test to be pretty-printed, so that commit just removed the opt-in for a bunch of tests.

I speculate that that commit fixed most of the tests that were failing for this issue. i.e. The majority of tests that use format_args! probably do so because they panic via assert_eq!.

I'll work on this issue again soon, probably after rebasing my other macro-related PR.