parser: recover on `for<'a> |...| body` closures #70209

Centril · 2020-03-21T07:36:32Z

When encountering for and < is 1 token ahead, interpret this as an explicitly quantified generic closure and recover, rather than attempting to parse a for loop. This provides both improved diagnostics as well as an insurance policy for the ability to use this as the syntax for generic closures in the future.

As requested by r? @eddyb

eddyb · 2020-03-21T07:38:19Z

src/librustc_parse/parser/expr.rs

+    /// Recover on an explicitly quantified closure expression, e.g., `for<'a> |x: &'a u8| *x + 1`.
+    fn recover_quantified_closure_expr(&mut self, attrs: AttrVec) -> PResult<'a, P<Expr>> {
+        let lo = self.token.span;
+        let _ = self.parse_late_bound_lifetime_defs()?;


I guess what I was worried about is this any early exist from here resulting in a for loop successfully parsing but I guess that's not possible.

eddyb · 2020-03-21T07:40:48Z

src/librustc_parse/parser/expr.rs

+        let span_for = lo.to(self.prev_token.span);
+        let closure = self.parse_closure_expr(attrs)?;
+
+        self.struct_span_err(span_for, "cannot introduce explicit parameters for a closure")


I wonder if the language here could be more focused on the present rather than sounding absolute. Like "not yet supported" but maybe not suggesting future support is certain?

We tend to shy away from "not yet" so as to not suggest that it will happen; "cannot" is a fairly standard start of a diagnostic message in the compiler.

eddyb · 2020-03-21T07:42:53Z

cc @petrochenkov @nikomatsakis I don't want to unilaterally approve this, but I've been wanting make sure we don't paint ourselves into a corner wrt generic closure syntax (i.e. by it being ambiguous with for <...>::... in ... {...} loops), and it looks like irrefutable patterns starting with < have become possible more recently.

petrochenkov · 2020-03-21T08:17:02Z

I'm mildly skeptical about this, the syntax is unlikely to be written accidentally, so there's not much motivation for doing this for recovery.

Regarding future proofing, we already have ambiguities "< as a start of generics" vs "< as a start of qpath", and they are treated with choose_generics_over_qpath, so if this PR is primarily about future-proofing, it should use that function as well.

Centril · 2020-03-21T09:01:42Z

Switched over to choose_generics_over_qpath as this is indeed primarily about future proofing for generic closures (though the recovery diagnostics don't hurt imo).

As for the for-loop thing. Keeping in mind that these must be irrefutable patterns using <...>::Variant as a syntax in for loops specifically, that they must be enums (univariant ones), and that they have only worked semantically since 1.37, it's exceedingly unlikely that anything relies on this.

r? @petrochenkov

petrochenkov · 2020-03-21T12:26:06Z

@bors r+

bors · 2020-03-21T12:26:07Z

📌 Commit 4d30b92 has been approved by petrochenkov

@eddyb

…trochenkov parser: recover on `for<'a> |...| body` closures When encountering `for` and `<` is 1 token ahead, interpret this as an explicitly quantified generic closure and recover, rather than attempting to parse a `for` loop. This provides both improved diagnostics as well as an insurance policy for the ability to use this as the syntax for generic closures in the future. As requested by r? @eddyb

@ghost

Rollup of 10 pull requests Successful merges: - rust-lang#68099 (Amend Rc/Arc::from_raw() docs regarding unsafety) - rust-lang#70172 (parse/lexer: support `StringReader::retokenize` called on external files.) - rust-lang#70209 (parser: recover on `for<'a> |...| body` closures) - rust-lang#70223 (fix type of const params in associated types.) - rust-lang#70229 (more clippy fixes) - rust-lang#70240 (Return NonZeroU64 from ThreadId::as_u64.) - rust-lang#70250 (Remove wrong entry from RELEASES.md) - rust-lang#70253 (Remove another wrong entry from RELEASES.md) - rust-lang#70254 (couple more clippy fixes (let_and_return, if_same_then_else)) - rust-lang#70266 (proc_macro_harness: Use item header spans for errors) Failed merges: r? @ghost

qnighy · 2020-03-23T08:47:38Z

Is it ok that this artificial example no longer compiles in nightly? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c444d791d838b73c5ad3e9813e9838c6

#[derive(PartialEq, Eq)]
struct A;

impl A {
    const A: A = A;
}

fn main() {
    for <A>::A in vec![A] {}
}

nikomatsakis · 2020-03-23T19:32:51Z

So, let me understand what happened here:

We've modified the parser so that for< is now considered the start of a closure and not a for loop -- this is a change to the previous behavior for sure, right? Not saying I disagree with it, but it seems like it'd have been good to at least ping @rust-lang/lang and provide some motivation
Based on what I see elsewhere in the PR, my assumption is that the motivation is to eventually permit for<'a> |&'a u32| -> &'a u32 in closure type signature annotations?
Did we do any kind of crater run or check to see if this would be widely used? It seems like at minimum this probably merits a relnotes tag?
In any case, I'm nominating for lang team discussion.

fmease · 2022-01-14T16:06:57Z

for future reference, linking rust-lang/rfcs#3216

rust-highfive assigned eddyb Mar 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2020

eddyb reviewed Mar 21, 2020

View reviewed changes

Centril force-pushed the recover-quant-closure branch from 622dbc5 to 13eb4df Compare March 21, 2020 07:52

recover on for<'a> |...| body closures.

4d30b92

Centril force-pushed the recover-quant-closure branch from 13eb4df to 4d30b92 Compare March 21, 2020 08:54

rust-highfive assigned petrochenkov and unassigned eddyb Mar 21, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2020

Centril mentioned this pull request Mar 22, 2020

Rollup of 10 pull requests #70257

Closed

Dylan-DPC-zz mentioned this pull request Mar 22, 2020

Rollup of 10 pull requests #70275

Merged

bors merged commit ea44d71 into rust-lang:master Mar 22, 2020

Centril deleted the recover-quant-closure branch March 22, 2020 19:25

nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser: recover on `for<'a> |...| body` closures #70209

parser: recover on `for<'a> |...| body` closures #70209

Centril commented Mar 21, 2020

eddyb Mar 21, 2020

eddyb Mar 21, 2020

Centril Mar 21, 2020

eddyb commented Mar 21, 2020

petrochenkov commented Mar 21, 2020

Centril commented Mar 21, 2020

petrochenkov commented Mar 21, 2020

bors commented Mar 21, 2020

qnighy commented Mar 23, 2020 •

edited

Loading

nikomatsakis commented Mar 23, 2020

fmease commented Jan 14, 2022

parser: recover on for<'a> |...| body closures #70209

parser: recover on for<'a> |...| body closures #70209

Conversation

Centril commented Mar 21, 2020

eddyb Mar 21, 2020

Choose a reason for hiding this comment

eddyb Mar 21, 2020

Choose a reason for hiding this comment

Centril Mar 21, 2020

Choose a reason for hiding this comment

eddyb commented Mar 21, 2020

petrochenkov commented Mar 21, 2020

Centril commented Mar 21, 2020

petrochenkov commented Mar 21, 2020

bors commented Mar 21, 2020

qnighy commented Mar 23, 2020 • edited Loading

nikomatsakis commented Mar 23, 2020

fmease commented Jan 14, 2022

parser: recover on `for<'a> |...| body` closures #70209

parser: recover on `for<'a> |...| body` closures #70209

qnighy commented Mar 23, 2020 •

edited

Loading