Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor parsing of trait object types #40043

Merged
merged 1 commit into from Mar 22, 2017

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 23, 2017

Bugs are fixed and code is cleaned up.

User visible changes:

  • ty matcher in macros accepts trait object types like Write + Send (#39080)
  • Buggy priority of + in trait object types starting with for is fixed (#39317). &for<'a> Trait<'a> + Send is now parsed as (&for<'a> Trait<'a>) + Send and requires parens &(for<'a> Trait<'a> + Send). For comparison, &Send + for<'a> Trait<'a> was parsed like this since Nov 27, 2014.
  • Trailing +s are supported in trait objects, like in other bounds.
  • Better error reporting for trait objects starting with ?Sized.

Fixes #39080
Fixes #39317 [breaking-change]
Closes #39298
cc #39085 (fixed, then reverted #40043 (comment))
cc #39318 (fixed, then reverted #40043 (comment))

r? @nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 1, 2017

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

@petrochenkov petrochenkov force-pushed the petrochenkov:objpars branch from e783e10 to 9af340b Mar 1, 2017
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 1, 2017

Rebased.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 1, 2017

@petrochenkov these changes look good to me. Sorry for delay. Will queue up a crater run.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 3, 2017

Regression report: https://gist.github.com/nikomatsakis/42c2e8f0ff770878fed845d93c53c05e

So, I think we should fix the parenthesis thing. Not sure what to do about the precedence of for<'a> vis-a-vis + exactly. We might just submit a patch and try to issue a new point release (cc @alexander-irbis -- your crate incrust may have been relying on a parsing bug).

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 3, 2017

@nikomatsakis
jmespath-0.1.1, schedule-0.1.0 and shadowsocks-0.6.1 are #39318 and I don't think it should work because the fact that it works now is an accidental regression and parenthesized bounds are never accepted in other situations. I'll add code parsing these parentheses, but it needs at least a warning.

We might just submit a patch

Yeah, I'll send patches to affected crates.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 5, 2017

Updated with a warning instead of an error for (Trait) + Trait + 'lt.
Patches to affected crates are submitted.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 6, 2017

@petrochenkov I'm not sure I agree that () should not work. It may have been an accidental regression, but it like what I would naively expect. That is, + is a type that combines other things that fit the type grammar (which should be object-safe traits, essentially). It has lower precedence than prefix operators like &, hence we write &(Foo + Safe), as well as the fn "operator" (hence fn foo() -> (Foo + Safe)). But I see no reason that more parens can't work (e.g., &((Foo) + Safe)).

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 6, 2017

@nikomatsakis

That is, + is a type that combines other things that fit the type grammar

But... that's not true (the lifetime in Trait + 'a already doesn't fit into the type grammar). "+" is a list of generic bounds, exactly the same thing as in fn f<T: HERE>() {} or fn f() -> impl HERE and it should reuse the same grammar, for simplicity and clarity.
The current scheme treats the first bound specially and inconsistently with other bounds, my goal was to remove this special case. In theory parentheses may be permitted in all bounds (fn f<T: (Debug) + (Fn(u8) -> u8) + ('static)>, but it would be such a useless addition to the language, that I'd rather prefer to prohibit the existing special case instead. (This prohibition is also kind of an unused_parens lint that is always enabled (with a nice error message suggesting to remove the parens!))

let ty = ts.into_iter().nth(0).unwrap().unwrap();
match ty.node {
// Accept `(Trait1) + Trait2 + 'a` for now,
// but issue a future compatibility warning.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 7, 2017

Contributor

in what sense is this a "future-compatiblity warning"?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 7, 2017

Contributor

specifically, it doesn't seem to mention anything about the future? (and there is no associated tracking issue, right?)

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Mar 7, 2017

Author Contributor

I repurposed #39318 as a tracking issue, I'll link to it.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 7, 2017

@petrochenkov Hmm, first, it's not clear that 'a should be considered specially -- it may be reasonable to consider it as an "object type", just like any other. I also feel like I'd expect parens to be legal in a list of bounds, for that matter. But I'm not sure how much I care, ultimately, I'd probabbly be ok with the conservative route for now as long we get the crates updated.

cc @rust-lang/lang -- some questions here about the finer points of our grammar

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 7, 2017

@nikomatsakis

Hmm, first, it's not clear that 'a should be considered specially -- it may be reasonable to consider it as an "object type", just like any other.

That's what I originally did, but then added 63c5c0d just to be conservative and not accept a single lifetime in ty matcher, who knows what problems this could cause in the future.
I'm not really sure what is the right thing to do here and will gladly accept any consensus the lang team will come up with.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Mar 7, 2017

That's what I originally did, but then added 63c5c0d just to be conservative and not accept a single lifetime in ty matcher, who knows what problems this could cause in the future.

I can't think of any problems, but it feels like we've already made our bed here.

As useless as this is, philosophically it seems like a type like &'a 'b should be accepted.

This becomes more material in light of rust-lang/rfcs#1733, which would presumably allow these through an alias even if they're ungrammatical directly. That is, this would compile:

trait Static = 'static;
fn foo(_: Box<Static>) { }

So why wouldn't this?

fn foo(_: Box<'static>) { }
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 8, 2017

Sigh, I hadn't considered the parsing ambiguity here:

As useless as this is, philosophically it seems like a type like &'a 'b should be accepted.

But it can be resolved easily enough via some kind of rules that require an explicit paren, I guess, or something.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 8, 2017

This becomes more material in light of rust-lang/rfcs#1733, which would presumably allow these through an alias even if they're ungrammatical directly. That is, this would compile:

Well, this could be considered not object safe, if we had a rule that there must be at least 1 actionable trait or something.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Mar 8, 2017

Well, this could be considered not object safe, if we had a rule that there must be at least 1 actionable trait or something.

That's true. I'm okay with that if parsing these types is too complex.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 9, 2017

In @rust-lang/compiler meeting, we decided not to beta-backport this: it's a big change, and it doesn't really seem to fix anything tagged as regressions, except for #39318 -- and we're still debating what's the desired behavior here (and it would have a warning period anyhow).

@petrochenkov petrochenkov force-pushed the petrochenkov:objpars branch from 63c5c0d to 803e76c Mar 16, 2017
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 16, 2017

VVV
@nikomatsakis
Updated, the warning for parens is removed, Box<'a + Trait> is un-implemented.
/\/\/\


Nevertheless, this is all very strange

Why would we want to support Box<'a + Bounds>? It seems to me that this is actively undesirable. (In particular, because it seems to suggest that 'a by itself would be allowed, and we don't want that, and because it means that Box<'a> vs Box<'a+> seem quite different...?)

I guess what I mean is, if the lifetime bound can't be the only thing that is present, why allow it to go first?

impl 'a + Trait is supported => Box<'a + Trait> should be supported
'a has syntactic ambiguity, 'a + whatever doesn't have syntactic ambiguity and can be supported.
I don't see any link between 'a + Trait being allowed and and 'a being allowed.
I don't see any link between 'a being allowed or disallowed and 'a going first.
I don't see why the first bound need to be treated specially at all.

@solson

This comment has been minimized.

Copy link
Member

solson commented Mar 16, 2017

impl 'a + Trait is supported

To be honest, I find this surprising, too.

I don't see why the first bound need to be treated specially at all.

I read a trait object as MainTrait + extra where extra can include only a small selection of marker traits and a lifetime. I can see the value of relaxing the parsing rules, I suppose, since one might not expect a strict ordering around +, but when it's supported I will consider Box<Send + Trait> and Box<'a + Trait> examples of bad style.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 16, 2017

@solson

I read a trait object as MainTrait + extra where extra can include only a small selection of marker traits and a lifetime.

This is a model dictated by limitations of the current implementation - it is quite reasonable to expect Box<Write + Read + Debug> to work, and to point to something implementing all these traits (in any order), but there you have implementation issues like how to combine vtables, etc.
But foremost, this all is not a parser's concern (rejecting legal but dubious style is not a parser's concern either, IMO).

@solson

This comment has been minimized.

Copy link
Member

solson commented Mar 16, 2017

@petrochenkov Multiple trait object support would definitely change how I think about this, although I would recommend always putting the traits with methods first in the list.

But foremost, this all is not a parser's concern (rejecting legal but dubious style is not a parser's concern either, IMO).

Yeah, that's fair. I do see the sense in relaxing the parsing rules and/or making them more consistent. I'm basically just saying the restriction isn't very strange from a user point-of-view.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 20, 2017

@petrochenkov

Well, I guess my sense is that if A + B parses, I expect A to parse, and not have a radically different meaning. But I agree there is no technical objection per se. (And, to be fair, I also expect B + A to parse, so... there is no winning. =)

I suppose that if we adopted the dyn Trait proposal, then both impl and dyn unambiguously take a list of bounds afterwards, and all potential confusion is removed (modulo questions of precedence. Perhaps a point in favor.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Mar 20, 2017

So I've just discovered that this is valid:

fn foo<'a, T: 'a + ToString>() { }

I think trait object syntax should be consistent with bounds syntax, so Box<'a + ToString> should be valid. Maybe if we decide to lint more stylistic things we should lint a preference to lifetime constraints coming last.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 20, 2017

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 20, 2017

📌 Commit 803e76c has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 21, 2017

🔒 Merge conflict

@retep998 retep998 force-pushed the petrochenkov:objpars branch from 803e76c to eff97c7 Mar 21, 2017
@petrochenkov petrochenkov force-pushed the petrochenkov:objpars branch from eff97c7 to b5e8897 Mar 21, 2017
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 21, 2017

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 21, 2017

📌 Commit b5e8897 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 22, 2017

⌛️ Testing commit b5e8897 with merge 8c4f2c6...

bors added a commit that referenced this pull request Mar 22, 2017
Refactor parsing of trait object types

Bugs are fixed and code is cleaned up.

User visible changes:
- `ty` matcher in macros accepts trait object types like `Write + Send` (#39080)
- Buggy priority of `+` in trait object types starting with `for` is fixed (#39317). `&for<'a> Trait<'a> + Send` is now parsed as `(&for<'a> Trait<'a>) + Send` and requires parens `&(for<'a> Trait<'a> + Send)`. For comparison, `&Send + for<'a> Trait<'a>` was parsed like this since [Nov 27, 2014](#19298).
- Trailing `+`s are supported in trait objects, like in other bounds.
- Better error reporting for trait objects starting with `?Sized`.

Fixes #39080
Fixes #39317 [breaking-change]
Closes #39298
cc #39085 (fixed, then reverted #40043 (comment))
cc #39318 (fixed, then reverted #40043 (comment))

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 8c4f2c6 to master...

@bors bors merged commit b5e8897 into rust-lang:master Mar 22, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
jonhoo added a commit to jonhoo/tarpc that referenced this pull request Mar 23, 2017
The merge of rust-lang/rust#40043 removes parse_ty_path in the latest
nightly, which we depended on. This patch rewrites that code path using
parse_path, and in the process eliminates an unreachable!() if let arm.
tikue added a commit to google/tarpc that referenced this pull request Mar 23, 2017
The merge of rust-lang/rust#40043 removes parse_ty_path in the latest
nightly, which we depended on. This patch rewrites that code path using
parse_path, and in the process eliminates an unreachable!() if let arm.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 28, 2017
syntax: Parse trait object types starting with a lifetime bound

Fixes rust-lang#39085

This was originally implemented in rust-lang#40043, then reverted, then there was some [agreement](rust-lang#39318 (comment)) that it should be supported.
(This is hopefully the last PR related to bound parsing.)
bors added a commit that referenced this pull request Apr 28, 2017
syntax: Parse trait object types starting with a lifetime bound

Fixes #39085

This was originally implemented in #40043, then reverted, then there was some [agreement](#39318 (comment)) that it should be supported.
(This is hopefully the last PR related to bound parsing.)
@petrochenkov petrochenkov deleted the petrochenkov:objpars branch Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.