add dyn to display of dynamic (trait) types#51104
Conversation
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Legit test failures. |
|
@zackmdavis (btw, |
nikomatsakis
left a comment
There was a problem hiding this comment.
This seems... basically right? The main thing I see is the precedence issue, but perhaps I am overlooking some subtlety? (cc @eddyb @petrochenkov)
src/librustc/util/ppaux.rs
Outdated
There was a problem hiding this comment.
Hmm, I think this may do the wrong thing in terms of precedence in some cases. For example, &dyn Foo + 'a needs some parens, right? I'm not 100% sure how this is handled already, since the same applies to &Foo + 'a... it kind of looks like we just get it wrong now?
Maybe we can concoct a test case to demonstrate this...
|
Oh, I sort of missed this:
Or at least forgot about it. Let me look at that specifically. |
So, all of that eventually ends up probably in the |
|
Right, right. Let me take a look. I forget how it works now; I know I have had at least one branch that tried to refactor precisely this point but I think it never landed (was part of a lazy norm push on my part)... |
src/test/ui/const-unsized.stderr
Outdated
There was a problem hiding this comment.
I have to admit — tracing through ppaux.rs — I couldn't quite tel how this arises. Can somebody clarify for me? I imagine that this message is produced from traits/error_reporting.rs:
rust/src/librustc/traits/error_reporting.rs
Lines 598 to 601 in 90f34b5
But it seems like that is printing a PolyTraitRef, and I'm not clear on the path that leads from that code to the TyDynamic code. What am I missing?
There was a problem hiding this comment.
The TyDynamic is the Self type, printed by this:
rust/src/librustc/util/ppaux.rs
Line 1258 in 4a9c58c
There was a problem hiding this comment.
Oh; in that case, this seems...correct.
There was a problem hiding this comment.
it might be nicer to print "the trait Sized is not implemented for dyn ...", like we used to, but that seems separate
|
@zackmdavis so specifically which cases were worrying you? It seems like (apart from the matter of |
The message that says "the trait bound |
|
@zackmdavis IMO it's fine, since "the trait bound |
|
Oh, right! 😳 OK, so then the only things left to do are:
—which I should get to early next week. |
|
☔ The latest upstream changes (presumably #51042) made this pull request unmergeable. Please resolve the merge conflicts. |
|
(note for future triage: the author has not so much time for rust at the moment, let's keep this open though #51149 (comment)) |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@nikomatsakis updated to add parentheses when there's a |
|
☔ The latest upstream changes (presumably #51463) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The `dyn Trait` syntax was stabilized in 199ee32. Resolves rust-lang#49277.
|
@nikomatsakis rebased 🏁 |
|
@bors r+ |
|
📌 Commit 4b18085 has been approved by |
|
⌛ Testing commit 4b18085 with merge 9b4bd1dfe1d2889a24b16f6f4a62dc13ba254c8b... |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
add `dyn ` to display of dynamic (trait) types
~~I'm not sure we want the `dyn` in the E0277 "trait bound [...] is not satisfied" messages ("bound" sounds like a different thing in contrast to the names of specific trait-object types like `Box<dyn Trait>`), but I'm finding the code I would need to change that hard to follow—the [display object seems to](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/traits/error_reporting.rs#L600) be a [`Predicate::Trait`](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/ty/mod.rs#L962) variant, whose [`Display` implementation](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/util/ppaux.rs#L1309) calls `.print` on its `PolyTraitPredicate` member, [which is a type alias](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/ty/mod.rs#L1112) for `ty::Binder<TraitPredicate<'tcx>>`, whose [`Display` implementation](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/util/ppaux.rs#L975-L985) ... _&c._— so maybe it's time to pull-request this and see what reviewers think.~~
Resolves rust-lang#49277 (?).
r? @nikomatsakis
Rollup of 11 pull requests Successful merges: - #51104 (add `dyn ` to display of dynamic (trait) types) - #51153 (Link panic and compile_error docs) - #51642 (Fix unknown windows build) - #51730 (New safe associated functions for PinMut) - #51731 (Fix ICEs when using continue as an array length inside closures (inside loop conditions)) - #51747 (Add error for using null characters in #[export_name]) - #51769 (Update broken rustc-guide links) - #51786 (Remove unnecessary stat64 pointer casts) - #51788 (Fix typo) - #51789 (Don't ICE when performing `lower_pattern_unadjusted` on a `TyError`) - #51791 (Minify css) Failed merges: r? @ghost
I'm not sure we want thedynin the E0277 "trait bound [...] is not satisfied" messages ("bound" sounds like a different thing in contrast to the names of specific trait-object types likeBox<dyn Trait>), but I'm finding the code I would need to change that hard to follow—the display object seems to be aPredicate::Traitvariant, whoseDisplayimplementation calls.printon itsPolyTraitPredicatemember, which is a type alias forty::Binder<TraitPredicate<'tcx>>, whoseDisplayimplementation ... &c.— so maybe it's time to pull-request this and see what reviewers think.Resolves #49277 (?).
r? @nikomatsakis