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

Rebased.

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

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

• c_vec-1.2.0 (before) (after) -- timeout
• jmespath-0.1.1 (before) (after) -- Box<(Fn(&[Rcvar], &mut Context) -> SearchResult) + Sync>, it feels like this should work
• cat-0.0.1 (before) (after) -- timeout
• incrust-0.2.12 (before) (after) -- & for <'t> Type<'t> + 'r -- I think this is a bug you intended to fix?
• plex-0.0.3 (before) (after) -- "no method named eat_lifetime found for type syntax::parse::parser::Parser<'_> in the current scope", use of unstable API
• schedule-0.1.0 (before) (after) -- (FnMut() -> ()) + 'a, also seems like this should work
• shadowsocks-0.6.1 (before) (after) -- Box<(Fn(D::Error) -> E) + 'static+ Send+Sync>, seems like this should work
• throw-0.1.1 (before) (after) -- timeout

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

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

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

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

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.

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.

@bors r+

📌 Commit 803e76c has been approved by nikomatsakis

🔒 Merge conflict

@bors r=nikomatsakis

📌 Commit b5e8897 has been approved by nikomatsakis

⌛ Testing commit b5e8897 with merge 8c4f2c6...

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