Wrap multi-bound impl/dyn after prefix type constructors in the type printer - #156844
Wrap multi-bound impl/dyn after prefix type constructors in the type printer#156844onehr wants to merge 5 commits into
Conversation
rustbot
added
S-waiting-on-author
rustbot
added
the
S-waiting-on-review
rustbot
removed
the
S-waiting-on-author
|
rustbot has assigned @ShoyuVanilla. Use Why was this reviewer chosen?The reviewer was selected based on:
|
rustbot
added
S-waiting-on-author
6d008b2 to
cb755cf
Compare
This comment has been minimized.
This comment has been minimized.
cb755cf to
0478db9
Compare
This comment has been minimized.
This comment has been minimized.
0478db9 to
6b4d2ac
Compare
rustbot
added
the
S-waiting-on-review
rustbot
removed
the
S-waiting-on-author
|
Hi @cjgillot Thanks. |
onehr
changed the title
| // though the docs permit drift. | ||
| fn add_disambiguating_parens_in_prefix_position(&self) -> bool { | ||
| false | ||
| } |
There was a problem hiding this comment.
If the docs permit drift, I don't see why we should bother keeping wrong code.
| ty::Dynamic(predicates, region) => { | ||
| if self.should_print_optional_region(*region) { | ||
| // `ty::Dynamic` self-wraps when it has an explicit region. | ||
| false |
There was a problem hiding this comment.
Should we return true here, and remove the self-wrap from ty::Dynamic?
There was a problem hiding this comment.
my understanding (correct me if I'm wrong):
First: Fn(..) -> T + B hits the same +-ambiguity as &T + B -- the parser refuses to commit and emits error: ambiguous + in a type. So the same disambiguation hook also runs from pretty_print_fn_sig's output position, and the trait method is renamed add_disambiguating_parens since it now covers both prefix and return slots. Concrete: Box<dyn Fn() -> dyn Trait> defaults the inner dyn's lifetime to 'static, so dropping the self-wrap alone would print it as Box<dyn Fn() -> dyn Trait + 'static> -- parser-rejected. The fn-sig wrap restores Box<dyn Fn() -> (dyn Trait + 'static)>, which round-trips.
Second: the cleanup is broader than the prefix-position fix. Box<dyn A + 'a>, [dyn A + 'a; N], top-level dyn etc. drop the parens that the self-wrap used to add defensively in positions where the parser does not need them. The last commit touches 100+ files -- 8 stderr/fixed gain prefix or fn-return parens (the actual correctness fix), 3 carry both kinds of change, ~100 stderr/fixed drop defensive parens, and 26 .rs files get inline //~ ERROR annotation updates to match.
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
| let mut has_meta_sized_bound = false; | ||
|
|
||
| for (predicate, _) in | ||
| bounds.iter_instantiated_copied(tcx, args).map(Unnormalized::skip_norm_wip) |
There was a problem hiding this comment.
Do we really need to instantiate? As we are looking at the clause kind, is identity instantiation enough?
There was a problem hiding this comment.
Right, identity is enough -- only the clause kinds (Trait def_id + polarity, TypeOutlives) are inspected, all invariant under instantiation. Switched to iter_identity_copied() and dropped the args parameter
|
This PR modifies |
rustbot
added
the
WG-trait-system-refactor
This comment has been minimized.
This comment has been minimized.
f52d111 to
b18db32
Compare
This comment has been minimized.
This comment has been minimized.
b18db32 to
2a24db9
Compare
|
The Clippy subtree was changed cc @rust-lang/clippy |
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
2a24db9 to
b986210
Compare
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Hi, just a quick question — issue #144401 seems to be only about |
`&impl A + B`, `&mut impl A + B`, `*const impl A + B`, `*mut impl A + B`, `&dyn A + B` and similar shapes parse as the ambiguous `(&impl A) + B` and are rejected by the parser, so diagnostics that surface them produce non-compilable Rust. `refining_impl_trait` machine-applicable suggestions are the user-visible failure. In the `ty::Ref` and `ty::RawPtr` arms of `pretty_print_type`, wrap the inner type in parens when it would print with `+`-joined bounds. For `&mut`, emit the mutability prefix before the parens so the output is `&mut (impl A + B)`, not the ill-formed `&(mut impl A + B)`. `ty::Dynamic` already self-wraps under an explicit region, so that case is skipped. `opaque_has_multiple_bounds` mirrors `pretty_print_opaque_impl_type`'s sizedness handling, so `?Sized` and the synthetic `Sized` suffix are counted correctly. `LegacySymbolMangler` and `TypeNamePrinter` override `add_disambiguating_parens_in_prefix_position` to `false`, keeping mangled symbols and `std::any::type_name` output byte-identical.
`std::any::type_name`'s docs allow output to "change between versions of the compiler" and tie it to the same infrastructure as diagnostics, so pinning `add_disambiguating_parens_in_prefix_position` to `false` to keep historical bytes stable works against the doc contract. The only strict `type_name` assertions in `library/std/tests/type-name- unsized.rs` are on single-bound `dyn` and are unaffected.
The helper only inspects clause kinds (the trait `def_id` + polarity for `Trait`, and `TypeOutlives`), all invariant under instantiation. Switch from `iter_instantiated_copied(tcx, args)` to `iter_identity_copied()` and drop the `args` parameter.
rustbot
added
S-waiting-on-author
b986210 to
6ce3bfe
Compare
@manyiResearch Sorry for the slow reply — I missed the notification on this one. You are right that the issue only shows The same ambiguity exists after The first commit is the fix for #144401 itself. The rest is that printer cleanup and the test churn it causes. |
This comment has been minimized.
This comment has been minimized.
Wrap multi-bound `impl`/`dyn` at the positions where the parser is ambiguous: `&T`, `*const T`, `*mut T`, and the function-pointer / `Fn(..) -> T` return slot. The trait method covering this is renamed to `add_disambiguating_parens` since it now applies to both prefix and return positions; `inner_needs_disambiguating_parens` for `Dynamic` counts the printable region alongside the trait predicates that the self-wrap used to handle. Non-prefix positions where the parser does not need parens (`Box<dyn A + 'a>`, `[dyn A + 'a; N]`, top-level dyn) drop them. The bulk of the touched `.stderr` baselines reflect this cleanup; inline `//~ ERROR` annotations are adjusted to match.
`opaque_has_multiple_bounds` hand-mirrored the bound gathering and sizedness handling of `pretty_print_opaque_impl_type` in order to predict how many `+`-joined components the opaque would print as. The copy had already drifted: it counted lifetime bounds unconditionally, while the printer only renders them outside `with_forced_trimmed_paths`, so `&impl Ta + 'a` was printed as `&(impl Ta)` — parens around a type with no top-level `+`. Collect the bounds once in `collect_opaque_bounds` and have both the printer and the paren check consume the result, so the two cannot disagree. The check stays on identity args, as only the number of printed components matters and that is invariant under instantiation.
6ce3bfe to
3db8625
Compare
6 participants
View all comments
Closes #144401
When the
Typretty printer emits a prefix type constructor whose inner type prints with+-joined bounds -&impl A + B,&mut impl A + B,*const impl A + B,&dyn A + B,&(impl A + ?Sized), ... - the resulting text is parser-ambiguous and rejected as(&impl A) + B. Diagnostics that surface such types -refining_impl_traitmachine-applicable suggestions, argument-of-type hints, etc. - therefore emit non-compilable Rust.Per review feedback that the fix belongs in the default printer, this PR handles the ambiguity in the
ty::Refandty::RawPtrarms ofpretty_print_type.What changed
ty::Refandty::RawPtrincompiler/rustc_middle/src/ty/print/pretty.rsnow wrap the inner type in parens when it would print with+-joined bounds. For&mut, the mutability prefix is printed before the parens so the output is&mut (impl A + B), not the ill-formed&(mut impl A + B).ty::Dynamicalready wraps itself when carrying an explicit region; that case is skipped to avoid double-wrapping.should_print_verbose()is also skipped because the-Zverbose-internalsform (Opaque(DefId, ..)) does not contain a+.opaque_has_multiple_boundshelper keeps its count aligned withpretty_print_opaque_impl_type's trait/lifetime emission plus the synthetic sizedness suffix block (add_sized/add_maybe_sized/has_pointee_sized_bound), so:&impl Trait/&dyn Traitkeeps printing unchanged,impl A + Bkeeps printing without parens,&(impl A + B),&mut (impl A + B),&'a (impl A + 'a),&&(impl A + B)are all wrapped,&(impl A + ?Sized)is wrapped (?Sizedroutes throughMetaSized+ the synthetic suffix on stable, which the helper accounts for),&(impl Iter<Item = u8> + B)is wrapped (projection inlined into the principal trait, second trait counts),&(dyn A + B)is wrapped (subsumes the symmetricdyn-side ambiguity),*const (impl A + B)and*mut (impl A + B)are wrapped (prefix-type generalization).PrettyPrinter::add_disambiguating_parens_in_prefix_positionmethod makes this opt-out.LegacySymbolMangleroverrides it tofalseso legacy mangled bytes stay byte-identical (any change would be an ABI break);TypeNamePrinteroverrides it tofalsesostd::any::type_nameruntime output stays byte-identical. The v0 mangler is unaffected because it carries its ownty::Refencoding.Tests
tests/ui/impl-trait/in-trait/refine-rustfix-parens.rs(run-rustfix) covers:&(impl A + B)- multi positive trait,&(impl A + ?Sized)- positive trait + relaxed sizedness (exercises the synthetic?Sizedsuffix path),&mut (impl A + B)-mutprefix must come before the disambiguating parens,&'a (impl A + 'a)- lifetime as a joinable part,&(impl Iter<Item = u8> + B)- associated-type projection inlined, second trait counts,&impl Iter<Item = u8>- no-wrap control: projection only, single joinable,&&(impl A + B)- nested ref: only the inner&wraps,*const (impl A + B)and*mut (impl A + B)- raw-pointer prefix generalization,impl A + B- must not pick up parens.Re-blessed baselines that intentionally pick up the new
&(...)form:tests/ui/argument-suggestions/display-is-suggestable.stderrtests/ui/cast/casts-differing-anon.stderrtests/ui/impl-trait/opaque-used-in-extraneous-argument.stderrtests/ui/symbol-names/impl1.{legacy,v0}.stderrandimpl1.rs(only thedef-path(...)line and its matching//~ ERROR def-path(...)annotations; the legacy mangled symbol bytes and thedemangling-altline are unchanged becauseLegacySymbolMangleropts out)tests/ui/traits/default_auto_traits/maybe-bounds-in-dyn-traits.stderrLocal verification: