Bounds parsing refactoring #37511

matklad · 2016-11-01T14:07:37Z

A follow up of #37278.

parse_opt_ty_param_bounds is introduced to make sure that we don't parse empty bonds. This fixes a couple of bugs along the way, where the check was missing. This is [breaking-change].

Require at least one predicate for a lifetime in a where clause

rust-highfive · 2016-11-01T14:07:50Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

matklad · 2016-11-01T14:11:44Z

src/libsyntax/parse/parser.rs

                                    bound_lifetimes: bound_lifetimes,
                                    bounded_ty: bounded_ty,
                                    bounds: bounds,
                        }));

                        parsed_something = true;
-                    } else if self.eat(&token::Eq) {


For some reason I can't use else if here: #37510 :(

matklad · 2016-11-01T14:14:47Z

src/libsyntax/parse/parser.rs

        }
+
+        let bounds = self.parse_ty_param_bounds(mode)?;


I'd rather remove plain parse_ty_param_bounds and leave only the opt version, but unfortunately this wont work for parse_impl_trait_type, because it used a keyword (impl) as an introducing_token, and you can't eat a keyword token. Or can you?

Or can you?

parser.eat(&token::Ident(keywords::Impl.ident())) should work.

bluss · 2016-11-01T15:34:52Z

This kind of change would normally first have warnings, then enable the breaking change later.

matklad · 2016-11-01T15:42:11Z

Yep, a warning is a good idea. But perhaps we need a crater run first, to estimate the amount of breakage just in case? Or is crater capable of reporting a diff in warnings?

bluss · 2016-11-01T15:54:32Z

A deny by default warning works well just for crater (how it was done in /pull/37378 ). That also helps to only catch the actual crates that regress.

matklad · 2016-11-02T12:07:25Z

And can I emit a deny by default warning directly from the parser? I believe I can't do this in ast_validation.rs in a straightforward way, because it's impossible to distinguish absence of bounds from the empty bounds, both case produce AST of the same shape. I think I can look a the spans and try to determine if bounds are empty, but : is present, but this feels a bit too hacky.

An alternative which I do know should work is to run crater with span.error and then change to span.warn. warn_missing_semicolon produces a similar warning now.

matklad · 2016-11-07T15:14:30Z

@pnkfelix ping :)

alexcrichton · 2016-11-10T21:35:23Z

@jseyfried you may also be able to take a look and help with review

jseyfried · 2016-11-11T06:07:07Z

src/libsyntax/parse/parser.rs

-                                           at least one bound in it");
-                        }
+                    if let Some(bounds) = self.parse_opt_ty_param_bounds(&token::Colon,
+                                                                         BoundParsingMode::Bare)? {


If you refactor this to

let opt_bounds = self.parse_opt_ty_param_bounds(&token::Colon, BoundParsingMode::Bare)?; if let Some(bounds) = opt_bounds {

then you can continue using else if below.

jseyfried · 2016-11-11T06:49:03Z

If I understand correctly, there are three separate [breaking-change]s here:

where 'a, ... is no longer allowed. I believe it equivalent to where ... today.
where 'a 'b, ... is no longer allowed. Today, it is equivalent to where 'a: 'b, ....
We require a type parameter bound after : or + (in the appropriate context). Today, a bound is not required in the two cases from empty-type-parameter-bounds.rs.

Could someone do a Crater run to determine which of these changes need a warning cycle?
cc @eddyb

petrochenkov · 2016-11-11T09:31:24Z

We require a type parameter bound after : or +

I haven't read the patch, but always requiring type after + seems incorrect, it's a usual case of trailing separator, they are always allowed _{^{(except for trailing | in match arms)}}.

jseyfried · 2016-11-11T09:36:34Z

I think of + not as a separator but as a associative operator -- e.g. let x = 1 +; wouldn't make sense.

eddyb · 2016-11-11T13:05:50Z

Started crater run (sorry for the delay).

eddyb · 2016-11-11T16:32:07Z

Crater report shows 8 regressions (only eetf is a false positive).
Most of them have something like trait Provider: { which is just weird.
However, the ones which generate empty lists of bounds through macros seem legitimate.

jseyfried · 2016-11-11T22:24:36Z

@eddyb I think a macro generating empty lists of bounds could have a legitimate use case, but it looks like the macro-expanded bounds from the Crater run are always empty and can be easily avoided. Also, we already forbid empty lists of bounds in other contexts today.

@matklad The all the breakage appears to be due to empty bounds lists after :. Could you make this a warning instead of a hard error?

petrochenkov · 2016-11-12T15:05:33Z

I've audited ast.rs and it turns out there are very few lists where empty list is syntactically invalid.

Path segments. Empty path may make sense semantically - current module if it's local or crate root if it's global, but there are obvious parsing problems.
Patterns in match arm. A match arm without patterns (impossible arm) doesn't make sense semantically.
Type or lifetime bounds. Empty list is semantically valid (no bounds) and may be generated by macros.
Where predicates. Empty list is semantically valid (no predicates) and may be generated by macros.

Basically, I think this should be valid, even if it looks somewhat weird:

struct S<T:> where {
    field: T,
}

petrochenkov · 2016-11-12T15:10:34Z

@jseyfried

I think of + not as a separator but as a associative operator -- e.g. let x = 1 +; wouldn't make sense.

I admit it may be interpreted as addition in type sums like type T = Trait + Send;, but it's certainly a list separator in bounds.
If both cases are parsed by the same routine now, then I don't mind prohibiting trailing +, it's not a big deal and can always be allowed later if convincing use case is found.

matklad · 2016-11-12T21:38:39Z

Looks like it is not clear if empty parameter bounds should be allowed. So I've resurrected #37278 which only fixes a clear bug with lifetimes in the where clause.

So I think we need to tag this with T-lang and decide what syntax should be allowed. Here is the status quo:

// These are accepted
trait A: {}
fn b<'a:, U:>() {}
type C = for<'a> Clone+;

// These are forbidden

// bounds on where clauses must be non empty
fn d<T>() where T: {}

// In type grammar, `+` is treated like a binary operator,
// and hence both L and R side are required.
fn e(f: &(A+)) {}

jseyfried · 2016-11-14T06:46:17Z

I'm leaning toward @petrochenkov's proposal to allow empty bounds lists (e.g. fn f<T:>() {}).
I don't have a strong opinion trailing +s, which aren't as important since we can forbid them consistently without without breakage in practice and decide later.

@rust-lang/lang do we want to allow empty bounds lists and/or trailing +s? Both are sometimes allowed today. Forbidding empty bounds lists would cause some trivially fixable breakage, so we would need a warning cycle.

bors · 2016-11-14T14:04:40Z

☔ The latest upstream changes (presumably #37278) made this pull request unmergeable. Please resolve the merge conflicts.

nrc · 2016-11-15T20:02:59Z

IMO we should allow neither empty bounds lists nor trailing + both are weird. However, I'm somewhat sympathetic to the macro/codegen argument for the bounds list case.

matklad · 2016-11-15T20:06:28Z

Worth mentioning that currently where clauses allow empty bounds for lifetimes, but not for types.

Valid: where 'a:, T: Foo

Invalid: where 'a: 'b, T:

nikomatsakis · 2016-12-01T22:15:14Z

At the @rust-lang/lang meeting recently we settled on:

accept empty bounds list for both types/lifetime parameters
do not accept trailing + for now because @nrc thinks it looks weird
- macros can instead generate multiple where-clauses:
  - T: Foo, T: Bar,

Sound good?

matklad · 2016-12-05T14:06:51Z

Sound good?

Yep. Stuff like trait Provider: { can be cleaned up by IDE / rustfmt.

pnkfelix · 2016-12-22T22:19:00Z

@matklad does the current PR encode that semantics yet, or does it need further revision?

matklad · 2016-12-23T15:23:38Z

@pnkfelix yep, this does not implement the suggestion yet. I'll try to look into that, thanks for the reminder!

matklad · 2017-01-10T12:42:02Z

I'll try to look into that, thanks for the reminder!

Looks like I don't have enough free time after all to actually make a fix :(

petrochenkov · 2017-01-11T08:46:45Z

@matklad
I can rebase this and finish the work if you want (not right now though, but ~~in a couple of weeks~~ actually maybe sooner).

matklad · 2017-01-11T09:00:27Z

@petrochenkov that'd be great! Not sure that rebasing makes sense though: the current implementation is rather different from the final proposal we arrived at!

petrochenkov · 2017-01-18T17:10:04Z

#39158 is submitted.

aturon · 2017-01-26T20:57:06Z

Should we close this PR in favor of #39158?

matklad · 2017-01-26T21:14:40Z

Surely!

@matklad

Bounds parsing refactoring 2 See #37511 for previous discussion. cc @matklad Relaxed parsing rules: - zero bounds after `:` are allowed in all contexts. - zero predicates are allowed after `where`. - trailing separator `,` is allowed after predicates in `where` clauses not followed by `{`. Other parsing rules: - trailing separator `+` is still allowed in all bound lists. Code is also cleaned up and tests added. I haven't touched parsing of trait object types yet, I'll do it later.

matklad added 2 commits October 19, 2016 18:58

Fix where clauses parsing

9d179e6

Require at least one predicate for a lifetime in a where clause

Don't allow empty type parameter bounds

8909e1b

rust-highfive assigned pnkfelix Nov 1, 2016

matklad mentioned this pull request Nov 1, 2016

Fix syntax error in the compiler #37278

Merged

matklad commented Nov 1, 2016

View reviewed changes

alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 10, 2016

jseyfried assigned jseyfried and unassigned pnkfelix Nov 11, 2016

jseyfried reviewed Nov 11, 2016

View reviewed changes

jseyfried added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 11, 2016

eddyb removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 11, 2016

petrochenkov mentioned this pull request Nov 12, 2016

Nightly regression: empty lifetime list in for<lifetime-list> doesn't parse anymore #37733

Closed

jseyfried added I-nominated T-lang Relevant to the language team and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 14, 2016

nikomatsakis removed the I-nominated label Dec 1, 2016

This was referenced Jan 15, 2017

Can't compile type with lifetime bound in certain order #39085

Closed

Bounds parsing refactoring 2 #39158

Merged

matklad closed this Jan 26, 2017

petrochenkov mentioned this pull request Apr 15, 2017

Bound and supertrait bounds can be empty #30106

Closed

matklad deleted the bounds-parsing-refactoring branch July 9, 2019 12:34

Bounds parsing refactoring #37511

Bounds parsing refactoring #37511

Uh oh!

Conversation

matklad commented Nov 1, 2016 • edited by jseyfried Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Nov 1, 2016

Uh oh!

matklad Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

matklad Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfried Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluss commented Nov 1, 2016

Uh oh!

matklad commented Nov 1, 2016

Uh oh!

bluss commented Nov 1, 2016

Uh oh!

matklad commented Nov 2, 2016

Uh oh!

matklad commented Nov 7, 2016

Uh oh!

alexcrichton commented Nov 10, 2016

Uh oh!

jseyfried Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

jseyfried commented Nov 11, 2016

Uh oh!

petrochenkov commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jseyfried commented Nov 11, 2016

Uh oh!

eddyb commented Nov 11, 2016

Uh oh!

eddyb commented Nov 11, 2016

Uh oh!

jseyfried commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Nov 12, 2016

Uh oh!

jseyfried commented Nov 14, 2016

Uh oh!

bors commented Nov 14, 2016

Uh oh!

nrc commented Nov 15, 2016

Uh oh!

matklad commented Nov 15, 2016

Uh oh!

nikomatsakis commented Dec 1, 2016

Uh oh!

matklad commented Dec 5, 2016

Uh oh!

pnkfelix commented Dec 22, 2016

Uh oh!

matklad commented Dec 23, 2016

Uh oh!

matklad commented Jan 10, 2017

Uh oh!

petrochenkov commented Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Jan 11, 2017

Uh oh!

petrochenkov commented Jan 18, 2017

matklad commented Nov 1, 2016 •

edited by jseyfried

Loading

jseyfried Nov 11, 2016 •

edited

Loading

petrochenkov commented Nov 11, 2016 •

edited

Loading

jseyfried commented Nov 11, 2016 •

edited

Loading

petrochenkov commented Nov 12, 2016 •

edited

Loading

petrochenkov commented Nov 12, 2016 •

edited

Loading

petrochenkov commented Jan 11, 2017 •

edited

Loading