Refactor parsing of trait object types by petrochenkov · Pull Request #40043 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation37 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

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

This was referenced

Mar 4, 2017

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@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

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

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) { }

So why wouldn't this?

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

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.

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.

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.

brson added beta-nominated

Nominated for backporting to the compiler in the beta channel.

relnotes

Marks issues that should be documented in the release notes of the next release.

labels

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

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.

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.

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

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

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

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.

📌 Commit 803e76c has been approved by nikomatsakis

📌 Commit b5e8897 has been approved by nikomatsakis

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.
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 mentioned this pull request

Mar 22, 2017

This was referenced

Mar 22, 2017

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 pushed a commit to google/tarpc that referenced this pull request

Mar 23, 2017

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

Labels

relnotes

Marks issues that should be documented in the release notes of the next release.

T-lang

Relevant to the language team