-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Wrap multi-bound impl/dyn after prefix type constructors in the type printer #156844
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
6b4d2ac
112b04f
1853100
b986210
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -352,6 +352,15 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { | |
| /// will always be printed.) | ||
| fn should_print_optional_region(&self, region: ty::Region<'tcx>) -> bool; | ||
|
|
||
| /// Whether `pretty_print_type` should wrap a multi-bound `impl` / `dyn` | ||
| /// inner in parens after a prefix type constructor (`&`, `&mut`, `*const`, | ||
| /// `*mut`), so the output is `&(impl A + B)` rather than the parser- | ||
| /// ambiguous `&impl A + B`. Byte-stable printers (mangling, `type_name`) | ||
| /// override this to `false`. | ||
| fn add_disambiguating_parens_in_prefix_position(&self) -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn reset_type_limit(&mut self) {} | ||
|
|
||
| // Defaults (should not be overridden): | ||
|
|
@@ -723,15 +732,17 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { | |
| } | ||
| ty::RawPtr(ty, mutbl) => { | ||
| write!(self, "*{} ", mutbl.ptr_str())?; | ||
| ty.print(self)?; | ||
| self.print_inner_with_disambiguating_parens(ty)?; | ||
| } | ||
| ty::Ref(r, ty, mutbl) => { | ||
| write!(self, "&")?; | ||
| if self.should_print_optional_region(r) { | ||
| r.print(self)?; | ||
| write!(self, " ")?; | ||
| } | ||
| ty::TypeAndMut { ty, mutbl }.print(self)?; | ||
| // `&mut (impl A + B)`, not `&(mut impl A + B)`: emit `mut ` before the parens. | ||
| write!(self, "{}", mutbl.prefix_str())?; | ||
| self.print_inner_with_disambiguating_parens(ty)?; | ||
| } | ||
| ty::Never => write!(self, "!")?, | ||
| ty::Tuple(tys) => { | ||
|
|
@@ -1071,6 +1082,121 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Prints `ty` after a prefix type constructor, wrapping in parens iff | ||
| /// [`Self::inner_needs_disambiguating_parens`] returns `true`. | ||
| fn print_inner_with_disambiguating_parens(&mut self, ty: Ty<'tcx>) -> Result<(), PrintError> { | ||
| let need_paren = self.inner_needs_disambiguating_parens(ty); | ||
| if need_paren { | ||
| write!(self, "(")?; | ||
| } | ||
| ty.print(self)?; | ||
| if need_paren { | ||
| write!(self, ")")?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Whether `ty`, sitting right after a prefix type constructor, would | ||
| /// print with a top-level `+`. Without the wrap, `&impl A + B` parses as | ||
| /// the ambiguous `(&impl A) + B`. | ||
| fn inner_needs_disambiguating_parens(&self, ty: Ty<'tcx>) -> bool { | ||
| if !self.add_disambiguating_parens_in_prefix_position() || self.should_print_verbose() { | ||
| return false; | ||
| } | ||
| match ty.kind() { | ||
| // RPITIT trait-side appears as `Projection`, not `Opaque`. | ||
| ty::Alias(ty::AliasTy { kind: ty::Opaque { def_id }, args, .. }) => { | ||
| self.opaque_has_multiple_bounds(*def_id, args) | ||
| } | ||
| ty::Alias(ty::AliasTy { kind: ty::Projection { def_id }, args, .. }) | ||
| if self.tcx().is_impl_trait_in_trait(*def_id) => | ||
| { | ||
| self.opaque_has_multiple_bounds(*def_id, args) | ||
| } | ||
| ty::Dynamic(predicates, region) => { | ||
| if self.should_print_optional_region(*region) { | ||
| // `ty::Dynamic` self-wraps when it has an explicit region. | ||
| false | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we return true here, and remove the self-wrap from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my understanding (correct me if I'm wrong): First: Second: the cleanup is broader than the prefix-position fix. I put the whole change in a single commit on top of the commits for type_name and identity-instantiation ones, so if the scope is too wide for this PR it can be dropped |
||
| } else { | ||
| // Projections inline into the principal as `<Item = X>`; | ||
| // only principal/auto traits produce a top-level `+`. | ||
| predicates | ||
| .iter() | ||
| .filter(|pred| { | ||
| matches!( | ||
| pred.skip_binder(), | ||
| ty::ExistentialPredicate::Trait(_) | ||
| | ty::ExistentialPredicate::AutoTrait(_) | ||
| ) | ||
| }) | ||
| .count() | ||
| > 1 | ||
| } | ||
| } | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| /// Whether `Alias(Opaque)` would print with more than one top-level | ||
| /// `+`-joined component. Must stay in sync with | ||
| /// `pretty_print_opaque_impl_type`'s sized-bound handling. `?Sized` and | ||
| /// the synthetic `Sized` / `?Sized` / `MetaSized` / `PointeeSized` suffix | ||
| /// each contribute one top-level joinable component. Regressions land in | ||
| /// `tests/ui/impl-trait/in-trait/refine-rustfix-parens.rs`. | ||
| fn opaque_has_multiple_bounds(&self, def_id: DefId, args: ty::GenericArgsRef<'tcx>) -> bool { | ||
| let tcx = self.tcx(); | ||
| let bounds = tcx.explicit_item_bounds(def_id); | ||
|
|
||
| // Mirror `pretty_print_opaque_impl_type`'s sized-bound handling: | ||
| // positive `Sized` and `MetaSized` are absorbed into the synthetic | ||
| // suffix below; negative `Sized` (`?Sized`) falls through and is | ||
| // printed inline. | ||
| let mut trait_emits = 0usize; | ||
| let mut lifetimes_count = 0usize; | ||
| let mut has_sized_bound = false; | ||
| let mut has_negative_sized_bound = false; | ||
| let mut has_meta_sized_bound = false; | ||
|
|
||
| for (predicate, _) in | ||
| bounds.iter_instantiated_copied(tcx, args).map(Unnormalized::skip_norm_wip) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to instantiate? As we are looking at the clause kind, is identity instantiation enough?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, identity is enough -- only the clause kinds ( |
||
| { | ||
| match predicate.kind().skip_binder() { | ||
| ty::ClauseKind::Trait(pred) => match tcx.as_lang_item(pred.def_id()) { | ||
| Some(LangItem::Sized) => match pred.polarity { | ||
| ty::PredicatePolarity::Positive => { | ||
| has_sized_bound = true; | ||
| } | ||
| ty::PredicatePolarity::Negative => { | ||
| has_negative_sized_bound = true; | ||
| trait_emits += 1; | ||
| } | ||
| }, | ||
| Some(LangItem::MetaSized) => { | ||
| has_meta_sized_bound = true; | ||
| } | ||
| Some(LangItem::PointeeSized) => {} | ||
| _ => trait_emits += 1, | ||
| }, | ||
| ty::ClauseKind::TypeOutlives(_) => lifetimes_count += 1, | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| // The synthetic suffix in `pretty_print_opaque_impl_type` emits at most | ||
| // one extra bound (`Sized` / `?Sized` / `MetaSized` / `PointeeSized`). | ||
| let using_sized_hierarchy = tcx.features().sized_hierarchy(); | ||
| let add_sized = has_sized_bound && (trait_emits == 0 || has_negative_sized_bound); | ||
| let add_maybe_sized = | ||
| has_meta_sized_bound && !has_negative_sized_bound && !using_sized_hierarchy; | ||
| let has_pointee_sized = | ||
| !has_sized_bound && !has_meta_sized_bound && !has_negative_sized_bound; | ||
| let synthetic = add_sized | ||
| || add_maybe_sized | ||
| || (using_sized_hierarchy && (has_meta_sized_bound || has_pointee_sized)); | ||
|
|
||
| trait_emits + lifetimes_count + usize::from(synthetic) > 1 | ||
| } | ||
|
|
||
| fn pretty_print_opaque_impl_type( | ||
| &mut self, | ||
| def_id: DefId, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
If the docs permit drift, I don't see why we should bother keeping wrong code.
View changes since the review
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.
Removed.