-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Support ?Sized
in where clauses
#37791
Conversation
if name == ty_param.ident.name { | ||
add_bounds.entry(ty_param.id).or_insert(Vec::new()) | ||
.push(bound.clone()); | ||
break 'next_bound; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/break/continue/
bound_pred.bound_lifetimes.is_empty() => { | ||
let name = path.segments[0].identifier.name; | ||
for ty_param in &g.ty_params { | ||
// Don't bother looking up the definition, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't macros make a mess of this in their evil macro-ey ways? Better look up the definition to avoid the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type parameters are not hygienic now, but it may make sense to lookup definitions for future proofing. Even if this is a loop it's probably not very performance critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Randomly implementing unhygienic lookup is a sure way to cause bugs in the future.
@petrochenkov do you have an alternate, more hygiene-friendly impl in mind? |
@nikomatsakis |
@petrochenkov ok seems better |
Updated. |
I am against this because I believe that being sized is such an important part of the type that the annotation removing it should be in a prominent location, not hidden among the where clauses. |
@thepowersgang I like to place all constraints after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, I just think we can tweak the error message.
if let TraitTyParamBound(_, TraitBoundModifier::Maybe) = *bound { | ||
let report_error = |this: &mut Self| { | ||
this.diagnostic().span_err(bound_pred.bounded_ty.span, | ||
"`?Trait` bounds are permitted only on fresh type parameters"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe "only permitted at the point where a type parameter is declared"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the message.
struct S4<T>(T) where for<'a> T: ?Trait<'a>; | ||
//~^ ERROR `?Trait` bounds are permitted only on fresh type parameters | ||
|
||
struct S5<T>(*const T) where T: ?Trait<'static> + ?Sized; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what on earth is ?Trait<'static>
? we permit that??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, apparently. I would have thought it'd be an error. Oh well, obviously pre-existing.
@rfcbot fcp merge Strictly speaking, this is an "insta-stable" lang change. We could use a stability period, not sure if it is necessary in this case. I propose we merge, as this feels more like a bugfix to me, but am amenable to a stability period as well. But let's FCP it. OK @rust-lang/lang, what do you think? The change is to permit |
Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:
No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
@rfcbot reviewed Seems like a bug fix to me. It should always be possible to transpose bounds from the parameter introduction clause to the where clause. "Special cases aren't special enough to break the rules." etc |
@bors r+ |
📌 Commit 7d15250 has been approved by |
Support `?Sized` in where clauses Implemented as described in #20503 (comment) - `?Trait` bounds are moved on type parameter definitions when possible, reported as errors otherwise. (It'd be nice to unify bounds and where clauses in HIR, but this is mostly blocked by rustdoc now - it needs to render bounds in pleasant way and the best way to do it so far is to mirror what was written in source code.) Fixes #20503 r? @nikomatsakis
Implemented as described in #20503 (comment) -
?Trait
bounds are moved on type parameter definitions when possible, reported as errors otherwise.(It'd be nice to unify bounds and where clauses in HIR, but this is mostly blocked by rustdoc now - it needs to render bounds in pleasant way and the best way to do it so far is to mirror what was written in source code.)
Fixes #20503
r? @nikomatsakis