Pre-expansion gating collects spans from unSuccessful macro matchers

[Mark-Simulacrum]

Setting high priority as this affects performance collection.

Sample error, likely possible to see more at https://perf.rust-lang.org/status.html

error[E0658]: type ascription is experimental
   --> components/style/stylesheets/viewport_rule.rs:290:43
    |
290 |                 declaration!($declaration(value: try!($parse(input)),
    |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^
...
325 |             "orientation" => ok!(Orientation(Orientation::parse)),
    |                              ------------------------------------ in this macro invocation
    |
    = note: for more information, see https://github.com/rust-lang/rust/issues/23416
    = help: add `#![feature(type_ascription)]` to the crate attributes to enable

[Mark-Simulacrum]

I suspect this was caused by #65742 (definitely something in #65793) -- presumably, that PR does something wrong, as I suspect that the expression here is not type ascription -- try!($parse(input)) is not a type -- so we're probably being overeager. If we don't have immediate ideas as to why then I think we should revert the PR.

cc @Centril @davidtwco

[Centril]

Reduced to:

macro_rules! mac {
    ($e:expr) => {}; // Removing this line makes it compile again.
    (label: $v:expr, MARKER) => {};
}

mac!(label: 0, MARKER);

which results in:

error: expected type, found `0`
 --> src/lib.rs:6:13
  |
6 | mac!(label: 0, MARKER);
  |           - ^ expected type
  |           |
  |           tried to parse a type due to this

on stable, and:

error: expected type, found `0`
 --> src/lib.rs:6:13
  |
6 | mac!(label: 0, MARKER);
  |           - ^ expected type
  |           |
  |           tried to parse a type due to this type ascription
  |
  = note: `#![feature(type_ascription)]` lets you annotate an expression with a type: `<expr>: <type>`
  = note: for more information, see https://github.com/rust-lang/rust/issues/23416

on nightly.

[Centril]

I'm not sure how this code managed to work before because it is parsing a type ascription (successfully when using mac!(label: try!(), MARKER);). Just to be sure, I verified this with:

         let parser_snapshot_before_type = self.clone();
         match self.parse_ty_no_plus() {
             Ok(rhs) => {
-                Ok(mk_expr(self, rhs))
+                let expr = mk_expr(self, rhs);
+                if let ExprKind::Type(..) = expr.kind  {
+                    panic!("PARSED TYPE ASCRIPTION!");
+                }
+                Ok(expr)
             }

where the input was:

macro_rules! mac {
    ($e:expr) => {};
    (label: $v:expr, MARKER) => {};
}

mac!(label: try!(), MARKER)

Somehow this type ascription disappears later before PostExpansionVisitor?

---

All in all, I believe that at least from the POV of the parser, the behavior is expected and therefore there is no bug. This should be fixed on perf's end, at least in the interim, by switching to a token besides :.

[Mark-Simulacrum]

This is still a breaking change - and I would expect it to be fixed in the parser, especially as in this case the macro input doesn't even look like type ascription, it's token colon expression, whereas type ascription would be token colon type, right?
Plus, the token colon expression macro form seems pretty common (used all over the place, I imagine, in many macros). I don't think we can just wholesale break things like this.

I'll look into a fix for perf, just to get us unbroken, but I feel pretty strongly so far that this is a breaking change and a bug.

[Centril]

especially as in this case the macro input doesn't even look like type ascription, it's token colon expression, whereas type ascription would be token colon type, right?

Well it is parsing a type ascription because it is constructing an ExprKind::Type. Otherwise #65742 would have no effect. For example, consider (playground):

macro_rules! m {
    ($e:expr) => { dbg!(0); };
    (lab as $e:expr, foo) => { dbg!(1); };
}

fn main() {
    m!(lab as 1, foo);
}

Because of the first arm, this will error with "error: expected type, found 1" despite , foo not matching the first arm.

Another case to consider is (playground):

macro_rules! m {
    ($e:expr) => { dbg!(0); };
    (box $e:expr, foo) => { dbg!(1); };
}

fn main() {
    m!(box 1, foo);
}

This also used to compile and now emits a feature gate error? Why? Because macro expansion will try each arm in a macro_rules! item one at a time until it reaches the first Success(...) and then do transcription.

rust/src/libsyntax_expand/mbe/macro_rules.rs

Lines 186 to 248 in 46e6c53

	for (i, lhs) in lhses.iter().enumerate() {
	// try each arm's matchers
	let lhs_tt = match *lhs {
	mbe::TokenTree::Delimited(_, ref delim) => &delim.tts[..],
	_ => cx.span_bug(sp, "malformed macro lhs"),
	};
	match parse_tt(cx, lhs_tt, arg.clone()) {
	Success(named_matches) => {
	let rhs = match rhses[i] {
	// ignore delimiters
	mbe::TokenTree::Delimited(_, ref delimed) => delimed.tts.clone(),
	_ => cx.span_bug(sp, "malformed macro rhs"),
	};
	let arm_span = rhses[i].span();
	let rhs_spans = rhs.iter().map(|t| t.span()).collect::<Vec<_>>();
	// rhs has holes ( `$id` and `$(...)` that need filled)
	let mut tts = transcribe(cx, &named_matches, rhs, transparency);
	// Replace all the tokens for the corresponding positions in the macro, to maintain
	// proper positions in error reporting, while maintaining the macro_backtrace.
	if rhs_spans.len() == tts.len() {
	tts = tts.map_enumerated(|i, mut tt| {
	let mut sp = rhs_spans[i];
	sp = sp.with_ctxt(tt.span().ctxt());
	tt.set_span(sp);
	tt
	});
	}
	if cx.trace_macros() {
	let msg = format!("to `{}`", pprust::tts_to_string(tts.clone()));
	trace_macros_note(cx, sp, msg);
	}
	let directory = Directory {
	path: Cow::from(cx.current_expansion.module.directory.as_path()),
	ownership: cx.current_expansion.directory_ownership,
	};
	let mut p = Parser::new(cx.parse_sess(), tts, Some(directory), true, false, None);
	p.root_module_name =
	cx.current_expansion.module.mod_path.last().map(|id| id.as_str().to_string());
	p.last_type_ascription = cx.current_expansion.prior_type_ascription;
	p.process_potential_macro_variable();
	// Let the context choose how to interpret the result.
	// Weird, but useful for X-macros.
	return Box::new(ParserAnyMacro {
	parser: p,
	// Pass along the original expansion site and the name of the macro
	// so we can print a useful error message if the parse of the expanded
	// macro leaves unparsed tokens.
	site_span: sp,
	macro_ident: name,
	arm_span,
	});
	}
	Failure(token, msg) => match best_failure {
	Some((ref best_token, _)) if best_token.span.lo() >= token.span.lo() => {}
	_ => best_failure = Some((token, msg)),
	},

In this case, once we have parsed ExprKind::Type, a span has been added to GatedSpans by the parser. Once we reach feature gate checking, all of those spans will turn into emitted feature gating errors. This is well illustrated by the following example (playground):

macro_rules! m {
    ($e:expr) => { dbg!(0); };
    (box $e:expr, foo2) => { dbg!(1); };
}

fn main() {
    m!(box 1, foo);
}

The only way I know of to not do that which might possibly be correct and still perform pre-expansion gating would be to snapshot GatedSpans and only commit to additions it once we have reached Success(...) =>. This would need to be maintained as a stack to handle macros calling macros (should be taken care of by recursion in the compiler itself, hopefully).

Plus, the token colon expression macro form seems pretty common (used all over the place, I imagine, in many macros). I don't think we can just wholesale break things like this.

#64672 (comment) (crates.io had 5 regressions where 4 of those were from the same project with the same root and then there was another root; github had 2 regressions)

I'll look into a fix for perf, just to get us unbroken, but I feel pretty strongly so far that this is a breaking change and a bug.

If there is a bug, having done the investigation, I can say with certainty that it is not in #65742 but rather a pre-existing one in fn generic_extension.

Also, we should absolutely continue performing pre-expansion gating. To not do so would mean leaking all new syntax into stable immediately! In the long run, that will make both users and the language team unhappy. Moreover, making things which were accidentally stable unstable is legitimized by rust-lang/rfcs#2405, which you signed off on yourself.

[Mark-Simulacrum]

I would expect pre-expansion gating in this form (i.e., macro input parsing) to either emit a future-incompatibility warning (like we do for methods that might conflict with a future stabilization, I believe) or simply pretend that the gated form does not exist unless the feature gate is enabled.

I get that this is likely hard -- and might not even be practical -- but as it is currently, even though I personally do not expect us to stabilize box expressions in their current syntax/form for what is likely on the order of years, if ever, it seems really odd to forbid the <ident> <expr> form specifically for box but nothing else. In some sense, this is making it appear that the syntax has been chosen "consciously" rather than just being a historical artifact, for the most part.

To not do so would mean leaking all new syntax into stable immediately!

This is basically my point -- as it is implemented today, all Rust users pay the pain of new syntax in macros immediately upon its addition. That gives us no time for experimentation or to decide if that's a good thing; furthermore, Rust users who use stable may experience breakage from unstable features. Maybe that's fine -- and we just need to be really careful when adding/modifying unstable syntax parsing -- but to me it seems like that's probably not quite what we want. I, at least, would expect that an unstable feature, even when it comes to syntax, is invisible, possibly modulo diagnostics, to stable users of the compiler (or those not using the specific feature gate).

[Mark-Simulacrum]

We've fixed perf.rlo so removing high priority label.

[Mark-Simulacrum]

Closing in favor of #65860.

[Centril]

That's a distinct problem. That one is about cfg-expansion and this is about macro matchers. They will have different solutions. I'll try to look at fixing this one today.

[Centril]

I have a fix in #65974.

[pnkfelix]

triage: P-high